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.

Sign up for GitHub

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

Jump to bottom

#163383200 Users should see their reading stats #45

Merged
merged 1 commit into from
Feb 21, 2019
Merged

#163383200 Users should see their reading stats #45

merged 1 commit into from
Feb 21, 2019

Conversation

codjoero
Copy link
Contributor

@codjoero codjoero commented Feb 19, 2019 •
edited
Loading

What does this PR do?

This PR enables a user to be able to see their reading stats
This task should also allow an author to see the reading stats for each of their article

Description of Task to be completed?

  • add a model for a user'reading stats
  • add functionality for when a user reads article.
  • add a read_stats field to handle the data for the number of times an article has been read.
  • add view functionality to handle the endpoint that retrieves the read stats.

How should this be manually tested?

  • Visit the PR's app with postman.

Screenshots

Get read stats for a user /api/v1/profiles/user/readstats
image

Reading an article /api/v1/articles/<slug>
image

Author get their article /api/v1/articles/<slug>
image

Related story

163383200

@pep8speaks
Copy link

pep8speaks commented Feb 19, 2019 •
edited
Loading

Hello @codjoero! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on February 21, 2019 at 12:23 Hours UTC

@karuhanga karuhanga temporarily deployed to ah-backend-summer-stagin-pr-45 February 19, 2019 20:42 Inactive
@karuhanga karuhanga temporarily deployed to ah-backend-summer-stagin-pr-45 February 19, 2019 20:46 Inactive
missvicki
missvicki approved these changes Feb 20, 2019
View reviewed changes
Copy link
Contributor

@missvicki missvicki 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 explaining this to me, I understand it a little bit better. Nice work you did here, I appreciate that you went ahead and implemented authors being able to see read stats of their articles.

karuhanga
karuhanga requested changes Feb 20, 2019
View reviewed changes
Copy link
Collaborator

@karuhanga karuhanga 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 on this @codjoero.
I have left specific comments below.

authors/apps/articles/views.py
}, status=status.HTTP_404_NOT_FOUND)
serialized_data = self.serializer_class(article,
context=context)
if not request.user.is_anonymous and\
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments here would be helpful @codjoero

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karuhanga thanks for the suggestion, I went ahead and added some comments where I thought the code was abit complex.

authors/apps/profiles/urls.py Outdated
path("", UserProfileListAPIView.as_view(), name="profile-list"),
path("", UserProfileListAPIView.as_view(),
name="profile-list"),
path('<username>/readstats', ReadStatsView.as_view(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to specify a username if the user is already logged in, and from what I understand, a user can only get their stats?

Copy link
Contributor Author

@codjoero codjoero Feb 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karuhanga As you might notice if you read the API spec, GET /profiles/:username would try to fetch you the profile of <username> hence the use of GET /profiles/:username/readstats to fetch the user's reading stats.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I have noticed that. But in this case one cannot retrieve another user's reading stats the way they can retrieve their profile. In light of this therefore, I do not see the need to require the API consumer to supply a user for a user that is already logged in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karuhanga , thanks for the feedback, the issue has been resolved

authors/apps/articles/models.py Outdated
@@ -60,6 +60,7 @@ class Article(models.Model):
disliked_by = models.ManyToManyField(to=settings.AUTH_USER_MODEL,
related_name='disliked_articles',
related_query_name='disliked_article')
read_stats = models.IntegerField(default=0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to understand the use of this field, and where exactly it is being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karuhanga , thanks for the catch, had planned to use it to store the Article readstats before i changed to a MethodSerializer.

@karuhanga karuhanga temporarily deployed to ah-backend-summer-stagin-pr-45 February 20, 2019 19:30 Inactive
@pbkabali
Copy link
Contributor

Great work here @codjoero. Your code is easy to follow and understand.

@karuhanga karuhanga temporarily deployed to ah-backend-summer-stagin-pr-45 February 21, 2019 02:53 Inactive
@malep2007
Copy link
Contributor

@codjoero ,please resolve merge conflicts.

@karuhanga karuhanga temporarily deployed to ah-backend-summer-stagin-pr-45 February 21, 2019 07:12 Inactive
@karuhanga karuhanga temporarily deployed to ah-backend-summer-stagin-pr-45 February 21, 2019 10:00 Inactive
@malep2007
Copy link
Contributor

@codjoero some merge conflits

@karuhanga karuhanga temporarily deployed to ah-backend-summer-stagin-pr-45 February 21, 2019 11:47 Inactive
@codjoero
feat(reading stats): Users should see their reading stats
fc21eb9 
- add a model for the stats
- add the ReadStatsSerializer class to handle the model information
- add the view endpoint for all read stats
- add functionality to get the data from the read articles
- update the urls to add the stats endpoint
- write tests for the functionality

[Starts #163383200]
@malep2007 malep2007 merged commit 1a63b96 into develop Feb 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karuhanga karuhanga karuhanga requested changes

@malep2007 malep2007 malep2007 approved these changes

@fahdjamy fahdjamy fahdjamy approved these changes

@missvicki missvicki missvicki approved these changes

@pbkabali pbkabali pbkabali approved these changes

@oma0256 oma0256 Awaiting requested review from oma0256

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@codjoero @pep8speaks @pbkabali @malep2007 @karuhanga @fahdjamy @missvicki