From 23e7542f6095027a016bdc0418172f4a77742e0c Mon Sep 17 00:00:00 2001 From: Gary Pendergast Date: Tue, 21 Oct 2025 11:13:13 +1100 Subject: [PATCH] DEV: Improve the helper methods for defining reviewable actions (#35406) This change adds a new `ReviewableActionBuilder#build_bundle` helper for quickly defining action bundles that can be performed on reviewables. `ReviewableActionBuilder#build_action` has also been updated to allow plugin-defined actions to appear correctly. The core reviewable types have been updated to use this new method, and I've also added support for reviewable chat messages, to demonstrate plugin support. Co-authored-by: Krzysztof Kotlarek --- .../concerns/reviewable_action_builder.rb | 161 +++++++++--------- app/models/reviewable_flagged_post.rb | 6 +- app/models/reviewable_post.rb | 6 +- app/models/reviewable_queued_post.rb | 6 +- app/models/reviewable_user.rb | 4 +- .../app/models/chat/reviewable_message.rb | 62 ++++--- .../reviewable-refresh/chat-message.gjs | 7 + plugins/chat/config/locales/server.en.yml | 11 ++ .../reviewable_action_builder_spec.rb | 10 +- 9 files changed, 163 insertions(+), 110 deletions(-) create mode 100644 plugins/chat/assets/javascripts/discourse/components/reviewable-refresh/chat-message.gjs diff --git a/app/models/concerns/reviewable_action_builder.rb b/app/models/concerns/reviewable_action_builder.rb index e70ca5748d5..c5961bd913e 100644 --- a/app/models/concerns/reviewable_action_builder.rb +++ b/app/models/concerns/reviewable_action_builder.rb @@ -3,81 +3,69 @@ module ReviewableActionBuilder extend ActiveSupport::Concern + attr_accessor :actions, :guardian, :action_args + # Standard post-actions bundle. Consumers should use the returned # bundle value when adding post-focused actions. # - # @param actions [Reviewable::Actions] Actions instance to add the bundle to. - # @param guardian [Guardian] Guardian instance to check permissions. - # # @return [Reviewable::Actions::Bundle] The created post actions bundle. - def build_post_actions_bundle(actions, guardian) - bundle = - actions.add_bundle( - "#{id}-post-actions", - label: "reviewables.actions.post_actions.bundle_title", - ) - - # Always include the no-op action - build_action(actions, :no_action_post, bundle:) - - return bundle unless target_post - - if target_post.trashed? && guardian.can_recover_post?(target_post) - build_action(actions, :restore_post, bundle:) - end - - if target_post.hidden? - build_action(actions, :unhide_post, bundle:) if !target_post.user_deleted? - else - build_action(actions, :hide_post, bundle:) - end - - if guardian.can_delete_post_or_topic?(target_post) - build_action(actions, :delete_post, bundle:) - if target_post.reply_count > 0 - build_action(actions, :delete_post_and_replies, bundle:, confirm: true) + def build_post_actions_bundle + bundle_actions = { no_action_post: {} } + if target_post + if target_post.trashed? && @guardian.can_recover_post?(target_post) + bundle_actions[:restore_post] = {} end + + if target_post.hidden? + bundle_actions[:unhide_post] = {} if !target_post.user_deleted? + else + bundle_actions[:hide_post] = {} + end + + if @guardian.can_delete_post_or_topic?(target_post) + bundle_actions[:delete_post] = {} + bundle_actions[:delete_post_and_replies] = { confirm: true } if target_post.reply_count > 0 + end + + bundle_actions[:edit_post] = { client_action: "edit" } + + bundle_actions[:convert_to_pm] = {} end - build_action(actions, :edit_post, bundle:, client_action: "edit") - - build_action(actions, :convert_to_pm, bundle:) - - bundle + build_bundle( + "#{id}-post-actions", + "reviewables.actions.post_actions.bundle_title", + bundle_actions, + source: "core", + ) end # Standard user-actions bundle and default user actions. # - # @param actions [Reviewable::Actions] Actions instance to add the bundle to. - # @param guardian [Guardian] Guardian instance to check permissions. - # # @return [Reviewable::Actions::Bundle] The created user actions bundle. - def build_user_actions_bundle(actions, guardian) - bundle = - actions.add_bundle( - "#{id}-user-actions", - label: "reviewables.actions.user_actions.bundle_title", - ) + def build_user_actions_bundle + bundle_actions = { no_action_user: {} } + if target_user + if @guardian.can_silence_user?(target_user) + bundle_actions[:silence_user] = { client_action: "silence" } + end - # Always include the no-op action - build_action(actions, :no_action_user, bundle:) + if @guardian.can_suspend?(target_user) + bundle_actions[:suspend_user] = { client_action: "suspend" } + end - return bundle unless target_user - - if guardian.can_silence_user?(target_user) - build_action(actions, :silence_user, bundle:, client_action: "silence") + if @guardian.can_delete_user?(target_user) + bundle_actions[:delete_user] = {} + bundle_actions[:delete_and_block_user] = {} + end end - if guardian.can_suspend?(target_user) - build_action(actions, :suspend_user, bundle:, client_action: "suspend") - end - - if guardian.can_delete_user?(target_user) - build_action(actions, :delete_user, bundle:) - build_action(actions, :delete_and_block_user, bundle:) - end - - bundle + build_bundle( + "#{id}-user-actions", + "reviewables.actions.user_actions.bundle_title", + bundle_actions, + source: "core", + ) end # Build actions for the reviewable based on the current state and guardian permissions. @@ -90,8 +78,12 @@ module ReviewableActionBuilder # # @return [void] def build_actions(actions, guardian, args) + @actions = actions + @guardian = guardian + @action_args = args + if guardian.can_see_reviewable_ui_refresh? - build_new_separated_actions(actions, guardian, args) + build_new_separated_actions else build_legacy_combined_actions(actions, guardian, args) end @@ -120,26 +112,42 @@ module ReviewableActionBuilder # # @TODO (reviewable-refresh) Remove this method once the new UI is fully implemented. # - # @param actions [Reviewable::Actions] Actions instance to add the bundle to. - # @param guardian [Guardian] Guardian instance to check permissions. - # @param args [Hash] Additional arguments for building actions. - # # @return [void] - def build_new_separated_actions(actions, guardian, args) + def build_new_separated_actions raise NotImplementedError, "Including class must implement #build_new_separated_actions" end + # Build a bundle of actions and add it to the provided actions list. + # + # @param id [String] ID for the bundle, used to derive I18n keys. + # @param label [String] I18n key for the bundle label. + # @param bundle_actions [Hash] Hash of action IDs and optional params to pass to build_action. + # @option bundle_actions [Symbol] :client_action Optional client-side action identifier (e.g. "edit"). + # @option bundle_actions [Symbol] :confirm When true, uses "reviewables.actions..confirm" for confirm_message. + # @option bundle_actions [Symbol] :require_reject_reason When true, requires a rejection reason for the action. + # @param source [String] Optional source string for namespacing I18n keys. Will default to `type_source`. + # + # @return [Reviewable::Actions::Bundle] The created bundle. + def build_bundle(id, label, bundle_actions = {}, source: nil) + bundle = @actions.add_bundle(id, label:) + bundle_actions.each do |action_id, action_params| + build_action(@actions, action_id, bundle:, **action_params || {}, source:) + end + bundle + end + # Build a single reviewable action and add it to the provided actions list. # This is the canonical API used by both the legacy and refreshed UI code paths. # # @param actions [Reviewable::Actions] Actions instance to add to. # @param id [Symbol] Symbol for the action, used to derive I18n keys. # @param icon [String] Optional name of the icon to display with the action. Ignored in the refreshed UI. - # @param button_class [String] Optional CSS class for buttons in clients that render it. + # @param button_class [String] Optional CSS class for buttons in clients that render it. Ignored in the refreshed UI. # @param bundle [Reviewable::Actions::Bundle] Optional bundle object returned by add_bundle to group actions. # @param client_action [String] Optional client-side action identifier (e.g. "edit"). # @param confirm [Boolean] When true, uses "reviewables.actions..confirm" for confirm_message. # @param require_reject_reason [Boolean] When true, requires a rejection reason for the action. + # @param source [String] Optional source string for namespacing I18n keys. Will default to `type_source`. # # @return [Reviewable::Actions] The updated actions instance. def build_action( @@ -150,10 +158,17 @@ module ReviewableActionBuilder bundle: nil, client_action: nil, confirm: false, - require_reject_reason: false + require_reject_reason: false, + source: nil ) actions.add(id, bundle: bundle) do |action| - prefix = "reviewables.actions.#{id}" + source ||= type_source + if source == "core" + prefix = "reviewables.actions.#{id}" + else + prefix = "#{source.underscore}.reviewables.actions.#{id}" + end + action.icon = icon if icon action.button_class = button_class if button_class action.label = "#{prefix}.title" @@ -222,14 +237,8 @@ module ReviewableActionBuilder end def perform_convert_to_pm(performed_by, _args) - topic = target_post.topic - - if topic && Guardian.new(performed_by).can_moderate?(topic) - topic.convert_to_private_message(performed_by) - create_result(:success, :approved, [created_by_id], false) - else - create_result(:failure, :approved) { |r| r.errors = ["Cannot convert to PM"] } - end + # TODO (reviewable-refresh): Implement convert to PM logic + create_result(:success, :rejected, [created_by_id], false) end private diff --git a/app/models/reviewable_flagged_post.rb b/app/models/reviewable_flagged_post.rb index 43d767c0ffa..15129b3df37 100644 --- a/app/models/reviewable_flagged_post.rb +++ b/app/models/reviewable_flagged_post.rb @@ -149,9 +149,9 @@ class ReviewableFlaggedPost < Reviewable end # TODO (reviewable-refresh): Merge into build_actions post rollout. - def build_new_separated_actions(actions, guardian, args) - build_post_actions_bundle(actions, guardian) - build_user_actions_bundle(actions, guardian) + def build_new_separated_actions + build_post_actions_bundle + build_user_actions_bundle end def perform_ignore(performed_by, args) diff --git a/app/models/reviewable_post.rb b/app/models/reviewable_post.rb index fe696dee506..49f0dc37611 100644 --- a/app/models/reviewable_post.rb +++ b/app/models/reviewable_post.rb @@ -79,9 +79,9 @@ class ReviewablePost < Reviewable end # TODO (reviewable-refresh): Merge this method into build_actions when fully migrated to new UI - def build_new_separated_actions(actions, guardian, args) - build_post_actions_bundle(actions, guardian) - build_user_actions_bundle(actions, guardian) + def build_new_separated_actions + build_post_actions_bundle + build_user_actions_bundle end # TODO (reviewable-refresh): Remove combined actions below when fully migrated to new UI diff --git a/app/models/reviewable_queued_post.rb b/app/models/reviewable_queued_post.rb index 2545d0ec180..3d1071a3c82 100644 --- a/app/models/reviewable_queued_post.rb +++ b/app/models/reviewable_queued_post.rb @@ -71,9 +71,9 @@ class ReviewableQueuedPost < Reviewable build_action(actions, :delete) if guardian.can_delete?(self) end - def build_new_separated_actions(actions, guardian, args) + def build_new_separated_actions # Because a queued post isn't a real post, we need to create our own post actions bundle - post_actions_bundle = build_post_actions_bundle(actions, guardian) + post_actions_bundle = build_post_actions_bundle unless approved? if topic&.closed? @@ -89,7 +89,7 @@ class ReviewableQueuedPost < Reviewable end # User actions bundle - build_user_actions_bundle(actions, guardian) if pending? + build_user_actions_bundle if pending? end def build_editable_fields(fields, guardian, args) diff --git a/app/models/reviewable_user.rb b/app/models/reviewable_user.rb index 5e4f0c5150a..75141590a90 100644 --- a/app/models/reviewable_user.rb +++ b/app/models/reviewable_user.rb @@ -22,8 +22,8 @@ class ReviewableUser < Reviewable delete_user_actions(actions, require_reject_reason: !is_a_suspect_user?) end - def build_new_separated_actions(actions, guardian, args) - build_legacy_combined_actions(actions, guardian, args) + def build_new_separated_actions + build_legacy_combined_actions(@actions, @guardian, @action_args) end def perform_approve_user(performed_by, args) diff --git a/plugins/chat/app/models/chat/reviewable_message.rb b/plugins/chat/app/models/chat/reviewable_message.rb index a696e31fb98..9bf53945e50 100644 --- a/plugins/chat/app/models/chat/reviewable_message.rb +++ b/plugins/chat/app/models/chat/reviewable_message.rb @@ -2,6 +2,8 @@ module Chat class ReviewableMessage < Reviewable + include ReviewableActionBuilder + validates :type, length: { maximum: 100 } validates :target_type, length: { maximum: 100 } @@ -38,7 +40,8 @@ module Chat nil end - def build_actions(actions, guardian, args) + # TODO (reviewable-refresh): Remove this method when fully migrated to new UI + def build_legacy_combined_actions(actions, guardian, args) return unless pending? return build_action(actions, :ignore, icon: "up-right-from-square") if chat_message.blank? @@ -86,6 +89,24 @@ module Chat end end + # TODO (reviewable-refresh): Merge this method into build_actions when fully migrated to new UI + def build_new_separated_actions + bundle_actions = { no_action_message: {} } + if chat_message.deleted_at? + bundle_actions[:restore_message] = {} + else + bundle_actions[:delete_message] = {} + end + build_bundle( + "#{id}-message-actions", + "chat.reviewables.actions.message_actions.bundle_title", + bundle_actions, + ) + + build_user_actions_bundle + end + + # TODO (reviewable-refresh): Remove combined actions below when fully migrated to new UI def perform_agree_and_keep_message(performed_by, args) agree end @@ -117,6 +138,25 @@ module Chat def perform_agree_and_keep_deleted(performed_by, args) agree end + # TODO (reviewable-refresh): Remove combined actions above when fully migrated to new UI + + def perform_no_action_message(performed_by, args) + if chat_message.deleted_at? + create_result(:success, :approved, [created_by_id], true) + else + create_result(:success, :rejected, [created_by_id], true) + end + end + + def perform_restore_message(_performed_by, args) + chat_message.recover! + create_result(:success, :rejected, [created_by_id], true) + end + + def perform_delete_message(performed_by, args) + chat_message.trash!(performed_by) + create_result(:success, :approved, [created_by_id], true) + end private @@ -145,26 +185,6 @@ module Chat result.update_flag_stats = { status: :ignored, user_ids: flagged_by_user_ids } end end - - def build_action( - actions, - id, - icon:, - button_class: nil, - bundle: nil, - client_action: nil, - confirm: false - ) - actions.add(id, bundle: bundle) do |action| - prefix = "reviewables.actions.#{id}" - action.icon = icon - action.button_class = button_class - action.label = "chat.#{prefix}.title" - action.description = "chat.#{prefix}.description" - action.client_action = client_action - action.confirm_message = "#{prefix}.confirm" if confirm - end - end end end diff --git a/plugins/chat/assets/javascripts/discourse/components/reviewable-refresh/chat-message.gjs b/plugins/chat/assets/javascripts/discourse/components/reviewable-refresh/chat-message.gjs new file mode 100644 index 00000000000..0a12165f22e --- /dev/null +++ b/plugins/chat/assets/javascripts/discourse/components/reviewable-refresh/chat-message.gjs @@ -0,0 +1,7 @@ +import ReviewableChatMessage from "../reviewable-chat-message"; + + diff --git a/plugins/chat/config/locales/server.en.yml b/plugins/chat/config/locales/server.en.yml index b479876579f..1e1b910cb0a 100644 --- a/plugins/chat/config/locales/server.en.yml +++ b/plugins/chat/config/locales/server.en.yml @@ -101,6 +101,17 @@ en: reviewables: message_already_handled: "Thanks, but we've already reviewed this message and determined it does not need to be flagged again." actions: + message_actions: + bundle_title: "What do you want to do with the message?" + no_action_message: + title: "No action" + description: "Leave the message as is." + delete_message: + title: "Delete message" + description: "Permanently delete the message." + restore_message: + title: "Restore message" + description: "Restore the message so that users can see it." agree: title: "Agree..." agree_and_keep_message: diff --git a/spec/models/concerns/reviewable_action_builder_spec.rb b/spec/models/concerns/reviewable_action_builder_spec.rb index 91c1a2009ee..2029b2e36a6 100644 --- a/spec/models/concerns/reviewable_action_builder_spec.rb +++ b/spec/models/concerns/reviewable_action_builder_spec.rb @@ -12,8 +12,14 @@ RSpec.describe ReviewableActionBuilder do end fab!(:post_actions) { Reviewable::Actions.new(reviewable_post, guardian) } + before do + reviewable_post.instance_variable_set(:@actions, post_actions) + reviewable_post.instance_variable_set(:@guardian, guardian) + reviewable_post.instance_variable_set(:@action_args, {}) + end + it "creates a user bundle with standard actions when allowed" do - bundle = reviewable_post.build_user_actions_bundle(post_actions, guardian) + bundle = reviewable_post.build_user_actions_bundle # bundle id and label expect(bundle.id).to eq("#{reviewable_post.id}-user-actions") @@ -37,7 +43,7 @@ RSpec.describe ReviewableActionBuilder do it "includes only the no-op action when user is nil" do allow(reviewable_post).to receive(:target_created_by).and_return(nil) - bundle = reviewable_post.build_user_actions_bundle(post_actions, guardian) + bundle = reviewable_post.build_user_actions_bundle server_actions = bundle.actions.map(&:server_action) expect(server_actions).to include("no_action_user")