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

[CORL-3224]: ensure replies to rejected parent comments are not included in comment count #4757

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

kabeaty
Copy link
Contributor

@kabeaty kabeaty commented Feb 28, 2025

What does this PR do?

These changes ensure that when replies in the stream have rejected ancestor comments, they are not included in the comment count shown since they are not visible. The changes use totalPublishedAndVisible for the stream now. A new rejectedAncestorIDs on comments is used to keep track of whether a comment has any rejected ancestors and is therefore hidden. A new PUBLISHED_COMMENTS_WITH_REJECTED_ANCESTORS has been added to counts to keep track of these in a count that can then be used to determine the new totalPublishedAndVisible.

Having rejectedAncestorIDs on comments may be useful to us in other places too. For example, currently, we have a hasRejectedAncestors function in server/src/core/server/services/comments/comments.ts. We could refactor this to utilize the new rejectedAncestorIDs.

These changes will impact:

  • commenters
  • moderators
  • admins
  • developers

What changes to the GraphQL/Database Schema does this PR introduce?

To GraphQL, these changes add rejectedAncestorIDs. Also, to comment counts, totalPublishedAndVisible and relationships: {PUBLISHED_COMMENTS_WITH_REJECTED_ANCESTORS: number}.

To database schema, rejectedAncestorIDs has been added to comments. Also, to comment counts, relationships: {PUBLISHED_COMMENTS_WITH_REJECTED_ANCESTORS: number} has been added.

Does this PR introduce any new environment variables or feature flags?

no

If any indexes were added, were they added to INDEXES.md?

TODO: Confirm if any new indexes are needed

How do I test this PR?

You can test these changes by testing that comment counts are still working as expected.

Also, create a bunch of comments in the stream. Include levels of replies. Reject and approve comments and replies at all different levels. Reject and approve replies to already rejected ancestors. See that the stream's comment count updates according to how many comments are actually visible in the stream.

Were any tests migrated to React Testing Library?

no

How do we deploy this PR?

Copy link

netlify bot commented Feb 28, 2025

Deploy Preview for gallant-galileo-14878c canceled.

Name Link
🔨 Latest commit 7583279
🔍 Latest deploy log https://app.netlify.com/sites/gallant-galileo-14878c/deploys/67c70a155a5f0b0008f746db

@kabeaty kabeaty requested a review from nick-funk February 28, 2025 14:02
@@ -645,13 +645,6 @@ const enhanced = withPaginationContainer<
}
mode
}
commentCounts {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this since it doesn't seem to be used...

Copy link
Contributor

@nick-funk nick-funk left a comment

Choose a reason for hiding this comment

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

Tested and approved!
This all appears very consistent with the existing counts logic we have in Coral currently. The graphLookUp being done only during the approve and reject steps looks good as well!

Nice work!

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.

3 participants