From 6f367dde26b37738fcd0cbf850e9e81cfd06fe80 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Thu, 1 Aug 2019 11:23:23 -0400 Subject: [PATCH] UX: Rename "Keep Post" to "Keep Post Hidden" when hidden (#7767) * UX: Rename "Keep Post" to "Keep Post Hidden" when hidden This is based on this feedback: https://meta.discourse.org/t/category-group-review-moderation/116478/19 When a post is hidden this makes the operation much more clear. * REFACTOR: Better support for aliases for actions Allow calls on alias actions and delegate to the original one. This is less code but also simplifies tests where the action might be "agree_and_keep" or "agree_and_keep_hidden" which are the same. --- app/models/reviewable.rb | 13 ++++++++-- app/models/reviewable_flagged_post.rb | 28 ++++++++++----------- config/locales/server.en.yml | 3 +++ spec/models/reviewable_flagged_post_spec.rb | 8 ++++++ 4 files changed, 36 insertions(+), 16 deletions(-) diff --git a/app/models/reviewable.rb b/app/models/reviewable.rb index 1bdb587346..7558dec431 100644 --- a/app/models/reviewable.rb +++ b/app/models/reviewable.rb @@ -41,6 +41,11 @@ class Reviewable < ActiveRecord::Base Jobs.enqueue(:notify_reviewable, reviewable_id: self.id) if pending? end + # Can be used if several actions are equivalent + def self.action_aliases + {} + end + # The gaps are in case we want more precision in the future def self.priorities @priorities ||= Enum.new( @@ -283,11 +288,15 @@ class Reviewable < ActiveRecord::Base def perform(performed_by, action_id, args = nil) args ||= {} + # Support this action or any aliases + aliases = self.class.action_aliases + valid = [ action_id, aliases.to_a.select { |k, v| v == action_id }.map(&:first) ].flatten + # Ensure the user has access to the action actions = actions_for(Guardian.new(performed_by), args) - raise InvalidAction.new(action_id, self.class) unless actions.has?(action_id) + raise InvalidAction.new(action_id, self.class) unless valid.any? { |a| actions.has?(a) } - perform_method = "perform_#{action_id}".to_sym + perform_method = "perform_#{aliases[action_id] || action_id}".to_sym raise InvalidAction.new(action_id, self.class) unless respond_to?(perform_method) result = nil diff --git a/app/models/reviewable_flagged_post.rb b/app/models/reviewable_flagged_post.rb index 5ff749158f..009c2a78bf 100644 --- a/app/models/reviewable_flagged_post.rb +++ b/app/models/reviewable_flagged_post.rb @@ -4,6 +4,14 @@ require_dependency 'reviewable' class ReviewableFlaggedPost < Reviewable + # Penalties are handled by the modal after the action is performed + def self.action_aliases + { agree_and_keep_hidden: :agree_and_keep, + agree_and_silence: :agree_and_keep, + agree_and_suspend: :agree_and_keep, + disagree_and_restore: :disagree } + end + def self.counts_for(posts) result = {} @@ -40,7 +48,12 @@ class ReviewableFlaggedPost < Reviewable build_action(actions, :agree_and_hide, icon: 'far-eye-slash', bundle: agree) end - build_action(actions, :agree_and_keep, icon: 'thumbs-up', bundle: agree) + if post.hidden? + build_action(actions, :agree_and_keep_hidden, icon: 'thumbs-up', bundle: agree) + else + build_action(actions, :agree_and_keep, icon: 'thumbs-up', bundle: agree) + end + if guardian.can_suspend?(target_created_by) build_action(actions, :agree_and_suspend, icon: 'ban', bundle: agree, client_action: 'suspend') build_action(actions, :agree_and_silence, icon: 'microphone-slash', bundle: agree, client_action: 'silence') @@ -119,19 +132,10 @@ class ReviewableFlaggedPost < Reviewable end end - # Penalties are handled by the modal after the action is performed def perform_agree_and_keep(performed_by, args) agree(performed_by, args) end - def perform_agree_and_suspend(performed_by, args) - agree(performed_by, args) - end - - def perform_agree_and_silence(performed_by, args) - agree(performed_by, args) - end - def perform_delete_spammer(performed_by, args) UserDestroyer.new(performed_by).destroy( post.user, @@ -159,10 +163,6 @@ class ReviewableFlaggedPost < Reviewable end end - def perform_disagree_and_restore(performed_by, args) - perform_disagree(performed_by, args) - end - def perform_disagree(performed_by, args) # -1 is the automatic system clear action_type_ids = diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 18bec78090..7ba365e958 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -4492,6 +4492,9 @@ en: agree_and_keep: title: "Keep Post" description: "Agree with flag and keep the post unchanged." + agree_and_keep_hidden: + title: "Keep Post Hidden" + description: "Agree with flag and leave the post hidden." agree_and_suspend: title: "Suspend User" description: "Agree with flag and suspend the user." diff --git a/spec/models/reviewable_flagged_post_spec.rb b/spec/models/reviewable_flagged_post_spec.rb index 52413d16f5..5323efcc41 100644 --- a/spec/models/reviewable_flagged_post_spec.rb +++ b/spec/models/reviewable_flagged_post_spec.rb @@ -31,6 +31,7 @@ RSpec.describe ReviewableFlaggedPost, type: :model do actions = reviewable.actions_for(guardian) expect(actions.has?(:agree_and_hide)).to eq(true) expect(actions.has?(:agree_and_keep)).to eq(true) + expect(actions.has?(:agree_and_keep_hidden)).to eq(false) expect(actions.has?(:agree_and_silence)).to eq(true) expect(actions.has?(:agree_and_suspend)).to eq(true) expect(actions.has?(:delete_spammer)).to eq(true) @@ -54,6 +55,13 @@ RSpec.describe ReviewableFlaggedPost, type: :model do expect(actions.has?(:delete_and_replies)).to eq(false) end + it "changes `agree_and_keep` to `agree_and_keep_hidden` if it's been hidden" do + post.hidden = true + actions = reviewable.actions_for(guardian) + expect(actions.has?(:agree_and_keep)).to eq(false) + expect(actions.has?(:agree_and_keep_hidden)).to eq(true) + end + it "returns `agree_and_restore` if the post is user deleted" do post.update(user_deleted: true) expect(reviewable.actions_for(guardian).has?(:agree_and_restore)).to eq(true)