UX: Rename "Keep Post" to "Keep Post Hidden" when hidden (#7767)

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: Category Group Review/Moderation - announcements - Discourse Meta

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.

diff --git a/app/models/reviewable.rb b/app/models/reviewable.rb
index 1bdb587..7558dec 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 5ff7491..009c2a7 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 18bec78..7ba365e 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 52413d1..5323efc 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)

GitHub sha: 6f367dde

1 Like