Page MenuHomePhabricator

[lib] Refactor report store reducer
ClosedPublic

Authored by inka on Jan 12 2024, 6:24 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 22, 9:31 PM
Unknown Object (File)
Fri, Nov 22, 9:24 PM
Unknown Object (File)
Fri, Nov 22, 9:22 PM
Unknown Object (File)
Fri, Nov 22, 9:11 PM
Unknown Object (File)
Fri, Nov 22, 9:10 PM
Unknown Object (File)
Fri, Nov 22, 8:55 PM
Unknown Object (File)
Fri, Nov 22, 5:04 PM
Unknown Object (File)
Tue, Nov 5, 4:12 PM
Subscribers

Details

Summary

issue: https://linear.app/comm/issue/ENG-5813/refactor-report-store-ops
We send reports to the report store, not the keyservers. So I will remove them on identity login / logout, not on keyserver login / logout / setNewSession.
Identity will be handled in ENG-6424

Test Plan

Tested that reports are stll removed on logout

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

inka requested review of this revision.Jan 12 2024, 6:52 AM
inka planned changes to this revision.Jan 12 2024, 7:01 AM
inka added inline comments.
lib/reducers/report-store-reducer.js
98 ↗(On Diff #35572)

This is actually unnecessary, if logInActionTypes or siweAuthActionTypes are used we know that !usingCommServicesAccessToken

This revision is now accepted and ready to land.Jan 16 2024, 6:30 AM

As mentioned in other diffs, not sure we need to change anything for logOutActionTypes.success

inka planned changes to this revision.Jan 17 2024, 8:49 AM

We decided to have a single action for logging out of keyservers and identity.

This revision is now accepted and ready to land.Jan 18 2024, 1:35 AM
inka requested review of this revision.Jan 18 2024, 1:36 AM
lib/reducers/report-store-reducer.js
88 ↗(On Diff #35769)

Weird to see deleteKeyserverAccountActionTypes.success contemplated only in a case where !usingCommServicesAccessToken... I assume it's only relevant in the opposite case. But presumably logic will be added past line 102 later?

lib/reducers/report-store-reducer.js
88 ↗(On Diff #35769)

As noted in the task, we don't want to be deleting reports when user disconnects from a keyserver, because the reports will be sent to the report service. So this is not really connected to keyservers, but to being logged in with identity

lib/reducers/report-store-reducer.js
88 ↗(On Diff #35769)

So reports will be deleted on identity logout / delete account.

lib/reducers/report-store-reducer.js
88 ↗(On Diff #35769)

I don't understand why deleteKeyserverAccountActionTypes.success is here. What happens if we remove it? Is it even possible to dispatch deleteKeyserverAccountActionTypes.success when !usingCommServicesAccessToken?

lib/reducers/report-store-reducer.js
88 ↗(On Diff #35769)

Is the reason because deleteKeyserverAccountActionTypes is used for both whole-app account deletion (when !usingCommServicesAccessToken), and single-keyserver account deletion (when usingCommServicesAccessToken)?

If so, I think this is really confusing, and should be simplified. I think we should keep the deleteAccountActionTypes from before for whole-app login (identity + keyserver) – this is the same principle that we discussed for the old log out action.

Meanwhile, we should probably introduce a new action (presumably this deleteKeyserverAccountActionTypes) that is used only for deleting your account from a single keyserver.

Merge identity delete account with keyserver delete account

lib/reducers/report-store-reducer.js
94 ↗(On Diff #35921)

This condition can be lifted into the above condition

This revision is now accepted and ready to land.Jan 24 2024, 11:23 AM
This revision was automatically updated to reflect the committed changes.