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

fix(research): use the getter from the research store to get the more correct count of all the comments for a research article #3513

Closed
wants to merge 1 commit into from

Conversation

codisart
Copy link
Contributor

@codisart codisart commented May 9, 2024

PR Checklist

PR Type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Developer experience (improves developer workflows for contributing to the project)

Description

What this PR does

It remplaces the usage of the denormalized field to an accurate count of all comments for a research article.
The denormalized field is kept for sorting in list
It removes seemly useless update when editing a comment
It deletes dead code and especially an helper function that was redundant with the store getter.
It moves and updates old helper functions unit tests to the research store unit tests file.

Git Issues

Closes #3510


What happens next?

Thanks for the contribution! We try to make sure all PRs are reviewed ahead of our monthly maintainers call (first Monday of the month)

If the PR is working as intended it'll be merged and included in the next platform release, if not changes will be requested and re-reviewed once updated.

If you need more immediate feedback you can try reaching out on Discord in the Community Platform development channel.

… correct count of all the comments for a research article
@codisart codisart requested a review from a team as a code owner May 9, 2024 10:39
@benfurber
Copy link
Member

Thanks for this @codisart. The idea is good but probably not the direction we actually want to go in on this one.

The issue you've come across is that two comments approaches are used across the platform. We're moving towards the close of discussion documents and away from comments being stored on research (or how-tos eventually).

You can see me starting to play around with this decoupling here: #3519

As comments are done on a research update level, it's probably unavoidable that a research document does have the totalComments field, but this should really be set by a cloud function like what you can see in functions/src/userUpdates.

Then commentsCount() can be removed from the research store entirely.

Fancy giving that a go...?

@codisart
Copy link
Contributor Author

@benfurber if I understand correctly your comment, the migration from comments to discussion is not done for question and research and both entities coexist.

Maybe a step back could help for the discussions and comments.

I saw the issue #3505 that asks for the new discussion replies behavior for the Howto module. But also there is your pull request #3519 to refactor the discussion behavior. But also the different bugs about comments (the count and the reply to deleted comment).

I think if we try to fix and refactor everything at once by several people in isolated PRs, we may create big conflicts and maybe more regressions. For me, I think your PR is the most important work to do about discussions. It should make the correct encapsulation of the discussion and maybe even create deprecations for store, helpers and service. Then we could build onto it unit tests and others fixes.

Maybe there is a need to define what is the expected result before removing "simple" comments entities (Again if I understood correctly). If I read correctly we alread have the discussion container without replies for howtos. Are "simple" comments use as the sole truth source ?

What do you think ?

@benfurber
Copy link
Member

@codisart Now I've merged in #3519, what do you think?

@benfurber benfurber closed this Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[bug] Wrong comments count for an updated research item
2 participants