From dcf3733c1321cfe4c24677b902797cc7c6ee0090 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Mon, 1 Nov 2021 13:31:17 +1000 Subject: [PATCH] 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. --- .../discourse/app/controllers/topic.js | 27 +++++++------- .../javascripts/discourse/app/models/topic.js | 10 ++++-- .../tests/acceptance/bookmarks-test.js | 35 +++++++++++++++++++ 3 files changed, 57 insertions(+), 15 deletions(-) diff --git a/app/assets/javascripts/discourse/app/controllers/topic.js b/app/assets/javascripts/discourse/app/controllers/topic.js index 331db6d8ba2..f625c28da3b 100644 --- a/app/assets/javascripts/discourse/app/controllers/topic.js +++ b/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, + }) + ); } }, diff --git a/app/assets/javascripts/discourse/app/models/topic.js b/app/assets/javascripts/discourse/app/models/topic.js index e7c102cfab6..b2b638bdc25 100644 --- a/app/assets/javascripts/discourse/app/models/topic.js +++ b/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 }, }); }, }); diff --git a/app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js b/app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js index c3895abe34f..d5eadc0748a 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js +++ b/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);