From ef63366de1a942992474df1329d46afa8f574da2 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 7 Sep 2021 10:39:16 +1000 Subject: [PATCH] FEATURE: Go to unread post for topic-level bookmarks Instead of going to the OP of the topic for topic-level bookmarks 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 | 18 +++++++++++++++--- .../app/widgets/quick-access-bookmarks.js | 13 ++++++++----- app/serializers/user_bookmark_serializer.rb | 7 ++++++- lib/bookmark_query.rb | 2 ++ spec/lib/bookmark_query_spec.rb | 10 ++++++++++ .../user_bookmark_serializer_spec.rb | 15 ++++++++++++++- 6 files changed, 55 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/discourse/app/models/bookmark.js b/app/assets/javascripts/discourse/app/models/bookmark.js index 52b032e264..159160a0db 100644 --- a/app/assets/javascripts/discourse/app/models/bookmark.js +++ b/app/assets/javascripts/discourse/app/models/bookmark.js @@ -125,9 +125,21 @@ 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() + topicLink() { + // 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 linked_post_number = + this.linked_post_number === 1 ? null : this.linked_post_number; + + return Topic.create({ + id: this.topic_id, + fancy_title: this.fancy_title, + linked_post_number, + 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/widgets/quick-access-bookmarks.js b/app/assets/javascripts/discourse/app/widgets/quick-access-bookmarks.js index 6d1e846336..0f64ce067c 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,16 @@ 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 = bookmark.linked_post_number; + if (postNumber === 1) { + postNumber = bookmark.last_read_post_number + 1; + } + 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 39a494c579..58c93d29a8 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, @@ -83,7 +84,11 @@ class UserBookmarkSerializer < ApplicationSerializer end def highest_post_number - topic.highest_post_number + scope.user.staff? ? topic.highest_staff_post_number : topic.highest_post_number + end + + def last_read_post_number + topic.topic_users.find { |tu| tu.user_id == scope.user.id }&.last_read_post_number end def bumped_at diff --git a/lib/bookmark_query.rb b/lib/bookmark_query.rb index 02dee3b8d2..63bcc79c4e 100644 --- a/lib/bookmark_query.rb +++ b/lib/bookmark_query.rb @@ -71,7 +71,9 @@ class BookmarkQuery Bookmark.where(user: @user) .includes(topic: :tags) .includes(post: :user) + .includes(topic: :topic_users) .references(:topic) .references(:post) + .where(topic_users: { user_id: [@user.id, nil] }) end end diff --git a/spec/lib/bookmark_query_spec.rb b/spec/lib/bookmark_query_spec.rb index 5511164d01..6ae3aedc56 100644 --- a/spec/lib/bookmark_query_spec.rb +++ b/spec/lib/bookmark_query_spec.rb @@ -32,6 +32,16 @@ RSpec.describe BookmarkQuery do expect(bookmark_query.list_all.count).to eq(1) end + it "does not query topic_users for the bookmark topic that are not the current user" do + topic_user1 = Fabricate(:topic_user, topic: bookmark1.topic, user: user) + topic_user2 = Fabricate(:topic_user, topic: bookmark1.topic) + expect( + bookmark_query.list_all.find do |b| + b.topic_id == bookmark1.topic_id + end.topic.topic_users.map(&:user_id) + ).to eq([user.id]) + end + it "runs the on_preload block provided passing in bookmarks" do preloaded_bookmarks = [] BookmarkQuery.on_preload do |bookmarks, bq| diff --git a/spec/serializers/user_bookmark_serializer_spec.rb b/spec/serializers/user_bookmark_serializer_spec.rb index a192cae912..57390ac52d 100644 --- a/spec/serializers/user_bookmark_serializer_spec.rb +++ b/spec/serializers/user_bookmark_serializer_spec.rb @@ -9,7 +9,7 @@ RSpec.describe UserBookmarkSerializer do 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_list.last, scope: Guardian.new(user)) expect(s.id).to eq(bookmark.id) expect(s.created_at).to eq_time(bookmark.created_at) @@ -33,6 +33,19 @@ RSpec.describe UserBookmarkSerializer do expect(s.post_user_avatar_template).not_to eq(nil) 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) + list = BookmarkQuery.new(user: bookmark.user).list_all.to_ary + s = UserBookmarkSerializer.new(list.last, scope: Guardian.new(user)) + expect(s.highest_post_number).to eq(3) + user.update!(admin: true) + list = BookmarkQuery.new(user: bookmark.user).list_all.to_ary + s = UserBookmarkSerializer.new(list.last, scope: Guardian.new(user)) + expect(s.highest_post_number).to eq(4) + end + context "when the topic is deleted" do before do bookmark.topic.trash!