Page MenuHomePhabricator

[lib] introduce community reducer
ClosedPublic

Authored by ginsu on Feb 26 2024, 12:01 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Apr 28, 4:01 PM
Unknown Object (File)
Sun, Apr 28, 4:01 PM
Unknown Object (File)
Sun, Apr 28, 4:01 PM
Unknown Object (File)
Sun, Apr 28, 4:01 PM
Unknown Object (File)
Sun, Apr 28, 4:01 PM
Unknown Object (File)
Sun, Apr 28, 3:59 PM
Unknown Object (File)
Apr 3 2024, 12:41 AM
Unknown Object (File)
Mar 31 2024, 10:25 AM
Subscribers

Details

Summary

This diff introduces the barebones community reducer and all the necessary logic to get it integrated into our codebase. Subsequent diffs will handle extending the community reducer to handle other cases we need to cover in our business logic

Linear task: https://linear.app/comm/issue/ENG-6159/introduce-community-reducer and https://linear.app/comm/issue/ENG-6535/start-processing-community-store-ops-on-sqllite

Depends on D11165

Test Plan

Ran the following code + confirmed that a community was being added from this dispatched action

const dispatch = useDispatch();

const fakeCommunityID = '256|84261';

const onPressAddCommunity = React.useCallback(() => {
  const communityInfo: CommunityInfo = {
    enabledApps: {
      calendar: true,
      wiki: false,
      tasks: true,
      files: true,
    },
  };

  dispatch({
    type: addCommunityActionType,
    payload: {
      id: fakeCommunityID,
      newCommunityInfo: communityInfo,
    },
  });
}, [dispatch]);

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: atul, inka, kamil.
Harbormaster returned this revision to the author for changes because remote builds failed.Feb 26 2024, 12:42 AM
Harbormaster failed remote builds in B27155: Diff 37575!
ginsu requested review of this revision.Feb 26 2024, 1:13 AM

will make sure ci passes before landing

atul requested changes to this revision.Feb 26 2024, 2:13 PM

Mostly looks good, but do we need to handle clearing the data on log out/delete account/etc?

This revision now requires changes to proceed.Feb 26 2024, 2:13 PM
In D11166#323019, @atul wrote:

Mostly looks good, but do we need to handle clearing the data on log out/delete account/etc?

There is some ongoing work connected to clearing data right now, some context in ENG-6807, ENG-6424#comment-979868aa and this project link.

In short, on log out/delete account/etc we delete DB anyway so it should be safe - and generating ops like remove_all interferes with the backup (we'll backup logs deleting everything so after restoration we'll have empty tables...).

It might be good to get @tomek approval as he is working on Resilient user-specific state reset in March.


One question to @ginsu:
In this diff, you are starting to process store ops on DB - and it looks like CommunityInfo has a field called enabledApps which I think we already have defined somewhere. Do we need some sort of migration or this is just a brand-new store and we'll start populating it after adding business logic/UI?

In D11166#323019, @atul wrote:

Mostly looks good, but do we need to handle clearing the data on log out/delete account/etc?

In our current approach, we don't need to do anything: resetUserSpecificState should handle it. Only the fields from nonUserSpecificFieldsWeb and nonUserSpecificFieldsNative survive these actions. But please test it.

ginsu requested review of this revision.Feb 27 2024, 8:42 PM
In D11166#323019, @atul wrote:

Mostly looks good, but do we need to handle clearing the data on log out/delete account/etc?

In our current approach, we don't need to do anything: resetUserSpecificState should handle it. Only the fields from nonUserSpecificFieldsWeb and nonUserSpecificFieldsNative survive these actions. But please test it.

Tested and confirmed that the community store is cleared on log out

In this diff, you are starting to process store ops on DB - and it looks like CommunityInfo has a field called enabledApps which I think we already have defined somewhere. Do we need some sort of migration or this is just a brand-new store and we'll start populating it after adding business logic/UI?

This is a good call out, yes we will need a migration to handle initially populating the community store with all the threads that are community root. Here is a linear task to track that:
https://linear.app/comm/issue/ENG-7056/create-script-to-populate-community-store-with-all-the-threads-that

In this diff, you are starting to process store ops on DB - and it looks like CommunityInfo has a field called enabledApps which I think we already have defined somewhere. Do we need some sort of migration or this is just a brand-new store and we'll start populating it after adding business logic/UI?

This is a good call out, yes we will need a migration to handle initially populating the community store with all the threads that are community root. Here is a linear task to track that:
https://linear.app/comm/issue/ENG-7056/create-script-to-populate-community-store-with-all-the-threads-that

thanks for response!

tomek added inline comments.
lib/reducers/community-reducer.js
14 ↗(On Diff #37575)

We usually don't export reducers like this. Instead, we're using a named export at the end of the file

export { reduceCommunityStore };

Overall, it doesn't matter too much, but there doesn't seem to be a good reason for breaking a convention.

18–19 ↗(On Diff #37575)
26–28 ↗(On Diff #37575)

Why do we spread the value?

This revision is now accepted and ready to land.Feb 29 2024, 3:00 PM

address comments + rebase before landing

This revision was landed with ongoing or failed builds.Mar 4 2024, 12:39 PM
This revision was automatically updated to reflect the committed changes.
lib/reducers/community-reducer.js
14 ↗(On Diff #37575)

I don't think we should prioritize cleaning this up - we can clean this when we touch this code.