Skip to content
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

Merged
merged 1 commit into from
Jan 3, 2019

Conversation

haroldadmin
Copy link
Contributor

@haroldadmin haroldadmin commented Dec 18, 2018

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

Code/Quality Assurance Only

  • New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

I have tested the API using postman, and created my own unit tests as well.

Checklist:

Delete irrelevant options.

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have commented my code or provided relevant documentation, particularly in hard-to-understand areas

Code/Quality Assurance Only

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@haroldadmin
Copy link
Contributor Author

haroldadmin commented Dec 20, 2018

I have now added all the required changes. The /home endpoint now returns the following details:

  1. Number of requests and relations
  2. User's name which will used to display in the welcome text on the Android app homescreen
  3. A max of 3 recent achievements of the user

I have also added tests for this endpoint. Please review soon :)
If this PR gets merged, then I would also like to work on using this API endpoint in the Android app to create the homescreen (if the issue is still available)

EDIT: Whoops, been spotting a lot of last minute issues, sorry for this messy PR. It's all done now, hopefully.

app/api/dao/user.py Outdated Show resolved Hide resolved
Copy link
Member

@isabelcosta isabelcosta left a 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:

app/api/dao/user.py Outdated Show resolved Hide resolved
app/api/dao/user.py Outdated Show resolved Hide resolved
app/api/dao/user.py Outdated Show resolved Hide resolved
app/api/resources/user.py Outdated Show resolved Hide resolved
app/api/resources/user.py Outdated Show resolved Hide resolved
@haroldadmin
Copy link
Contributor Author

@isabelcosta @ramitsawhney27 I have added the requested changes in the latest commit.

Copy link
Member

@isabelcosta isabelcosta left a 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 :)

app/api/dao/user.py Outdated Show resolved Hide resolved
app/api/dao/user.py Outdated Show resolved Hide resolved
app/api/resources/user.py Outdated Show resolved Hide resolved
tests/users/test_api_home_statistics.py Show resolved Hide resolved
@haroldadmin haroldadmin force-pushed the home-api branch 2 times, most recently from d90c1fd to 1517a4c Compare December 27, 2018 03:49
@haroldadmin
Copy link
Contributor Author

All requested changes added. Thank you for helping me @isabelcosta
For the failing codacy check, I've just removed the named arguments syntax from the DAO functions. This prevents the appearance of the word 'password' in there. This change should hopefully make the check pass.

@ramitsawhney27 @m-murad Your feedback on this PR would be very welcome.

isabelcosta
isabelcosta previously approved these changes Dec 27, 2018
@isabelcosta
Copy link
Member

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

@isabelcosta
Copy link
Member

@haroldadmin left some comments from unnecessary blank lines you had. Could you please remove those?

@haroldadmin
Copy link
Contributor Author

haroldadmin commented Dec 29, 2018

@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.

@isabelcosta
Copy link
Member

@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

@haroldadmin
Copy link
Contributor Author

Oh, got it. I'll remove them.

@haroldadmin
Copy link
Contributor Author

haroldadmin commented Dec 29, 2018

...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.

This commit adds a /home endpoint which returns statistics regarding the
current user. Fixes Issue anitab-org#120. Also adds tests for these changes.
@haroldadmin
Copy link
Contributor Author

@isabelcosta All done now, hopefully. Please let me know if any further changes are required.

Copy link
Member

@isabelcosta isabelcosta left a 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.

@haroldadmin
Copy link
Contributor Author

@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.

@isabelcosta
Copy link
Member

@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.

@isabelcosta isabelcosta merged commit 2b8395a into anitab-org:develop Jan 3, 2019
@isabelcosta
Copy link
Member

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create GET /home API to get some statistics about the user usage
3 participants