Accelerate - Janice Lichtman #7
Conversation
…net, get a specific planet
There was a problem hiding this comment.
Great job!
The provided tests and your additional tests are all passing. You did a really nice job refactoring the routes and helpers, and the code is very readable throughout.
You've done a great job of starting to address some of the repetitiveness of CRUD code with your helpers and json serialization code. There are a couple places where we can make even more progress, especially around error handling/validation. Making use of exceptions or decorators could lead to an even more streamlined implementation!
It's great to see the thought you put into the optional features you added.
Nice work!
| def create_app(test_config=None): | ||
| app = Flask(__name__) | ||
| app.url_map.strict_slashes = False |
There was a problem hiding this comment.
👍
Nice way to set this across all your routes!
| import app.routes.task_routes as task_routes | ||
| import app.routes.goal_routes as goal_routes |
There was a problem hiding this comment.
The as should only be required if we need to rename the module to something else (like to create an alias). We should be able to do either
import app.routes.task_routes
import app.routes.goal_routesor
from app.routes import task_routes
from app.routes import goal_routes| # This fixture creates 2 tasks with | ||
| # the same title | ||
| @pytest.fixture | ||
| def duplicate_title_tasks(app): |
There was a problem hiding this comment.
👍
Nice custom fixture to use for testing that your title filtering can properly return multiple results!
| @@ -3,4 +3,12 @@ | |||
|
|
|||
|
|
|||
| class Goal(db.Model): | |||
| goal_id = db.Column(db.Integer, primary_key=True) | |||
| id = db.Column(db.Integer, primary_key=True) | |||
| title = db.Column(db.String) | ||
| tasks = db.relationship('Task', backref='goal', lazy=True) | ||
|
|
||
| def to_json(self): |
There was a problem hiding this comment.
Great decision to bring this helper into the model!
| search_title_query = request.args.get("search_title") | ||
| if sort_query: | ||
| if sort_query == "asc": | ||
| tasks = Task.query.order_by(Task.title).all() |
There was a problem hiding this comment.
Rather than calling SqlAlchemy stuff directly from the routes, one thing we can consider is to make class helper methods to represent the operations we expect to perform. This forms a more self-documenting interface about what operations we expect to perform. This also follows the Law of Demeter.
We could make helper equivalents of things like get, order_by, filter_by, and all. Really, any of the query methods.
| @@ -0,0 +1,91 @@ | |||
| from app import db | |||
There was a problem hiding this comment.
👍
This all looks good, but consider how the feedback I provided on Task could apply here as well.
| from app.models.task import Task | ||
| import datetime | ||
|
|
||
| def test_create_task_with_valid_datetime_string(client): |
There was a problem hiding this comment.
👍
Nice coverage for the date format tests
| assert response.status_code == 400 | ||
| assert response_text == "ASC is not a valid sort parameter. Please use sort=asc or sort=desc." | ||
|
|
||
| def test_task_to_json_no_goal(one_task): |
There was a problem hiding this comment.
👍
Nice unit tests for your task serialization code.
| assert task_json["is_complete"] == task.is_complete() | ||
| assert task_json.get("goal_id") == task.goal_id | ||
|
|
||
| def test_get_tasks_search_by_title_that_exists(client, three_tasks): |
There was a problem hiding this comment.
👍
Nice test coverage for title filtering
| task = Task.query.get(id) | ||
| goal_response.tasks.append(task) | ||
|
|
||
| db.session.add(goal_response) |
There was a problem hiding this comment.
Since goal_response came from the DB already, we don't need to add it into the session. We should also be sure to validate the lookup of the individual tasks. What if one of them couldn't be found?
Also, what would happen if the goal previously had some tasks set. Do we want to add the new tasks to the existing tasks? Do we want to replace them and sever any prior task → goal relationships? What behavior is implemented here?
No description provided.