Remove totalUseful aggregations

[mariojsnunes]

PR Checklist

• [x] - Commit messages are descriptive, it will be used in our Release Notes

PR Type

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

Description

Moves totalUseful property to the user profile (and removes its aggregation). Refactor users_verified to not use streaming.

Due to coupling I also had to refactor the function that processes user notifications. It was optimized to only update user_notifications.pending_email if there were actual notification changes (previously wasn't comparing to before values)

Why?

• Currently the we are getting totalUseful value of all users from the aggregations collection. This means that all users are subscribing to that one document, thus, when anything on that document changes (ie: new useful vote), all users that are browsing any profile will get an update (causes an extra read per browsing user) even if they are not browsing the updated user.
• This can make sense for the verified property as we want to access it on lists, so we need only 1 read to get all verified users (although still not great if we have thousands of users).
• The current solution tries to avoid some extra reads by unsubscribing and is likely successful to some extent.
• But, we can do much better... just by moving these fields to the user profile, we won't need any extra read as we are already querying the user profile anyway, so we reduce all this down to 0.
• Plus, we remove complexity from the code.

This PR requires a migration to create totalUseful field on the users, with the current value from the aggregations collection.

Git Issues

Closes #

[cypress[bot]]

Passing run #5581 ↗︎

0

80

1

0

0

Details:

fix: remove totalUseful aggregation; refactor verified aggregation
Project: onearmy-community-platform	Commit: 4870b1aae5
Status: Passed	Duration: 04:20 💡
Started: May 13, 2024 8:18 AM	Ended: May 13, 2024 8:22 AM

Review all test suite changes for PR #3460 ↗︎

[mariojsnunes]

Still need to fix tests... but is ready for review

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 47.82609% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 67.30%. Comparing base (145dd02) to head (4870b1a). Report is 18 commits behind head on master.

Files	Patch %	Lines
...src/aggregations/userNotifications.aggregations.ts	35.29%	10 Missing and 1 partial :warning:
src/stores/Aggregations/aggregations.store.tsx	0.00%	8 Missing :warning:
src/stores/User/user.store.ts	0.00%	2 Missing :warning:
src/pages/Howto/Content/HowtoList/HowToCard.tsx	0.00%	1 Missing :warning:
src/pages/User/content/MemberProfile.tsx	0.00%	1 Missing :warning:
src/stores/Maps/maps.store.ts	0.00%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3460      +/-   ##
==========================================
- Coverage   67.42%   67.30%   -0.13%     
==========================================
  Files         433      431       -2     
  Lines       13707    13664      -43     
  Branches     2467     2465       -2     
==========================================
- Hits         9242     9196      -46     
- Misses       4419     4421       +2     
- Partials       46       47       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[mariojsnunes]

Okay this might be it, test are fixed

[benfurber]

Thanks @mariojsnunes. I'm starting to look through this.

@thisislawatts - I know general guidance in the past has been to move onwards aggregations? Thoughts on this approach?

[mariojsnunes]

Thanks for the notes @benfurber The main reason to remove this aggregation is that we can build the same functionality without it, just need an extra profile field which is free in terms of reads and bandwidth.

[thisislawatts]

There is a lot going on in that PR @mariojsnunes , which makes it a bit harder to reason with. My preference would be for distinct changes to be split across multiple commits and/or PRs to reduce the number of changes here. For example a preparatory refactoring PR/commit that then makes the change you want to make, easier](https://x.com/KentBeck/status/250733358307500032).

Otherwise the ambition of this PR to remove the TotalUseful Aggregation seems reasonable, I am missing the context here on what problem we were trying to solve with this aggregation.

[thisislawatts]

@benfurber did you get this from some documentation?

I know general guidance in the past has been to move onwards aggregations?

My understanding is that aggregations aim at solving a very specific data access pattern rather than something we should default to.

The firebase product has also evolved since we introduced our aggregations, so we may want to review our usage of aggregations to see if there are alternatives in Firestore which would simplify the code and reduce maintenance costs. I found this useful entry point to the conversation here: https://stackoverflow.com/questions/77461961/how-should-i-handle-aggregated-values-in-firestore

[mariojsnunes]

I've updated the description to better reflect what this PR does.

1. Is our e2e coverage really good enough that no changes are needed?
2. I've tried to use FieldValue.increment a couple of times and found it unreliable at low number changes. I had issues that lead me to this: Firestore Fieldvalue.increment and duplicate writes invertase/react-native-firebase#5914
3. There's still a test commented out in src/pages/Howto/Content/Howto/Howto.test.tsx.
4. Lots of changes aren't paired with unit test changes, which I'm assuming means gaps in coverage, eeerrr, fancy adding the coverage please? At least behaviour linked to the PR.

Thank you @benfurber

1. e2e test coverage is unchanged.
2. From what I understand it shouldn't be an issue unless there are concurrent requests, which isn't the case here
3. Good catch! Uncommented
4. The coverage is the same, we had duplicated tests at howto and research, it is now ensured at toggleDocUsefulByUser.test

[onearmy-bot]

:tada: This PR is included in version 1.182.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

[onearmy-bot]

:tada: This PR is included in version 1.182.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: