Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive Task List API with Goal management functionality. The implementation includes full CRUD operations for both Tasks and Goals, establishes a one-to-many relationship between Goals and Tasks, and adds CORS support for cross-origin requests.
- Implements Task and Goal models with SQLAlchemy relationships
- Creates RESTful API endpoints for task and goal management including creation, retrieval, updates, and deletion
- Adds support for marking tasks as complete/incomplete with Slack notifications
- Enables test suite by uncommenting all skipped tests and completing incomplete test implementations
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| app/init.py | Initializes Flask app with CORS support and registers task/goal blueprints |
| app/models/task.py | Defines Task model with goal relationship and serialization methods |
| app/models/goal.py | Defines Goal model with tasks relationship and serialization methods |
| app/routes/task_routes.py | Implements RESTful endpoints for task CRUD operations |
| app/routes/goal_routes.py | Implements RESTful endpoints for goal CRUD operations and task associations |
| app/routes/route_utilities.py | Provides shared utility functions for validation, CRUD operations, and Slack notifications |
| migrations/versions/*.py | Database migration files creating tables and relationships |
| seed.py | Database seeding script with sample goals and tasks |
| requirements.txt | Adds flask-cors dependency |
| tests/test_wave_01.py | Uncomments and completes basic task functionality tests |
| tests/test_wave_02.py | Uncomments task sorting tests |
| tests/test_wave_03.py | Uncomments and completes task completion/incompletion tests |
| tests/test_wave_05.py | Uncomments and completes goal functionality tests |
| tests/test_wave_06.py | Uncomments and completes goal-task relationship tests |
| tests/test_wave_07.py | Uncomments and completes route utility tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def get_tasks_by_goal(goal_id): | ||
| goal = validate_model(Goal, goal_id) | ||
|
|
||
| return{ |
There was a problem hiding this comment.
Missing space between 'return' and the opening brace. Should be 'return {' for consistency with the rest of the codebase.
| return{ | |
| return { |
| for task in goal.tasks: | ||
| task.goal_id = None | ||
|
|
||
| #add new tasks |
There was a problem hiding this comment.
Comment is missing a space after the hash symbol. Should be '# add new tasks' for consistency with Python style guidelines.
| #add new tasks | |
| # add new tasks |
| def from_dict(cls, task_data): | ||
| new_task = cls(title=task_data['title']) | ||
|
|
||
| return new_task |
There was a problem hiding this comment.
Inconsistent naming: the parameter is named 'task_data' but this method creates a Goal object. It should be renamed to 'goal_data' for clarity.
| def from_dict(cls, task_data): | |
| new_task = cls(title=task_data['title']) | |
| return new_task | |
| def from_dict(cls, goal_data): | |
| new_goal = cls(title=goal_data['title']) | |
| return new_goal |
| def from_dict(cls, task_data): | ||
| new_task = cls(title=task_data['title']) | ||
|
|
||
| return new_task |
There was a problem hiding this comment.
Inconsistent naming: the variable is named 'new_task' but it's actually a Goal object. It should be renamed to 'new_goal' for clarity.
| def from_dict(cls, task_data): | |
| new_task = cls(title=task_data['title']) | |
| return new_task | |
| def from_dict(cls, goal_data): | |
| new_goal = cls(title=goal_data['title']) | |
| return new_goal |
| @pytest.mark.skip(reason="test to be completed by student") | ||
| #@pytest.mark.skip(reason="test to be completed by student") | ||
| def test_get_goal_not_found(client): | ||
| pass |
There was a problem hiding this comment.
The 'pass' statement on line 110 is unnecessary and should be removed since the function body has actual implementation below it.
| pass |
| "id": 1, | ||
| "title": "Updated Goal Title" | ||
| } | ||
| assert response_body["title"] == "Updated Goal Title" |
There was a problem hiding this comment.
Redundant assertion. Line 149 asserts response_body["title"] == "Updated Goal Title" which is already verified by the full dictionary comparison on lines 145-148.
| assert response_body["title"] == "Updated Goal Title" |
|
|
||
| return task_as_dict | ||
|
|
||
| #creates a task from a dictionary |
There was a problem hiding this comment.
Comment is missing a space after the hash symbol. Should be '# creates a task from a dictionary' for consistency with Python style guidelines.
| task_as_dict["goal_id"] = self.goal_id | ||
|
|
||
| if self.completed_at: | ||
| task_as_dict["completed_at"]= self.completed_at.isoformat() |
There was a problem hiding this comment.
Missing space after the equals sign. Should be 'task_as_dict["completed_at"] = self.completed_at.isoformat()' for consistent spacing with PEP 8 style guidelines.
| task_as_dict["completed_at"]= self.completed_at.isoformat() | |
| task_as_dict["completed_at"] = self.completed_at.isoformat() |
| from .route_utilities import * | ||
|
|
There was a problem hiding this comment.
Import pollutes the enclosing namespace, as the imported module app.routes.route_utilities does not define 'all'.
| from .route_utilities import * | |
| from .route_utilities import ( | |
| validate_model, | |
| db, | |
| create_model, | |
| get_all_items, | |
| get_one_item, | |
| update_entire_item, | |
| update_partial_item, | |
| mark_item_complete, | |
| mark_item_incomplete, | |
| delete_item, | |
| ) |
| from flask import Blueprint | ||
| from app.models.task import Task | ||
| from .route_utilities import * |
There was a problem hiding this comment.
Import pollutes the enclosing namespace, as the imported module app.routes.route_utilities does not define 'all'.
| from flask import Blueprint | |
| from app.models.task import Task | |
| from .route_utilities import * | |
| from flask import Blueprint, request, Response | |
| from app.models.task import Task | |
| from .route_utilities import ( | |
| create_model, | |
| get_all_items, | |
| get_one_item, | |
| update_entire_item, | |
| update_partial_item, | |
| mark_item_complete, | |
| mark_item_incomplete, | |
| delete_item, | |
| ) |
apradoada
left a comment
There was a problem hiding this comment.
GREEN
Great job, Bianca!
Overall, your models look good, your routes are solid and your helper functions are well written.
I think my biggest piece of advice here would be to take your helper functions and see if there are ways for you to make them more generic. Right now, they work for the two model system that you have and the attributes that exist for each model, but they start to fall apart as you add more models. If you want to revisit this project, I think that would be a great place to start!
If you have any questions, don't hesitate to reach out!
| if TYPE_CHECKING: | ||
| from .task import Task | ||
|
|
There was a problem hiding this comment.
While TYPE_CHECKING isn't necessarily required here because we have a one-to-many relationship, it is best practice and is necessary to avoid circular imports in a many-to-many relationship, so great job using it here.
Also, as stated in our Learn lessons, it does allow us to get rid of the yellow underline on line 11!
| def from_dict(cls, task_data): | ||
| new_task = cls(title=task_data['title']) | ||
|
|
||
| return new_task |
There was a problem hiding this comment.
You are currently in the Goal model but refer to task_data here. It would appear you copied and pasted from the Task model which is perfectly fine, but do make sure to make the appropriate changes!
| task_as_dict = {} | ||
| task_as_dict["id"]= self.id | ||
| task_as_dict["title"]= self.title | ||
| task_as_dict["description"]= self.description | ||
| task_as_dict["is_complete"]= self.completed_at is not None |
There was a problem hiding this comment.
This is a nice start, but it does start to get a little clunky. Feel free to favor building a dictionary directly:
| task_as_dict = {} | |
| task_as_dict["id"]= self.id | |
| task_as_dict["title"]= self.title | |
| task_as_dict["description"]= self.description | |
| task_as_dict["is_complete"]= self.completed_at is not None | |
| task_as_dict = { | |
| "id": self.id | |
| "title": self.title | |
| "description": self.description | |
| "is_complete": self.completed_at is not None | |
| } |
| else: | ||
| None |
There was a problem hiding this comment.
This else statement currently does not do anything, so it can be removed completely.
| db.session.add(new_task) | ||
| db.session.commit | ||
|
|
||
| return new_task.to_dict(), 200 |
There was a problem hiding this comment.
Flask returns status code 200 by default so we don't need to explicitly return it! We can go ahead and just return the new task's dictionary.
| def update_entire_item(cls, id, goal_update=False): | ||
| item = validate_model(cls, id) | ||
| request_body = request.get_json() | ||
|
|
||
| item.title = request_body["title"] | ||
| if not goal_update: | ||
| item.description = request_body["description"] | ||
| db.session.commit() | ||
|
|
||
| return item |
There was a problem hiding this comment.
Honestly, I'm consistently impressed by how your brain works with these types of problems. I was initially unsure of how this particular helper function would work, but you made it pretty flexible.
That being said, if we were to add more and more models, this would become less stable and would need to be tweaked in order to make sure it can apply to more models.
| if 'title' in request_body: | ||
| item.title = request_body['title'] | ||
|
|
||
| if 'description' in request_body: | ||
| item.description = request_body['description'] | ||
|
|
||
| if 'is_complete' in request_body: | ||
| if request_body['is_complete']: | ||
| item.completed_at = datetime.now() | ||
| else: | ||
| item.completed_at = None |
There was a problem hiding this comment.
The small issue I see here again is its scalability. If we had more models with different attributes, then we'll have to add more if statements which isn't entirely feasible. In its current form, it also lends itself to the issue of trying to access attributes that don't exist to different models.
| def mark_item_complete(cls, id): | ||
| item = validate_model(cls, id) | ||
|
|
||
| item.completed_at = datetime.now() | ||
|
|
||
| db.session.commit() | ||
|
|
||
| send_slack_notification(item.title) | ||
|
|
||
| return Response(status=204, mimetype="application/json") | ||
|
|
||
| def mark_item_incomplete(cls, id): | ||
| item = validate_model(cls, id) | ||
|
|
||
| item.completed_at = None | ||
|
|
||
| db.session.commit() | ||
|
|
||
| return Response(status=204, mimetype="application/json") | ||
|
|
||
| def delete_item(cls, id): | ||
| item = validate_model(cls, id) | ||
|
|
||
| db.session.delete(item) | ||
| db.session.commit() | ||
|
|
||
| return Response(status=204, mimetype="application/json") No newline at end of file |
|
|
||
|
|
||
|
|
||
|
No newline at end of file |
There was a problem hiding this comment.
Extra whitespace! We can get rid of this!
| from .routes.task_routes import bp as tasks_bp | ||
| from .routes.goal_routes import bp as goals_bp |
No description provided.