FEATURE: Go to last unread for topic-level bookmark links (#14396)

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.

diff --git a/app/assets/javascripts/discourse/app/models/bookmark.js b/app/assets/javascripts/discourse/app/models/bookmark.js
index 52b032e..97d45b2 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 8816e46..9dbfd96 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}}
                 </div>
               </span>
               <div class="link-bottom-line">
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 6d1e846..f315114 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 67d1095..d408878 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 01d8395..34ba831 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 b53ed7b..b1a9446 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 a6327c3..b20edd5 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 4225a2e..8376c98 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) }

[... diff too long, it was truncated ...]

GitHub sha: 27699648ef8a73d20f0729e97b8ca684513edfd5

This commit appears in #14396 which was approved by tgxworld. It was merged by martin.