FEATURE: Bookmark pinning (#12431)

Users can now pin bookmarks from their bookmark list. This will anchor the bookmark to the top of the list, and show a pin icon next to it. This also applies in the nav bookmarks panel. If there are multiple pinned bookmarks they sort by last updated order.

app/assets/javascripts/discourse/app/components/bookmark-actions-dropdown.js

@@ -1,7 +1,12 @@
-import { action, computed } from "@ember/object";
+import { action } from "@ember/object";
+import discourseComputed from "discourse-common/utils/decorators";
 import DropdownSelectBoxComponent from "select-kit/components/dropdown-select-box";
 import I18n from "I18n";
 
+const ACTION_REMOVE = "remove";
+const ACTION_EDIT = "edit";
+const ACTION_PIN = "pin";
+
 export default DropdownSelectBoxComponent.extend({
   classNames: ["bookmark-actions-dropdown"],
   pluginApiIdentifiers: ["bookmark-actions-dropdown"],
@@ -11,10 +16,11 @@ export default DropdownSelectBoxComponent.extend({
     showFullTitle: true,
   },
 
-  content: computed(() => {
+  @discourseComputed("bookmark")
+  content(bookmark) {
     return [
       {
-        id: "remove",
+        id: ACTION_REMOVE,
         icon: "trash-alt",
         name: I18n.t("post.bookmarks.actions.delete_bookmark.name"),
         description: I18n.t(
@@ -22,20 +28,32 @@ export default DropdownSelectBoxComponent.extend({
         ),
       },
       {
-        id: "edit",
+        id: ACTION_EDIT,
         icon: "pencil-alt",
         name: I18n.t("post.bookmarks.actions.edit_bookmark.name"),
         description: I18n.t("post.bookmarks.actions.edit_bookmark.description"),
       },
+      {
+        id: ACTION_PIN,
+        icon: "thumbtack",
+        name: I18n.t(
+          `post.bookmarks.actions.${bookmark.pinAction()}_bookmark.name`
+        ),
+        description: I18n.t(
+          `post.bookmarks.actions.${bookmark.pinAction()}_bookmark.description`
+        ),
+      },
     ];
-  }),
+  },
 
   @action
   onChange(selectedAction) {
-    if (selectedAction === "remove") {
+    if (selectedAction === ACTION_REMOVE) {
       this.removeBookmark(this.bookmark);
-    } else if (selectedAction === "edit") {
+    } else if (selectedAction === ACTION_EDIT) {
       this.editBookmark(this.bookmark);
+    } else if (selectedAction === ACTION_PIN) {
+      this.togglePinBookmark(this.bookmark);
     }
   },
 });

app/assets/javascripts/discourse/app/components/bookmark-list.js

@@ -61,7 +61,12 @@ export default Component.extend({
       title: "post.bookmarks.edit",
       modalClass: "bookmark-with-reminder",
     });
-    controller.set("afterSave", () => this.reload());
+    controller.set("afterSave", this.reload);
+  },
+
+  @action
+  togglePinBookmark(bookmark) {
+    bookmark.togglePin().then(this.reload);
   },
 
   _removeBookmarkFromList(bookmark) {

app/assets/javascripts/discourse/app/controllers/user-activity-bookmarks.js

@@ -2,7 +2,7 @@ import Controller, { inject } from "@ember/controller";
 import Bookmark from "discourse/models/bookmark";
 import I18n from "I18n";
 import { Promise } from "rsvp";
-import { action } from "@ember/object";
+import EmberObject, { action } from "@ember/object";
 import discourseComputed from "discourse-common/utils/decorators";
 
 export default Controller.extend({
@@ -100,9 +100,19 @@ export default Controller.extend({
     this.model.more_bookmarks_url = response.more_bookmarks_url;
 
     if (response.bookmarks) {
-      this.content.pushObjects(
+      const bookmarkModels = response.bookmarks.map((bookmark) => {
-        response.bookmarks.map((bookmark) => Bookmark.create(bookmark))
+        const bookmarkModel = Bookmark.create(bookmark);
-      );
+        bookmarkModel.topicStatus = EmberObject.create({
+          closed: bookmark.closed,
+          archived: bookmark.archived,
+          is_warning: bookmark.is_warning,
+          pinned: false,
+          unpinned: false,
+          invisible: bookmark.invisible,
+        });
+        return bookmarkModel;
+      });
+      this.content.pushObjects(bookmarkModels);
     }
   },
 });

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

@@ -36,6 +36,20 @@ const Bookmark = RestModel.extend({
     });
   },
 
+  togglePin() {
+    if (this.newBookmark) {
+      return Promise.resolve();
+    }
+
+    return ajax(this.url + "/toggle_pin", {
+      type: "PUT",
+    });
+  },
+
+  pinAction() {
+    return this.pinned ? "unpin" : "pin";
+  },
+
   @discourseComputed("highest_post_number", "url")
   lastPostUrl(highestPostNumber) {
     return this.urlForPostNumber(highestPostNumber);

app/assets/javascripts/discourse/app/templates/components/bookmark-list.hbs

@@ -41,8 +41,13 @@
                   {{/if}}
                 </div>
 
-                {{topic-status topic=bookmark}}
+                <div class="bookmark-status-with-link">
-                {{topic-link bookmark}}
+                  {{#if bookmark.pinned}}
+                    {{d-icon "thumbtack" class="bookmark-pinned"}}
+                  {{/if}}
+                  {{topic-status topic=bookmark.topicStatus}}
+                  {{topic-link bookmark}}
+                </div>
               </span>
               {{#if bookmark.excerpt}}
                 {{!-- template-lint-disable  --}}
@@ -69,6 +74,7 @@
               bookmark=bookmark
               removeBookmark=(action "removeBookmark")
               editBookmark=(action "editBookmark")
+              togglePinBookmark=(action "togglePinBookmark")
               }}
             </td>
           </tr>

app/assets/stylesheets/common/components/bookmark-list.scss

@@ -32,6 +32,17 @@ $mobile-breakpoint: 700px;
       padding-right: 0.5em;
     }
   }
+  .main-link {
+    .bookmark-status-with-link {
+      a.title {
+        padding: 0;
+      }
+    }
+  }
+  .d-icon.bookmark-pinned {
+    font-size: $font-down-2;
+    margin-right: 0.2em;
+  }
   .bookmark-metadata {
     font-size: $font-down-2;
     display: flex;
@@ -58,4 +69,13 @@ $mobile-breakpoint: 700px;
       }
     }
   }
+  .bookmark-status-with-link {
+    display: flex;
+    flex-direction: row;
+    align-items: center;
+
+    .topic-statuses {
+      float: none;
+    }
+  }
 }

app/controllers/bookmarks_controller.rb

@@ -54,4 +54,17 @@ class BookmarksController < ApplicationController
 
     render json: failed_json.merge(errors: bookmark_manager.errors.full_messages), status: 400
   end
+
+  def toggle_pin
+    params.require(:bookmark_id)
+
+    bookmark_manager = BookmarkManager.new(current_user)
+    bookmark_manager.toggle_pin(bookmark_id: params[:bookmark_id])
+
+    if bookmark_manager.errors.empty?
+      return render json: success_json
+    end
+
+    render json: failed_json.merge(errors: bookmark_manager.errors.full_messages), status: 400
+  end
 end

app/models/bookmark.rb

@@ -143,6 +143,7 @@ end
 #  reminder_last_sent_at  :datetime
 #  reminder_set_at        :datetime
 #  auto_delete_preference :integer          default(0), not null
+#  pinned                 :boolean          default(FALSE)
 #
 # Indexes
 #

app/serializers/user_bookmark_serializer.rb

@@ -14,6 +14,7 @@ class UserBookmarkSerializer < ApplicationSerializer
              :post_id,
              :name,
              :reminder_at,
+             :pinned,
              :title,
              :deleted,
              :hidden,

config/locales/client.en.yml

@@ -3068,6 +3068,12 @@ en:
           edit_bookmark:
             name: "Edit bookmark"
             description: "Edit the bookmark name or change the reminder date and time"
+          pin_bookmark:
+            name: "Pin bookmark"
+            description: "Pin the bookmark. This will make it appear at the top of your bookmarks list."
+          unpin_bookmark:
+            name: "Unpin bookmark"
+            description: "Unpin the bookmark. It will no longer appear at the top of your bookmarks list."
 
       filtered_replies:
         viewing_posts_by: "Viewing %{post_count} posts by"

config/routes.rb

@@ -623,7 +623,9 @@ Discourse::Application.routes.draw do
       end
     end
 
-    resources :bookmarks, only: %i[create destroy update]
+    resources :bookmarks, only: %i[create destroy update] do
+      put "toggle_pin"
+    end
 
     resources :notifications, except: :show do
       collection do

db/migrate/20210318020143_add_pinned_column_to_bookmarks.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddPinnedColumnToBookmarks < ActiveRecord::Migration[6.0]
+  def change
+    add_column :bookmarks, :pinned, :boolean, default: false
+  end
+end

lib/bookmark_manager.rb

@@ -40,10 +40,7 @@ class BookmarkManager
   end
 
   def destroy(bookmark_id)
-    bookmark = Bookmark.find_by(id: bookmark_id)
+    bookmark = find_bookmark_and_check_access(bookmark_id)
-
-    raise Discourse::NotFound if bookmark.blank?
-    raise Discourse::InvalidAccess.new if !Guardian.new(@user).can_delete?(bookmark)
 
     bookmark.destroy
 
@@ -72,10 +69,7 @@ class BookmarkManager
   end
 
   def update(bookmark_id:, name:, reminder_type:, reminder_at:, options: {})
-    bookmark = Bookmark.find_by(id: bookmark_id)
+    bookmark = find_bookmark_and_check_access(bookmark_id)
-
-    raise Discourse::NotFound if bookmark.blank?
-    raise Discourse::InvalidAccess.new if !Guardian.new(@user).can_edit?(bookmark)
 
     reminder_type = parse_reminder_type(reminder_type)
 
@@ -95,8 +89,27 @@ class BookmarkManager
     success
   end
 
+  def toggle_pin(bookmark_id:)
+    bookmark = find_bookmark_and_check_access(bookmark_id)
+    bookmark.pinned = !bookmark.pinned
+    success = bookmark.save
+
+    if bookmark.errors.any?
+      return add_errors_from(bookmark)
+    end
+
+    success
+  end
+
   private
 
+  def find_bookmark_and_check_access(bookmark_id)
+    bookmark = Bookmark.find_by(id: bookmark_id)
+    raise Discourse::NotFound if !bookmark
+    raise Discourse::InvalidAccess.new if !Guardian.new(@user).can_edit?(bookmark)
+    bookmark
+  end
+
   def update_topic_user_bookmarked(topic, opts = {})
     # PostCreator can specify whether auto_track is enabled or not, don't want to
     # create a TopicUser in that case

lib/bookmark_query.rb

@@ -27,7 +27,9 @@ class BookmarkQuery
   end
 
   def list_all
-    results = user_bookmarks.order('bookmarks.updated_at DESC')
+    results = user_bookmarks.order(
+      '(CASE WHEN bookmarks.pinned THEN 0 ELSE 1 END), bookmarks.updated_at DESC'
+    )
 
     topics = Topic.listable_topics.secured(@guardian)
     pms = Topic.private_messages_for_user(@user)

spec/lib/bookmark_manager_spec.rb

@@ -159,7 +159,7 @@ RSpec.describe BookmarkManager do
     end
 
     context "if the bookmark no longer exists" do
-      it "raises an invalid access error" do
+      it "raises a not found error" do
         expect { subject.destroy(9999) }.to raise_error(Discourse::NotFound)
       end
     end
@@ -220,7 +220,7 @@ RSpec.describe BookmarkManager do
       before do
         bookmark.destroy!
       end
-      it "raises an invalid access error" do
+      it "raises a not found error" do
         expect { update_bookmark }.to raise_error(Discourse::NotFound)
       end
     end
@@ -293,4 +293,36 @@ RSpec.describe BookmarkManager do
       Notification.where(notification_type: Notification.types[:bookmark_reminder], user_id: bookmark.user.id)
     end
   end
+
+  describe ".toggle_pin" do
+    let!(:bookmark) { Fabricate(:bookmark, user: user) }
+
+    it "sets pinned to false if it is true" do
+      bookmark.update(pinned: true)
+      subject.toggle_pin(bookmark_id: bookmark.id)
+      expect(bookmark.reload.pinned).to eq(false)
+    end
+
+    it "sets pinned to true if it is false" do
+      bookmark.update(pinned: false)
+      subject.toggle_pin(bookmark_id: bookmark.id)
+      expect(bookmark.reload.pinned).to eq(true)
+    end
+
+    context "if the bookmark is belonging to some other user" do
+      let!(:bookmark) { Fabricate(:bookmark, user: Fabricate(:admin)) }
+      it "raises an invalid access error" do
+        expect { subject.toggle_pin(bookmark_id: bookmark.id) }.to raise_error(Discourse::InvalidAccess)
+      end
+    end
+
+    context "if the bookmark no longer exists" do
+      before do
+        bookmark.destroy!
+      end
+      it "raises a not found error" do
+        expect { subject.toggle_pin(bookmark_id: bookmark.id) }.to raise_error(Discourse::NotFound)
+      end
+    end
+  end
 end

spec/lib/bookmark_query_spec.rb

@@ -173,6 +173,7 @@ RSpec.describe BookmarkQuery do
     let!(:bookmark3) { Fabricate(:bookmark, user: user, updated_at: 6.days.ago) }
     let!(:bookmark4) { Fabricate(:bookmark, user: user, updated_at: 4.days.ago) }
     let!(:bookmark5) { Fabricate(:bookmark, user: user, updated_at: 3.days.ago) }
+
     it "orders by updated_at" do
       expect(bookmark_query.list_all.map(&:id)).to eq([
         bookmark1.id,
@@ -182,5 +183,17 @@ RSpec.describe BookmarkQuery do
         bookmark3.id
       ])
     end
+
+    it "puts pinned bookmarks first, in updated at order, then the rest in updated at order" do
+      bookmark3.update_column(:pinned, true)
+      bookmark4.update_column(:pinned, true)
+      expect(bookmark_query.list_all.map(&:id)).to eq([
+        bookmark4.id,
+        bookmark3.id,
+        bookmark1.id,
+        bookmark2.id,
+        bookmark5.id
+      ])
+    end
   end
 end