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!