Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sharks - Morgan B. #101

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Sharks - Morgan B. #101

wants to merge 22 commits into from

Conversation

Morfiend
Copy link

No description provided.

Copy link

@yangashley yangashley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on Task List, Morgan! You earned a 🟢 on this project. Really nice to see you making frequent commits too.

From your code, I can see that you've got a nice handle on Flask and SqlAlchemy. Good to see you consistently using jsonify or make response so that you properly return JSON to the client.

I added a few comments about using list comprehension, where you could use a helper function, and using nullable=False.

Let me know if you have any questions!

@@ -3,3 +3,21 @@

class Goal(db.Model):
goal_id = db.Column(db.Integer, primary_key=True)
title = db.Column(db.String)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding nullable=False to ensure every goal requires a title.

@@ -3,3 +3,36 @@

class Task(db.Model):
task_id = db.Column(db.Integer, primary_key=True)
title = db.Column(db.String)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding nullable=False to ensure every task requires a title.

Comment on lines +27 to +33
"is_complete": True if self.completed_at else False
}

if self.goal:
task_dict["goal_id"] = self.goal_id

return task_dict

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

from app.models.goal import Goal
from .helpers import validate_goal, validate_task

goals_bp = Blueprint("goals", __name__, url_prefix="/goals")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work creating two separate route files for each model and adding them to a routes directory

Comment on lines +12 to +15
if request_body.get("title"):
new_goal = Goal.create(request_body)
else:
abort(make_response({"details": "Invalid data"}, 400))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice error handling

Comment on lines +27 to +28
for goal in goals:
goal_response_body.append(goal.to_json())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well and is readable, but if you'd like to incorporate list comprehensions into your code, you could write it like this:

goals_response = [goal.to_json() for goal in goals]

Comment on lines +44 to +47
if request_body.get("title"):
goal.update(request_body)
else:
abort(make_response({"details": "Invalid data"}, 400))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 we can't assume that the client will always pass us a correctly formatted request so good job adding error handling here too

tasks.append(task.to_json())

goal_response = goal.to_json()
goal_response["tasks"] = tasks

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder if you could refactor to_json() so that the logic on line 88 gets moved into the method

Comment on lines +86 to +94
if not already_completed:
key = os.environ.get("SLACK_API")
payload = {
"channel": "task-notifications",
"text": f"Someone just completed the task {task.title}"
}
header = {"Authorization": f"Bearer {key}"}
requests.post("https://slack.com/api/chat.postMessage", params=payload,
headers=header)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider putting this in a helper function and calling it here to make mark_complete() a bit more concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants