From 367de2594d7951789c243bf4a4d57ada0b62bb22 Mon Sep 17 00:00:00 2001 From: Blake Erickson Date: Wed, 19 Aug 2020 09:21:02 -0600 Subject: [PATCH] FIX: Unlike own posts on ownership transfer (#10446) * FIX: Unlike own posts on ownership transfer If a user has liked a post that has passed the `post_undo_action_window_mins` system setting window and you transfer ownership of that post to that user you will be the owner of a post that you have liked, but cannot unlike resulting in a weird UI behavior. This commit fixes this issue. The existing tests didn't check for the timeout window for unliking posts so I added that in. I couldn't find a good way to do this logic inside of the guardian class so rather than duplicating behavior of the `PostActionDestroyer` class inside of the `PostOwnerChanger` I decided to pass in a "bypass" variable that could be used to check if the calling class is the 'post_owner_changer' and bypass the guardian instead. I went this route because the guardian `can_delete_post_action` method has no way of distinguishing how to allow a user to be able to unlike their own posts after the timeout window but only on a post owner change. * use an options hash instead --- app/services/post_owner_changer.rb | 2 +- lib/post_action_destroyer.rb | 10 +++++----- spec/requests/topics_controller_spec.rb | 17 +++++++++++++++++ spec/services/post_owner_changer_spec.rb | 3 +++ 4 files changed, 26 insertions(+), 6 deletions(-) diff --git a/app/services/post_owner_changer.rb b/app/services/post_owner_changer.rb index 9a2d128c452..785c2cf97b5 100644 --- a/app/services/post_owner_changer.rb +++ b/app/services/post_owner_changer.rb @@ -23,7 +23,7 @@ class PostOwnerChanger post.topic = @topic post.set_owner(@new_owner, @acting_user, @skip_revision) - PostActionDestroyer.destroy(@new_owner, post, :like) + PostActionDestroyer.destroy(@new_owner, post, :like, skip_delete_check: true) level = post.is_first_post? ? :watching : :tracking TopicUser.change(@new_owner.id, @topic.id, notification_level: NotificationLevels.topic_levels[level]) diff --git a/lib/post_action_destroyer.rb b/lib/post_action_destroyer.rb index cf2280a5877..77d004b2b84 100644 --- a/lib/post_action_destroyer.rb +++ b/lib/post_action_destroyer.rb @@ -5,12 +5,12 @@ class PostActionDestroyer attr_accessor :post end - def initialize(destroyed_by, post, post_action_type_id) - @destroyed_by, @post, @post_action_type_id = destroyed_by, post, post_action_type_id + def initialize(destroyed_by, post, post_action_type_id, opts = {}) + @destroyed_by, @post, @post_action_type_id, @opts = destroyed_by, post, post_action_type_id, opts end - def self.destroy(destroyed_by, post, action_key) - new(destroyed_by, post, PostActionType.types[action_key]).perform + def self.destroy(destroyed_by, post, action_key, opts = {}) + new(destroyed_by, post, PostActionType.types[action_key], opts).perform end def perform @@ -34,7 +34,7 @@ class PostActionDestroyer return result end - unless guardian.can_delete?(post_action) + unless @opts[:skip_delete_check] == true || guardian.can_delete?(post_action) result.forbidden = true result.add_error(I18n.t("invalid_access")) return result diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 311a7bf6125..17a896bac93 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -715,6 +715,23 @@ RSpec.describe TopicsController do expect(t2.deleted_at).to be_nil expect(p3.user).to eq(user_a) end + + it "removes likes by new owner" do + now = Time.zone.now + freeze_time(now - 1.day) + PostActionCreator.like(user_a, p1) + p1.reload + freeze_time(now) + post "/t/#{topic.id}/change-owner.json", params: { + username: user_a.username_lower, post_ids: [p1.id] + } + topic.reload + p1.reload + expect(response.status).to eq(200) + expect(topic.user.username).to eq(user_a.username) + expect(p1.user.username).to eq(user_a.username) + expect(p1.like_count).to eq(0) + end end end diff --git a/spec/services/post_owner_changer_spec.rb b/spec/services/post_owner_changer_spec.rb index d2cf53b6add..a6c4d3a8569 100644 --- a/spec/services/post_owner_changer_spec.rb +++ b/spec/services/post_owner_changer_spec.rb @@ -24,12 +24,15 @@ describe PostOwnerChanger do it "changes the user" do bumped_at = freeze_time topic.bumped_at + now = Time.zone.now + freeze_time(now - 1.day) old_user = p1.user PostActionCreator.like(user_a, p1) p1.reload expect(p1.topic.like_count).to eq(1) + freeze_time(now) PostOwnerChanger.new(post_ids: [p1.id], topic_id: topic.id, new_owner: user_a, acting_user: editor).change_owner! p1.reload expect(p1.topic.like_count).to eq(0)