diff --git a/app/jobs/regular/export_user_archive.rb b/app/jobs/regular/export_user_archive.rb index 158f2a72f5e..c07590c8af4 100644 --- a/app/jobs/regular/export_user_archive.rb +++ b/app/jobs/regular/export_user_archive.rb @@ -362,7 +362,7 @@ module Jobs # Most forums should not have post_action records other than flags and likes, but they are possible in historical oddities. PostAction .where(user_id: @current_user.id) - .where.not(post_action_type_id: PostActionType.flag_types.values + [PostActionType.types[:like], PostActionType.types[:bookmark]]) + .where.not(post_action_type_id: PostActionType.flag_types.values + [PostActionType.types[:like]]) .exists? end @@ -371,7 +371,7 @@ module Jobs PostAction .with_deleted .where(user_id: @current_user.id) - .where.not(post_action_type_id: PostActionType.flag_types.values + [PostActionType.types[:like], PostActionType.types[:bookmark]]) + .where.not(post_action_type_id: PostActionType.flag_types.values + [PostActionType.types[:like]]) .order(:created_at) .each do |pa| yield [ diff --git a/app/models/post_action.rb b/app/models/post_action.rb index 1974370b702..e2fa99e45ff 100644 --- a/app/models/post_action.rb +++ b/app/models/post_action.rb @@ -150,10 +150,6 @@ class PostAction < ActiveRecord::Base save end - def is_bookmark? - post_action_type_id == PostActionType.types[:bookmark] - end - def is_like? post_action_type_id == PostActionType.types[:like] end @@ -169,11 +165,11 @@ class PostAction < ActiveRecord::Base # A custom rate limiter for this model def post_action_rate_limiter - return unless is_flag? || is_bookmark? || is_like? + return unless is_flag? || is_like? return @rate_limiter if @rate_limiter.present? - %w(like flag bookmark).each do |type| + %w(like flag).each do |type| if public_send("is_#{type}?") limit = SiteSetting.get("max_#{type}s_per_day") diff --git a/app/models/post_action_type.rb b/app/models/post_action_type.rb index eac965c54fb..c73e171a1fb 100644 --- a/app/models/post_action_type.rb +++ b/app/models/post_action_type.rb @@ -66,8 +66,9 @@ class PostActionType < ActiveRecord::Base def types unless @types + # NOTE: Previously bookmark was type 1 but that has been superseded + # by the separate Bookmark model and functionality @types = Enum.new( - bookmark: 1, like: 2 ) @types.merge!(flag_settings.flag_types) diff --git a/app/models/topic_list.rb b/app/models/topic_list.rb index bfa6960e0f3..192687d1779 100644 --- a/app/models/topic_list.rb +++ b/app/models/topic_list.rb @@ -104,19 +104,12 @@ class TopicList post_action_type = if @current_user if @opts[:filter].present? - if @opts[:filter] == "bookmarked" - PostActionType.types[:bookmark] - elsif @opts[:filter] == "liked" + if @opts[:filter] == "liked" PostActionType.types[:like] end end end - # Include bookmarks if you have bookmarked topics - if @current_user && !post_action_type - post_action_type = PostActionType.types[:bookmark] if @topic_lookup.any? { |_, tu| tu && tu.bookmarked } - end - # Data for bookmarks or likes post_action_lookup = PostAction.lookup_for(@current_user, @topics, post_action_type) if post_action_type diff --git a/app/models/user_action.rb b/app/models/user_action.rb index 95a388afbeb..34872487dec 100644 --- a/app/models/user_action.rb +++ b/app/models/user_action.rb @@ -10,7 +10,6 @@ class UserAction < ActiveRecord::Base LIKE = 1 WAS_LIKED = 2 - BOOKMARK = 3 NEW_TOPIC = 4 REPLY = 5 RESPONSE = 6 @@ -32,7 +31,6 @@ class UserAction < ActiveRecord::Base WAS_LIKED, MENTION, QUOTE, - BOOKMARK, EDIT, SOLVED, ASSIGNED, @@ -42,7 +40,8 @@ class UserAction < ActiveRecord::Base @types ||= Enum.new( like: 1, was_liked: 2, - bookmark: 3, + # NOTE: Previously type 3 was bookmark but this was removed when we + # changed to using the Bookmark model. new_topic: 4, reply: 5, response: 6, @@ -416,10 +415,6 @@ class UserAction < ActiveRecord::Base builder.where("t.visible") end - unless guardian.can_see_notifications?(User.where(id: user_id).first) - builder.where("a.action_type not in (#{BOOKMARK})") - end - filter_private_messages(builder, user_id, guardian, ignore_private_messages) filter_categories(builder, guardian) end diff --git a/app/serializers/post_serializer.rb b/app/serializers/post_serializer.rb index b604b1b7faf..bfb37faf26a 100644 --- a/app/serializers/post_serializer.rb +++ b/app/serializers/post_serializer.rb @@ -282,7 +282,7 @@ class PostSerializer < BasicPostSerializer result = [] can_see_post = scope.can_see_post?(object) - PostActionType.types.except(:bookmark).each do |sym, id| + PostActionType.types.each do |sym, id| count_col = "#{sym}_count".to_sym count = object.public_send(count_col) if object.respond_to?(count_col) diff --git a/app/serializers/topic_list_item_serializer.rb b/app/serializers/topic_list_item_serializer.rb index a0addd8b66c..4a8f12b37c8 100644 --- a/app/serializers/topic_list_item_serializer.rb +++ b/app/serializers/topic_list_item_serializer.rb @@ -11,7 +11,6 @@ class TopicListItemSerializer < ListableTopicSerializer :category_id, :op_like_count, :pinned_globally, - :bookmarked_post_numbers, :liked_post_numbers, :featured_link, :featured_link_root_domain, @@ -46,10 +45,6 @@ class TopicListItemSerializer < ListableTopicSerializer object.participants_summary || [] end - def include_bookmarked_post_numbers? - include_post_action? :bookmark - end - def include_liked_post_numbers? include_post_action? :like end @@ -64,10 +59,6 @@ class TopicListItemSerializer < ListableTopicSerializer object.user_data.post_action_data[PostActionType.types[:like]] end - def bookmarked_post_numbers - object.user_data.post_action_data[PostActionType.types[:bookmark]] - end - def include_participants? object.private_message? end diff --git a/app/services/user_action_manager.rb b/app/services/user_action_manager.rb index b3c169466a8..ae460b6235e 100644 --- a/app/services/user_action_manager.rb +++ b/app/services/user_action_manager.rb @@ -110,7 +110,6 @@ private end def self.post_action_rows(post_action) - action = UserAction::BOOKMARK if post_action.is_bookmark? action = UserAction::LIKE if post_action.is_like? return [] unless action diff --git a/db/fixtures/003_post_action_types.rb b/db/fixtures/003_post_action_types.rb index 21350f55669..c6a49495738 100644 --- a/db/fixtures/003_post_action_types.rb +++ b/db/fixtures/003_post_action_types.rb @@ -1,12 +1,5 @@ # frozen_string_literal: true -PostActionType.seed do |s| - s.id = PostActionType.types[:bookmark] - s.name_key = 'bookmark' - s.is_flag = false - s.position = 1 -end - PostActionType.seed do |s| s.id = PostActionType.types[:like] s.name_key = 'like' diff --git a/lib/guardian/post_guardian.rb b/lib/guardian/post_guardian.rb index 8e67cfaf190..f20920cf696 100644 --- a/lib/guardian/post_guardian.rb +++ b/lib/guardian/post_guardian.rb @@ -232,9 +232,6 @@ module PostGuardian def can_delete_post_action?(post_action) return false unless is_my_own?(post_action) && !post_action.is_private_message? - # Bookmarks do not have a time constraint - return true if post_action.is_bookmark? - post_action.created_at > SiteSetting.post_undo_action_window_mins.minutes.ago end diff --git a/lib/post_action_creator.rb b/lib/post_action_creator.rb index 7b8256b380f..93cc6f7a912 100644 --- a/lib/post_action_creator.rb +++ b/lib/post_action_creator.rb @@ -19,7 +19,7 @@ class PostActionCreator ).perform end - [:like, :off_topic, :spam, :inappropriate, :bookmark].each do |action| + [:like, :off_topic, :spam, :inappropriate].each do |action| define_method(action) do |created_by, post, silent = false| create(created_by, post, action, silent: silent) end @@ -285,7 +285,6 @@ private post_action rescue ActiveRecord::RecordNotUnique # can happen despite being .create - # since already bookmarked PostAction.where(where_attrs).first end diff --git a/lib/topic_query.rb b/lib/topic_query.rb index 5918eafc0e6..f5c2ab12962 100644 --- a/lib/topic_query.rb +++ b/lib/topic_query.rb @@ -793,9 +793,7 @@ class TopicQuery if (filter = (options[:filter] || options[:f])) && @user action = - if filter == "bookmarked" - PostActionType.types[:bookmark] - elsif filter == "liked" + if filter == "liked" PostActionType.types[:like] end if action diff --git a/plugins/discourse-narrative-bot/plugin.rb b/plugins/discourse-narrative-bot/plugin.rb index 78c4d9188a8..989f6ef19eb 100644 --- a/plugins/discourse-narrative-bot/plugin.rb +++ b/plugins/discourse-narrative-bot/plugin.rb @@ -267,8 +267,6 @@ after_initialize do self.post_action_type_id == PostActionType.types[:inappropriate] ? "flag" : "reply" when PostActionType.types[:like] "like" - when PostActionType.types[:bookmark] - "bookmark" end if input diff --git a/spec/fabricators/user_action_fabricator.rb b/spec/fabricators/user_action_fabricator.rb index 29beb6ab28f..119e8426093 100644 --- a/spec/fabricators/user_action_fabricator.rb +++ b/spec/fabricators/user_action_fabricator.rb @@ -2,5 +2,5 @@ Fabricator(:user_action) do user - action_type UserAction::BOOKMARK + action_type UserAction::EDIT end diff --git a/spec/lib/guardian_spec.rb b/spec/lib/guardian_spec.rb index 57a464fc193..9c10598a7be 100644 --- a/spec/lib/guardian_spec.rb +++ b/spec/lib/guardian_spec.rb @@ -431,7 +431,6 @@ describe Guardian do guardian = Guardian.new(user) expect(guardian.can_see_post_actors?(nil, PostActionType.types[:like])).to be_falsey expect(guardian.can_see_post_actors?(topic, PostActionType.types[:like])).to be_truthy - expect(guardian.can_see_post_actors?(topic, PostActionType.types[:bookmark])).to be_falsey expect(guardian.can_see_post_actors?(topic, PostActionType.types[:off_topic])).to be_falsey expect(guardian.can_see_post_actors?(topic, PostActionType.types[:spam])).to be_falsey expect(guardian.can_see_post_actors?(topic, PostActionType.types[:notify_user])).to be_falsey @@ -2013,21 +2012,6 @@ describe Guardian do end - context "can_delete_post_action?" do - fab!(:post) { Fabricate(:post) } - - it "allows us to remove a bookmark" do - pa = PostActionCreator.bookmark(user, post).post_action - expect(Guardian.new(user).can_delete_post_action?(pa)).to eq(true) - end - - it "allows us to remove a very old bookmark" do - pa = PostActionCreator.bookmark(user, post).post_action - pa.update(created_at: 2.years.ago) - expect(Guardian.new(user).can_delete_post_action?(pa)).to eq(true) - end - end - context 'can_delete?' do it 'returns false with a nil object' do diff --git a/spec/lib/post_action_destroyer_spec.rb b/spec/lib/post_action_destroyer_spec.rb index ad0d2f7db00..362291b8fc1 100644 --- a/spec/lib/post_action_destroyer_spec.rb +++ b/spec/lib/post_action_destroyer_spec.rb @@ -62,25 +62,5 @@ describe PostActionDestroyer do expect(messages.last.data[:type]).to eq(:acted) end end - - context 'not notifyable type' do - before do - PostActionCreator.new(user, post, PostActionType.types[:bookmark]).perform - end - - it 'destroys the post action' do - expect { - PostActionDestroyer.destroy(user, post, :bookmark) - }.to change { PostAction.count }.by(-1) - end - - it 'doesn’t notify subscribers' do - messages = MessageBus.track_publish do - PostActionDestroyer.destroy(user, post, :bookmark) - end - - expect(messages).to be_blank - end - end end end diff --git a/spec/lib/post_destroyer_spec.rb b/spec/lib/post_destroyer_spec.rb index 91efb9e238b..949e3a1af44 100644 --- a/spec/lib/post_destroyer_spec.rb +++ b/spec/lib/post_destroyer_spec.rb @@ -876,7 +876,6 @@ describe PostDestroyer do describe "post actions" do let(:second_post) { Fabricate(:post, topic_id: post.topic_id) } - let!(:bookmark) { PostActionCreator.create(moderator, second_post, :bookmark).post_action } let(:flag_result) { PostActionCreator.off_topic(moderator, second_post) } let!(:flag) { flag_result.post_action } @@ -888,14 +887,11 @@ describe PostDestroyer do url = second_post.url PostDestroyer.new(moderator, second_post).destroy - expect(PostAction.find_by(id: bookmark.id)).to eq(nil) - off_topic = PostAction.find_by(id: flag.id) expect(off_topic).not_to eq(nil) expect(off_topic.agreed_at).not_to eq(nil) second_post.reload - expect(second_post.bookmark_count).to eq(0) expect(second_post.off_topic_count).to eq(1) jobs = Jobs::SendSystemMessage.jobs @@ -948,11 +944,6 @@ describe PostDestroyer do expect(Jobs::SendSystemMessage.jobs.size).to eq(0) expect(ReviewableFlaggedPost.pending.count).to eq(0) end - - it "should set the deleted_public_actions custom field" do - PostDestroyer.new(moderator, second_post).destroy - expect(second_post.custom_fields["deleted_public_actions"]).to eq("#{bookmark.id}") - end end describe "user actions" do @@ -968,12 +959,10 @@ describe PostDestroyer do end it "should delete the user actions" do - bookmark = create_user_action(UserAction::BOOKMARK) like = create_user_action(UserAction::LIKE) PostDestroyer.new(moderator, second_post).destroy - expect(UserAction.find_by(id: bookmark.id)).to be_nil expect(UserAction.find_by(id: like.id)).to be_nil end end diff --git a/spec/lib/topic_query_spec.rb b/spec/lib/topic_query_spec.rb index 6fa6367b884..ec59e77b83f 100644 --- a/spec/lib/topic_query_spec.rb +++ b/spec/lib/topic_query_spec.rb @@ -159,25 +159,6 @@ describe TopicQuery do end end - context 'bookmarks' do - it "filters and returns bookmarks correctly" do - post = Fabricate(:post) - reply = Fabricate(:post, topic: post.topic) - - post2 = Fabricate(:post) - - PostActionCreator.create(user, post, :bookmark) - PostActionCreator.create(user, reply, :bookmark) - TopicUser.change(user, post.topic, notification_level: 1) - TopicUser.change(user, post2.topic, notification_level: 1) - - query = TopicQuery.new(user, filter: 'bookmarked').list_latest - - expect(query.topics.length).to eq(1) - expect(query.topics.first.user_data.post_action_data).to eq(PostActionType.types[:bookmark] => [1, 2]) - end - end - context 'tracked' do it "filters tracked topics correctly" do SiteSetting.tagging_enabled = true diff --git a/spec/models/post_action_spec.rb b/spec/models/post_action_spec.rb index 3acc449b8e8..be75e351e02 100644 --- a/spec/models/post_action_spec.rb +++ b/spec/models/post_action_spec.rb @@ -9,7 +9,6 @@ describe PostAction do fab!(:admin) { Fabricate(:admin) } fab!(:post) { Fabricate(:post) } fab!(:second_post) { Fabricate(:post, topic: post.topic) } - let(:bookmark) { PostAction.new(user_id: post.user_id, post_action_type_id: PostActionType.types[:bookmark] , post_id: post.id) } def value_for(user_id, dt) GivenDailyLike.find_for(user_id, dt).pluck(:likes_given)[0] || 0 @@ -131,31 +130,6 @@ describe PostAction do tu = TopicUser.get(post.topic, codinghorror) expect(tu.liked).to be true - expect(tu.bookmarked).to be false - end - - end - - describe "when a user bookmarks something" do - it "increases the post's bookmark count when saved" do - expect { bookmark.save; post.reload }.to change(post, :bookmark_count).by(1) - end - - describe 'when deleted' do - - before do - bookmark.save - post.reload - @topic = post.topic - @topic.reload - bookmark.deleted_at = DateTime.now - bookmark.save - end - - it 'reduces the bookmark count of the post' do - expect { post.reload }.to change(post, :bookmark_count).by(-1) - end - end end @@ -879,7 +853,7 @@ describe PostAction do describe ".lookup_for" do it "returns the correct map" do user = Fabricate(:user) - post_action = PostActionCreator.create(user, post, :bookmark).post_action + post_action = PostActionCreator.create(user, post, :like).post_action map = PostAction.lookup_for(user, [post.topic], post_action.post_action_type_id) expect(map).to eq(post.topic_id => [post.post_number]) diff --git a/spec/models/post_action_type_spec.rb b/spec/models/post_action_type_spec.rb index f4300b39762..209f32289f5 100644 --- a/spec/models/post_action_type_spec.rb +++ b/spec/models/post_action_type_spec.rb @@ -26,10 +26,6 @@ describe PostActionType do @types = PostActionType.types end - it "'bookmark' should be at 1st position" do - expect(@types[:bookmark]).to eq(1) - end - it "'spam' should be at 8th position" do expect(@types[:spam]).to eq(8) end diff --git a/spec/models/user_action_spec.rb b/spec/models/user_action_spec.rb index 4360624aa8d..d83b8492ee3 100644 --- a/spec/models/user_action_spec.rb +++ b/spec/models/user_action_spec.rb @@ -59,13 +59,12 @@ describe UserAction do log_test_action log_test_action(action_type: UserAction::GOT_PRIVATE_MESSAGE) log_test_action(action_type: UserAction::NEW_TOPIC, target_topic_id: public_topic.id, target_post_id: public_post.id) - log_test_action(action_type: UserAction::BOOKMARK) Jobs.run_immediately! PostActionNotifier.enable mystats = stats_for_user(user) - expecting = [UserAction::NEW_TOPIC, UserAction::NEW_PRIVATE_MESSAGE, UserAction::GOT_PRIVATE_MESSAGE, UserAction::BOOKMARK].sort + expecting = [UserAction::NEW_TOPIC, UserAction::NEW_PRIVATE_MESSAGE, UserAction::GOT_PRIVATE_MESSAGE].sort expect(mystats).to eq(expecting) expect(stream(user).map(&:action_type)) @@ -280,72 +279,6 @@ describe UserAction do end end - describe 'when user bookmarks' do - before do - @post = Fabricate(:post) - @user = @post.user - PostActionCreator.create(@user, @post, :bookmark) - @action = @user.user_actions.find_by(action_type: UserAction::BOOKMARK) - end - - it 'creates the bookmark, and removes it properly' do - expect(@action.action_type).to eq(UserAction::BOOKMARK) - expect(@action.target_post_id).to eq(@post.id) - expect(@action.acting_user_id).to eq(@user.id) - expect(@action.user_id).to eq(@user.id) - - PostActionDestroyer.destroy(@user, @post, :bookmark) - expect(@user.user_actions.find_by(action_type: UserAction::BOOKMARK)).to eq(nil) - end - end - - describe 'secures private messages' do - - fab!(:user) do - Fabricate(:user) - end - - fab!(:user2) do - Fabricate(:user) - end - - let(:private_message) do - PostCreator.create(user, - raw: 'this is a private message', - title: 'this is the pm title', - target_usernames: user2.username, - archetype: Archetype::private_message - ) - end - - def count_bookmarks - UserAction.stream( - user_id: user.id, - action_types: [UserAction::BOOKMARK], - ignore_private_messages: false, - guardian: Guardian.new(user) - ).count - end - - it 'correctly secures stream' do - PostActionCreator.create(user, private_message, :bookmark) - - expect(count_bookmarks).to eq(1) - - private_message.topic.topic_allowed_users.where(user_id: user.id).destroy_all - - expect(count_bookmarks).to eq(0) - - group = Fabricate(:group) - group.add(user) - private_message.topic.topic_allowed_groups.create(group_id: group.id) - - expect(count_bookmarks).to eq(1) - - end - - end - describe 'synchronize_target_topic_ids' do it 'correct target_topic_id' do post = Fabricate(:post) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 6842bd1f4c1..73d1b5fc5c9 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -265,31 +265,6 @@ describe User do end end - describe 'bookmark' do - before_all do - @post = Fabricate(:post) - end - - it "creates a bookmark with the true parameter" do - expect { - PostActionCreator.create(@post.user, @post, :bookmark) - }.to change(PostAction, :count).by(1) - end - - describe 'when removing a bookmark' do - before do - PostActionCreator.create(@post.user, @post, :bookmark) - end - - it 'reduces the bookmark count of the post' do - active = PostAction.where(deleted_at: nil) - expect { - PostActionDestroyer.destroy(@post.user, @post, :bookmark) - }.to change(active, :count).by(-1) - end - end - end - describe 'delete posts in batches' do fab!(:post1) { Fabricate(:post) } fab!(:user) { post1.user } diff --git a/spec/requests/post_actions_controller_spec.rb b/spec/requests/post_actions_controller_spec.rb index 9e62d48f150..00f0ec80cce 100644 --- a/spec/requests/post_actions_controller_spec.rb +++ b/spec/requests/post_actions_controller_spec.rb @@ -23,7 +23,7 @@ RSpec.describe PostActionsController do end it "returns 404 when the post action type doesn't exist for that user" do - delete "/post_actions/#{post.id}.json", params: { post_action_type_id: PostActionType.types[:bookmark] } + delete "/post_actions/#{post.id}.json", params: { post_action_type_id: PostActionType.types[:like] } expect(response.status).to eq(404) end @@ -32,36 +32,31 @@ RSpec.describe PostActionsController do PostAction.create!( user_id: user.id, post_id: post.id, - post_action_type_id: PostActionType.types[:bookmark] + post_action_type_id: PostActionType.types[:like] ) end it 'returns success' do - delete "/post_actions/#{post.id}.json", params: { post_action_type_id: PostActionType.types[:bookmark] } + delete "/post_actions/#{post.id}.json", params: { post_action_type_id: PostActionType.types[:like] } expect(response.status).to eq(200) end it 'deletes the action' do delete "/post_actions/#{post.id}.json", params: { - post_action_type_id: PostActionType.types[:bookmark] + post_action_type_id: PostActionType.types[:like] } expect(response.status).to eq(200) expect(PostAction.exists?( user_id: user.id, post_id: post.id, - post_action_type_id: PostActionType.types[:bookmark], + post_action_type_id: PostActionType.types[:like], deleted_at: nil )).to eq(false) end it "isn't deleted when the user doesn't have permission" do - pa = PostAction.create!( - post: post, - user: user, - post_action_type_id: PostActionType.types[:like], - created_at: 1.day.ago - ) + post_action.update!(created_at: 1.day.ago) delete "/post_actions/#{post.id}.json", params: { post_action_type_id: PostActionType.types[:like] @@ -85,7 +80,7 @@ RSpec.describe PostActionsController do post "/post_actions.json", params: { id: pm.id, - post_action_type_id: PostActionType.types[:bookmark] + post_action_type_id: PostActionType.types[:like] } expect(response.status).to eq(403)