From 27699648ef8a73d20f0729e97b8ca684513edfd5 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 21 Sep 2021 13:49:56 +1000 Subject: [PATCH] FEATURE: Go to last unread for topic-level bookmark links (#14396) Instead of going to the OP of the topic for topic-level bookmarks (which are bookmarks where for_topic is true) when clicking on the bookmark in the quick access menu or on the user bookmark list, this commit takes the user to the last unread post in the topic instead. This should be generally more useful than landing on the unchanging OP. To make this work nicely, I needed to add the last_read_post_number to the BookmarkQuery based on the TopicUser association. It should not add too much extra weight to the query, because it is limited to the user that we are fetching bookmarks for. Also fixed an issue where the bookmark serializer highest_post_number was not taking into account whether the user was staff, which is when we should use highest_staff_post_number instead. --- .../discourse/app/models/bookmark.js | 17 ++++++++-- .../templates/components/bookmark-list.hbs | 2 +- .../app/widgets/quick-access-bookmarks.js | 15 ++++++--- app/serializers/user_bookmark_serializer.rb | 11 ++++++- lib/bookmark_query.rb | 4 +++ spec/lib/bookmark_query_spec.rb | 21 +++++++++++- spec/models/user_bookmark_list_spec.rb | 3 +- .../user_bookmark_serializer_spec.rb | 32 +++++++------------ 8 files changed, 73 insertions(+), 32 deletions(-) diff --git a/app/assets/javascripts/discourse/app/models/bookmark.js b/app/assets/javascripts/discourse/app/models/bookmark.js index 52b032e2642..97d45b219ed 100644 --- a/app/assets/javascripts/discourse/app/models/bookmark.js +++ b/app/assets/javascripts/discourse/app/models/bookmark.js @@ -125,9 +125,20 @@ const Bookmark = RestModel.extend({ ).capitalize(); }, - @discourseComputed("linked_post_number", "fancy_title", "topic_id") - topicLink(linked_post_number, fancy_title, id) { - return Topic.create({ id, fancy_title, linked_post_number }); + @discourseComputed() + topicForList() { + // for topic level bookmarks we want to jump to the last unread post URL, + // which the topic-link helper does by default if no linked post number is + // provided + const linkedPostNumber = this.for_topic ? null : this.linked_post_number; + + return Topic.create({ + id: this.topic_id, + fancy_title: this.fancy_title, + linked_post_number: linkedPostNumber, + last_read_post_number: this.last_read_post_number, + highest_post_number: this.highest_post_number, + }); }, loadItems(params) { 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 8816e46983b..9dbfd967959 100644 --- a/app/assets/javascripts/discourse/app/templates/components/bookmark-list.hbs +++ b/app/assets/javascripts/discourse/app/templates/components/bookmark-list.hbs @@ -32,7 +32,7 @@ {{d-icon "thumbtack" class="bookmark-pinned"}} {{/if}} {{~topic-status topic=bookmark.topicStatus~}} - {{topic-link bookmark.topicLink}} + {{topic-link bookmark.topicForList}}
diff --git a/app/assets/javascripts/discourse/app/widgets/quick-access-bookmarks.js b/app/assets/javascripts/discourse/app/widgets/quick-access-bookmarks.js index 6d1e8463366..f315114ca6f 100644 --- a/app/assets/javascripts/discourse/app/widgets/quick-access-bookmarks.js +++ b/app/assets/javascripts/discourse/app/widgets/quick-access-bookmarks.js @@ -42,13 +42,18 @@ createWidgetFrom(QuickAccessPanel, "quick-access-bookmarks", { }, itemHtml(bookmark) { + // for topic level bookmarks we want to jump to the last unread post + // instead of the OP + let postNumber; + if (bookmark.for_topic) { + postNumber = bookmark.last_read_post_number + 1; + } else { + postNumber = bookmark.linked_post_number; + } + return this.attach("quick-access-item", { icon: this.icon(bookmark), - href: postUrl( - bookmark.slug, - bookmark.topic_id, - bookmark.post_number || bookmark.linked_post_number - ), + href: postUrl(bookmark.slug, bookmark.topic_id, postNumber), title: bookmark.name, content: bookmark.title, username: bookmark.post_user_username, diff --git a/app/serializers/user_bookmark_serializer.rb b/app/serializers/user_bookmark_serializer.rb index 67d10950509..d408878adfd 100644 --- a/app/serializers/user_bookmark_serializer.rb +++ b/app/serializers/user_bookmark_serializer.rb @@ -24,6 +24,7 @@ class UserBookmarkSerializer < ApplicationSerializer :archived, :archetype, :highest_post_number, + :last_read_post_number, :bumped_at, :slug, :post_user_username, @@ -88,7 +89,15 @@ class UserBookmarkSerializer < ApplicationSerializer end def highest_post_number - topic.highest_post_number + scope.is_staff? ? topic.highest_staff_post_number : topic.highest_post_number + end + + def last_read_post_number + topic_user&.last_read_post_number + end + + def topic_user + @topic_user ||= topic.topic_users.find { |tu| tu.user_id == scope.user.id } end def bumped_at diff --git a/lib/bookmark_query.rb b/lib/bookmark_query.rb index 01d839554a5..34ba8318b6a 100644 --- a/lib/bookmark_query.rb +++ b/lib/bookmark_query.rb @@ -68,9 +68,13 @@ class BookmarkQuery private def user_bookmarks + # There is guaranteed to be a TopicUser record if the user has bookmarked + # a topic, see BookmarkManager Bookmark.where(user: @user) .includes(post: :user) .includes(post: { topic: :tags }) + .includes(topic: :topic_users) .references(:post) + .where(topic_users: { user_id: @user.id }) end end diff --git a/spec/lib/bookmark_query_spec.rb b/spec/lib/bookmark_query_spec.rb index b53ed7b8151..b1a94462e66 100644 --- a/spec/lib/bookmark_query_spec.rb +++ b/spec/lib/bookmark_query_spec.rb @@ -17,6 +17,8 @@ RSpec.describe BookmarkQuery do describe "#list_all" do fab!(:bookmark1) { Fabricate(:bookmark, user: user) } fab!(:bookmark2) { Fabricate(:bookmark, user: user) } + let!(:topic_user1) { Fabricate(:topic_user, topic: bookmark1.topic, user: user) } + let!(:topic_user2) { Fabricate(:topic_user, topic: bookmark2.topic, user: user) } it "returns all the bookmarks for a user" do expect(bookmark_query.list_all.count).to eq(2) @@ -41,11 +43,22 @@ RSpec.describe BookmarkQuery do expect(preloaded_bookmarks.any?).to eq(true) end + it "does not query topic_users for the bookmark topic that are not the current user" do + topic_user3 = Fabricate(:topic_user, topic: bookmark1.topic) + bookmark = bookmark_query.list_all.find do |b| + b.topic_id == bookmark1.topic_id + end + + expect(bookmark.topic.topic_users.map(&:user_id)).to contain_exactly(user.id) + end + context "when q param is provided" do before do @post = Fabricate(:post, raw: "Some post content here", topic: Fabricate(:topic, title: "Bugfix game for devs")) @bookmark3 = Fabricate(:bookmark, user: user, name: "Check up later") @bookmark4 = Fabricate(:bookmark, user: user, post: @post) + Fabricate(:topic_user, user: user, topic: @bookmark3.topic) + Fabricate(:topic_user, user: user, topic: @bookmark4.topic) end it "can search by bookmark name" do @@ -174,6 +187,13 @@ RSpec.describe BookmarkQuery do let!(:bookmark4) { Fabricate(:bookmark, user: user, updated_at: 4.days.ago, reminder_at: nil) } let!(:bookmark5) { Fabricate(:bookmark, user: user, updated_at: 3.days.ago, reminder_at: nil) } + before do + [bookmark1, bookmark2, bookmark3, bookmark4, bookmark5].each do |bm| + Fabricate(:topic_user, topic: bm.topic, user: user) + bm.reload + end + end + it "order defaults to updated_at DESC" do expect(bookmark_query.list_all.map(&:id)).to eq([ bookmark1.id, @@ -195,7 +215,6 @@ RSpec.describe BookmarkQuery do bookmark2.id, bookmark3.id ]) - end it "shows pinned bookmarks first ordered by reminder_at ASC then updated_at DESC" do diff --git a/spec/models/user_bookmark_list_spec.rb b/spec/models/user_bookmark_list_spec.rb index a6327c349da..b20edd5e846 100644 --- a/spec/models/user_bookmark_list_spec.rb +++ b/spec/models/user_bookmark_list_spec.rb @@ -9,7 +9,8 @@ RSpec.describe UserBookmarkList do before do 22.times do - Fabricate(:bookmark, user: user) + bookmark = Fabricate(:bookmark, user: user) + Fabricate(:topic_user, topic: bookmark.topic, user: user) end end diff --git a/spec/serializers/user_bookmark_serializer_spec.rb b/spec/serializers/user_bookmark_serializer_spec.rb index 4225a2ed12a..8376c98cd3e 100644 --- a/spec/serializers/user_bookmark_serializer_spec.rb +++ b/spec/serializers/user_bookmark_serializer_spec.rb @@ -6,10 +6,9 @@ RSpec.describe UserBookmarkSerializer do let(:user) { Fabricate(:user) } let(:post) { Fabricate(:post, user: user) } let!(:bookmark) { Fabricate(:bookmark, name: 'Test', user: user, post: post) } - let(:bookmark_list) { BookmarkQuery.new(user: bookmark.user).list_all.to_ary } it "serializes all properties correctly" do - s = UserBookmarkSerializer.new(bookmark_list.last) + s = UserBookmarkSerializer.new(bookmark, scope: Guardian.new(user)) expect(s.id).to eq(bookmark.id) expect(s.created_at).to eq_time(bookmark.created_at) @@ -33,24 +32,17 @@ RSpec.describe UserBookmarkSerializer do expect(s.post_user_avatar_template).not_to eq(nil) end - context "when the topic is deleted" do - before do - bookmark.topic.trash! - bookmark.reload - end - it "it has nothing to serialize" do - expect(bookmark_list).to eq([]) - end - end + it "uses the correct highest_post_number column based on whether the user is staff" do + Fabricate(:post, topic: bookmark.topic) + Fabricate(:post, topic: bookmark.topic) + Fabricate(:whisper, topic: bookmark.topic) + bookmark.reload + serializer = UserBookmarkSerializer.new(bookmark, scope: Guardian.new(user)) - context "when the post is deleted" do - before do - bookmark.post.trash! - bookmark.reload - end - it "it has nothing to serialize" do - expect(bookmark_list).to eq([]) - end - end + expect(serializer.highest_post_number).to eq(3) + user.update!(admin: true) + + expect(serializer.highest_post_number).to eq(4) + end end