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.

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) {

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,

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

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

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|

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!