Skip to content

Prep commits for new avatar_url handling (#4157) #4171

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
chrisbobbe merged 3 commits into from
Aug 3, 2020
+132 −70
Merged

Prep commits for new avatar_url handling (#4157) #4171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b2279bc
migrateMessages: Typecheck test file and use example data.
chrisbobbe Jun 22, 2020
a7df567
msglist html: Type-check render-test.js and use example data.
chrisbobbe Jun 22, 2020
9c52958
webViewHandleUpdates tests: Start typechecking, use example data.
chrisbobbe Jul 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 47 additions & 6 deletions src/api/messages/__tests__/migrateMessages-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,53 @@
/* @flow strict-local */
import omit from 'lodash.omit';

import { migrateMessages } from '../getMessages';
import * as eg from '../../../__tests__/lib/exampleData';
import type { ServerMessage, ServerReaction } from '../getMessages';
import type { Message } from '../../modelTypes';

describe('migrateMessages', () => {
test('Replace user object with `user_id`', () => {
const messages = [
{ reactions: [{ user: { id: 1, full_name: 'name', email: 'a@a.com' }, code: 'code' }] },
];
const expectedOutput = [{ reactions: [{ user_id: 1, code: 'code' }] }];
const reactingUser = eg.makeUser();

const serverReaction: ServerReaction = {
emoji_name: '+1',
reaction_type: 'unicode_emoji',
emoji_code: '1f44d',
user: {
email: reactingUser.email,
full_name: reactingUser.full_name,
id: reactingUser.user_id,
},
};

const serverMessage: ServerMessage = {
// The `omit` shouldn't be necessary with Flow v0.111: "Spreads
// now overwrite properties instead of inferring unions"
// (https://medium.com/flow-type/spreads-common-errors-fixes-9701012e9d58).
...(omit(eg.streamMessage(), 'reactions'): $Diff<Message, { reactions: mixed }>),
Copy link
Copy Markdown
Member

@gnprice gnprice Jul 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the omit needed here? The property mentioned after the spread should clobber it anyway.

Copy link
Copy Markdown
Collaborator Author

@chrisbobbe chrisbobbe Jul 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely sounds reasonable.

It seems like Flow happily verifies that there will be a correct type at reactions after the spread, but only with the omit (or something like it) in place. Even though it's in the spec (er, I'm pretty sure) that later-mentioned properties will clobber earlier-mentioned ones, it seems like Flow maybe doesn't know that, or in any case doesn't like there being two different, competing (and both present) values for a single property.

If I take away the omit:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/api/messages/__tests__/migrateMessages-test.js:23:40

Cannot assign object literal to serverMessage because:
 • property user is missing in object type [1] but exists in object type [2] in array element of property reactions.
 • property user_id is missing in object type [2] but exists in object type [1] in array element of property
   reactions.

     src/api/messages/__tests__/migrateMessages-test.js
      20│     },
      21│   };
      22│
      23│   const serverMessage: ServerMessage = {
      24│     ...eg.streamMessage(),
      25│     reactions: [serverReaction],
      26│   };
      27│
      28│   const input: ServerMessage[] = [serverMessage];
      29│

     src/api/messages/getMessages.js
 [2]  33│   reactions: $ReadOnlyArray<ServerReaction>,

     src/api/modelTypes.js
 [1] 493│   reactions: $ReadOnlyArray<Reaction>,

Copy link
Copy Markdown
Member

@gnprice gnprice Jul 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, indeed.

It looks like this is one of the issues discussed here:
https://medium.com/flow-type/spreads-common-errors-fixes-9701012e9d58

Spreads now overwrite properties instead of inferring unions

and it's fixed in Flow v0.111.

Which, checking our next RN upgrade issue #3782... we'll get with that upgrade to RN v0.62! Should be a lot of Flow improvement in that upgrade -- that'll be a happy day.

Copy link
Copy Markdown
Member

@gnprice gnprice Jul 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is a fine workaround; but let's add a comment mentioning Flow v0.111. (So that when we do that upgrade we can easily find this in a grep and make it one of the spots where we improve the types as a followup.)

reactions: [serverReaction],
};

expect(migrateMessages(messages)).toEqual(expectedOutput);
const input: ServerMessage[] = [serverMessage];

const expectedOutput: Message[] = [
{
// The `omit` shouldn't be necessary with Flow v0.111.
...(omit(serverMessage, 'reactions'): $Diff<ServerMessage, { reactions: mixed }>),
reactions: [
{
user_id: reactingUser.user_id,
emoji_name: serverReaction.emoji_name,
reaction_type: serverReaction.reaction_type,
emoji_code: serverReaction.emoji_code,
},
],
},
];

const actualOutput: Message[] = migrateMessages(input);

test('Replace user object with `user_id`', () => {
expect(actualOutput.map(m => m.reactions)).toEqual(expectedOutput.map(m => m.reactions));
});
});
4 changes: 2 additions & 2 deletions src/api/messages/getMessages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ type ApiResponseMessages = {|
* Note that reaction events have a *different* variation; see their
* handling in `eventToAction`.
*/
type ServerReaction = $ReadOnly<{|
export type ServerReaction = $ReadOnly<{|
...$Diff<Reaction, {| user_id: mixed |}>,
user: $ReadOnly<{|
email: string,
Expand All @@ -28,7 +28,7 @@ type ServerReaction = $ReadOnly<{|
|}>,
|}>;

type ServerMessage = $ReadOnly<{|
export type ServerMessage = $ReadOnly<{|
...$Exact<Message>,
reactions: $ReadOnlyArray<ServerReaction>,
|}>;
Expand Down
125 changes: 72 additions & 53 deletions src/webview/__tests__/webViewHandleUpdates-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,51 @@
/* @flow strict-local */
import deepFreeze from 'deep-freeze';

import { getUpdateEvents } from '../webViewHandleUpdates';
import { flagsStateToStringList } from '../html/messageAsHtml';
import { HOME_NARROW } from '../../utils/narrow';
import * as eg from '../../__tests__/lib/exampleData';
import type { Props } from '../MessageList';

describe('getUpdateEvents', () => {
const emptyFlags = eg.baseReduxState.flags;

const baseBackgroundData = deepFreeze({
flags: emptyFlags,
auth: {},
alertWords: [],
allImageEmojiById: eg.action.realm_init.data.realm_emoji,
auth: eg.selfAuth,
debug: eg.baseReduxState.session.debug,
flags: eg.baseReduxState.flags,
mute: [],
ownUser: eg.selfUser,
subscriptions: [],
theme: 'default',
twentyFourHourTime: false,
});

const baseProps = deepFreeze({
const baseSelectorProps = deepFreeze({
backgroundData: baseBackgroundData,
initialScrollMessageId: null,
fetching: { older: false, newer: false },
messages: [],
renderedMessages: [],
typingUsers: [],
});

type FudgedProps = {|
...Props,

// `intl` property is complicated and not worth testing
_: $FlowFixMe,
|};

const baseProps: FudgedProps = deepFreeze({
narrow: HOME_NARROW,
showMessagePlaceholders: false,
startEditMessage: jest.fn(),
dispatch: jest.fn(),
...baseSelectorProps,
showActionSheetWithOptions: jest.fn(),

_: jest.fn(),
});

test('missing prev and next props returns no messages', () => {
Expand All @@ -41,6 +72,7 @@ describe('getUpdateEvents', () => {
expect(messages).toEqual([
{
type: 'fetching',
showMessagePlaceholders: nextProps.showMessagePlaceholders,
fetchingNewer: true,
fetchingOlder: false,
},
Expand Down Expand Up @@ -69,7 +101,7 @@ describe('getUpdateEvents', () => {
};
const nextProps = {
...baseProps,
typingUsers: [{ id: 10 }],
typingUsers: [eg.makeUser()],
};

const messages = getUpdateEvents(prevProps, nextProps);
Expand All @@ -95,29 +127,15 @@ describe('getUpdateEvents', () => {

test('when the rendered messages differ (even deeply) a "content" message is returned', () => {
const prevProps = {
backgroundData: {
alertWords: {},
auth: { realm: '' },
flags: { starred: {}, has_alert_word: {} },
ownUser: { user_id: 1432 },
},
narrow: HOME_NARROW,
messages: [],
...baseProps,
renderedMessages: [{ key: 0, data: [], message: {} }],
};
const nextProps = {
backgroundData: {
alertWords: {},
auth: { realm: '' },
flags: { starred: {}, has_alert_word: {} },
ownUser: { user_id: 1432 },
},
narrow: HOME_NARROW,
messages: [],
...baseProps,
renderedMessages: [
{
key: 0,
data: [{ key: 123, type: 'message', isBrief: false, message: { id: 0, reactions: [] } }],
data: [{ key: 123, type: 'message', isBrief: false, message: eg.streamMessage() }],
message: {},
},
],
Expand All @@ -140,7 +158,7 @@ describe('getUpdateEvents', () => {
...baseProps,
narrow: HOME_NARROW,
fetching: { older: false, newer: true },
typingUsers: [{ id: 10 }],
typingUsers: [eg.makeUser()],
};

const messages = getUpdateEvents(prevProps, nextProps);
Expand All @@ -152,33 +170,19 @@ describe('getUpdateEvents', () => {

test('when there are several diffs but messages differ too return only a single "content" message', () => {
const prevProps = {
backgroundData: {
alertWords: {},
auth: { realm: '' },
flags: { starred: {}, has_alert_word: {} },
ownUser: { user_id: 1432 },
},
narrow: HOME_NARROW,
...baseProps,
fetching: { older: false, newer: false },
typingUsers: [],
messages: [],
renderedMessages: [{ key: 0, data: [], message: {} }],
};
const nextProps = {
backgroundData: {
alertWords: {},
auth: { realm: '' },
flags: { starred: {}, has_alert_word: {} },
ownUser: { user_id: 1432 },
},
narrow: HOME_NARROW,
...baseProps,
fetching: { older: false, newer: true },
typingUsers: [{ id: 10 }],
messages: [],
typingUsers: [eg.makeUser()],
renderedMessages: [
{
key: 0,
data: [{ key: 123, type: 'message', isBrief: false, message: { id: 0, reactions: [] } }],
data: [{ key: 123, type: 'message', isBrief: false, message: eg.streamMessage() }],
message: {},
},
],
Expand All @@ -191,18 +195,28 @@ describe('getUpdateEvents', () => {
});

test('if a new message is read send a "read" update', () => {
const message1 = eg.streamMessage({ id: 1 });
const message2 = eg.streamMessage({ id: 2 });
const message3 = eg.streamMessage({ id: 3 });

const prevProps = {
auth: { realm: '' },
typingUsers: [],
...baseProps,
backgroundData: {
flags: { read: { 2: true } },
...baseBackgroundData,
flags: {
...baseBackgroundData.flags,
read: { [message2.id]: true },
},
},
};
const nextProps = {
auth: { realm: '' },
typingUsers: [],
...baseProps,
backgroundData: {
flags: { read: { 1: true, 2: true, 3: true } },
...baseBackgroundData,
flags: {
...baseBackgroundData.flags,
read: { [message1.id]: true, [message2.id]: true, [message3.id]: true },
},
},
};

Expand All @@ -216,16 +230,21 @@ describe('getUpdateEvents', () => {
});

describe('flagsStateToStringList', () => {
const message1 = eg.streamMessage({ id: 1 });
const message2 = eg.streamMessage({ id: 2 });
const message3 = eg.streamMessage({ id: 3 });

const flags = {
...eg.baseReduxState.flags,
read: {
1: true,
2: true,
[message1.id]: true,
[message2.id]: true,
},
starred: {
1: true,
3: true,
[message1.id]: true,
[message3.id]: true,
},
mentions: {},
mentioned: {},
// ...
// the actual state keeps track of many more flags
};
Expand Down
20 changes: 11 additions & 9 deletions src/webview/html/__tests__/render-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
/* @flow strict-local */
import messageTypingAsHtml from '../messageTypingAsHtml';
import * as eg from '../../../__tests__/lib/exampleData';

describe('typing', () => {
it('escapes values', () => {
expect(
messageTypingAsHtml('&<r', [
{
avatar_url: '&<av',
email: '&<e',
},
]),
).not.toContain('&<');
it('escapes &< (e.g., in `avatar_url` and `email`', () => {
const name = '&<name';
const user = {
...eg.makeUser({ name }),
avatar_url: `https://zulip.example.org/yo/avatar-${name}.png`,
email: `${name}@example.com`,
};

expect(messageTypingAsHtml('&<r', [user])).not.toContain('&<');
});
});