#163383182: Users can comment on articles #31

pbkabali · 2019-02-11T11:49:49Z

What does this PR do?

create Comment and Reply models
Implement CRUD operations for comments and replies

Description of the task to be completed?

Create API endpoint for users to comment on an article.
Create API endpoint for users to create, delete and update a comment
Create API endpoint for users to reply to a comment.
Create API endpoint for users to create, delete and update a reply on a comment
It includes unit tests for the functionality

How should this be manually tested?

Visit the PR's app in the postman to view all comment or create a new comment for an article.
Get all comment for an article
Create a comment
To retrieve, update or delete a comment Visit the PR's app in the postman to view all articles or create a new article.
Update a comment
Delete a comment
Retrieve a comment.

What are the relevant pivotal tracker stories?

#163383182

pep8speaks · 2019-02-11T11:49:54Z

Hello @pbkabali! Thanks for updating the PR.

In the file authors/apps/authentication/views.py, following are the PEP8 issues :

Line 28:26: E275 missing whitespace after keyword
Line 183:56: W291 trailing whitespace
Line 202:37: E128 continuation line under-indented for visual indent
Line 223:57: W291 trailing whitespace
Line 224:77: W291 trailing whitespace
Line 225:79: W291 trailing whitespace
Line 226:76: W291 trailing whitespace
Line 253:64: W291 trailing whitespace

In the file authors/apps/comments/migrations/0001_initial.py, following are the PEP8 issues :

Line 20:80: E501 line too long (114 > 79 characters)
Line 24:80: E501 line too long (115 > 79 characters)
Line 25:80: E501 line too long (114 > 79 characters)
Line 34:80: E501 line too long (114 > 79 characters)
Line 38:80: E501 line too long (114 > 79 characters)
Line 39:80: E501 line too long (115 > 79 characters)

In the file authors/apps/comments/serializers.py, following are the PEP8 issues :

Line 8:68: W291 trailing whitespace

In the file authors/apps/comments/views.py, following are the PEP8 issues :

Line 42:13: E122 continuation line missing indentation or outdented
Line 60:21: E122 continuation line missing indentation or outdented
Line 67:70: W291 trailing whitespace
Line 89:21: E122 continuation line missing indentation or outdented
Line 106:21: E128 continuation line under-indented for visual indent
Line 149:13: E122 continuation line missing indentation or outdented
Line 168:17: E122 continuation line missing indentation or outdented
Line 175:75: W291 trailing whitespace
Line 197:21: E122 continuation line missing indentation or outdented
Line 213:80: E501 line too long (80 > 79 characters)

In the file authors/settings/base.py, following are the PEP8 issues :

Line 89:80: E501 line too long (91 > 79 characters)
Line 92:80: E501 line too long (81 > 79 characters)
Line 95:80: E501 line too long (82 > 79 characters)
Line 98:80: E501 line too long (83 > 79 characters)

Comment last updated on February 12, 2019 at 13:28 Hours UTC

fahdjamy

Please Install and follow pep8 standards @pbkabali.

missvicki · 2019-02-11T18:38:22Z

authors/apps/comments/migrations/0001_initial.py

+                'ordering': ['created_at'],
+            },
+        ),
+        migrations.CreateModel(


Your PR seems to have updated one of the previous migration files and this could break the code in staging and production. If you could please move the added migrations for comments to a new migration file that would be amazing.

@missvicki I did not touch any existing models. These are new models for comments and replies and should not affect any existing setup.

@missvicki, please clarify on this. I seem to agree with @pbkabali on this one.

authors/apps/comments/urls.py

karuhanga · 2019-02-12T07:09:43Z

authors/apps/comments/views.py

+    def get(self, request, slug):
+        """Retrieve all comments on an article"""
+        self.get_required_objects(request, slug)
+        comments = Comment.objects.filter(article=self.article)


Don't you think it would make sense to sort the comments by timestamp, given we shall be displaying them based on that order?

Yes, you are right @karuhanga . The ordering on the serializer depends on the created_at field

karuhanga · 2019-02-12T07:11:36Z

authors/apps/comments/views.py

+            data=self.data
+        )
+        serializer.is_valid(raise_exception=True)
+        serializer.save(


Thumbs up on using the serializer properly.

karuhanga · 2019-02-12T07:11:56Z

authors/apps/comments/views.py

+            data=self.data
+        )
+        serializer.is_valid(raise_exception=True)
+        serializer.save(


Thumbs up on using the serializer pattern the right way.

karuhanga

Very good work on this @pbkabali, especially on using the serializer pattern correctly, including the save.
I have put specific concerns in the comments, and would also suggest you look into drastically reducing the pep8 violations.

oma0256 · 2019-02-12T07:16:27Z

authors/apps/comments/views.py

+            'reply') if 'reply' in request.data else request.data
+
+    def get_author_profile(self, request):        
+        self.requester = Profile.objects.get(user=request.user)


@pbkabali I think you should use either a get_object_or_404 or a try and except just incase a Profile does not exist a get might throw an error if that profile does not exist

@oma0256 I get your point because the get endpoints do not need authentication. But also this function is not called on any get endpoint. So I think it should be fine

malep2007 · 2019-02-12T09:06:51Z

@pbkabali there are still PEP8 issues. Please rectify those ASAP and get back to me

malep2007 · 2019-02-12T10:44:21Z

Please Install and follow pep8 standards @pbkabali.

Was this done, @pbkabali?

fahdjamy

Thanks for implementing the requested changes.

-implement CRUD operations for comments on articles and replies to comments [Starts: #163383182]

karuhanga temporarily deployed to ah-backend-summer-stagin-pr-31 February 11, 2019 11:50 Inactive

pbkabali requested review from fahdjamy, codjoero, karuhanga, missvicki and oma0256 February 11, 2019 11:51

fahdjamy suggested changes Feb 11, 2019

View reviewed changes

missvicki reviewed Feb 11, 2019

View reviewed changes

authors/apps/comments/urls.py Outdated Show resolved Hide resolved

pbkabali force-pushed the ft-comment-on-article-163383182 branch from 7f97c89 to 18c6a9e Compare February 11, 2019 21:54

karuhanga temporarily deployed to ah-backend-summer-stagin-pr-31 February 11, 2019 21:55 Inactive

pbkabali changed the title ~~feat:users can comment on articles~~ #163383182: Users can comment on articles Feb 11, 2019

karuhanga reviewed Feb 12, 2019

View reviewed changes

authors/apps/comments/urls.py Outdated Show resolved Hide resolved

karuhanga reviewed Feb 12, 2019

View reviewed changes

karuhanga requested changes Feb 12, 2019

View reviewed changes

oma0256 suggested changes Feb 12, 2019

View reviewed changes

pbkabali force-pushed the ft-comment-on-article-163383182 branch from 18c6a9e to c0fb489 Compare February 12, 2019 11:41

karuhanga temporarily deployed to ah-backend-summer-stagin-pr-31 February 12, 2019 11:41 Inactive

fahdjamy approved these changes Feb 12, 2019

View reviewed changes

missvicki approved these changes Feb 12, 2019

View reviewed changes

oma0256 approved these changes Feb 12, 2019

View reviewed changes

karuhanga deployed to ah-backend-summer-stagin-pr-31 February 12, 2019 13:00 View deployment

pbkabali force-pushed the ft-comment-on-article-163383182 branch from c0fb489 to a88a357 Compare February 12, 2019 13:22

feat:users can comment on articles

216bf0a

-implement CRUD operations for comments on articles and replies to comments [Starts: #163383182]

pbkabali force-pushed the ft-comment-on-article-163383182 branch from a88a357 to 216bf0a Compare February 12, 2019 13:28

malep2007 merged commit 3f2347e into develop Feb 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#163383182: Users can comment on articles #31

#163383182: Users can comment on articles #31

pbkabali commented Feb 11, 2019 •

edited

Loading

pep8speaks commented Feb 11, 2019 •

edited

Loading

fahdjamy left a comment

missvicki Feb 11, 2019

pbkabali Feb 11, 2019

karuhanga Feb 12, 2019

karuhanga Feb 12, 2019 •

edited

Loading

pbkabali Feb 12, 2019

karuhanga Feb 12, 2019

pbkabali Feb 12, 2019

karuhanga Feb 12, 2019

karuhanga left a comment

oma0256 Feb 12, 2019

pbkabali Feb 12, 2019

malep2007 commented Feb 12, 2019

malep2007 commented Feb 12, 2019

fahdjamy left a comment

#163383182: Users can comment on articles #31

#163383182: Users can comment on articles #31

Conversation

pbkabali commented Feb 11, 2019 • edited Loading

What does this PR do?

Description of the task to be completed?

How should this be manually tested?

What are the relevant pivotal tracker stories?

pep8speaks commented Feb 11, 2019 • edited Loading

Comment last updated on February 12, 2019 at 13:28 Hours UTC

fahdjamy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karuhanga Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karuhanga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malep2007 commented Feb 12, 2019

malep2007 commented Feb 12, 2019

fahdjamy left a comment

Choose a reason for hiding this comment

pbkabali commented Feb 11, 2019 •

edited

Loading

pep8speaks commented Feb 11, 2019 •

edited

Loading

karuhanga Feb 12, 2019 •

edited

Loading