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

comments: Fix total comments count calculation #997

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

pkvach
Copy link
Contributor

@pkvach pkvach commented Mar 7, 2024

Checklist

  • All new and existing tests are passing
  • I have added an entry to CHANGES.rst because this is a user-facing change or an important bugfix
  • I have written proper commit message(s)

What changes does this Pull Request introduce?

Changes to properly count hidden comments when calculating the total number of comments.

  • isso/views/comments.py: The calculation of total_replies is modified. Instead of directly assigning the value of reply_counts[root_id] to total_replies, it now calculates the sum of all values in reply_counts if root_id is None, otherwise it assigns the value of reply_counts[root_id].

  • isso/js/embed.js: The redundant incrementation of the count variable with comment.total_replies was removed.

  • isso/tests/test_comments.py: Several assertions are added to various test cases to check the value of total_replies in the response data.

Why is this necessary?

Fixes #448

Demo

  • Before fix

image

  • After fix

image

@jelmer jelmer marked this pull request as draft March 10, 2024 16:52
@jelmer
Copy link
Member

jelmer commented Mar 10, 2024

please complete the checklist in the PR template

@pkvach
Copy link
Contributor Author

pkvach commented Mar 10, 2024

Added an entry to CHANGES.rst

@pkvach pkvach marked this pull request as ready for review March 11, 2024 18:43
@pkvach pkvach force-pushed the fix/total-comments-count branch from ef019ef to 4fa53aa Compare March 20, 2024 18:52
@pkvach
Copy link
Contributor Author

pkvach commented Mar 30, 2024

Please make a review of this PR.

Copy link
Member

@ix5 ix5 left a comment

Choose a reason for hiding this comment

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

Changes itself LGTM.

I found this really hard to review because of the terminologies (some values point are dicts (replies) and others are integers (hidden_replies), the request param parent becomes id in the response, and the meaning/calculation of reply_counts was hard to wrap my head around). The existing code is quite clunky and I'm glad you nevertheless took the time to look into it and fix this longstanding issue @pkvach.

Please fix the two identically-named commits (either squash or split the tests/CHANGES changes into appropriately named commits)

@pkvach pkvach force-pushed the fix/total-comments-count branch from cc31a31 to 6437845 Compare April 14, 2024 08:13
Changes to properly count hidden comments when calculating the total number of comments.

Fixes isso-comments#448
@pkvach pkvach force-pushed the fix/total-comments-count branch from 6437845 to a952595 Compare April 14, 2024 08:26
@pkvach
Copy link
Contributor Author

pkvach commented Apr 14, 2024

@ix5
Thank you for the review! You're right, it was a tricky one.
To make things easier to follow, I've reformatted the changes into a single Git commit.

Copy link
Member

@ix5 ix5 left a comment

Choose a reason for hiding this comment

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

Thanks for squashing, LGTM

@ix5 ix5 merged commit 0986104 into isso-comments:master Apr 23, 2024
18 checks passed
@pkvach pkvach deleted the fix/total-comments-count branch April 23, 2024 21:37
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.

Counter problem
3 participants