From e26b4bbd3e37c2b6b755142f0bd1603561b538b2 Mon Sep 17 00:00:00 2001 From: evykassirer Date: Fri, 2 Feb 2024 11:56:49 -0800 Subject: [PATCH] drafts: Reimplement handlers for buggy drafts using zod. --- web/src/drafts.js | 116 +++++++++++++++++++++++++------------- web/tests/compose.test.js | 1 + web/tests/drafts.test.js | 115 ++++++++++++++++--------------------- 3 files changed, 127 insertions(+), 105 deletions(-) diff --git a/web/src/drafts.js b/web/src/drafts.js index b1767aed99..6f023fa9c1 100644 --- a/web/src/drafts.js +++ b/web/src/drafts.js @@ -3,6 +3,7 @@ import Handlebars from "handlebars/runtime"; import $ from "jquery"; import _ from "lodash"; import tippy from "tippy.js"; +import {z} from "zod"; import render_confirm_delete_all_drafts from "../templates/confirm_dialog/confirm_delete_all_drafts.hbs"; @@ -30,18 +31,94 @@ function getTimestamp() { return Date.now(); } +const possibly_buggy_draft_schema = z.intersection( + z.object({ + content: z.string(), + updatedAt: z.number(), + }), + z.union([ + z.object({ + type: z.literal("stream"), + topic: z.string().optional(), + stream_id: z.number().optional(), + stream: z.string().optional(), + }), + z.object({ + type: z.literal("private"), + reply_to: z.string(), + private_message_recipient: z.string(), + }), + ]), +); + +const possibly_buggy_drafts_schema = z.record(z.string(), possibly_buggy_draft_schema); + export const draft_model = (function () { const exports = {}; // the key that the drafts are stored under. const KEY = "drafts"; const ls = localstorage(); + let fixed_buggy_drafts = false; function get() { - return ls.get(KEY) || {}; + const drafts = ls.get(KEY); + if (ls.get(KEY) === undefined) { + return {}; + } + if (!fixed_buggy_drafts) { + fix_buggy_drafts(); + return ls.get(KEY); + } + return drafts; } exports.get = get; + function fix_buggy_drafts() { + const drafts = ls.get(KEY); + const parsed_drafts = possibly_buggy_drafts_schema.parse(drafts); + const valid_drafts = {}; + for (const [draft_id, draft] of Object.entries(parsed_drafts)) { + if (draft.type !== "stream") { + valid_drafts[draft_id] = draft; + continue; + } + + // draft.stream is deprecated but might still exist on old drafts + if (draft.stream !== undefined) { + const sub = stream_data.get_sub(draft.stream); + if (sub) { + draft.stream_id = sub.stream_id; + } + delete draft.stream; + } + + // A one-time fix for buggy drafts that had their topics renamed to + // `undefined` when the topic was moved to another stream without + // changing the topic. The bug was introduced in + // 4c8079c49a81b08b29871f9f1625c6149f48b579 and fixed in + // aebdf6af8c6675fbd2792888d701d582c4a1110a; but servers running + // intermediate versions may have generated some bugged drafts with + // this invalid topic value. + // + // TODO/compatibility: This can be deleted once servers can no longer + // directly upgrade from Zulip 6.0beta1 and earlier development branch where the bug was present, + // since we expect bugged drafts will have either been run through + // this code or else been deleted after 30 (DRAFT_LIFETIME) days. + if (draft.topic === undefined) { + draft.topic = ""; + } + + valid_drafts[draft_id] = { + ...draft, + topic: draft.topic, + }; + } + ls.set(KEY, valid_drafts); + set_count(Object.keys(valid_drafts).length); + fixed_buggy_drafts = true; + } + exports.getDraft = function (id) { return get()[id] || false; }; @@ -97,32 +174,6 @@ export const draft_model = (function () { return exports; })(); -// A one-time fix for buggy drafts that had their topics renamed to -// `undefined` when the topic was moved to another stream without -// changing the topic. The bug was introduced in -// 4c8079c49a81b08b29871f9f1625c6149f48b579 and fixed in -// aebdf6af8c6675fbd2792888d701d582c4a1110a; but servers running -// intermediate versions may have generated some bugged drafts with -// this invalid topic value. -// -// TODO/compatibility: This can be deleted once servers can no longer -// directly upgrade from Zulip 6.0beta1 and earlier development branch where the bug was present, -// since we expect bugged drafts will have either been run through -// this code or else been deleted after 30 (DRAFT_LIFETIME) days. -let fixed_buggy_drafts = false; -export function fix_drafts_with_undefined_topics() { - const data = draft_model.get(); - for (const draft_id of Object.keys(data)) { - const draft = data[draft_id]; - if (draft.type === "stream" && draft.topic === undefined) { - const draft = data[draft_id]; - draft.topic = ""; - draft_model.editDraft(draft_id, draft); - } - } - fixed_buggy_drafts = true; -} - export function sync_count() { const drafts = draft_model.get(); set_count(Object.keys(drafts).length); @@ -391,13 +442,6 @@ export function format_draft(draft) { let sub; if (draft.stream_id) { 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) { - draft.stream_id = sub.stream_id; - } } if (sub) { stream_name = sub.name; @@ -439,10 +483,6 @@ export function format_draft(draft) { export function initialize() { remove_old_drafts(); - if (!fixed_buggy_drafts) { - fix_drafts_with_undefined_topics(); - } - window.addEventListener("beforeunload", () => { update_draft(); }); diff --git a/web/tests/compose.test.js b/web/tests/compose.test.js index a75735711e..ba2b173c78 100644 --- a/web/tests/compose.test.js +++ b/web/tests/compose.test.js @@ -219,6 +219,7 @@ test_ui("send_message_success", ({override, override_rewire}) => { test_ui("send_message", ({override, override_rewire, mock_template}) => { mock_banners(); MockDate.set(new Date(fake_now * 1000)); + override_rewire(drafts, "sync_count", noop); // This is the common setup stuff for all of the four tests. let stub_state; diff --git a/web/tests/drafts.test.js b/web/tests/drafts.test.js index ce92f9563c..a0d06bc299 100644 --- a/web/tests/drafts.test.js +++ b/web/tests/drafts.test.js @@ -9,7 +9,6 @@ const {run_test, noop} = require("./lib/test"); const $ = require("./lib/zjquery"); const {user_settings} = require("./lib/zpage_params"); -const blueslip = zrequire("blueslip"); const compose_pm_pill = zrequire("compose_pm_pill"); const user_pill = zrequire("user_pill"); const people = zrequire("people"); @@ -97,6 +96,54 @@ function test(label, f) { }); } +// There were some buggy drafts that had their topics +// renamed to `undefined` in #23238. +// TODO/compatibility: The next two tests can be deleted +// when we get to delete drafts.fix_drafts_with_undefined_topics. +// +// This test must run before others, so that +// fixed_buggy_drafts is false. +test("fix buggy drafts", ({override_rewire}) => { + override_rewire(drafts, "set_count", noop); + + const stream_A = { + subscribed: false, + name: "A", + stream_id: 1, + }; + stream_data.add_sub(stream_A); + const stream_B = { + subscribed: false, + name: "B", + stream_id: 2, + }; + stream_data.add_sub(stream_B); + + const buggy_draft = { + stream_id: stream_B.stream_id, + topic: undefined, + type: "stream", + content: "Test stream message", + updatedAt: Date.now(), + }; + const data = {id1: buggy_draft}; + const ls = localstorage(); + ls.set("drafts", data); + const draft_model = drafts.draft_model; + + // The draft is fixed in this codepath. + drafts.rename_stream_recipient( + stream_B.stream_id, + "old_topic", + stream_A.stream_id, + "new_topic", + ); + + const draft = draft_model.getDraft("id1"); + assert.equal(draft.stream_id, stream_B.stream_id); + assert.equal(draft.topic, ""); +}); + test("draft_model add", () => { const draft_model = drafts.draft_model; const ls = localstorage(); @@ -378,72 +425,6 @@ test("rename_stream_recipient", ({override_rewire}) => { assert_draft("id4", stream_B.stream_id, "e"); }); -// There were some buggy drafts that had their topics -// renamed to `undefined` in #23238. -// TODO/compatibility: The next two tests can be deleted -// when we get to delete drafts.fix_drafts_with_undefined_topics. -test("catch_buggy_draft_error", () => { - const stream_A = { - subscribed: false, - name: "A", - stream_id: 1, - }; - stream_data.add_sub(stream_A); - const stream_B = { - subscribed: false, - name: "B", - stream_id: 2, - }; - stream_data.add_sub(stream_B); - - const buggy_draft = { - stream_id: stream_B.stream_id, - topic: undefined, - type: "stream", - content: "Test stream message", - updatedAt: Date.now(), - }; - const data = {id1: buggy_draft}; - const ls = localstorage(); - ls.set("drafts", data); - const draft_model = drafts.draft_model; - - // An error is logged but the draft isn't fixed in this codepath. - blueslip.expect("error", "Cannot compare strings; at least one value is undefined"); - drafts.rename_stream_recipient( - stream_B.stream_id, - "old_topic", - stream_A.stream_id, - "new_topic", - ); - const draft = draft_model.getDraft("id1"); - assert.equal(draft.stream_id, stream_B.stream_id); - assert.equal(draft.topic, undefined); -}); - -test("fix_buggy_draft", ({override_rewire}) => { - override_rewire(drafts, "set_count", noop); - - const buggy_draft = { - stream_id: 1, - // This is the bug: topic never be undefined for a stream - // message draft. - topic: undefined, - type: "stream", - content: "Test stream message", - updatedAt: Date.now(), - }; - const data = {id1: buggy_draft}; - const ls = localstorage(); - ls.set("drafts", data); - const draft_model = drafts.draft_model; - - drafts.fix_drafts_with_undefined_topics(); - const draft = draft_model.getDraft("id1"); - assert.equal(draft.stream_id, buggy_draft.stream_id); - assert.equal(draft.topic, ""); -}); - test("delete_all_drafts", () => { const draft_model = drafts.draft_model; const ls = localstorage();