Add reactions #10

abhiabhi94 · 2020-05-08T22:21:05Z

Add reactions to comments.

Tasks:

The reaction process currently looks something like this:

Fixes #8, maybe we can move adding flag to a separate issue.

Make comment models a python package - make other necessary changes to imports everywhere - move code for manager to manager module Add reaction models - add some general property functions to be useful in future Reorder some import to conform to PEP8

Radi85

Hey,

Many thanks for you efforts.

I would separate show reacted user feature in a different issue so for now just ignore it.

Please consider PEP 8 compatibility and ignore typing for now.

comment/admin.py

comment/models/reactions.py

- likes and dislikes are now part of a column. - add functions to update reactions and their count. Make changes to the ReactionInstance model - reaction choices now uses IntegerChoices which is similar to IntEnum Add the reaction model and reaction instance model to the admin Change import of comment model - it is directly imported from the model package rather than going inside the comments module.

comment/admin.py

comment/migrations/0004_reaction_reactioninstance.py

comment/models/comments.py

comment/models/reactions.py

- all comment models are now imported in the __init__ file

Changes made to Reaction model - Reaction choices are now made from IntEnum, since models.TextChoices is only supported in Django 3.0 - related_name for foreign keys made plural - move logic for set_reaction to ReactionInstance model Changes to ReactionInstance - add function to override save function. - add a function to be executed on pre-delete signal. - set_reaction method is now a classmethod. - clean up set_reaction and refactor it. Changes to Comment model - add property for showing like and dislike count. Add urlpattern for accepting requests for reaction. Changes to views - views is now a package, move earlier code to comments module inside it - add module for reactions Changes to templates - show like and dislike count alongside the comment content

Radi85

hey,
Thanks a lot. A lot of work has been done.
I would like to use icons instead, thumbs up and down, if your question is realted to the font, feel free to use any light weight version.
As suggested, please move reply, like and dislike to the second row.
Unfortunately I have no more time but I would like to get back to review the models again, nice work!

comment/models/reactions.py

comment/models/comments.py

comment/api/urls.py

Radi85 · 2020-05-12T07:26:45Z

Btw, the work you have already done, could be splitted to more features :D
I will have a look at the tasks a bit later.

comment/views/reactions.py

comment/models/reactions.py

Add svg icons for like and dislike - fill them if the logged in user has liked or disliked a comment - add templatetags that make this feature possible Add post save signal to Comment model - Reaction instance is created when a new comment is created. Changes in ReactionInstance model - change related name for user attribute -> 'reactions' seems more natural.

Radi85 · 2020-05-13T06:59:17Z

Well done so far. I like how it looks like.

Other changes to comply with flake8

- Reaction icon's color and count are updated. Changes in reaction views - View all returns latest reaction count for the updated reaction -> Updates reaction count as per the last HTTP request Changes to comment content template - Remove block for like and dislike icon colors -> the color values are used in javascript too. Obtaining them to javascript from the template would require a hack. Can be done in the future. - Make or remove element properties as per changes in the javascript.

- cite was probably mistakenly used here.

comment/templates/comment/content.html

abhiabhi94 · 2020-05-13T22:28:21Z

Hey, currently the reaction thing works well for authenticated users.

For unauthenticated users, I was thinking of following this workflow

Inside the template, add an attribute to the anchor element and set the href attribute accordingly. Then inside the javascript, when the onclick event is triggered, I could check if the attribute is set, redirect them to the login page.
- An issue here is the next path to the login URL in django redirects through GET requests only?(I may be wrong here). Currently, I had allowed only POST requests for accepting reactions. Should I change the request method there?
How should we handle oauth?

Changes in view handling reaction - bad react requests are handled now

- they were anyway used outside the class Add tests for unauthenticated users for the has_reacted template tag - mock an unauthenticated user for the template -> complete coverage for template tags.

Radi85 · 2020-05-14T07:02:25Z

For unauthenticated users, I was thinking of following this workflow

To avoid this complex, please use a simple if statement inside the template:

{% if user.is_authenticated %}
 a_tag
{% else %}
div_tag
{% endif %}

How should we handle oauth?

I believe django will authenticate the user when using oauth, so you have nothing to do here.

comment/models/reactions.py

comment/templates/comment/content.html

comment/templatetags/comment_tags.py

abhiabhi94 · 2020-05-14T21:21:17Z

To avoid this complex, please use a simple if statement inside the template:

How will this help in handling the event when triggered through javascript?

I believe django will authenticate the user when using oauth, so you have nothing to do here.

I was concerned about redirecting the user to the like link for the comment. How do we do that?

Radi85 · 2020-05-15T09:10:57Z

How will this help in handling the event when triggered through javascript?

Javascript fucntion is triggered here:

 $(document).on("click", ".js-comment-reaction", commentReact);

Add this class to html tag only if the user is authenticated:

<small class="{% if user.is_authenticated %}js-comment-like {% endif %} {% block like_link_cls %} btn btn-link" {% endblock like_link_cls %} href="{% url 'comment:react' comment_id=obj.id reaction='like' %}">{% trans "Like" %}</small>

I was concerned about redirecting the user to the like link for the comment. How do we do that?

I am not sure what redirecting the user to the like link for the comment means. Our app is associated with a given model and our urls are related to that model's urls. I believe this redirecting part shall be handeled by the developer who uses our app.

abhiabhi94 · 2020-05-15T11:10:39Z

I am not sure what redirecting the user to the like link for the comment means. Our app is associated with a given model and our urls are related to that model's urls. I believe this redirecting part shall be handeled by the developer who uses our app

Sorry, I didn't mention this explicitly, but I was talking about unauthenticated users. If they click on the like icon, we would want to redirect them to like URL after they are authenticated. For normal auth, we can put the link something like this /login?next=redirect_link but what to do in this case.

Also, how should we handle the response in this case(after performing the action on the redirected link)? Currently, I am returning JSON response which might not be an ideal scenario.

abhiabhi94 · 2020-05-15T14:45:57Z

We also need to add exception handling to the comment views.
Testing your comment views I found that exception handling has not been handled if the URLs are accessed directly through the browser without form attributes or other values.

- move reactions part to a separate template - add translation capabilities to this template. Create new templates for reactions templates - handles url for unauthenticated users. - like and dislike icon moved to a separate template. Remove Reaction manager - it wasn't used anywhere in the application. Change request method for reaction - GET allows redirect for unauthenticated users - make the corresponding change to the javascript function Changes in tests - add tests in models for post delete signal. - refactor code for view tests. - remove unnecessary part.

- it was mistakenly copied from the like icon Add an empty line at the end of the file

abhiabhi94 · 2020-05-16T11:51:56Z

I got one JS issue when running your code locally.

comment.js:238 Uncaught TypeError: Cannot read property 'style' of undefined
    at fillReaction (comment.js:238)
    at Object.commentReactionDone [as success] (comment.js:281)

The issue might be in this line:

if (colorIsNone($likeIcon.style.fill) && colorIsNone($dislikeIcon.style.fill)) {
     $targetReaction.style.fill = fillColor;
}

I have fixed it in the latest commit. It was caused when I was refactoring the templates. I had accidently forgot to change the icon class and filter to the dislike icon file after copying things from the like icon file.

abhiabhi94 · 2020-05-16T11:55:46Z

Any particular reason for this? Each individual commit generally has a reason and descriptive message that indicates the reasons for the change.

I would like to keep git tree clean and I prefer to have one commit per feature.

One feature has a lot of sub-features or substantial subtilities that are required for its implementation. I don't think you can put the whole thing in a single commit.

Anyways it is your call. I believe Github gives the owners/maintainers option to squash the commits while merging.

Radi85 · 2020-05-16T11:56:35Z

Unexpected behavior here ⬇️ like and dislike both are filled for the same user.

Radi85 · 2020-05-16T11:58:14Z

One feature has a lot of sub-features or substantial subtilities that are required for its implementation. I don't think you can put the whole thing in a single commit.

Anyways it is your call. I believe Github gives the owners/maintainers option to squash the commits while merging.

Don't worry about it then, I will take care of it.

abhiabhi94 · 2020-05-16T12:01:18Z

Unexpected behavior here arrow_down like and dislike both are filled for the same user.

Was this behaviour after you clicked on the icons or when the page was rendered?

abhiabhi94 · 2020-05-16T12:06:47Z

If you mean the migration file, yes please.

No the db.sqlite3 file.

Doesn't matter since it is used for testing purpose only.

Maybe we can put the file in .gitignore to stop it from being tracked, to stop it from causing issues with version control.

Radi85 · 2020-05-16T12:16:00Z

Was this behaviour after you clicked on the icons or when the page was rendered?

After rendreing the page.

Radi85 · 2020-05-16T12:20:37Z

Maybe we can put the file in .gitignore to stop it from being tracked, to stop it from causing issues with version control.

I actually keep this db for the demo project so who would like to use app, can directly find an example with less efforts.

abhiabhi94 · 2020-05-16T12:26:42Z

Was this behaviour after you clicked on the icons or when the page was rendered?

After rendreing the page.

Are you sure? This means the template filter is not working correctly then. Just to make sure, have you pulled in the latest commits?

abhiabhi94 · 2020-05-16T13:13:55Z

I forgot to mention the icons were picked from https://feathericons.com. Please don't forget to give them credit in the documentation.

Radi85 · 2020-05-16T13:26:53Z

I forgot to mention the icons were picked from https://feathericons.com. Please don't forget to give them credit in the documentation.

Thanks for mentioning. Just small thing regarding the icons, could you please make them with the count text a little bit small?

abhiabhi94 · 2020-05-16T13:47:28Z

I forgot to mention the icons were picked from https://feathericons.com. Please don't forget to give them credit in the documentation.

Thanks for mentioning. Just small thing regarding the icons, could you please make them with the count text a little bit small?

Height and width of size 20 for icons look fine to me. Just try and tell me if it looks good to you before I push in the change.
.

- remove the filter used for chaining. - change tests accordingly Like and Dislike icon are now filled using a class - make appropriate changes to the javascript file - add classes to the css file - this change was made because style property was sometimes not overiding the fill property of svg element Fixes like and dislike icons filling now hopefully.

- make corresponding changes in the url and tests.

Radi85 · 2020-05-17T11:01:01Z

FAIL: test_create_comment (comment.tests.test_views.CreateCommentTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/Radi85/Comment/comment/tests/test_views.py", line 45, in test_create_comment
    self.assertIsNone(response.context.get('comment'))
AssertionError: reply by test-1: child comment body is not None

The test is failing due to passing the reply comment object to the context data as comment which is treated as a parent, I used to differentiate the parent from the child in the context data like so:

context['comment'] = PARENT_COMMENT
context['reply'] = CHILD_COMMENT

As I see this is a bad design and I am going to refactor the code.
For now please comment out this line self.assertIsNone(response.context.get('comment')) in test_create_comment line 45 in order to make the test passing and merge your PR before start refacotring.

Add type checking for reaction type in model - add clean function in views also

Radi85

I got 405 (Method Not Allowed) error when try to react to a comment.

abhiabhi94 · 2020-05-17T14:15:12Z

I got 405 (Method Not Allowed) error when try to react to a comment.

Yes, I too get this error now. I had not checked the response by testing it from a browser after the database issue. Since the tests were passing, I assumed it should pass. I will fix this soon and commit.

- change made after refactoring view to a class based one.

Radi85 · 2020-05-17T15:41:55Z

Merged in #16

abhiabhi94 added 3 commits May 8, 2020 16:58

Add patterns for config files from other IDEs

42b8e58

Work In Progress - Add reactions

8a27efd

Make comment models a python package - make other necessary changes to imports everywhere - move code for manager to manager module Add reaction models - add some general property functions to be useful in future Reorder some import to conform to PEP8

Merge branch 'master' of github.com:Radi85/Comment into add-reactions

8ea5066

abhiabhi94 marked this pull request as draft May 8, 2020 22:21

Radi85 reviewed May 9, 2020

View reviewed changes

comment/admin.py Outdated Show resolved Hide resolved

comment/models/reactions.py Outdated Show resolved Hide resolved

comment/models/reactions.py Outdated Show resolved Hide resolved

Radi85 requested changes May 11, 2020

View reviewed changes

comment/admin.py Outdated Show resolved Hide resolved

comment/migrations/0004_reaction_reactioninstance.py Show resolved Hide resolved

comment/models/comments.py Show resolved Hide resolved

comment/models/reactions.py Outdated Show resolved Hide resolved

abhiabhi94 added 2 commits May 11, 2020 23:42

Change import in admin

bb0a7bf

- all comment models are now imported in the __init__ file

Radi85 requested changes May 12, 2020

View reviewed changes

comment/models/reactions.py Show resolved Hide resolved

comment/models/comments.py Show resolved Hide resolved

comment/api/urls.py Outdated Show resolved Hide resolved

Modify urls as per convention

96e9f0a

Radi85 requested changes May 12, 2020

View reviewed changes

comment/views/reactions.py Outdated Show resolved Hide resolved

comment/models/reactions.py Show resolved Hide resolved

comment/models/reactions.py Show resolved Hide resolved

abhiabhi94 added 3 commits May 13, 2020 16:26

Add block to allow customisation of liked and disliked comments

042ef32

Other changes to comply with flake8

Change cite element to span element

736b1fe

- cite was probably mistakenly used here.

abhiabhi94 commented May 13, 2020

View reviewed changes

comment/templates/comment/content.html Outdated Show resolved Hide resolved

Remoce whitespaces to make the code flake8 compliant.

2c4243c

abhiabhi94 added 2 commits May 14, 2020 08:19

Add unittests for models, views and templatetags

ee1a7e4

Changes in view handling reaction - bad react requests are handled now

Make methods to increase/decrease reaction count public.

e9913da

- they were anyway used outside the class Add tests for unauthenticated users for the has_reacted template tag - mock an unauthenticated user for the template -> complete coverage for template tags.

Radi85 requested changes May 14, 2020

View reviewed changes

comment/models/reactions.py Show resolved Hide resolved

comment/templates/comment/content.html Show resolved Hide resolved

comment/templatetags/comment_tags.py Outdated Show resolved Hide resolved

Fix dislike icon class and filter

5e6a202

- it was mistakenly copied from the like icon Add an empty line at the end of the file

abhiabhi94 added 3 commits May 17, 2020 03:44

Merge branch 'master' of github.com:Radi85/Comment into add-reactions

27f2a58

Change react view to a class based view

67a998a

- make corresponding changes in the url and tests.

Radi85 referenced this pull request May 17, 2020

Stop tracking of database file

5bbda08

abhiabhi94 added 2 commits May 17, 2020 18:49

Merge branch 'master' of github.com:Radi85/Comment into add-reactions

ca03a76

Comment out failing test

7afcb59

Add type checking for reaction type in model - add clean function in views also

Radi85 approved these changes May 17, 2020

View reviewed changes

Radi85 requested changes May 17, 2020

View reviewed changes

Fix parameter name in url

1a61e0b

- change made after refactoring view to a class based one.

abhiabhi94 requested a review from Radi85 May 17, 2020 15:00

Radi85 approved these changes May 17, 2020

View reviewed changes

Radi85 mentioned this pull request May 17, 2020

Abhiabhi94 add reactions #16

Merged

Radi85 closed this May 17, 2020

Radi85 added the merged label May 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add reactions #10

Add reactions #10

abhiabhi94 commented May 8, 2020 •

edited by Radi85

Radi85 left a comment

Radi85 left a comment

Radi85 commented May 12, 2020

Radi85 commented May 13, 2020

abhiabhi94 commented May 13, 2020

Radi85 commented May 14, 2020

abhiabhi94 commented May 14, 2020 •

edited

Radi85 commented May 15, 2020

abhiabhi94 commented May 15, 2020 •

edited

abhiabhi94 commented May 15, 2020

abhiabhi94 commented May 16, 2020

abhiabhi94 commented May 16, 2020

Radi85 commented May 16, 2020

Radi85 commented May 16, 2020

abhiabhi94 commented May 16, 2020

abhiabhi94 commented May 16, 2020

Radi85 commented May 16, 2020

Radi85 commented May 16, 2020

abhiabhi94 commented May 16, 2020

abhiabhi94 commented May 16, 2020 •

edited

Radi85 commented May 16, 2020

abhiabhi94 commented May 16, 2020 •

edited

Radi85 commented May 17, 2020

Radi85 left a comment

abhiabhi94 commented May 17, 2020

Radi85 commented May 17, 2020

Add reactions #10

Add reactions #10

Conversation

abhiabhi94 commented May 8, 2020 • edited by Radi85

Radi85 left a comment

Choose a reason for hiding this comment

Radi85 left a comment

Choose a reason for hiding this comment

Radi85 commented May 12, 2020

Radi85 commented May 13, 2020

abhiabhi94 commented May 13, 2020

Radi85 commented May 14, 2020

abhiabhi94 commented May 14, 2020 • edited

Radi85 commented May 15, 2020

abhiabhi94 commented May 15, 2020 • edited

abhiabhi94 commented May 15, 2020

abhiabhi94 commented May 16, 2020

abhiabhi94 commented May 16, 2020

Radi85 commented May 16, 2020

Radi85 commented May 16, 2020

abhiabhi94 commented May 16, 2020

abhiabhi94 commented May 16, 2020

Radi85 commented May 16, 2020

Radi85 commented May 16, 2020

abhiabhi94 commented May 16, 2020

abhiabhi94 commented May 16, 2020 • edited

Radi85 commented May 16, 2020

abhiabhi94 commented May 16, 2020 • edited

Radi85 commented May 17, 2020

Radi85 left a comment

Choose a reason for hiding this comment

abhiabhi94 commented May 17, 2020

Radi85 commented May 17, 2020

abhiabhi94 commented May 8, 2020 •

edited by Radi85

abhiabhi94 commented May 14, 2020 •

edited

abhiabhi94 commented May 15, 2020 •

edited

abhiabhi94 commented May 16, 2020 •

edited

abhiabhi94 commented May 16, 2020 •

edited