Skip to content

Commit

Permalink
model: Make REGISTER_COMPLETE reset some state we forgot about
Browse files Browse the repository at this point in the history
This addresses the second item in the list at #5605:

> - [ ] `REGISTER_COMPLETE` should reset or replace all server data
>       and server-data metadata, but it isn't in some cases:
>       - server data: `topics`, `typing` (should reset)
>       - server-data metadata: `fetching` (should reset)

Although, I've left a comment on `fetchingReducer`, where doing so
felt a bit funny because morally it shouldn't be necessary.

Fixes-partly: #5605
  • Loading branch information
chrisbobbe committed Dec 14, 2022
1 parent 304e676 commit 408d9ca
Show file tree
Hide file tree
Showing 6 changed files with 47 additions and 1 deletion.
9 changes: 9 additions & 0 deletions src/chat/__tests__/fetchingReducer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,15 @@ import { MESSAGE_FETCH_START, MESSAGE_FETCH_ERROR } from '../../actionConstants'
import { DEFAULT_FETCHING } from '../fetchingSelectors';

describe('fetchingReducer', () => {
test('REGISTER_COMPLETE', () => {
expect(
[
{ type: MESSAGE_FETCH_START, narrow: HOME_NARROW, numBefore: 10, numAfter: 10 },
eg.action.register_complete,
].reduce(fetchingReducer, eg.baseReduxState.fetching),
).toEqual(eg.baseReduxState.fetching);
});

describe('MESSAGE_FETCH_START', () => {
test('if messages are fetched before or after the corresponding flag is set', () => {
const prevState = deepFreeze({ [HOME_NARROW_STR]: { older: false, newer: false } });
Expand Down
9 changes: 9 additions & 0 deletions src/chat/__tests__/narrowsReducer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,15 @@ describe('narrowsReducer', () => {
const egTopic = eg.streamMessage().subject;
const topicNarrowStr = keyFromNarrow(topicNarrow(eg.stream.stream_id, egTopic));

test('REGISTER_COMPLETE', () => {
expect(
[
{ ...eg.action.message_fetch_complete, messages: [eg.streamMessage()] },
eg.action.register_complete,
].reduce(narrowsReducer, eg.baseReduxState.narrows),
).toEqual(eg.baseReduxState.narrows);
});

describe('EVENT_NEW_MESSAGE', () => {
test('if not caught up in narrow, do not add message in home narrow', () => {
const initialState = Immutable.Map({ [HOME_NARROW_STR]: [1, 2] });
Expand Down
7 changes: 7 additions & 0 deletions src/chat/fetchingReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
MESSAGE_FETCH_ERROR,
MESSAGE_FETCH_COMPLETE,
RESET_ACCOUNT_DATA,
REGISTER_COMPLETE,
} from '../actionConstants';
import { NULL_OBJECT } from '../nullObjects';
import { DEFAULT_FETCHING } from './fetchingSelectors';
Expand Down Expand Up @@ -67,6 +68,12 @@ export default (
): FetchingState => {
switch (action.type) {
case RESET_ACCOUNT_DATA:

// Reset just because `fetching` is server-data metadata, and we're
// resetting the server data it's supposed to apply to… but really, if
// there's an in-progress message fetch now, it's a bug, perhaps #5609.
// TODO(#5609): Remove "perhaps #5609" when that specific bug is fixed.
case REGISTER_COMPLETE: // eslint-disable-line no-fallthrough
return initialState;

case MESSAGE_FETCH_START:
Expand Down
9 changes: 9 additions & 0 deletions src/topics/__tests__/topicsReducer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,15 @@ import { INIT_TOPICS } from '../../actionConstants';
import { NULL_OBJECT } from '../../nullObjects';

describe('topicsReducer', () => {
describe('REGISTER_COMPLETE', () => {
test('resets state to initial state', () => {
const prevState = deepFreeze({ [eg.stream.stream_id]: [{ max_id: 1, name: 'some topic' }] });
expect(topicsReducer(prevState, eg.action.register_complete)).toEqual(
eg.baseReduxState.topics,
);
});
});

describe('RESET_ACCOUNT_DATA', () => {
test('resets state to initial state', () => {
const prevState = deepFreeze({ [eg.stream.stream_id]: [{ max_id: 1, name: 'some topic' }] });
Expand Down
10 changes: 9 additions & 1 deletion src/topics/topicsReducer.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
/* @flow strict-local */
import type { TopicsState, PerAccountApplicableAction } from '../types';
import { INIT_TOPICS, EVENT_NEW_MESSAGE, RESET_ACCOUNT_DATA } from '../actionConstants';
import {
INIT_TOPICS,
EVENT_NEW_MESSAGE,
REGISTER_COMPLETE,
RESET_ACCOUNT_DATA,
} from '../actionConstants';
import { NULL_OBJECT } from '../nullObjects';
import { replaceItemInArray } from '../utils/immutability';

Expand Down Expand Up @@ -42,6 +47,9 @@ export default (
): TopicsState => {
switch (action.type) {
case RESET_ACCOUNT_DATA:

// Reset to clear stale data; payload has no initial data for this model
case REGISTER_COMPLETE: // eslint-disable-line no-fallthrough
return initialState;

case INIT_TOPICS:
Expand Down
4 changes: 4 additions & 0 deletions src/typing/typingReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
CLEAR_TYPING,
DEAD_QUEUE,
RESET_ACCOUNT_DATA,
REGISTER_COMPLETE,
} from '../actionConstants';
import { pmTypingKeyFromRecipients } from '../utils/recipient';
import { NULL_OBJECT } from '../nullObjects';
Expand Down Expand Up @@ -97,6 +98,9 @@ export default (

case DEAD_QUEUE:
case RESET_ACCOUNT_DATA:

// Reset to clear stale data; payload has no initial data for this model
case REGISTER_COMPLETE: // eslint-disable-line no-fallthrough
return initialState;

default:
Expand Down

0 comments on commit 408d9ca

Please sign in to comment.