Skip to content

Permit use of commas in stream names #3734

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
gnprice merged 3 commits into from
Feb 7, 2020
Merged

Permit use of commas in stream names #3734

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
240f150
api.sendMessage [nfc]: Accept a params-object.
rk-for-zulip Dec 20, 2019
8158dae
outbox: Discard messages unsendable for more than a week.
rk-for-zulip Jan 8, 2020
e4a83be
sending messages: Always use JSON for `to` field for streams.
rk-for-zulip Dec 20, 2019
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
26 changes: 14 additions & 12 deletions src/api/messages/sendMessage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,20 @@ import { apiPost } from '../apiFetch';
/** See https://zulipchat.com/api/send-message */
export default async (
auth: Auth,
type: 'private' | 'stream',
to: string,
subject: string,
content: string,
localId: number,
eventQueueId: number,
params: {|
type: 'private' | 'stream',
to: string,
subject: string,
content: string,
localId: number,
eventQueueId: number,
|},
): Promise<ApiResponse> =>
apiPost(auth, 'messages', {
type,
to,
subject,
content,
local_id: localId,
queue_id: eventQueueId,
type: params.type,
to: params.to,
subject: params.subject,
content: params.content,
local_id: params.localId,
queue_id: params.eventQueueId,
});
41 changes: 32 additions & 9 deletions src/outbox/outboxActions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,17 +50,40 @@ export const trySendMessages = (dispatch: Dispatch, getState: GetState): boolean
const state = getState();
const auth = getAuth(state);
const outboxToSend = state.outbox.filter(outbox => !outbox.isSent);
const oneWeekAgoTimestamp = Date.now() / 1000 - 60 * 60 * 24 * 7;
try {
outboxToSend.forEach(async item => {
await api.sendMessage(
auth,
item.type,
isPrivateOrGroupNarrow(item.narrow) ? item.narrow[0].operand : item.display_recipient,
item.subject,
item.markdownContent,
item.timestamp,
state.session.eventQueueId,
);
// If a message has spent over a week in the outbox, it's probably too
// stale to try sending it.
//
// TODO: instead of just throwing these away, create an "unsendable" state
// (including a reason for unsendability), and transition old messages to
// that instead.
if (item.timestamp < oneWeekAgoTimestamp) {
dispatch(deleteOutboxMessage(item.id));
return; // i.e., continue
}

const to = ((): string => {
const { narrow } = item;
// TODO: can this test be `if (item.type === private)`?
if (isPrivateOrGroupNarrow(narrow)) {
Comment thread
rk-for-zulip marked this conversation as resolved.
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]);
Copy link
Copy Markdown
Member

@gnprice gnprice Feb 6, 2020

Choose a reason for hiding this comment

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

Huh, interesting that this is a list -- so I think it'll be like ["general"]. Is that really the right form?

The API docs, fwiw, say (at https://zulipchat.com/api/send-message):

The destination stream, or a CSV/JSON-encoded list containing the usernames (emails) of the recipients.

which suggests a list is only applicable for PMs.

When changing the format of what we send in a request, it'd also be good to confirm that older server versions already accept the new format; then if they don't, we'll figure out how to handle that.

Copy link
Copy Markdown
Contributor Author

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

Choose a reason for hiding this comment

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

JSON has been accepted since at least zulip/zulip@97d7d31 – sometime in the Humbug 0.1.4 period, in mid-2013 – and given the presence of json_to_list in the "before" side of that diff, it's probably been accepted for longer than that.

(I'll make a note in the commit message.)

[...] which suggests a list is only applicable for PMs.

Arguably it's a server bug that it tries to interpret to as a list even for stream messages, but that is indeed what it does. (Reported as zulip/zulip#13836.)

Copy link
Copy Markdown
Member

@gnprice gnprice Feb 7, 2020

Choose a reason for hiding this comment

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

Cool. That sure is reassuringly old. :-)

I think then my remaining question is: the commit message says "Always use JSON" for this field; but it looks like the code actually uses JSON just for the stream case, and continues to use a simple CSV for the PMs case.

I think I'd be OK with that difference between the two cases; but I'd want to have the commit message, and the comments, be in agreement with what's happening.

Alternatively, I think it would work well to do as the commit message says: always use JSON. More specifically, always use a JSON-encoded list. In the PMs case, I think this would be just something like

- return narrow[0].operand;
+ return JSON.stringify(narrow[0].operand.split(','));

(And then perhaps celebrate the shared JSON.stringify part by pulling it out to deduplicate.)

}
})();

await api.sendMessage(auth, {
type: item.type,
to,
subject: item.subject,
content: item.markdownContent,
localId: item.timestamp,
eventQueueId: state.session.eventQueueId,
});
dispatch(messageSendComplete(item.timestamp));
});
return true;
Expand Down