Conversation
…delete_task, test_delete_task_not_found all completed and passed.
…e_model, get_model_with_filters, added goals and goal descriptions, all of wave 5 tests have passed, added slack post api functionality to the mark complete function of tasks.
…. Currently working on getting correct response body for the final two tests in wave 6
… entering wave 7 now, will refactor later if time permits
Madeline/backup branch
… to use utilities function
… will now test externally
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Looks good overall. Some tests were modified to account for the response approach taken, but we should leave them as is and make changes to the response approach. Please restore the tests back to their original version, and get them to pass by modifying your route logic. Review my comments, and let me know if you have any questions.
edits included: -- fix goals to_dict/model so new keys are not added incorrectly. -- added error handeling -- fixed typos -- corrected errors in wave 5 of tests to ensure project aligns with readme expectations. -- fixed date-time issue by setting datetime to always get current utc timezone to prevent issues for users in multiple timezones. -- fixed slack message functions so they align with slack documentation standards.
|
@anselrognlie I apologize for the delay, it took a bit longer to get to as the original computer that I had vscode installed on died during capstone and I had to do install fest on my new device which took additional time. I believe I was able to address all of your concerns in the most recent update. Happy to do another pull request if you would like. |
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
👍 Your updates look good. Just a few recommendations mostly around using the utility functions.
| def check_for_completion(self): | ||
| if self.completed_at is not None: | ||
| return True | ||
| else : | ||
| return False No newline at end of file |
There was a problem hiding this comment.
If we're passing self, prefer this to be an actual instnce method (indent it into the class).
Nit: no space between else and :
def check_for_completion(self):
if self.completed_at is not None:
return True
else:
return False| "id": self.id, | ||
| "title": self.title, | ||
| "description": self.description, | ||
| "is_complete": check_for_completion(self) |
There was a problem hiding this comment.
If check_for_completion is an instance method, call it as
"is_complete": self.check_for_completion()Python will shuffle things around internally.
| from app.models.task import Task | ||
| from ..db import db | ||
| from datetime import datetime | ||
| from app.routes.utilities_routes import * |
There was a problem hiding this comment.
Prefer to explicitly list the symbols being imported rather than using import *
| goal = create_model(Goal, request_body) | ||
| return make_response(goal, 201) |
There was a problem hiding this comment.
With the create_model refactor, create_model is already creating an appropriate response, so we can return it as
return create_model(Goal, request_body)| if not goals: | ||
| return make_response([], 200) |
There was a problem hiding this comment.
This check is redundant. goals will alreayd be an empty list is nothing was found, so the return on line 58 willwork identically.
| except: | ||
| return make_response({"details": f"Goal {goal_id} not found"}, 404) |
There was a problem hiding this comment.
The try/except his unnecessary. validate_model will already take care of generating the 404 response and halting further processing of the request.
2 participants
No description provided.