per-account 7/n: Sort out plain actions #5113
Merged
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
|
Thanks, this LGTM! Looks like just a Prettier failure on the last commit, otherwise please merge at will. |
gnprice
force-pushed
the
pr-peraccount-part7
branch
from
d621daa
to
1369c6f
Compare
This was basically an attempted workaround for zulip#3999. But it falsely tells the user that the settings have actually taken -- there's no indication that we're still saving them. And once the user wises to the fact that we pretend to have saved them when we haven't, there's no way for them to tell when we *have* actually succeeded in saving them. It'd be good to do better here; details on that in the issue. For the moment, just stop pretending.
We'll want to separate per-account from global actions. This one is now used only in global contexts, so make it global. If we need a per-account version in the future, we can add one. (We'll also give it a more specific name, in the next commit.)
This was called just SETTINGS_CHANGE. We want to specifically distinguish changing a global from a per-account setting, because those will start being stored in different places. The action already only touches global settings -- we don't have any per-account settings that aren't server data, and therefore covered by event actions instead -- so all it needs is a rename to make that clear, and so that if we add a local per-account setting in the future it'll be natural to make a new action type for that.
We're about to sort our actions into those acting on per-account vs. global state. These two sub-groupings straddle those categories; and all these sub-groupings exist only for local convenience in this file, so just dissolve these.
We'll turn these groupings into named, usable types shortly. Doing this separately first simplifies the diffs.
We'll use these subtypes to tighten down which actions can be dispatched from per-account vs. global contexts, and which actions can be expected by per-account vs. global reducers. That will help put us in a position to have a multi-account-friendly schema, where everything per-account can get from its caller which account it's supposed to be operating on rather than resorting to an implicit choice of "the active account". Also add some comments about this structure.
This is NFC because these sub-reducers already ignore any actions that aren't in PerAccountAction. (Flow won't check this for us; but I just grepped for for `case` labels in these reducers' files, piped that to `sort -u`, and confirmed that all the results are as expected.) Here "ignore" means they return the state they're given, but after applying JS optional-argument semantics to it -- so when passed `undefined`, they return their respective `initialState` values.
Also PerAccountState while we're at it, for those of them that take the (relatively) "global" state as a third argument.
gnprice
force-pushed
the
pr-peraccount-part7
branch
from
1369c6f
to
0d29d04
Compare
|
Ah indeed -- fixed, and merged. Thanks for the review! |
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.
This is the next PR in the series after #5105, #5083, #5066, #5030, #5016, #5017, and #5023, produced from the branch described at #5006 (comment).
From the foreshadowing last time:
That's this PR.
And this is the end of that draft branch! 🎉
There's still more to do for #5006. But at the end of this PR, we've accomplished this (from that comment #5006 (comment) where I described the draft branch these came from):
The next task will be to fix those spots one by one.