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.

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

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}}
                 </div>
               </span>
               <div class="link-bottom-line">

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,

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

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

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

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
 

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