diff --git a/app/assets/javascripts/discourse/app/controllers/topic.js b/app/assets/javascripts/discourse/app/controllers/topic.js index 30becd174d..3eee01298f 100644 --- a/app/assets/javascripts/discourse/app/controllers/topic.js +++ b/app/assets/javascripts/discourse/app/controllers/topic.js @@ -240,7 +240,10 @@ export default Controller.extend(bufferedProperty("model"), { this.model.removeBookmark(post.bookmark_id); }); } - const forTopicBookmark = this.model.bookmarks.findBy("for_topic", true); + const forTopicBookmark = this.model.bookmarks.findBy( + "bookmarkable_type", + "Topic" + ); if ( forTopicBookmark?.auto_delete_preference === AUTO_DELETE_PREFERENCES.ON_OWNER_REPLY diff --git a/app/assets/javascripts/discourse/app/models/bookmark.js b/app/assets/javascripts/discourse/app/models/bookmark.js index cd8c81dfc9..ff8dda8a4e 100644 --- a/app/assets/javascripts/discourse/app/models/bookmark.js +++ b/app/assets/javascripts/discourse/app/models/bookmark.js @@ -143,7 +143,8 @@ const Bookmark = RestModel.extend({ // 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; + const linkedPostNumber = + this.bookmarkable_type === "Topic" ? null : this.linked_post_number; return Topic.create({ id: this.topic_id, diff --git a/app/assets/javascripts/discourse/tests/unit/models/bookmark-test.js b/app/assets/javascripts/discourse/tests/unit/models/bookmark-test.js new file mode 100644 index 0000000000..01b453eb14 --- /dev/null +++ b/app/assets/javascripts/discourse/tests/unit/models/bookmark-test.js @@ -0,0 +1,42 @@ +import { module, test } from "qunit"; +import Bookmark from "discourse/models/bookmark"; + +module("Unit | Model | bookmark", function () { + test("topicForList - Topic bookmarkable", function (assert) { + let bookmark = Bookmark.create({ + id: 1, + bookmarkable_type: "Topic", + bookmarkable_id: 999, + linked_post_number: null, + topic_id: 999, + fancy_title: "Some test topic", + last_read_post_number: 23, + highest_post_number: 30, + }); + + assert.strictEqual( + bookmark.topicForList.linked_post_number, + null, + "linked_post_number is null" + ); + }); + + test("topicForList - Post bookmarkable", function (assert) { + let bookmark = Bookmark.create({ + id: 1, + bookmarkable_type: "Post", + bookmarkable_id: 999, + linked_post_number: 787, + topic_id: 999, + fancy_title: "Some test topic", + last_read_post_number: 23, + highest_post_number: 30, + }); + + assert.strictEqual( + bookmark.topicForList.linked_post_number, + 787, + "linked_post_number is correct" + ); + }); +}); diff --git a/app/serializers/user_topic_bookmark_serializer.rb b/app/serializers/user_topic_bookmark_serializer.rb index 8aa78bc88b..84d14d1620 100644 --- a/app/serializers/user_topic_bookmark_serializer.rb +++ b/app/serializers/user_topic_bookmark_serializer.rb @@ -1,12 +1,11 @@ # frozen_string_literal: true class UserTopicBookmarkSerializer < UserPostTopicBookmarkBaseSerializer - # it does not matter what the linked post number is for topic bookmarks, - # on the client we always take the user to the last unread post in the - # topic when the bookmark URL is clicked - attributes :last_read_post_number + # NOTE: It does not matter what the linked post number is for topic bookmarks, + # on the client we always take the user to the last unread post in the + # topic when the bookmark URL is clicked def linked_post_number 1 end @@ -28,28 +27,7 @@ class UserTopicBookmarkSerializer < UserPostTopicBookmarkBaseSerializer end def cooked - @cooked ||= \ - if last_read_post_number.present? - for_topic_cooked_post - else - first_post.cooked - end - end - - def for_topic_cooked_post - post_number = [last_read_post_number + 1, highest_post_number].min - sorted_regular_posts = topic.posts.sort_by(&:post_number).select do |post| - post.post_type == Post.types[:regular] - end - first_unread_post = sorted_regular_posts.find do |post| - post.post_number >= post_number - end - - # if first_unread_cooked is blank this likely means that the last - # read post was either deleted or is a small action post. - # in this case we should just get the last regular post and - # use that for the cooked value so we have something to show - (first_unread_post || sorted_regular_posts.last).cooked + first_post.cooked end def bookmarkable_user diff --git a/app/services/topic_bookmarkable.rb b/app/services/topic_bookmarkable.rb index 67c474664c..5a5276f64a 100644 --- a/app/services/topic_bookmarkable.rb +++ b/app/services/topic_bookmarkable.rb @@ -12,7 +12,7 @@ class TopicBookmarkable < BaseBookmarkable end def self.preload_associations - [:tags, { posts: :user }] + [:tags, { first_post: :user }] end def self.perform_custom_preload!(topic_bookmarks, guardian) diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 466273e16a..88ff88ce94 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -5485,10 +5485,7 @@ RSpec.describe UsersController do let!(:post) { Fabricate(:post, topic: topic) } let!(:bookmark) { Fabricate(:bookmark, name: 'Test', user: user, bookmarkable: topic) } - it "uses the last_read_post_number + 1 for the bookmarks excerpt" do - next_unread_post = Fabricate(:post_with_long_raw_content, topic: bookmark.bookmarkable) - Fabricate(:post_with_external_links, topic: bookmark.bookmarkable) - bookmark.reload + it "uses the first post of the topic for the bookmarks excerpt" do TopicUser.change(user.id, bookmark.bookmarkable.id, { last_read_post_number: post.post_number }) sign_in(user) @@ -5496,42 +5493,10 @@ RSpec.describe UsersController do get "/u/#{user.username}/bookmarks.json" expect(response.status).to eq(200) bookmark_list = response.parsed_body["user_bookmark_list"]["bookmarks"] - expected_excerpt = PrettyText.excerpt(next_unread_post.cooked, 300, keep_emoji_images: true) + expected_excerpt = PrettyText.excerpt(topic.first_post.cooked, 300, keep_emoji_images: true) expect(bookmark_list.first["excerpt"]).to eq(expected_excerpt) end - it "does not use a small post for the last unread cooked post" do - small_action_post = Fabricate(:small_action, topic: bookmark.bookmarkable) - next_unread_post = Fabricate(:post_with_long_raw_content, topic: bookmark.bookmarkable) - Fabricate(:post_with_external_links, topic: bookmark.bookmarkable) - bookmark.reload - TopicUser.change(user.id, bookmark.bookmarkable.id, { last_read_post_number: post.post_number }) - - sign_in(user) - - get "/u/#{user.username}/bookmarks.json" - expect(response.status).to eq(200) - bookmark_list = response.parsed_body["user_bookmark_list"]["bookmarks"] - - expect(bookmark_list.first["excerpt"]).to eq(PrettyText.excerpt(next_unread_post.cooked, 300, keep_emoji_images: true)) - end - - it "handles the last read post in the topic being a small post by getting the last read regular post" do - last_regular_post = Fabricate(:post_with_long_raw_content, topic: bookmark.bookmarkable) - small_action_post = Fabricate(:small_action, topic: bookmark.bookmarkable) - bookmark.reload - topic.reload - TopicUser.change(user.id, bookmark.bookmarkable.id, { last_read_post_number: small_action_post.post_number }) - - sign_in(user) - - get "/u/#{user.username}/bookmarks.json" - expect(response.status).to eq(200) - bookmark_list = response.parsed_body["user_bookmark_list"]["bookmarks"] - - expect(bookmark_list.first["excerpt"]).to eq(PrettyText.excerpt(last_regular_post.cooked, 300, keep_emoji_images: true)) - end - describe "bookmarkable_url" do context "with the link_to_first_unread_post option" do it "is a full topic URL to the first unread post in the topic when the option is set" do