Mirror
/
zulip
mirror of https://github.com/zulip/zulip.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

drafts: Use stream_id instead of stream names. 

This should cause no functional changes.

This is part of a multi-step effort to move away
from using stream names to reference streams, now
that it's impossible for a user to write a message
with an invalid stream name (since switching to
the dropdown).
This commit is contained in:
evykassirer 2023-08-03 22:36:15 -07:00 committed by Tim Abbott
parent 846b470b99
commit 37ce223b91
2 changed files with 56 additions and 67 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

41
web/src/drafts.js
View File

@ -164,14 +164,6 @@ export function rename_stream_recipient(old_stream_id, old_topic, new_stream_id,
// If new_stream_id is undefined, that means the stream wasn't updated.
if (new_stream_id !== undefined) {
draft.stream_id = new_stream_id;
// TODO: For now we need both a stream_id and stream (stream name)
// because there can be partial input in the stream field.
// Once we complete our UI plan to change the stream input field
// to a dropdown_list_widget, there will no longer be the possibility
// of invalid partial input in the stream field, and we can have the
// drafts system ignore the legacy `stream` field, using only `stream_id`.
// After enough drafts are autodeleted, we'd no longer have a `stream` field.
draft.stream = sub_store.get(new_stream_id).name;
}
// If new_topic is undefined, that means the topic wasn't updated.
if (new_topic !== undefined) {
@ -213,9 +205,11 @@ export function restore_message(draft) {
let compose_args;
if (draft.type === "stream") {
const stream_name = stream_data.get_stream_name_from_id(draft.stream_id);
compose_args = {
type: "stream",
stream_id: draft.stream_id,
stream_name,
topic: draft.topic,
content: draft.content,
};
@ -293,10 +287,10 @@ export function restore_draft(draft_id) {
const compose_args = restore_message(draft);
if (compose_args.type === "stream") {
if (draft.stream !== "" && draft.topic !== "") {
if (draft.stream_id !== "" && draft.topic !== "") {
narrow.activate(
[
{operator: "stream", operand: compose_args.stream},
{operator: "stream", operand: compose_args.stream_name},
{operator: "topic", operand: compose_args.topic},
],
{trigger: "restore draft"},
@ -344,26 +338,23 @@ export function format_draft(draft) {
if (draft.type === "stream") {
// In case there is no stream for the draft, we need a
// single space char for proper rendering of the stream label
const space_string = new Handlebars.SafeString(" ");
let stream_name = draft.stream.length > 0 ? draft.stream : space_string;
let stream_name = new Handlebars.SafeString(" ");
let sub;
if (draft.stream_id) {
const sub = sub_store.get(draft.stream_id);
if (sub) {
invite_only = sub.invite_only;
is_web_public = sub.is_web_public;
if (sub.name !== stream_name) {
stream_name = sub.name;
draft.stream = stream_name;
draft_model.editDraft(id, draft);
}
}
} else {
sub = sub_store.get(draft.stream_id);
} else if (draft.stream && draft.stream.length > 0) {
// draft.stream is deprecated but might still exist on old drafts
stream_name = draft.stream;
const sub = stream_data.get_sub(stream_name);
if (sub !== undefined) {
if (sub) {
draft.stream_id = sub.stream_id;
}
}
if (sub) {
stream_name = sub.name;
invite_only = sub.invite_only;
is_web_public = sub.is_web_public;
}
const draft_topic = draft.topic || compose.empty_topic_placeholder();
const draft_stream_color = stream_data.get_color(draft.stream_id);

82
web/tests/drafts.test.js
View File

@ -67,7 +67,6 @@ const messages_overlay_ui = zrequire("messages_overlay_ui");
const timerender = zrequire("timerender");
const draft_1 = {
stream: "stream",
stream_id: 30,
topic: "topic",
type: "stream",
@ -80,7 +79,6 @@ const draft_2 = {
content: "Test direct message",
};
const short_msg = {
stream: "stream",
stream_id: 30,
topic: "topic",
type: "stream",
@ -180,7 +178,7 @@ test("snapshot_message", ({override_rewire}) => {
const stream = {
stream_id: draft_1.stream_id,
name: draft_1.stream,
name: "stream name",
};
stream_data.add_sub(stream);
compose_state.set_stream_id(stream.stream_id);
@ -221,7 +219,6 @@ test("initialize", ({override_rewire}) => {
test("remove_old_drafts", () => {
const draft_3 = {
stream: "stream",
topic: "topic",
type: "stream",
content: "Test stream message",
@ -325,7 +322,6 @@ test("rename_stream_recipient", ({override_rewire}) => {
stream_data.add_sub(stream_B);
const draft_1 = {
stream: stream_A.name,
stream_id: stream_A.stream_id,
topic: "a",
type: "stream",
@ -333,7 +329,6 @@ test("rename_stream_recipient", ({override_rewire}) => {
updatedAt: Date.now(),
};
const draft_2 = {
stream: stream_A.name,
stream_id: stream_A.stream_id,
topic: "b",
type: "stream",
@ -341,7 +336,6 @@ test("rename_stream_recipient", ({override_rewire}) => {
updatedAt: Date.now(),
};
const draft_3 = {
stream: stream_B.name,
stream_id: stream_B.stream_id,
topic: "a",
type: "stream",
@ -349,7 +343,6 @@ test("rename_stream_recipient", ({override_rewire}) => {
updatedAt: Date.now(),
};
const draft_4 = {
stream: stream_B.name,
stream_id: stream_B.stream_id,
topic: "c",
type: "stream",
@ -361,39 +354,39 @@ test("rename_stream_recipient", ({override_rewire}) => {
ls.set("drafts", data);
const draft_model = drafts.draft_model;
function assert_draft(draft_id, stream_name, topic_name) {
function assert_draft(draft_id, stream_id, topic_name) {
const draft = draft_model.getDraft(draft_id);
assert.equal(draft.topic, topic_name);
assert.equal(draft.stream, stream_name);
assert.equal(draft.stream_id, stream_id);
}
// There are no drafts in B>b, so moving messages from there doesn't change drafts
drafts.rename_stream_recipient(stream_B.stream_id, "b", undefined, "c");
assert_draft("id1", "A", "a");
assert_draft("id2", "A", "b");
assert_draft("id3", "B", "a");
assert_draft("id4", "B", "c");
assert_draft("id1", stream_A.stream_id, "a");
assert_draft("id2", stream_A.stream_id, "b");
assert_draft("id3", stream_B.stream_id, "a");
assert_draft("id4", stream_B.stream_id, "c");
// Update with both stream and topic changes Bc -> Aa
drafts.rename_stream_recipient(stream_B.stream_id, "c", stream_A.stream_id, "a");
assert_draft("id1", "A", "a");
assert_draft("id2", "A", "b");
assert_draft("id3", "B", "a");
assert_draft("id4", "A", "a");
assert_draft("id1", stream_A.stream_id, "a");
assert_draft("id2", stream_A.stream_id, "b");
assert_draft("id3", stream_B.stream_id, "a");
assert_draft("id4", stream_A.stream_id, "a");
// Update with only stream change Aa -> Ba
drafts.rename_stream_recipient(stream_A.stream_id, "a", stream_B.stream_id, undefined);
assert_draft("id1", "B", "a");
assert_draft("id2", "A", "b");
assert_draft("id3", "B", "a");
assert_draft("id4", "B", "a");
assert_draft("id1", stream_B.stream_id, "a");
assert_draft("id2", stream_A.stream_id, "b");
assert_draft("id3", stream_B.stream_id, "a");
assert_draft("id4", stream_B.stream_id, "a");
// Update with only topic change, affecting three messages
drafts.rename_stream_recipient(stream_B.stream_id, "a", undefined, "e");
assert_draft("id1", "B", "e");
assert_draft("id2", "A", "b");
assert_draft("id3", "B", "e");
assert_draft("id4", "B", "e");
assert_draft("id1", stream_B.stream_id, "e");
assert_draft("id2", stream_A.stream_id, "b");
assert_draft("id3", stream_B.stream_id, "e");
assert_draft("id4", stream_B.stream_id, "e");
});
// There were some buggy drafts that had their topics
@ -415,7 +408,6 @@ test("catch_buggy_draft_error", () => {
stream_data.add_sub(stream_B);
const buggy_draft = {
stream: stream_B.name,
stream_id: stream_B.stream_id,
topic: undefined,
type: "stream",
@ -436,7 +428,7 @@ test("catch_buggy_draft_error", () => {
"new_topic",
);
const draft = draft_model.getDraft("id1");
assert.equal(draft.stream, stream_B.name);
assert.equal(draft.stream_id, stream_B.stream_id);
assert.equal(draft.topic, undefined);
});
@ -444,7 +436,6 @@ test("fix_buggy_draft", ({override_rewire}) => {
override_rewire(drafts, "set_count", noop);
const buggy_draft = {
stream: "stream name",
stream_id: 1,
// This is the bug: topic never be undefined for a stream
// message draft.
@ -460,7 +451,7 @@ test("fix_buggy_draft", ({override_rewire}) => {
drafts.fix_drafts_with_undefined_topics();
const draft = draft_model.getDraft("id1");
assert.equal(draft.stream, "stream name");
assert.equal(draft.stream_id, buggy_draft.stream_id);
assert.equal(draft.topic, "");
});
@ -489,7 +480,6 @@ test("format_drafts", ({override_rewire, mock_template}) => {
}
const draft_1 = {
stream: "stream",
topic: "topic",
type: "stream",
content: "Test stream message",
@ -504,9 +494,9 @@ test("format_drafts", ({override_rewire, mock_template}) => {
updatedAt: date(-1),
};
const draft_3 = {
stream: "stream 2",
topic: "topic",
type: "stream",
stream_id: 40,
content: "Test stream message 2",
updatedAt: date(-10),
};
@ -564,12 +554,12 @@ test("format_drafts", ({override_rewire, mock_template}) => {
draft_id: "id3",
is_stream: true,
stream_name: "stream 2",
stream_id: 40,
recipient_bar_color: "#ebebeb",
stream_privacy_icon_color: "#b9b9b9",
topic: "topic",
raw_content: "Test stream message 2",
time_stamp: "Jan 21",
stream_id: undefined,
invite_only: false,
is_web_public: false,
},
@ -589,8 +579,11 @@ test("format_drafts", ({override_rewire, mock_template}) => {
);
override_rewire(sub_store, "get", (stream_id) => {
assert.equal(stream_id, 30);
return {name: "stream"};
assert.ok([30, 40].includes(stream_id));
if (stream_id === 30) {
return {name: "stream", stream_id};
}
return {name: "stream 2", stream_id, invite_only: false, is_web_public: false};
});
override_rewire(user_pill, "get_user_ids", () => []);
@ -618,8 +611,11 @@ test("format_drafts", ({override_rewire, mock_template}) => {
$("#draft_overlay").css = () => {};
override_rewire(sub_store, "get", (stream_id) => {
assert.equal(stream_id, 30);
return {name: "stream-rename"};
assert.ok([30, 40].includes(stream_id));
if (stream_id === 30) {
return {name: "stream-rename", stream_id};
}
return {name: "stream 2", stream_id, invite_only: false, is_web_public: false};
});
expected[0].stream_name = "stream-rename";
@ -641,7 +637,6 @@ test("filter_drafts", ({override_rewire, mock_template}) => {
}
const stream_draft_1 = {
stream: "stream",
topic: "topic",
type: "stream",
content: "Test stream message",
@ -656,9 +651,9 @@ test("filter_drafts", ({override_rewire, mock_template}) => {
updatedAt: date(-1),
};
const stream_draft_2 = {
stream: "stream 2", // TODO_STREAM_ID
topic: "topic",
type: "stream",
stream_id: 40,
content: "Test stream message 2",
updatedAt: date(-10),
};
@ -719,7 +714,7 @@ test("filter_drafts", ({override_rewire, mock_template}) => {
draft_id: "id3",
is_stream: true,
stream_name: "stream 2",
stream_id: undefined,
stream_id: 40,
recipient_bar_color: "#ebebeb",
stream_privacy_icon_color: "#b9b9b9",
topic: "topic",
@ -750,8 +745,11 @@ test("filter_drafts", ({override_rewire, mock_template}) => {
);
override_rewire(sub_store, "get", (stream_id) => {
assert.equal(stream_id, 30);
return {name: "stream", invite_only: false, is_web_public: false};
assert.ok([30, 40].includes(stream_id));
if (stream_id === 30) {
return {name: "stream", stream_id, invite_only: false, is_web_public: false};
}
return {name: "stream 2", stream_id, invite_only: false, is_web_public: false};
});
mock_template("draft_table_body.hbs", false, (data) => {