-
Notifications
You must be signed in to change notification settings - Fork 446
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
feat: Add /home API for user stats #158
Conversation
c3e24c8
to
19fba9f
Compare
I have now added all the required changes. The /home endpoint now returns the following details:
I have also added tests for this endpoint. Please review soon :) EDIT: Whoops, been spotting a lot of last minute issues, sorry for this messy PR. It's all done now, hopefully. |
19fba9f
to
772fe91
Compare
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.
@haroldadmin you did a great job with this PR. I just have some small change requests. Can you do the following:
- create tests for DAO functions you created (the API tests are really complete)
- add some doc strings to DAO functions according to this style guide for doc strings in python by Google
- Don't forget to squash commits at the end.
772fe91
to
9d356ac
Compare
@isabelcosta @ramitsawhney27 I have added the requested changes in the latest commit. |
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.
Thank you for addressing the changes!
I requested some changes to the doc strings and one little thing in the API tests.
Once you address them I'll wait some time to see if there are other views on this PR and then I can merge this :)
d90c1fd
to
1517a4c
Compare
All requested changes added. Thank you for helping me @isabelcosta @ramitsawhney27 @m-murad Your feedback on this PR would be very welcome. |
1517a4c
to
7147e03
Compare
I approved this, I will wait for other PRs on the backend to be ready for merge so then I can merge more than one PR, since these will reset the current DB |
@haroldadmin left some comments from unnecessary blank lines you had. Could you please remove those? |
@isabelcosta PEP8 style guidelines for Python recommend having a blank line at the end of the file, so I added them there. The reason is that many text parsing tools like grep rely on a newline character to demarcate the end of a line. You can find more details in the discussion on this page. |
@haroldadmin I asked you to remove blank lines you have in the middle of the file, not in the end of the file. I agree with the ones in the end of the file, you some in the middle of the test definitions |
Oh, got it. I'll remove them. |
7147e03
to
3105c38
Compare
...And it's done! I've added the changes @isabelcosta EDIT: I don't know what's going on with Travis. Both the failing tests on Travis pass on my system. I ran the tests before pushing the commit. Also, the Travis error is a very weird one: name datetime is not defined, but clearly datetime is imported at that the top. I'll try pushing the same commit to trigger another build, maybe it will give me more information to work with. |
3105c38
to
b665e44
Compare
This commit adds a /home endpoint which returns statistics regarding the current user. Fixes Issue anitab-org#120. Also adds tests for these changes.
b665e44
to
f71ac2b
Compare
@isabelcosta All done now, hopefully. Please let me know if any further changes are required. |
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.
Good work @haroldadmin I don't have anything else to point out.
@isabelcosta Can this be merged now? I want to work on the Homescreen for the Android app, but this PR needs to be merged for that. |
@haroldadmin later tonight (my timezone) I can merge yes. I was waiting for a PR to be ready for merge. But I'll move on with this one and another one. |
@haroldadmin this PR was merged, but you did something here that flask-restplus does not understand, so the Swagger UI is not being generated. I think we don't need to revert this PR, because it still works on the Android client, but this has to be fixed for people who use the backend. I'll take a look at this, but if you could do that too it would be great. As you can see here (http:https://systers-mentorship-dev.eu-central-1.elasticbeanstalk.com/) Swagger UI does not show up. When adding an API, always to test it on a browser going to http:https://localhost:5000 as see if the Swagger UI is generated. |
This commit adds a /home endpoint which returns statistics regarding the
current user. Fixes Issue #120. Tests have been added as well.
Description
I have added an endpoint in the User Resource. This endpoint triggers a scan of all the relations of the user, and counts the number of these relations in all of the possible states. It then returns a dictionary containing these counted values
Fixes #120
Type of Change:
Delete irrelevant options.
Code/Quality Assurance Only
How Has This Been Tested?
I have tested the API using postman, and created my own unit tests as well.
Checklist:
Delete irrelevant options.
Code/Quality Assurance Only