diff --git a/app/assets/javascripts/discourse/app/components/bookmark-actions-dropdown.js b/app/assets/javascripts/discourse/app/components/bookmark-actions-dropdown.js index a8aaf749919..e039f4d5aea 100644 --- a/app/assets/javascripts/discourse/app/components/bookmark-actions-dropdown.js +++ b/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); } }, }); diff --git a/app/assets/javascripts/discourse/app/components/bookmark-list.js b/app/assets/javascripts/discourse/app/components/bookmark-list.js index 784f25d8318..30b7197c4c0 100644 --- a/app/assets/javascripts/discourse/app/components/bookmark-list.js +++ b/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) { diff --git a/app/assets/javascripts/discourse/app/controllers/user-activity-bookmarks.js b/app/assets/javascripts/discourse/app/controllers/user-activity-bookmarks.js index 86edb9b0236..a561911938e 100644 --- a/app/assets/javascripts/discourse/app/controllers/user-activity-bookmarks.js +++ b/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( - response.bookmarks.map((bookmark) => Bookmark.create(bookmark)) - ); + const bookmarkModels = response.bookmarks.map((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); } }, }); diff --git a/app/assets/javascripts/discourse/app/models/bookmark.js b/app/assets/javascripts/discourse/app/models/bookmark.js index d65b32272b7..63fc6b37213 100644 --- a/app/assets/javascripts/discourse/app/models/bookmark.js +++ b/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); diff --git a/app/assets/javascripts/discourse/app/templates/components/bookmark-list.hbs b/app/assets/javascripts/discourse/app/templates/components/bookmark-list.hbs index 3dddc193505..6b770babdb1 100644 --- a/app/assets/javascripts/discourse/app/templates/components/bookmark-list.hbs +++ b/app/assets/javascripts/discourse/app/templates/components/bookmark-list.hbs @@ -41,8 +41,13 @@ {{/if}} - {{topic-status topic=bookmark}} - {{topic-link bookmark}} + {{#if bookmark.excerpt}} {{!-- template-lint-disable --}} @@ -69,6 +74,7 @@ bookmark=bookmark removeBookmark=(action "removeBookmark") editBookmark=(action "editBookmark") + togglePinBookmark=(action "togglePinBookmark") }} diff --git a/app/assets/stylesheets/common/components/bookmark-list.scss b/app/assets/stylesheets/common/components/bookmark-list.scss index 5d7d6c51c81..07042a6e640 100644 --- a/app/assets/stylesheets/common/components/bookmark-list.scss +++ b/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; + } + } } diff --git a/app/controllers/bookmarks_controller.rb b/app/controllers/bookmarks_controller.rb index aa804d1d4e5..c0645e2ffa2 100644 --- a/app/controllers/bookmarks_controller.rb +++ b/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 diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb index 26e63f12bd4..5ac37b106c7 100644 --- a/app/models/bookmark.rb +++ b/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 # diff --git a/app/serializers/user_bookmark_serializer.rb b/app/serializers/user_bookmark_serializer.rb index 06b9c1523b5..805be599ed1 100644 --- a/app/serializers/user_bookmark_serializer.rb +++ b/app/serializers/user_bookmark_serializer.rb @@ -14,6 +14,7 @@ class UserBookmarkSerializer < ApplicationSerializer :post_id, :name, :reminder_at, + :pinned, :title, :deleted, :hidden, diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index b70f238a9fe..34d228ac268 100644 --- a/config/locales/client.en.yml +++ b/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" diff --git a/config/routes.rb b/config/routes.rb index 8e27a365b08..65aa8444ac7 100644 --- a/config/routes.rb +++ b/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 diff --git a/db/migrate/20210318020143_add_pinned_column_to_bookmarks.rb b/db/migrate/20210318020143_add_pinned_column_to_bookmarks.rb new file mode 100644 index 00000000000..6a121169f78 --- /dev/null +++ b/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 diff --git a/lib/bookmark_manager.rb b/lib/bookmark_manager.rb index ba5c42a040d..6bd8f58316a 100644 --- a/lib/bookmark_manager.rb +++ b/lib/bookmark_manager.rb @@ -40,10 +40,7 @@ class BookmarkManager end def destroy(bookmark_id) - bookmark = Bookmark.find_by(id: bookmark_id) - - raise Discourse::NotFound if bookmark.blank? - raise Discourse::InvalidAccess.new if !Guardian.new(@user).can_delete?(bookmark) + bookmark = find_bookmark_and_check_access(bookmark_id) 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) - - raise Discourse::NotFound if bookmark.blank? - raise Discourse::InvalidAccess.new if !Guardian.new(@user).can_edit?(bookmark) + bookmark = find_bookmark_and_check_access(bookmark_id) 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 diff --git a/lib/bookmark_query.rb b/lib/bookmark_query.rb index f2122f7c6c7..fabf6f15bcf 100644 --- a/lib/bookmark_query.rb +++ b/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) diff --git a/spec/lib/bookmark_manager_spec.rb b/spec/lib/bookmark_manager_spec.rb index bd0836329c9..bf9f1d2f54e 100644 --- a/spec/lib/bookmark_manager_spec.rb +++ b/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 diff --git a/spec/lib/bookmark_query_spec.rb b/spec/lib/bookmark_query_spec.rb index 4f11254ad8d..f75156ce6cf 100644 --- a/spec/lib/bookmark_query_spec.rb +++ b/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