example data: Better support EVENT_NEW_MESSAGE. #4760
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
The new name `mkActionEventNewMessage` is a bit more annoyingly verbose... but it's more transparent about exactly what kind of action it produces. It also represents a completely regular way of naming such a function, suitable for use in the general example-data module `eg`.
We already use this in one spot outside the `unread` tests, and it'd be convenient to use it in more places too.
Small thing, not essential to do -- I'm not sure it's ever actually helped us catch a bug -- but easy, and so in this central library meant for wide reuse we make a practice of it.
This will let us use it in a wider range of tests.
In particular, drop the occasional `type: EVENT_NEW_MESSAGE` and `ownUserId: eg.selfUser.user_id`, which were already redundant with `eg.eventNewMessageActionBase`. On the other hand, there was one `ownUserId: selfUser.user_id`, referring to a *different* self-user defined in the specific test. Happily that now stands out better, making the test clearer. This should be nearly NFC; the exception is that by using `eg.mkActionEventNewMessage` we're now making sure `.message.flags` exists, as it always should on an EVENT_NEW_MESSAGE action. That change will in turn be helpful in letting us clean up the code that these tests are testing.
As seen in the one existing example of this idea, a factory function has the advantage that it can handle subtler bits of structure: here, the fact that the `Message` object found in an `EVENT_NEW_MESSAGE` action always has a `flags` property, even though other `Message` objects don't. As demonstrated in the preceding commit, where we converted uses of `eg.eventNewMessageActionBase` en masse to `eg.mkActionEventNewMessage`, it also produces shorter code at the call site. And the types sure are simpler to describe.
This should always be true in real life (as described at the `Message` definition), and we've just fixed our tests to make it true there too. So, simplify a bit the potential paths through this code by asserting the property is there before we try to use it, rather than giving it a bogus implicit default of all flags being unset.
Much like we did for the various `unread` sub-reducers in the previous commit, this simplifies a bit the potential paths through this code. There was a test for this impossible case; remove that. While we're here, also remove a similar impossible check on `messages` being falsy, as if we got an EVENT_UPDATE_MESSAGE_FLAGS action with no `messages` property.
chrisbobbe
force-pushed
the
pr-eg-event-new-message
branch
from
8eb9469
to
6327e93
Compare
|
LGTM, thanks, @gnprice! Merged. |
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 follows up on the thread at #4710 (comment).
We replace the "action fragments" idea with more-flexible factory functions. In particular all the places in test code where we used
eg.eventNewMessageActionBaseto create anEVENT_NEW_MESSAGEaction now make a call to the neweg.mkActionEventNewMessagewhich subsumes it -- and unlike the old fragment, the new factory can ensure thataction.message.flagsis always present, as it should be.Then that in turn lets us clean up some app code, in reducers, that had provided bogus default behavior in case of a missing
flagsin order to accommodate tests that didn't supply one.