FEATURE: Promote bookmarks with reminders to core functionality (#9369)

The main thrust of this PR is to take all the conditional checks based on the `enable_bookmarks_with_reminders` away and only keep the code from the `true` path, making bookmarks with reminders the core bookmarks feature. There is also a migration to create `Bookmark` records out of `PostAction` bookmarks for a site. ### Summary * Remove logic based on whether enable_bookmarks_with_reminders is true. This site setting is now obsolete, the old bookmark functionality is being removed. Retain the setting and set the value to `true` in a migration. * Use the code from the rake task to create a database migration that creates bookmarks from post actions. * Change the bookmark report to read from the new table. * Get rid of old endpoints for bookmarks * Link to the new bookmarks list from the user summary page
discourse · Apr 22, 2020 · 628ba9d · 628ba9d
1 parent 5a98869
commit 628ba9d
Show file tree

Hide file tree

Showing 41 changed files with 255 additions and 470 deletions.
diff --git a/app/assets/javascripts/discourse/controllers/keyboard-shortcuts-help.js b/app/assets/javascripts/discourse/controllers/keyboard-shortcuts-help.js
@@ -1,6 +1,5 @@
 import Controller from "@ember/controller";
 import ModalFunctionality from "discourse/mixins/modal-functionality";
-import { setting } from "discourse/lib/computed";
 
 const KEY = "keyboard_shortcuts_help";
 
@@ -57,8 +56,6 @@ export default Controller.extend(ModalFunctionality, {
     this.set("shortcuts", null);
   },
 
-  showBookmarkShortcuts: setting("enable_bookmarks_with_reminders"),
-
   _defineShortcuts() {
     this.set("shortcuts", {
       jump_to: {

diff --git a/app/assets/javascripts/discourse/controllers/topic.js b/app/assets/javascripts/discourse/controllers/topic.js
@@ -652,10 +652,7 @@ export default Controller.extend(bufferedProperty("model"), {
       if (!this.currentUser) {
         return bootbox.alert(I18n.t("bookmarks.not_bookmarked"));
       } else if (post) {
-        if (this.siteSettings.enable_bookmarks_with_reminders) {
-          return post.toggleBookmarkWithReminder();
-        }
-        return post.toggleBookmark().catch(popupAjaxError);
+        return post.toggleBookmarkWithReminder();
       } else {
         return this.model.toggleBookmark().then(changedIds => {
           if (!changedIds) {

diff --git a/app/assets/javascripts/discourse/controllers/user-activity.js b/app/assets/javascripts/discourse/controllers/user-activity.js
@@ -3,7 +3,6 @@ import { inject as service } from "@ember/service";
 import Controller, { inject as controller } from "@ember/controller";
 import { exportUserArchive } from "discourse/lib/export-csv";
 import { observes } from "discourse-common/utils/decorators";
-import { setting } from "discourse/lib/computed";
 
 export default Controller.extend({
   application: controller(),
@@ -12,7 +11,6 @@ export default Controller.extend({
   userActionType: null,
 
   canDownloadPosts: alias("user.viewingSelf"),
-  bookmarksWithRemindersEnabled: setting("enable_bookmarks_with_reminders"),
 
   @observes("userActionType", "model.stream.itemsLoaded")
   _showFooter: function() {

diff --git a/app/assets/javascripts/discourse/models/topic.js b/app/assets/javascripts/discourse/models/topic.js
@@ -400,10 +400,8 @@ const Topic = RestModel.extend({
   afterTopicBookmarked(firstPost) {
     if (firstPost) {
       firstPost.set("bookmarked", true);
-      if (this.siteSettings.enable_bookmarks_with_reminders) {
-        this.set("bookmark_reminder_at", firstPost.bookmark_reminder_at);
-        firstPost.set("bookmarked_with_reminder", true);
-      }
+      firstPost.set("bookmarked_with_reminder", true);
+      this.set("bookmark_reminder_at", firstPost.bookmark_reminder_at);
       return [firstPost.id];
     }
   },
@@ -438,24 +436,10 @@ const Topic = RestModel.extend({
     return this.firstPost().then(firstPost => {
       const toggleBookmarkOnServer = () => {
         if (bookmark) {
-          if (this.siteSettings.enable_bookmarks_with_reminders) {
-            return firstPost.toggleBookmarkWithReminder().then(response => {
-              this.set("bookmarking", false);
-              if (response && response.closedWithoutSaving) {
-                this.set("bookmarked", false);
-              } else {
-                return this.afterTopicBookmarked(firstPost);
-              }
-            });
-          } else {
-            return ajax(`/t/${this.id}/bookmark`, { type: "PUT" })
-              .then(() => {
-                this.toggleProperty("bookmarked");
-                return this.afterTopicBookmarked(firstPost);
-              })
-              .catch(popupAjaxError)
-              .finally(() => this.set("bookmarking", false));
-          }
+          return firstPost.toggleBookmarkWithReminder().then(() => {
+            this.set("bookmarking", false);
+            return this.afterTopicBookmarked(firstPost);
+          });
         } else {
           return ajax(`/t/${this.id}/remove_bookmarks`, { type: "PUT" })
             .then(() => {
@@ -474,10 +458,7 @@ const Topic = RestModel.extend({
                     post.set("bookmarked", false);
                     updated.push(post.id);
                   }
-                  if (
-                    this.siteSettings.enable_bookmarks_with_reminders &&
-                    post.bookmarked_with_reminder
-                  ) {
+                  if (post.bookmarked_with_reminder) {
                     post.setProperties(clearedBookmarkProps);
                     updated.push(post.id);
                   }

diff --git a/app/assets/javascripts/discourse/routes/discovery.js b/app/assets/javascripts/discourse/routes/discovery.js
@@ -17,10 +17,7 @@ export default DiscourseRoute.extend(OpenComposer, {
     // including being able to show links to multiple posts to the same topic
     // and being based on a different model. better to just redirect
     const url = transition.intent.url;
-    if (
-      this.siteSettings.enable_bookmarks_with_reminders &&
-      url === "/bookmarks"
-    ) {
+    if (url === "/bookmarks") {
       this.transitionTo(
         "userActivity.bookmarksWithReminders",
         this.currentUser

diff --git a/app/assets/javascripts/discourse/templates/modal/keyboard-shortcuts-help.hbs b/app/assets/javascripts/discourse/templates/modal/keyboard-shortcuts-help.hbs
@@ -55,24 +55,22 @@
         <li>{{html-safe shortcuts.actions.quote_post}}</li>
       </ul>
     </section>
-    {{#if showBookmarkShortcuts}}
-      <section class="keyboard-shortcuts-bookmark-section">
-        <h4>{{i18n "keyboard_shortcuts_help.bookmarks.title"}}</h4>
-        <ul>
-          <li>{{html-safe shortcuts.bookmarks.enter}}</li>
-          <li>{{html-safe shortcuts.bookmarks.later_today}}</li>
-          <li>{{html-safe shortcuts.bookmarks.later_this_week}}</li>
-          <li>{{html-safe shortcuts.bookmarks.tomorrow}}</li>
-          <li>{{html-safe shortcuts.bookmarks.next_week}}</li>
-          <li>{{html-safe shortcuts.bookmarks.next_month}}</li>
-          <li>{{html-safe shortcuts.bookmarks.next_business_week}}</li>
-          <li>{{html-safe shortcuts.bookmarks.next_business_day}}</li>
-          <li>{{html-safe shortcuts.bookmarks.custom}}</li>
-          <li>{{html-safe shortcuts.bookmarks.none}}</li>
-          <li>{{html-safe shortcuts.bookmarks.delete}}</li>
-        </ul>
-      </section>
-    {{/if}}
+    <section class="keyboard-shortcuts-bookmark-section">
+      <h4>{{i18n "keyboard_shortcuts_help.bookmarks.title"}}</h4>
+      <ul>
+        <li>{{html-safe shortcuts.bookmarks.enter}}</li>
+        <li>{{html-safe shortcuts.bookmarks.later_today}}</li>
+        <li>{{html-safe shortcuts.bookmarks.later_this_week}}</li>
+        <li>{{html-safe shortcuts.bookmarks.tomorrow}}</li>
+        <li>{{html-safe shortcuts.bookmarks.next_week}}</li>
+        <li>{{html-safe shortcuts.bookmarks.next_month}}</li>
+        <li>{{html-safe shortcuts.bookmarks.next_business_week}}</li>
+        <li>{{html-safe shortcuts.bookmarks.next_business_day}}</li>
+        <li>{{html-safe shortcuts.bookmarks.custom}}</li>
+        <li>{{html-safe shortcuts.bookmarks.none}}</li>
+        <li>{{html-safe shortcuts.bookmarks.delete}}</li>
+      </ul>
+    </section>
   </div>
   <div class="column">
     <section>

diff --git a/app/assets/javascripts/discourse/templates/user/activity.hbs b/app/assets/javascripts/discourse/templates/user/activity.hbs
@@ -18,15 +18,9 @@
       {{#link-to "userActivity.likesGiven"}}{{i18n "user_action_groups.1"}}{{/link-to}}
     </li>
     {{#if user.showBookmarks}}
-      {{#if bookmarksWithRemindersEnabled}}
-        <li>
-          {{#link-to "userActivity.bookmarksWithReminders"}}{{i18n "user_action_groups.3"}}{{/link-to}}
-        </li>
-      {{else}}
-        <li>
-          {{#link-to "userActivity.bookmarks"}}{{i18n "user_action_groups.3"}}{{/link-to}}
-        </li>
-      {{/if}}
+      <li>
+        {{#link-to 'userActivity.bookmarksWithReminders'}}{{i18n 'user_action_groups.3'}}{{/link-to}}
+      </li>
     {{/if}}
     {{plugin-outlet
       name="user-activity-bottom"

diff --git a/app/assets/javascripts/discourse/templates/user/summary.hbs b/app/assets/javascripts/discourse/templates/user/summary.hbs
@@ -27,7 +27,7 @@
         </li>
         {{#if model.bookmark_count}}
           <li class="linked-stat">
-            {{#link-to "userActivity.bookmarks"}}
+            {{#link-to "userActivity.bookmarksWithReminders"}}
               {{user-stat value=model.bookmark_count label="user.summary.bookmark_count"}}
             {{/link-to}}
           </li>

diff --git a/app/assets/javascripts/discourse/widgets/post-menu.js b/app/assets/javascripts/discourse/widgets/post-menu.js
@@ -302,8 +302,8 @@ registerButton("bookmark", attrs => {
   };
 });
 
-registerButton("bookmarkWithReminder", (attrs, state, siteSettings) => {
-  if (!attrs.canBookmark || !siteSettings.enable_bookmarks_with_reminders) {
+registerButton("bookmarkWithReminder", attrs => {
+  if (!attrs.canBookmark) {
     return;
   }
 
@@ -469,10 +469,7 @@ export default createWidget("post-menu", {
 
     // filter menu items based on site settings
     const orderedButtons = this.menuItems().filter(button => {
-      if (
-        this.siteSettings.enable_bookmarks_with_reminders &&
-        button === "bookmark"
-      ) {
+      if (button === "bookmark") {
         return false;
       }
       return true;

diff --git a/app/assets/javascripts/discourse/widgets/quick-access-bookmarks.js b/app/assets/javascripts/discourse/widgets/quick-access-bookmarks.js
@@ -16,23 +16,15 @@ createWidgetFrom(QuickAccessPanel, "quick-access-bookmarks", {
   },
 
   showAllHref() {
-    if (this.siteSettings.enable_bookmarks_with_reminders) {
-      return `${this.attrs.path}/activity/bookmarks-with-reminders`;
-    } else {
-      return `${this.attrs.path}/activity/bookmarks`;
-    }
+    return `${this.attrs.path}/activity/bookmarks-with-reminders`;
   },
 
   emptyStatePlaceholderItem() {
     return h("li.read", this.state.emptyStatePlaceholderItemText);
   },
 
   findNewItems() {
-    if (this.siteSettings.enable_bookmarks_with_reminders) {
-      return this.loadBookmarksWithReminders();
-    } else {
-      return this.loadUserActivityBookmarks();
-    }
+    return this.loadBookmarksWithReminders();
   },
 
   itemHtml(bookmark) {

diff --git a/app/assets/javascripts/discourse/widgets/user-menu.js b/app/assets/javascripts/discourse/widgets/user-menu.js
@@ -56,16 +56,13 @@ createWidget("user-menu-links", {
   },
 
   bookmarksGlyph() {
-    let path = this.siteSettings.enable_bookmarks_with_reminders
-      ? "bookmarks-with-reminders"
-      : "bookmarks";
     return {
       action: UserMenuAction.QUICK_ACCESS,
       actionParam: QuickAccess.BOOKMARKS,
       label: "user.bookmarks",
       className: "user-bookmarks-link",
       icon: "bookmark",
-      href: `${this.attrs.path}/activity/${path}`
+      href: `${this.attrs.path}/activity/bookmarks-with-reminders`
     };
   },
 

diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb
@@ -488,26 +488,6 @@ def notice
     render body: nil
   end
 
-  def bookmark
-    if params[:bookmarked] == "true"
-      post = find_post_from_params
-      result = PostActionCreator.create(current_user, post, :bookmark)
-      return render_json_error(result) if result.failed?
-    else
-      post_action = PostAction.find_by(post_id: params[:post_id], user_id: current_user.id)
-      raise Discourse::NotFound unless post_action
-
-      post = Post.with_deleted.find_by(id: post_action&.post_id)
-      raise Discourse::NotFound unless post
-
-      result = PostActionDestroyer.destroy(current_user, post, :bookmark)
-      return render_json_error(result) if result.failed?
-    end
-
-    topic_user = TopicUser.get(post.topic, current_user)
-    render_json_dump(topic_bookmarked: topic_user.try(:bookmarked))
-  end
-
   def destroy_bookmark
     params.require(:post_id)