Pine - Mac P. #71
Pine - Mac P. #71
Conversation
…ng or descending order
…ds patch request to mark a task as complete or incomplete
…en a patch request is sent to tasks/<task_id>/mark_complete. NOT WORKING YET
…e a one to many relationship
…est. Returns Goal ID and task IDs
…al object with list of dictionaries for related tasks
There was a problem hiding this comment.
Awesome job Mac! This is a huge project and it looks to all be working! You've got some really excellent docstrings and well-organized code. There's some more detailed feedback and suggestions for next time in the line-byline comments, but this project is definitely green! 🎉
| def valid_id(model, id): | ||
| """If ID is an int, returns the model object with that ID. | ||
| If ID is not an int, returns 400. | ||
| If model object with that ID doesn't exist, returns 404.""" | ||
| try: | ||
| id = int(id) | ||
| except: | ||
| abort(400, {"error": "invalid id"}) | ||
| return model.query.get_or_404(id) |
There was a problem hiding this comment.
Very cool method with great docstring! However, it's duplicated across both of your routes files. Consider adding an additional file, perhaps named validation.py where this function can live. Then each of your routes files can do from validation import valid_id.
| from app.models.task import Task | ||
| from dotenv import load_dotenv | ||
|
|
||
| load_dotenv() |
There was a problem hiding this comment.
We don't get the value of any environment variables in this file, so we don't need this line.
|
|
||
| @goals_bp.route("", methods=["POST", "GET"]) | ||
| def handle_goals(): | ||
| """Handles post and get requests for /goals""" |
There was a problem hiding this comment.
Nice docstring here and throughout all your code
|
|
||
| if request.method == "GET": | ||
| goals = Goal.query.all() | ||
| goals_response = [goal.to_dict() for goal in goals] |
| """Handles get, put, and delete requests for one goal \ | ||
| with a given id at goals/<goal_id>""" | ||
| goal_dict = {} | ||
| goal = valid_id(Goal, goal_id) |
| for task in tasks: | ||
| tasks_response.append(task.to_dict()) |
There was a problem hiding this comment.
This definitely works well! Can you think of what this would look like as a list comprehension instead. Either is good, it's just a matter of personal preference~
| tasks_dict = {} | ||
| tasks_dict["task"] = new_task.to_dict() |
There was a problem hiding this comment.
You could consider combining these two lines into one:
tasks_dict = {
"task": new_task.to_dict()
}
| SLACK_BOT_TOKEN = os.environ.get("SLACK_BOT_TOKEN") | ||
| SLACK_CHANNEL = os.environ.get("SLACK_CHANNEL") |
There was a problem hiding this comment.
Great variable names! And I really like that the channel is configurable via environment variables as well.
You might consider putting the os.environ.gets somewhere else. Right now, your code re-reads the env vars every time a task is completed. It might be better to read these variables just once when the app starts up, similar to what happens for the DB connection string in app/init.py
| "Authorization": f"Bearer {SLACK_BOT_TOKEN}" | ||
| } | ||
| # response variable left for debugging purposes | ||
| response = requests.post(url, data=query_params, headers=headers) |
There was a problem hiding this comment.
Nice post! An extension that we could do if this were a full production application would be to check the response code that we get back. If it's some error code, we could raise some exception for our application to handle.
| assert response.status_code == 200 | ||
| assert "goal" in response_body | ||
| assert response_body == { | ||
| "goal": { | ||
| "id": 1, | ||
| "title": "Updated Goal Title" | ||
| } | ||
| } | ||
| goal = Goal.query.get(1) | ||
| assert goal.title == "Updated Goal Title" |
There was a problem hiding this comment.
Nice job asserting both the values of the response and what's stored in the database.
Would love tips with refactoring and adding code to check for errors/invalid data.