Sea Turtles - Christina W #100
Conversation
There was a problem hiding this comment.
Great job!
Your tests are all passing and your code is very readable.
Fantastic job pulling logic out of the routes into model methods and other helpers. 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!
Your heroku deployment also looks good!
Overall, nicely done!
| id = db.Column(db.Integer, primary_key=True) | ||
| title = db.Column(db.String) | ||
| description = db.Column(db.String) | ||
| 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).
| @@ -0,0 +1,40 @@ | |||
| """empty message | |||
There was a problem hiding this comment.
It looks like your migrations made it all the way through to project completion! 🎉
One thing that I sometimes like to do is actually intentionally flatten the migrations (dropping everything, the db as well as the migration files) and generating one fresh, clean migration. When starting a project, there can be a lot of churn in the data, which can make it difficult to follow what's going on in the migrations. This wouldn't be recommended once an app has been released for public use, but up until then, you're really free to do anything you want to the database and migrations!
Also, consider using the -m option when we run flask db migrate to add a message about what we're doing. It can make it easier to figure out what the migration files are doing later on.
| @@ -17,7 +17,7 @@ Mako==1.1.4 | |||
| MarkupSafe==1.1.1 | |||
| packaging==20.9 | |||
| pluggy==0.13.1 | |||
| psycopg2-binary==2.8.6 | |||
| psycopg2-binary==2.9.3 | |||
| def test_get_task_not_found(client): | ||
| # Act | ||
| response = client.get("/tasks/1") | ||
| response_body = response.get_json() | ||
|
|
||
| # Assert | ||
| assert response.status_code == 404 | ||
|
|
||
| raise Exception("Complete test with assertion about response body") | ||
| assert response_body == {"details":"No task with ID 1. SORRY."} |
There was a problem hiding this comment.
I feel like this is sort of a sorry, not sorry situation! 😆
| # ***************************************************************** | ||
| # **Complete test with assertion about response body*************** | ||
| # ***************************************************************** | ||
|
|
||
|
|
||
| @pytest.mark.skip(reason="No way to test this feature yet") | ||
| # @pytest.mark.skip(reason="No way to test this pytfeature yet") |
There was a problem hiding this comment.
Looks like your cursor was in this file and you tried to run pytest? 😄
| def send_slack_message(message): | ||
| Slack_Key = os.environ.get("Slack_API_Token") | ||
| requests.post('https://slack.com/api/chat.postMessage', params={'text':message, 'channel':'task-notifications'}, headers={'Authorization':Slack_Key}) | ||
| return True |
There was a problem hiding this comment.
👍 Nice use of a helper method.
Consider moving this into its own file/class. That would allow it to be used in a variety of situations.
Also, since we are making a POST request, we should send the request data in the request body. For that, we would want to use either the json= or data= named parameters.
| updated_task = task.mark_done() | ||
|
|
||
| db.session.commit() | ||
| send_slack_message(f"Someone just completed the task {task.title}") |
There was a problem hiding this comment.
👍 Having these helpers makes this so easy to read.
| def error_message(message, status_code): | ||
| abort(make_response(jsonify(dict(details=message)), status_code)) | ||
|
|
||
| def make_goal_safely(data_dict): | ||
| try: | ||
| return Goal.from_dict(data_dict) | ||
| except KeyError as err: | ||
| error_message(f"Invalid data", 400) | ||
|
|
||
| def update_goal_safely(goal, data_dict): | ||
| try: | ||
| return goal.replace_details(data_dict) | ||
| except KeyError as err: | ||
| error_message(f"Invalid data", 400) | ||
|
|
||
| def validate_goal_id(id): | ||
| try: | ||
| id = int(id) | ||
| except ValueError: | ||
| error_message(f"Invalid id {id}", 400) | ||
| goal = Goal.query.get(id) | ||
| if goal: | ||
| return goal | ||
| error_message(f"No goal with ID {id}. SORRY.", 404) |
There was a problem hiding this comment.
These are really close to the similar functions in the task routes. Think about how we could pull them into their own helper file and what we would need to do to make them work for more than one class.
| for task in request_body["task_ids"]: | ||
| validate_task_id(task) | ||
| data_dict["task_ids"].append(task) |
There was a problem hiding this comment.
👍 Nice validation that all of the supplied task ids were valid.
Consider using the name task_id for the loop variable instead. I was initially confused since it looked like you were storing fully resolved tasks in a list keyed by task_ids.
If you are doing the resolving here, you could consider reworking your helper function to not use a dictionary. Simply pass in the list. Otherwise, consider moving all of the logic (including the task id validation) into a separate function, or even into Goal. But you would want to use a different approach to validating the tasks (since we wouldn't want to use validate_task_id in Goal as it would introduce a Flask dependency in our model class).
| if task not in self.tasks: | ||
| self.tasks.append(task) |
There was a problem hiding this comment.
Appending the task to the list of tasks for this goal will have the effect of adding the supplied tasks to the goal. If the intent of this operation was to set the tasks for the goal to be exactly what was passed in, we would need to take a slightly different approach. The tests for how this should behave are ambiguous, but think about how we might approach this if we wanted the list of tasks to replace the current tasks associated with this goal.
No description provided.