FIX: Deleting a for_topic bookmark caused JS error (#14781)

When deleting a for_topic bookmark, we were calling
bookmark.attachedTo() for the bookmarks:changed event,
but the bookmark was not always a Bookmark model instance,
so sometimes that call would error. Now we make sure that
the bookmarks in the topic.bookmarks JS array are all
bookmark model instances, and added a test to cover this
deleting for_topic bookmark case as well.

app/assets/javascripts/discourse/app/controllers/topic.js

@@ -755,11 +755,12 @@ export default Controller.extend(bufferedProperty("model"), {
           (bookmark) => bookmark.post_id === post.id && !bookmark.for_topic
         );
         return this._modifyPostBookmark(
-          bookmarkForPost || {
-            post_id: post.id,
-            topic_id: post.topic_id,
-            for_topic: false,
-          },
+          bookmarkForPost ||
+            Bookmark.create({
+              post_id: post.id,
+              topic_id: post.topic_id,
+              for_topic: false,
+            }),
           post
         );
       } else {
@@ -1228,7 +1229,6 @@ export default Controller.extend(bufferedProperty("model"), {
   },
 
   _modifyTopicBookmark(bookmark) {
-    bookmark = Bookmark.create(bookmark);
     return openBookmarkModal(bookmark, {
       onAfterSave: (savedData) => {
         this._syncBookmarks(savedData);
@@ -1251,7 +1251,6 @@ export default Controller.extend(bufferedProperty("model"), {
   },
 
   _modifyPostBookmark(bookmark, post) {
-    bookmark = Bookmark.create(bookmark);
     return openBookmarkModal(bookmark, {
       onCloseWithoutSaving: () => {
         post.appEvents.trigger("post-stream:refresh", {
@@ -1279,7 +1278,7 @@ export default Controller.extend(bufferedProperty("model"), {
 
     const bookmark = this.model.bookmarks.findBy("id", data.id);
     if (!bookmark) {
-      this.model.bookmarks.pushObject(data);
+      this.model.bookmarks.pushObject(Bookmark.create(data));
     } else {
       bookmark.reminder_at = data.reminder_at;
       bookmark.name = data.name;
@@ -1309,11 +1308,13 @@ export default Controller.extend(bufferedProperty("model"), {
 
     if (this.model.bookmarkCount === 0) {
       const firstPost = await this.model.firstPost();
-      return this._modifyTopicBookmark({
-        post_id: firstPost.id,
-        topic_id: this.model.id,
-        for_topic: true,
-      });
+      return this._modifyTopicBookmark(
+        Bookmark.create({
+          post_id: firstPost.id,
+          topic_id: this.model.id,
+          for_topic: true,
+        })
+      );
     }
   },
 

app/assets/javascripts/discourse/app/models/topic.js

@@ -2,6 +2,7 @@ import { alias, and, equal, notEmpty, or } from "@ember/object/computed";
 import { fmt, propertyEqual } from "discourse/lib/computed";
 import ActionSummary from "discourse/models/action-summary";
 import Category from "discourse/models/category";
+import Bookmark from "discourse/models/bookmark";
 import EmberObject from "@ember/object";
 import I18n from "I18n";
 import PreloadStore from "discourse/lib/preload-store";
@@ -447,7 +448,7 @@ const Topic = RestModel.extend({
       .then(() => {
         this.setProperties({
           deleted_at: new Date(),
-          deleted_by: deleted_by,
+          deleted_by,
           "details.can_delete": false,
           "details.can_recover": true,
         });
@@ -488,6 +489,11 @@ const Topic = RestModel.extend({
       }
     }
     keys.forEach((key) => this.set(key, json[key]));
+
+    if (this.bookmarks.length) {
+      this.bookmarks = this.bookmarks.map((bm) => Bookmark.create(bm));
+    }
+
     return this;
   },
 
@@ -619,7 +625,7 @@ const Topic = RestModel.extend({
 
     return ajax(`/t/${this.id}/tags`, {
       type: "PUT",
-      data: { tags: tags },
+      data: { tags },
     });
   },
 });

app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js

@@ -428,6 +428,41 @@ acceptance("Bookmarking", function (needs) {
     );
   });
 
+  test("Deleting a topic_level bookmark with a reminder", async function (assert) {
+    await visit("/t/internationalization-localization/280");
+    await click("#topic-footer-button-bookmark");
+    await click("#save-bookmark");
+
+    assert.equal(
+      query("#topic-footer-button-bookmark").innerText,
+      I18n.t("bookmarked.edit_bookmark"),
+      "A topic level bookmark button has a label 'Edit Bookmark'"
+    );
+
+    await click("#topic-footer-button-bookmark");
+    await fillIn("input#bookmark-name", "Test name");
+    await click("#tap_tile_tomorrow");
+
+    await click("#topic-footer-button-bookmark");
+    await click("#delete-bookmark");
+
+    assert.ok(exists(".bootbox.modal"), "it asks for delete confirmation");
+    assert.ok(
+      queryAll(".bootbox.modal")
+        .text()
+        .includes(I18n.t("bookmarks.confirm_delete")),
+      "it shows delete confirmation message"
+    );
+
+    await click(".bootbox.modal .btn-primary");
+
+    assert.equal(
+      query("#topic-footer-button-bookmark").innerText,
+      I18n.t("bookmarked.title"),
+      "A topic level bookmark button no longer says 'Edit Bookmark' after deletion"
+    );
+  });
+
   test("The topic level bookmark button opens the edit modal if only one post in the post stream is bookmarked", async function (assert) {
     await visit("/t/internationalization-localization/280");
     await openBookmarkModal(2);