diff --git a/app/assets/javascripts/discourse/app/models/bookmark.js b/app/assets/javascripts/discourse/app/models/bookmark.js
index 52b032e2642..97d45b219ed 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 8816e46983b..9dbfd967959 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}}
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 6d1e8463366..f315114ca6f 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 67d10950509..d408878adfd 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 01d839554a5..34ba8318b6a 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 b53ed7b8151..b1a94462e66 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 a6327c349da..b20edd5e846 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 4225a2ed12a..8376c98cd3e 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) }
- 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