-
Notifications
You must be signed in to change notification settings - Fork 146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TaskListProject_Cheetahs_Tapasya #118
base: master
Are you sure you want to change the base?
Conversation
… case, passed all tests.
…and passed all tests
…s and passed tests
…te if its not None.
…te if its not None.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work Tapasya! You hit the learning goals here. I especially like your tests for query params. Well done. I left some minor comments, but this is a solid solution. Well done.
try: | ||
new_goal = Goal.from_dict(request_body) | ||
except KeyError: | ||
return jsonify ({"details": "Invalid data"}), 400 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work with try-except, but it would be good to provide the user information about what data is invalid.
def get_all_goals(): | ||
all_goals = Goal.query.all() | ||
|
||
result = [item.to_dict() for item in all_goals] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice use of a list comprehension & helper method.
try: | ||
update_goal.title = request_body["title"] | ||
except KeyError: | ||
return jsonify({"msg": "Missing needed data"}), 400 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another similar try-except block.
|
||
|
||
|
||
|
||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def to_dict(self): | ||
return { | ||
"id": self.goal_id, | ||
"title": self.title | ||
} | ||
|
||
def to_dict_incl_tasks(self): | ||
tasks = self.get_task_items() | ||
|
||
return { | ||
"id": self.goal_id, | ||
"title": self.title, | ||
"tasks": tasks | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These seem redundant
if "sort"in request.args or "SORT" in request.args: | ||
sort_query_val = request.args.get("sort") if "sort"in request.args else \ | ||
request.args.get("SORT") | ||
|
||
if sort_query_val.lower() == "asc": | ||
tasks = Task.query.order_by(Task.title).all() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work with query params.
|
||
elif sort_query_val.lower() == "desc": | ||
tasks = Task.query.order_by(Task.title.desc()).all() | ||
# Source: https://stackoverflow.com/questions/4186062/sqlalchemy-order-by-descending |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice documentation!
def read_task_by_id(task_id): | ||
task = validate_model(Task, task_id) | ||
if task.goal_id is None: | ||
return jsonify({"task":task.to_dict()}), 200 | ||
return jsonify({"task":task.to_dict_incl_goal_id()}), 200 | ||
|
||
#Helper function for use in READ route: read_task_by_id and UPDATE route: update_task | ||
def validate_model(cls, id): | ||
try: | ||
model_id = int(id) | ||
except: | ||
abort(make_response(jsonify({"msg":f"invalid id: {model_id}"}), 400)) | ||
|
||
chosen_object = cls.query.get(model_id) | ||
|
||
if not chosen_object: | ||
abort(make_response(jsonify({"msg": f"No {cls.__name__.lower()} found with given id: {model_id}"}), 404)) | ||
|
||
return chosen_object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great helper functions
}}), 200 | ||
|
||
#UPDATE Routes (Wave 3: PATCH Routes) | ||
@tasks_bp.route("/<task_id>/<mark>", methods=["PATCH"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how you worked the "mark" into the route.
@@ -27,9 +27,85 @@ def test_get_tasks_sorted_asc(client, three_tasks): | |||
"description": "", | |||
"is_complete": False} | |||
] | |||
#@pytest.mark.skip(reason="No way to test this feature yet") | |||
def test_get_tasks_sorted_asc_ignore_case_SORT(client, three_tasks): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice test!
No description provided.