Details

Reviewers

inka
rohan
michal

Commits

rCOMMde96f810073b: [lib] add conditions for adding keyserver to the keyserver store

Summary

This diff adds two conditions for adding a new keyserver to the keyserver store. The first condition resets the keyserver store whenever we logout/delete account/or change session, the second adds the keyserver into the keyserver store

Making @inka a blocking reviewer since she is currently working a lot with this file and I want to make sure that these changes don't interfere with anything that is currently "in flight"

Linear task: https://linear.app/comm/issue/ENG-5342/add-a-condition-for-the-new-addkeyserveractiontype

Depends on D9591

Test Plan

Confirmed that the keyserver was added to the keyserver store

Logs of the keyserver store where I added a dummy keyserver info

Screenshot 2023-10-31 at 4.03.18 AM.png (1×1 px, 170 KB)

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.Oct 31 2023, 9:18 PM

Herald added subscribers: will, tomek, ashoat. · View Herald TranscriptOct 31 2023, 9:18 PM

update

ginsu edited the summary of this revision. (Show Details)Oct 31 2023, 9:29 PM

ginsu edited the test plan for this revision. (Show Details)

ginsu added reviewers: inka, rohan, michal.

ginsu attached a referenced file: F846438: Screenshot 2023-10-31 at 4.03.18 AM.png. (Show Details)

ginsu edited the summary of this revision. (Show Details)

ginsu added a parent revision: D9591: [native] introduce addKeyserverActionType.

ginsu added a subscriber: inka.

Harbormaster completed remote builds in B23631: Diff 32575.Oct 31 2023, 9:39 PM

Harbormaster completed remote builds in B23632: Diff 32576.

ginsu requested review of this revision.Oct 31 2023, 9:39 PM

ginsu added a child revision: D9665: [native] implement onPressSave keyserver button callback.Oct 31 2023, 9:49 PM

Nit – we usually use } else if in reducers. It doesn't matter since each condition returns, but it would be good to be consistent

address @ashoat's comment

ashoat added inline comments.Nov 1 2023, 11:45 AM

lib/reducers/keyserver-reducer.js
51 ↗	(On Diff #32598)	Convert this to `else if` as well

update

Harbormaster completed remote builds in B23648: Diff 32599.Nov 1 2023, 12:08 PM

Harbormaster completed remote builds in B23647: Diff 32598.Nov 1 2023, 12:18 PM

inka added inline comments.Nov 2 2023, 1:01 PM

lib/reducers/keyserver-reducer.js
32–37 ↗	(On Diff #32599)	`setNewSession` is an action that can be called for every keyserver separately. We shouldn't be clearing the whole keyserver store

On second thoughts, this cannot be handled yet. Disregard my comment
This will be refactored in ENG-5350

This revision is now accepted and ready to land.Nov 6 2023, 8:49 AM

Did you check if no errors are thrown when a user logs out and back in? I feel like the lack of ashoatKeyserverID might cause problems

Adding

if (
  action.type === logOutActionTypes.success ||
  (action.type === setNewSessionActionType &&
    action.payload.sessionChange.cookieInvalidated)
) {
  return {
    keyserverInfos: {},
  };
}

to my current code causes logout action to fail. Please check if this happens for you as well

This revision now requires changes to proceed.Nov 6 2023, 9:55 AM

fix logout action

Harbormaster completed remote builds in B23948: Diff 32999.Nov 9 2023, 2:09 AM

This diff will be conflicting with my changes in this stack: D9750, but if you land this before me I will just update it. Before I land my changes you cannot write it in a way that wouldn't conflict with me, so don't worry about it

lib/reducers/keyserver-reducer.js
37–48	Please follow the pattern of updating only what needs to be updated. This will make it easier to add new fields into `KeyserverStore` or `KeyserverInfos` in the future
37–48	Also - you are overwriting here fields that are reduced in other reducers (see the bottom of this file), and: you are returning, so the reducers don't get called you are putting in different values than those reducers would have For example on `logOutActionTypes.success` you make the `connection` null, whereas the `reduceConnectionInfo` would have made it { ...connection, queuedActivityUpdates: [], } `deviceToken` and `lastCommunicatedPlatformDetails` are not changed at all by their reducers on `logOutActionTypes.success`. And so on. Can you explain why that is?

update after syncing with @inka

fix

inka accepted this revision.Nov 13 2023, 8:59 AM

inka added inline comments.

lib/reducers/keyserver-reducer.js
37–41 ↗	(On Diff #33125)	@ginsu and I discussed this, and we actually want to remove the other keyservers, so we are avoiding the other spreads here. This will be refactored a bit when I rebase D9750 stack on it