c17 sea turtles - jae #111
Conversation
There was a problem hiding this comment.
Great job!
Your tests are all passing and your code is very readable.
Moving forward, try thinking more about how to separate the various responsibilities of your code. Deciding where things should go is really tricky. Often, we try to figure out what's the smallest number of things that needs to know some information to allow everything to run, and we'll organize our code to hide as much detail from the parts that don't need to know about it. We call this loose coupling, and it can make for much more reusable, flexible, maintainable, and testable code!
Overall, nicely done!
|
|
||
| from flask import jsonify, abort, make_response | ||
|
|
||
| def error_message(message, status_code): |
There was a problem hiding this comment.
👍
For the name of this file, the file doesn't contain routes for something called a helper, it contains helpers for routes, so I would name this route_helpers.py
| @@ -0,0 +1,43 @@ | |||
| """empty message | |||
There was a problem hiding this comment.
It's good practice to add a message when generating a migration to help no what's in which file. It's not such an issue with only one migration files, but it can be very helpful if we have several.
| "title": "New Year, New Goal" | ||
| } | ||
| } | ||
| goal = Goal.query.get(1) |
There was a problem hiding this comment.
👍 Good idea to verify that the change was actually saved and not just returned.
Personally, I like to keep tests at the same level of abstraction throughout, so here, since we're writing HTTP client tests, rather than going directly to the database (like lots of other tests here), I would prefer to issue a follow-up GET request.
| assert response.status_code == 404 | ||
|
|
||
| assert response_body == {"details": "Goal with id #1 not found"} | ||
| assert Goal.query.all() == [] |
There was a problem hiding this comment.
Personally, I wouldn't feel the need to check that the goals collection is empty. The test started out with no goals, and it would be particularly odd of trying to delete a non-existent goal would lead to the creation of any additional goals.
| id = db.Column(db.Integer, primary_key=True) | ||
| title = db.Column(db.String, nullable=False) | ||
| description = db.Column(db.String, nullable=False) | ||
| is_complete = db.Column(db.Boolean, default=False) |
There was a problem hiding this comment.
Consider not making an actual is_complete column. We can calculate whether the task is complete or not based on the completed_at timestamp. If we store redundant data, we run the risk of the two pieces of data getting out of sync (say we update one but forget to update the other).
| for id in request_body['task_ids']: | ||
| new_task = validate_task(id) | ||
| goal.tasks.append(new_task) |
There was a problem hiding this comment.
Nice use of the relationship to build the association between the tasks and the goal.
Note that as written, this would add the supplied tasks to any tasks already associated with the goal. The tests are ambiguous about whether we want that behavior, or whether the tasks should be replaced. Try thinking about what we would do if we wanted the replacement behavior.
| db.session.commit() | ||
| return {"id": goal.id, | ||
| "task_ids" : request_body['task_ids']} |
There was a problem hiding this comment.
Rather than echoing back the ids that came in the request id, consider loading the actual task ids from the goal. What if the goal had other associated tasks before adding this batch?
app/routes.py
Outdated
| text=f"Someone just completed {task.title}" | ||
| headers = {"Authorization": slack_bot_token} | ||
| slack_url = f'https://slack.com/api/chat.postMessage?channel={channel_name}&text={text}' |
There was a problem hiding this comment.
If using a POST, we should send the data to the endpoint using the request body rather than through query params. (Check out the requests.post data= or json= arguments).
app/routes.py
Outdated
| channel_name = 'test-channel' | ||
| text=f"Someone just completed {task.title}" | ||
| headers = {"Authorization": slack_bot_token} | ||
| slack_url = f'https://slack.com/api/chat.postMessage?channel={channel_name}&text={text}' | ||
|
|
||
| ## UNCOMMENT RESPONSE TO POST | ||
| #response = requests.post(slack_url, headers=headers) |
There was a problem hiding this comment.
Consider moving the logic here for messaging Slack into a helper method that just takes a message. THen we could call that method both from this route, as well as from the mark incomplete route.
app/routes.py
Outdated
| channel_name = 'test-channel' | ||
| text=f"Someone just completed {task.title}" | ||
| headers = {"Authorization": slack_bot_token} | ||
| slack_url = f'https://slack.com/api/chat.postMessage?channel={channel_name}&text={text}' | ||
|
|
||
| ## UNCOMMENT RESPONSE TO POST | ||
| #response = requests.post(slack_url, headers=headers) |
There was a problem hiding this comment.
Why is this line commented out? It looks like it should work!
No description provided.