Consistently clear old data on all ways of leaving an account #5606
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Most of these state subtrees hold per-account data we get from the server. In the current pre-zulip#5005 design, where we only store server data for one account, these should already have been clearing out on LOGIN_SUCCESS, like we do with messages, narrows, etc. Two of the subtrees, `outbox` and `drafts`, hold client-side per-account data. Just as with server data, we've only been keeping these around for the active account, and like the other reducers touched in this commit, we've forgotten to handle LOGIN_SUCCESS for them. So, do that. Now, all the server-data subtrees and `outbox` and `drafts` all reset on ACCOUNT_SWITCH, LOGIN_SUCCESS, and LOGOUT. That's part of Greg's proposed fix for zulip#4446. Next, we'll do the next part: > * I think it'd be a useful refactor to consolidate one action type > like `CLEAR_ACCOUNT_DATA`. > * The action creators for these three (in `accountActions.js`) can > dispatch that first, then also a `LOGOUT` etc. > * Almost all the reducers would respond only to the > `CLEAR_ACCOUNT_DATA`. > * There are a couple of exceptions, like `accountReducers` and > `sessionReducers`, that actually want to treat the three cases > differently. Those would be the only ones to continue responding > to `LOGOUT` etc.; and they'd stand out better as a result. > * Then there are further changes we might make to those, but > that'll be easier to see after that. Fixes-partly: zulip#4446
This type variable's name is so similar to PerAccountAction, AllAccountsAction, AccountIndependentAction, and PerAccountApplicableAction, that it's easy to be confused into thinking it's just as important as those. Remove it.
gnprice
reviewed
Dec 14, 2022
src/account/accountActions.js
Outdated
Comment on lines
61
to
85
| async (dispatch, getState) => { | ||
| (realm: URL, email: string, apiKey: string): GlobalThunkAction<Promise<void>> => | ||
| async (dispatch, getState, { activeAccountDispatch }) => { | ||
| NavigationService.dispatch(resetToMainTabs()); | ||
| dispatch(loginSuccessPlain(realm, email, apiKey)); | ||
|
|
||
| await dispatch(registerAndStartPolling()); | ||
| // Now dispatch some actions on the new, post-login active account. | ||
| // Because we just dispatched `loginSuccessPlain`, that new account is | ||
| // now the active account, so `activeAccountDispatch` will act on it. | ||
|
|
||
| await activeAccountDispatch(registerAndStartPolling()); |
There was a problem hiding this comment.
nit:
account actions types: Make loginSuccess thunk action a GlobalThunkAction
This does touch the actual logic, not only type annotations -- namely it starts using this activeAccountDispatch that this thunk can get from its extras object -- so "types" isn't right in the prefix, though "[nfc]" is.
src/account/logoutActions.js
Outdated
Comment on lines
7
to
13
| // Redefined here, instead of imported from accountActions, just to prevent | ||
| // an import cycle. | ||
| const resetAccountDataPlain = (): PerAccountAction => ({ | ||
| type: RESET_ACCOUNT_DATA, | ||
| }); |
There was a problem hiding this comment.
Hmm, that's a bit awkward.
It's convenient that we generally have just one action creator (outside tests) for each action type, because it makes it straightforward to find all the places we create such actions by finding that creator's call sites. So that'd be nice to preserve here.
It looks like the cycle can be fixed by moving the single definition here into logoutActions.js (so having accountActions.js import it from here.) That's inspired by this comment we have on the only existing export in this file:
// In its own file just to prevent import cycles.
export const logout = (): ThunkAction<Promise<void>> => async (dispatch, getState) => {I think "reset account data" is thematically close enough to "log out" that this file is a fine home for it.
…tion Just as much as accountSwitch, this thunk action does touch the global `state.accounts`, by either adding a new account at the front of the array, or -- just like accountSwitch -- moving an existing account to the front. So, make it so callers treat it as a GlobalThunkAction, and have it act like one in its implementation.
As Greg proposed in zulip#4446, so the pattern of consistently clearing data on all ways of leaving an account gets maintained naturally: (except we chose the name RESET_ACCOUNT_DATA instead of CLEAR_ACCOUNT_DATA) > * I think it'd be a useful refactor to consolidate one action type > like `CLEAR_ACCOUNT_DATA`. > * The action creators for these three (in `accountActions.js`) can > dispatch that first, then also a `LOGOUT` etc. > * Almost all the reducers would respond only to the > `CLEAR_ACCOUNT_DATA`. > * There are a couple of exceptions, like `accountReducers` and > `sessionReducers`, that actually want to treat the three cases > differently. Those would be the only ones to continue responding > to `LOGOUT` etc.; and they'd stand out better as a result. > * Then there are further changes we might make to those, but > that'll be easier to see after that. Fixes: zulip#4446
chrisbobbe
force-pushed
the
pr-data-clearing
branch
from
844b9a6 to
b6218db
Compare
|
Thanks for the review!
Done; merging. Oh, and I gave /**
* Reset per-account server data and some client-side data (drafts/outbox).
*/ |
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Dec 14, 2022
In zulip#5606, I updated all the existing tests for ACCOUNT_SWITCH, etc., to test RESET_ACCOUNT_DATA instead, naturally. …But that didn't give complete coverage of the RESET_ACCOUNT_DATA action, because some tests weren't testing for ACCOUNT_SWITCH, etc., in the first place. So, add the coverage now.
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Dec 14, 2022
Really an instance of zulip#4446 that I forgot about in zulip#5606, oops. Related: zulip#4446
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Dec 14, 2022
Really an instance of zulip#4446 that I forgot about in zulip#5606, oops. Related: zulip#4446
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Dec 14, 2022
In zulip#5606, I updated all the existing tests for ACCOUNT_SWITCH, etc., to test RESET_ACCOUNT_DATA instead, naturally. …But that didn't give complete coverage of the RESET_ACCOUNT_DATA action, because some tests weren't testing for ACCOUNT_SWITCH, etc., in the first place. So, add the coverage now.
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Dec 14, 2022
Really an instance of zulip#4446 that I forgot about in zulip#5606, oops. Related: zulip#4446
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Dec 14, 2022
Really an instance of zulip#4446 that I forgot about in zulip#5606, oops. Related: zulip#4446
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Dec 21, 2022
In zulip#5606, I updated all the existing tests for ACCOUNT_SWITCH, etc., to test RESET_ACCOUNT_DATA instead, naturally. …But that didn't give complete coverage of the RESET_ACCOUNT_DATA action, because some tests weren't testing for ACCOUNT_SWITCH, etc., in the first place. So, add the coverage now.
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Dec 21, 2022
Really an instance of zulip#4446 that I forgot about in zulip#5606, oops. Related: zulip#4446
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Dec 21, 2022
Really an instance of zulip#4446 that I forgot about in zulip#5606, oops. Related: zulip#4446
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
EDIT: See also #5612, which picks up some threads I'd forgotten about at the time we merged this.
This fixes the first item in the list in #5605.
Fixes: #4446