From ca539fdccfd2361775342392d8b534ea18550e83 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 30 Apr 2020 10:09:22 +1000 Subject: [PATCH] FIX: Rename all instances of bookmarkWithReminder to just bookmark (#9579) * Rename all instances of bookmarkWithReminder and bookmark_with_reminder to just bookmark * Delete old bookmark code at the same time * Add migration to remove the bookmarkWithReminder post menu item if people have it set in site settings --- .../discourse/app/controllers/topic.js | 12 +----- .../discourse/app/lib/transform-post.js | 1 - .../javascripts/discourse/app/models/post.js | 30 +-------------- .../javascripts/discourse/app/models/topic.js | 15 ++------ .../discourse/app/templates/topic.hbs | 1 - .../discourse/app/widgets/post-menu.js | 38 +++---------------- app/serializers/post_serializer.rb | 17 ++------- config/site_settings.yml | 6 +-- ..._bookmarks_with_reminder_post_menu_item.rb | 28 ++++++++++++++ .../initializers/new-user-narrative.js.es6 | 6 +-- spec/serializers/post_serializer_spec.rb | 22 ++--------- test/javascripts/helpers/site-settings.js | 6 +-- test/javascripts/widgets/post-test.js | 7 +--- 13 files changed, 55 insertions(+), 134 deletions(-) create mode 100644 db/migrate/20200429045956_remove_bookmarks_with_reminder_post_menu_item.rb diff --git a/app/assets/javascripts/discourse/app/controllers/topic.js b/app/assets/javascripts/discourse/app/controllers/topic.js index b934c13543a..a49cbaf2e24 100644 --- a/app/assets/javascripts/discourse/app/controllers/topic.js +++ b/app/assets/javascripts/discourse/app/controllers/topic.js @@ -652,7 +652,7 @@ export default Controller.extend(bufferedProperty("model"), { if (!this.currentUser) { return bootbox.alert(I18n.t("bookmarks.not_bookmarked")); } else if (post) { - return post.toggleBookmarkWithReminder(); + return post.toggleBookmark(); } else { return this.model.toggleBookmark().then(changedIds => { if (!changedIds) { @@ -665,16 +665,6 @@ export default Controller.extend(bufferedProperty("model"), { } }, - toggleBookmarkWithReminder(post) { - if (!this.currentUser) { - return bootbox.alert(I18n.t("bookmarks.not_bookmarked")); - } else if (post) { - return post.toggleBookmarkWithReminder(); - } else { - return this.model.toggleBookmarkWithReminder(); - } - }, - jumpToIndex(index) { this._jumpToIndex(index); }, diff --git a/app/assets/javascripts/discourse/app/lib/transform-post.js b/app/assets/javascripts/discourse/app/lib/transform-post.js index 986dc46c3c6..60823f04fb8 100644 --- a/app/assets/javascripts/discourse/app/lib/transform-post.js +++ b/app/assets/javascripts/discourse/app/lib/transform-post.js @@ -35,7 +35,6 @@ export function transformBasicPost(post) { username: post.username, avatar_template: post.avatar_template, bookmarked: post.bookmarked, - bookmarkedWithReminder: post.bookmarked_with_reminder, bookmarkReminderAt: post.bookmark_reminder_at, bookmarkReminderType: post.bookmark_reminder_type, yours: post.yours, diff --git a/app/assets/javascripts/discourse/app/models/post.js b/app/assets/javascripts/discourse/app/models/post.js index 550a1fbfacb..e7cf6028e57 100644 --- a/app/assets/javascripts/discourse/app/models/post.js +++ b/app/assets/javascripts/discourse/app/models/post.js @@ -310,32 +310,6 @@ const Post = RestModel.extend({ }, toggleBookmark() { - let bookmarkedTopic; - - this.toggleProperty("bookmarked"); - - if (this.bookmarked && !this.get("topic.bookmarked")) { - this.set("topic.bookmarked", true); - bookmarkedTopic = true; - } - - // need to wait to hear back from server (stuff may not be loaded) - - return Post.updateBookmark(this.id, this.bookmarked) - .then(result => { - this.set("topic.bookmarked", result.topic_bookmarked); - this.appEvents.trigger("page:bookmark-post-toggled", this); - }) - .catch(error => { - this.toggleProperty("bookmarked"); - if (bookmarkedTopic) { - this.set("topic.bookmarked", false); - } - throw new Error(error); - }); - }, - - toggleBookmarkWithReminder() { return new Promise(resolve => { let controller = showModal("bookmark", { model: { @@ -357,7 +331,7 @@ const Post = RestModel.extend({ afterSave: savedData => { this.setProperties({ "topic.bookmarked": true, - bookmarked_with_reminder: true, + bookmarked: true, bookmark_reminder_at: savedData.reminderAt, bookmark_reminder_type: savedData.reminderType, bookmark_name: savedData.name, @@ -373,7 +347,7 @@ const Post = RestModel.extend({ bookmark_reminder_type: null, bookmark_name: null, bookmark_id: null, - bookmarked_with_reminder: false + bookmarked: false }); this.appEvents.trigger("page:bookmark-post-toggled", this); } diff --git a/app/assets/javascripts/discourse/app/models/topic.js b/app/assets/javascripts/discourse/app/models/topic.js index 46be3e7feeb..00f8c8839f6 100644 --- a/app/assets/javascripts/discourse/app/models/topic.js +++ b/app/assets/javascripts/discourse/app/models/topic.js @@ -400,7 +400,6 @@ const Topic = RestModel.extend({ afterTopicBookmarked(firstPost) { if (firstPost) { firstPost.set("bookmarked", true); - firstPost.set("bookmarked_with_reminder", true); this.set("bookmark_reminder_at", firstPost.bookmark_reminder_at); return [firstPost.id]; } @@ -439,7 +438,7 @@ const Topic = RestModel.extend({ return this.firstPost().then(firstPost => { const toggleBookmarkOnServer = () => { if (bookmark) { - return firstPost.toggleBookmarkWithReminder().then(() => { + return firstPost.toggleBookmark().then(() => { this.set("bookmarking", false); return this.afterTopicBookmarked(firstPost); }); @@ -449,7 +448,7 @@ const Topic = RestModel.extend({ this.toggleProperty("bookmarked"); this.set("bookmark_reminder_at", null); let clearedBookmarkProps = { - bookmarked_with_reminder: false, + bookmarked: false, bookmark_id: null, bookmark_name: null, bookmark_reminder_at: null @@ -458,10 +457,6 @@ const Topic = RestModel.extend({ const updated = []; posts.forEach(post => { if (post.bookmarked) { - post.set("bookmarked", false); - updated.push(post.id); - } - if (post.bookmarked_with_reminder) { post.setProperties(clearedBookmarkProps); updated.push(post.id); } @@ -477,11 +472,7 @@ const Topic = RestModel.extend({ const unbookmarkedPosts = []; if (!bookmark && posts) { - posts.forEach( - post => - (post.bookmarked || post.bookmarked_with_reminder) && - unbookmarkedPosts.push(post) - ); + posts.forEach(post => post.bookmarked && unbookmarkedPosts.push(post)); } return new Promise(resolve => { diff --git a/app/assets/javascripts/discourse/app/templates/topic.hbs b/app/assets/javascripts/discourse/app/templates/topic.hbs index 6fadf0b9b42..488970c10e8 100644 --- a/app/assets/javascripts/discourse/app/templates/topic.hbs +++ b/app/assets/javascripts/discourse/app/templates/topic.hbs @@ -225,7 +225,6 @@ recoverPost=(action "recoverPost") expandHidden=(action "expandHidden") toggleBookmark=(action "toggleBookmark") - toggleBookmarkWithReminder=(action "toggleBookmarkWithReminder") togglePostType=(action "togglePostType") rebakePost=(action "rebakePost") changePostOwner=(action "changePostOwner") diff --git a/app/assets/javascripts/discourse/app/widgets/post-menu.js b/app/assets/javascripts/discourse/app/widgets/post-menu.js index 98046266bdd..73a786e0e3c 100644 --- a/app/assets/javascripts/discourse/app/widgets/post-menu.js +++ b/app/assets/javascripts/discourse/app/widgets/post-menu.js @@ -287,31 +287,11 @@ registerButton("bookmark", attrs => { return; } - let className = "bookmark"; - - if (attrs.bookmarked) { - className += " bookmarked"; - } - - return { - id: attrs.bookmarked ? "unbookmark" : "bookmark", - action: "toggleBookmark", - title: attrs.bookmarked ? "bookmarks.created" : "bookmarks.not_bookmarked", - className, - icon: "bookmark" - }; -}); - -registerButton("bookmarkWithReminder", attrs => { - if (!attrs.canBookmark) { - return; - } - let classNames = ["bookmark", "with-reminder"]; let title = "bookmarks.not_bookmarked"; let titleOptions = {}; - if (attrs.bookmarkedWithReminder) { + if (attrs.bookmarked) { classNames.push("bookmarked"); if (attrs.bookmarkReminderAt) { @@ -331,8 +311,8 @@ registerButton("bookmarkWithReminder", attrs => { } return { - id: attrs.bookmarkedWithReminder ? "unbookmark" : "bookmark", - action: "toggleBookmarkWithReminder", + id: attrs.bookmarked ? "unbookmark" : "bookmark", + action: "toggleBookmark", title, titleOptions, className: classNames.join(" "), @@ -451,10 +431,7 @@ export default createWidget("post-menu", { const hiddenSetting = siteSettings.post_menu_hidden_items || ""; const hiddenButtons = hiddenSetting .split("|") - .filter(s => !attrs.bookmarked || s !== "bookmark") - .filter( - s => !attrs.bookmarkedWithReminder || s !== "bookmarkWithReminder" - ); + .filter(s => !attrs.bookmarked || s !== "bookmark"); if (currentUser && keyValueStore) { const likedPostId = keyValueStore.getInt("likedPostId"); @@ -468,12 +445,7 @@ export default createWidget("post-menu", { let visibleButtons = []; // filter menu items based on site settings - const orderedButtons = this.menuItems().filter(button => { - if (button === "bookmark") { - return false; - } - return true; - }); + const orderedButtons = this.menuItems(); // If the post is a wiki, make Edit more prominent if (attrs.wiki && attrs.canEdit) { diff --git a/app/serializers/post_serializer.rb b/app/serializers/post_serializer.rb index 7bf90b98ebd..ef39e37caab 100644 --- a/app/serializers/post_serializer.rb +++ b/app/serializers/post_serializer.rb @@ -49,7 +49,6 @@ class PostSerializer < BasicPostSerializer :user_title, :reply_to_user, :bookmarked, - :bookmarked_with_reminder, :bookmark_reminder_at, :bookmark_id, :bookmark_reminder_type, @@ -316,32 +315,24 @@ class PostSerializer < BasicPostSerializer true end - def bookmarked_with_reminder - true - end - def include_bookmarked? - (actions.present? && actions.keys.include?(PostActionType.types[:bookmark])) - end - - def include_bookmarked_with_reminder? post_bookmark.present? end def include_bookmark_reminder_at? - include_bookmarked_with_reminder? + include_bookmarked? end def include_bookmark_reminder_type? - include_bookmarked_with_reminder? + include_bookmarked? end def include_bookmark_name? - include_bookmarked_with_reminder? + include_bookmarked? end def include_bookmark_id? - include_bookmarked_with_reminder? + include_bookmarked? end def post_bookmark diff --git a/config/site_settings.yml b/config/site_settings.yml index ed4a4f1b813..2a0733c75ab 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -189,7 +189,7 @@ basic: post_menu: client: true type: list - default: "read|like|share|flag|edit|bookmark|bookmarkWithReminder|delete|admin|reply" + default: "read|like|share|flag|edit|bookmark|delete|admin|reply" allow_any: false choices: - read @@ -201,11 +201,10 @@ basic: - bookmark - admin - reply - - bookmarkWithReminder post_menu_hidden_items: client: true type: list - default: "flag|bookmark|bookmarkWithReminder|edit|delete|admin" + default: "flag|bookmark|edit|delete|admin" allow_any: false choices: - like @@ -216,7 +215,6 @@ basic: - bookmark - admin - reply - - bookmarkWithReminder share_links: client: true type: list diff --git a/db/migrate/20200429045956_remove_bookmarks_with_reminder_post_menu_item.rb b/db/migrate/20200429045956_remove_bookmarks_with_reminder_post_menu_item.rb new file mode 100644 index 00000000000..ebbc5cf3438 --- /dev/null +++ b/db/migrate/20200429045956_remove_bookmarks_with_reminder_post_menu_item.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +class RemoveBookmarksWithReminderPostMenuItem < ActiveRecord::Migration[6.0] + def up + execute <<~SQL + UPDATE site_settings SET value = REPLACE(value, '|bookmarkWithReminder|', '|') WHERE name = 'post_menu'; + SQL + execute <<~SQL + UPDATE site_settings SET value = REPLACE(value, 'bookmarkWithReminder|', '') WHERE name = 'post_menu'; + SQL + execute <<~SQL + UPDATE site_settings SET value = REPLACE(value, '|bookmarkWithReminder', '') WHERE name = 'post_menu'; + SQL + execute <<~SQL + UPDATE site_settings SET value = REPLACE(value, '|bookmarkWithReminder|', '|') WHERE name = 'post_menu_hidden'; + SQL + execute <<~SQL + UPDATE site_settings SET value = REPLACE(value, 'bookmarkWithReminder|', '') WHERE name = 'post_menu_hidden'; + SQL + execute <<~SQL + UPDATE site_settings SET value = REPLACE(value, '|bookmarkWithReminder', '') WHERE name = 'post_menu_hidden'; + SQL + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/plugins/discourse-narrative-bot/assets/javascripts/initializers/new-user-narrative.js.es6 b/plugins/discourse-narrative-bot/assets/javascripts/initializers/new-user-narrative.js.es6 index 35a3e09fc1c..cc4c3d1bc13 100644 --- a/plugins/discourse-narrative-bot/assets/javascripts/initializers/new-user-narrative.js.es6 +++ b/plugins/discourse-narrative-bot/assets/javascripts/initializers/new-user-narrative.js.es6 @@ -14,19 +14,19 @@ function initialize(api) { }); api.modifyClass("model:post", { - toggleBookmarkWithReminder() { + toggleBookmark() { // if we are talking to discobot then any bookmarks should just // be created without reminder options, to streamline the new user // narrative. const discobotUserId = -2; - if (this.user_id === discobotUserId && !this.bookmarked_with_reminder) { + if (this.user_id === discobotUserId && !this.bookmarked) { return ajax("/bookmarks", { type: "POST", data: { post_id: this.id } }).then(response => { this.setProperties({ "topic.bookmarked": true, - bookmarked_with_reminder: true, + bookmarked: true, bookmark_id: response.id }); this.appEvents.trigger("post-stream:refresh", { id: this.id }); diff --git a/spec/serializers/post_serializer_spec.rb b/spec/serializers/post_serializer_spec.rb index 8791db56c6e..91ca676ecd3 100644 --- a/spec/serializers/post_serializer_spec.rb +++ b/spec/serializers/post_serializer_spec.rb @@ -235,28 +235,12 @@ describe PostSerializer do s end - context "when a user post action for the bookmark exists" do - before do - PostActionCreator.create(current_user, post, :bookmark) - end - - it "returns true" do - expect(serialized.as_json[:bookmarked]).to eq(true) - end - end - - context "when a user post action for the bookmark does not exist" do - it "does not return the bookmarked attribute" do - expect(serialized.as_json.key?(:bookmarked)).to eq(false) - end - end - context "when a Bookmark record exists for the user on the post" do let!(:bookmark) { Fabricate(:bookmark_next_business_day_reminder, user: current_user, post: post) } context "bookmarks with reminders" do it "returns true" do - expect(serialized.as_json[:bookmarked_with_reminder]).to eq(true) + expect(serialized.as_json[:bookmarked]).to eq(true) end it "returns the reminder_at for the bookmark" do @@ -266,8 +250,8 @@ describe PostSerializer do context "if topic_view is blank" do let(:topic_view) { nil } - it "does not return the bookmarked_with_reminder attribute" do - expect(serialized.as_json.key?(:bookmarked_with_reminder)).to eq(false) + it "does not return the bookmarked attribute" do + expect(serialized.as_json.key?(:bookmarked)).to eq(false) end end end diff --git a/test/javascripts/helpers/site-settings.js b/test/javascripts/helpers/site-settings.js index bf55374eb85..4d1181dbaf9 100644 --- a/test/javascripts/helpers/site-settings.js +++ b/test/javascripts/helpers/site-settings.js @@ -13,10 +13,8 @@ Discourse.SiteSettingsOriginal = { ga_universal_tracking_code: "", ga_universal_domain_name: "auto", top_menu: "latest|new|unread|categories|top", - post_menu: - "like|share|flag|edit|bookmark|bookmarkWithReminder|delete|admin|reply", - post_menu_hidden_items: - "flag|bookmark|bookmarkWithReminder|edit|delete|admin", + post_menu: "like|share|flag|edit|bookmark|delete|admin|reply", + post_menu_hidden_items: "flag|bookmark|edit|delete|admin", share_links: "twitter|facebook|email", category_colors: "BF1E2E|F1592A|F7941D|9EB83B|3AB54A|12A89D|25AAE2|0E76BD|652D90|92278F|ED207B|8C6238|231F20|27AA5B|B3B5B4|E45735", diff --git a/test/javascripts/widgets/post-test.js b/test/javascripts/widgets/post-test.js index b7131fc2a5b..bc12234c315 100644 --- a/test/javascripts/widgets/post-test.js +++ b/test/javascripts/widgets/post-test.js @@ -532,15 +532,12 @@ widgetTest("can't bookmark", { widgetTest("bookmark", { template: - '{{mount-widget widget="post" args=args toggleBookmarkWithReminder=(action "toggleBookmarkWithReminder")}}', + '{{mount-widget widget="post" args=args toggleBookmark=(action "toggleBookmark")}}', beforeEach() { const args = { canBookmark: true }; this.set("args", args); - this.on( - "toggleBookmarkWithReminder", - () => (args.bookmarked_with_reminder = true) - ); + this.on("toggleBookmark", () => (args.bookmarked = true)); }, async test(assert) { assert.equal(find(".post-menu-area .bookmark").length, 1);