Page MenuHomePhabricator

[lib] Refactor report store reducer
ClosedPublic

Authored by inka on Jan 12 2024, 6:24 AM.
Tags
None
Referenced Files
F3349084: D10619.diff
Fri, Nov 22, 5:04 PM
Unknown Object (File)
Tue, Nov 5, 4:12 PM
Unknown Object (File)
Tue, Nov 5, 4:11 PM
Unknown Object (File)
Tue, Nov 5, 3:51 PM
Unknown Object (File)
Tue, Nov 5, 1:24 PM
Unknown Object (File)
Fri, Nov 1, 3:34 PM
Unknown Object (File)
Oct 16 2024, 3:26 AM
Unknown Object (File)
Oct 16 2024, 3:26 AM
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
Branch
inka/login_reducers
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

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.