Skip to content

narrow [nfc]: Cut Narrow from the Outbox type. #3889

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

Closed
gnprice wants to merge 5 commits into from
+39 −37
Closed

narrow [nfc]: Cut Narrow from the Outbox type. #3889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
060751e
logging jsdoc: Move "see also" before parameters, to fix parse.
gnprice Feb 10, 2020
23ad3c1
outbox [nfc]: Replace narrowContains with something more explicit.
gnprice Feb 6, 2020
31dfe88
outbox [nfc]: Confirm a conjecture, to use `narrow` a bit less.
gnprice Feb 10, 2020
55da6ea
outbox [nfc]: Send based on `display_recipient`, not `narrow`.
gnprice Feb 10, 2020
f09a0f2
outbox [nfc]: And cut the Narrow property entirely!
gnprice Feb 6, 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
4 changes: 2 additions & 2 deletions src/chat/narrowsSelectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {
isPrivateNarrow,
isStreamOrTopicNarrow,
emailsOfGroupNarrow,
narrowContains,
narrowContainsOutbox,
} from '../utils/narrow';
import { shouldBeMuted } from '../utils/message';
import { NULL_ARRAY, NULL_SUBSCRIPTION } from '../nullObjects';
Expand All @@ -39,7 +39,7 @@ export const outboxMessagesForNarrow: Selector<Outbox[], Narrow> = createSelecto
if (!caughtUp.newer) {
return NULL_ARRAY;
}
const filtered = outboxMessages.filter(item => narrowContains(narrow, item.narrow));
const filtered = outboxMessages.filter(item => narrowContainsOutbox(narrow, item));
return isEqual(filtered, outboxMessages) ? outboxMessages : filtered;
},
);
Expand Down
22 changes: 10 additions & 12 deletions src/outbox/outboxActions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,17 +64,16 @@ export const trySendMessages = (dispatch: Dispatch, getState: GetState): boolean
return; // i.e., continue
}

const to = ((): string => {
const { narrow } = item;
// TODO: can this test be `if (item.type === private)`?
if (isPrivateOrGroupNarrow(narrow)) {
return narrow[0].operand;
} else {
// HACK: the server attempts to interpret this argument as JSON, then
// CSV, then a literal. To avoid misparsing, always use JSON.
return JSON.stringify([item.display_recipient]);
}
})();
// prettier-ignore
const to =
item.type === 'private'
Copy link
Copy Markdown
Contributor

@rk-for-zulip rk-for-zulip Feb 11, 2020

Choose a reason for hiding this comment

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

🎉

// This will include the self user, possibly twice. That's
// fine; on send, the server (since at least 2013) drops dupes
// and normalizes whether to include the sender.
? item.display_recipient.map(r => r.email).join(',')
Comment on lines +70 to +73
Copy link
Copy Markdown
Contributor

@rk-for-zulip rk-for-zulip Feb 11, 2020

Choose a reason for hiding this comment

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

I wouldn't want to just .slice off an element – but it does feel like the self-user should be .filtered out here, rather than relying on the permissiveness of the server implementation.

Also, if these are emails rather than IDs, we should probably use JSON rather than CSV. (If I'd realized that's what they were, I would have done the additional testing on #3734. 😞) I'm not about to ask you to do that for this PR, but tagging it with an inline TODO: would be appreciated.

Copy link
Copy Markdown
Member Author

@gnprice gnprice Nov 3, 2020

Choose a reason for hiding this comment

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

In #4299 I ended up taking care of that duplicate self-user by fixing how we construct display_recipient in the first place, elsewhere in this file. Having it wrong there turns out to cause its own buggy behavior, before any request even reaches the server.

// HACK: the server attempts to interpret this argument as JSON, then
// CSV, then a literal. To avoid misparsing, always use JSON.
: JSON.stringify([item.display_recipient]);

await api.sendMessage(auth, {
type: item.type,
Expand Down Expand Up @@ -164,7 +163,6 @@ export const addToOutbox = (narrow: Narrow, content: string) => async (
const localTime = Math.round(new Date().getTime() / 1000);
dispatch(
messageSendStart({
narrow,
isSent: false,
...extractTypeToAndSubjectFromNarrow(narrow, getUsersByEmail(state), userDetail),
markdownContent: content,
Expand Down
7 changes: 3 additions & 4 deletions src/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import type { IntlShape } from 'react-intl';
import type { DangerouslyImpreciseStyleProp } from 'react-native/Libraries/StyleSheet/StyleSheet';

import type { Auth, Topic, Message, Reaction, ReactionType, Narrow } from './api/apiTypes';
import type { Auth, Topic, Message, Reaction, ReactionType } from './api/apiTypes';
import type { ZulipVersion } from './utils/zulipVersion';

export type * from './generics';
Expand Down Expand Up @@ -171,10 +171,9 @@ export type Outbox = {|
*/
isSent: boolean,

// These fields don't exist in `Message`.
// They're used for sending the message to the server.
// `markdownContent` doesn't exist in `Message`.
// It's used for sending the message to the server.
markdownContent: string,
narrow: Narrow,

// These fields are modeled on `Message`.
avatar_url: string | null,
Expand Down
12 changes: 6 additions & 6 deletions src/utils/logging.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,14 +124,14 @@ const makeLogFunction = ({ consoleMethod, severity }: LogParams): LogFunction =>
* represents a bug. For conditions that can happen without a bug (e.g. a
* failure to reach the server), consider `logging.warn`.
*
* See also:
* * `logging.warn` for logging at lower severity
*
* @param event A string describing the nature of the event to be logged, or an
* exception whose `.message` is such a string. Related events should have
* identical such strings, when feasible.
* @param extras Diagnostic data which may differ between separate occurrences
* of the event.
*
* See also:
* * `logging.warn` for logging at lower severity
*/
export const error: (event: string | Error, extras?: Extras) => void = makeLogFunction({
consoleMethod: console.error,
Expand All @@ -148,14 +148,14 @@ export const error: (event: string | Error, extras?: Extras) => void = makeLogFu
* which have an inevitable background rate. For conditions which
* definitely represent a bug in the app, consider `logging.error` instead.
*
* See also:
* * `logging.error` for logging at higher severity
*
* @param event A string describing the nature of the event to be logged, or an
* exception whose `.message` is such a string. Related events should have
* identical such strings, when feasible.
* @param extras Diagnostic data which may differ between separate occurrences
* of the event.
*
* See also:
* * `logging.error` for logging at higher severity
*/
export const warn: (event: string | Error, extras?: Extras) => void = makeLogFunction({
consoleMethod: console.warn,
Expand Down
31 changes: 18 additions & 13 deletions src/utils/narrow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,19 +248,24 @@ export const canSendToNarrow = (narrow: Narrow): boolean =>
search: () => false,
});

/** True just if `haystack` contains all possible messages in `needle`. */
export const narrowContains = (haystack: Narrow, needle: Narrow): boolean => {
if (isHomeNarrow(haystack)) {
return true;
}
if (isAllPrivateNarrow(haystack) && isPrivateOrGroupNarrow(needle)) {
return true;
}
if (isStreamNarrow(haystack) && needle[0].operand === haystack[0].operand) {
return true;
}
return JSON.stringify(needle) === JSON.stringify(haystack);
};
export const narrowContainsOutbox = (haystack: Narrow, needle: Outbox): boolean =>
Comment thread
gnprice marked this conversation as resolved.
caseNarrowPartial(haystack, {
stream: name => needle.type === 'stream' && needle.display_recipient === name,
topic: (streamName, topic) =>
needle.type === 'stream'
&& needle.display_recipient === streamName
&& needle.subject === topic,
pm: emails => emails === needle.display_recipient.map(r => r.email).join(','),
Copy link
Copy Markdown
Contributor

@rk-for-zulip rk-for-zulip Feb 11, 2020

Choose a reason for hiding this comment

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

This line (pm: emails => ...) doesn't look like it will work, for multiple reasons:

  • It'll fail if narrow[0].operand and display_recipient are not in the same order. (In practice, we seem to frequently rely on serializations being consistent. 😞)
  • If one of your comments in a later commit is accurate – and, more questionably, if I'm understanding the code correctly – the left-hand side will lack the self-user, while the right-hand side will have it.
  • Both of the above bug(?)s are concealed by the fact that there's no groupPm case in this caseNarrowPartial. (I'd suggest using caseNarrow here instead.)
  • It's a TypeError when needle.type === 'stream', as "stream name".map is not a function.

Copy link
Copy Markdown
Member Author

@gnprice gnprice Oct 22, 2020

Choose a reason for hiding this comment

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

Eek, good catch re: missing the groupPm case!

I suspect what happened there is that this change (and the others in this branch) was originally taken from a longer Narrow-refactoring series where it came after a refactor to unify the pm and groupPm cases. I have a couple of longer Narrow-refactoring series as drafts, and I'm pretty sure I made that unification somewhere in there. Definitely caseNarrow, the total version, is the right one to use here for just that sort of reason; I have no recollection of what I might have been thinking when I wrote caseNarrowPartial instead.

  • It'll fail if narrow[0].operand and display_recipient are not in the same order. (In practice, we seem to frequently rely on serializations being consistent. disappointed)

Yeah, and I believe that's already the case in the existing version of this code, as a result of just string-comparing the results of JSON.stringify. Given that (which I'll re-check empirically, because although I think I did so originally, it's been some months since then and I'm not certain I did), I'm OK with preserving that behavior in the course of this basically orthogonal refactor. Probably deserves a comment about it, though.

  • If … the left-hand side will lack the self-user, while the right-hand side will have it.
  • It's a TypeError when needle.type === 'stream', as "stream name".map is not a function.

Hmm. Will check on these. And probably write some unit tests for this function, as I seem to have demonstrated the need for them >_>

Copy link
Copy Markdown
Member Author

@gnprice gnprice Oct 23, 2020

Choose a reason for hiding this comment

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

JSON.stringify … which I'll re-check empirically

Ah, no, here it is more directly:

  • In the old code, we're comparing the JSON.stringify of each narrow, which means the sequences of operators and operands must be equal. In particular, narrow[0].operand must be equal.
  • The display_recipient on an outbox message is constructed from the same Narrow value (passed to addToOutbox) that in the old code was included as a property. A quick look at the private helpers of addToOutbox shows that it's computed like so:
  narrow[0].operand
    .split(',')
    .map(item => {
      const user = usersByEmail.get(item) || NULL_USER;
      return { email: item, id: user.user_id, full_name: user.full_name };
    })
    .concat({ email: selfDetail.email, id: selfDetail.user_id, full_name: selfDetail.full_name });

So yep, we've been counting on the order of these.

I think that same look at the code also confirms that the display_recipient will have the self user added at the end, like you said, while the narrow's list of emails won't.

Copy link
Copy Markdown
Member Author

@gnprice gnprice Nov 3, 2020

Choose a reason for hiding this comment

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

In #4299 I ended up dropping this implementation entirely... because there's already an isMessageInNarrow! Which I noticed partly as a consequence of #3889 (comment) above. That function needed some significant work to become well tested, and then that testing turned up some bugs which needed fixing; those changes were #4294, and #4299 returns to the outbox code to switch to using that.


home: () => true,
allPrivate: () => needle.type === 'private',
starred: () => false,

// These two are uncommon cases it'd take some work to get right; just
// leave the outbox messages out.
Comment on lines +264 to +265
Copy link
Copy Markdown
Contributor

@rk-for-zulip rk-for-zulip Feb 11, 2020

Choose a reason for hiding this comment

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

It's almost certainly not worth the trouble to fix at the moment, but I'd suggest explicitly marking this as a // BUG:, and possibly filing an issue.

mentioned: () => false,
search: () => false,
});

export const getNarrowFromMessage = (message: Message | Outbox, ownEmail: string) => {
if (Array.isArray(message.display_recipient)) {
Expand Down