-
Notifications
You must be signed in to change notification settings - Fork 52
Feature/weekly tasks #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
Feature/weekly tasks #146
Conversation
...rs and basic functions for- edit, remove and add + demo functions for demo operations. added basic pages - add_weekly_task.html, weekly_tasks_manager.html
...ted all the demo functions. Arrange the code to be clearer and shorter.
...ode. and added wekly_tasks.py on the intertnal directory
... router, temporary
...nction and where it needs to be placed
Codecov Report
@@ Coverage Diff @@ ## develop #146 +/- ## =========================================== + Coverage 95.81% 96.01% +0.20% =========================================== Files 75 77 +2 Lines 3343 3515 +172 =========================================== + Hits 3203 3375 +172 Misses 140 140
Continue to review full report at Codecov.
|
Great work! I've added few of my insights :)
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.
Awesome work! Let's make another little push :)
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 improvement!
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! Maybe we should use it in more tests?
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.
the other functions (internal) are not client related functions
so it seems I can't use it on them.
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.
it seems the comment about the 'Depends(get_db)' encounters the same problem
the Depends is of no use if the function is not a router function.
I did not find a way to use it within a standard non-fastapi function
the internal functions are used by the routers functions which uses Depends
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 split the tests into two files: internal tests and route tests.
Tell me what you think about the rearrangement and my comments :)
app/routers/weekly_tasks.py
Outdated
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.
Deduplicate and move outside
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.
different functions, in the next commits I will change the names and variables to make it clearer
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.
changed the names so it will be more clear, please look again, tell me what you think :)
app/routers/weekly_tasks.py
Outdated
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.
Remove
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.
changed the names so it will be more clear, please look again, tell me what you think :)
Great work, but we still have a little way to go :)
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 progress. I love the expansion of the tests.
I've added few insights, please take a look :)
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 job!
app/database/models.py
Outdated
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.
Try to give more specific variable names.
is it the creation date, or maybe the deadline, it is hard to tell.
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 should not mess with this model, its not mine
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.
Split to functions
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 did not understand
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.
Add is_logged_in to the router's dependencies instead
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 realized that the function is meant to be used in this way as well.
In case I do not need to use the variable it return's inside the router.
This reverts commit dab05ee.
Added tables, routers, tests, templates, and internal functions
The fitter gives the option to set weekly tasks.
Comes with a page for managing weekly tasks and another page for editing the task.
Gives the ability to edit, add and delete a weekly task.
Along with this in the feature is included a function for creating the tasks - which aims to create tasks every week for the user (according to the definition of his weekly tasks)