FIX: Don't mix up action labels between different reviewables (#23365)

Currently, if the review queue has both a flagged post and a flagged chat message, one of the two will have some of the labels of their actions replaced by those of the other. In other words, the labels are getting mixed up. For example, a flagged chat message might show up with an action labelled "Delete post".

This is happening because when using bundles, we are sending along the actions in a separate part of the response, so they can be shared by many reviewables. The bundles then index into this bag of actions by their ID, which is something generic describing the server action, e.g. "agree_and_delete".

The problem here is the same action can have different labels depending on the type of reviewable. Now that the bag of actions contains multiple actions with the same ID, which one is chosen is arbitrary. I.e. it doesn't distinguish based on the type of the reviewable.

This change adds an additional field to the actions, server_action, which now contains what used to be the ID. Meanwhile, the ID has been turned into a concatenation of the reviewable type and the server action, e.g. post-agree_and_delete.

This still provides the upside of denormalizing the actions while allowing for different reviewable types to have different labels and descriptions.

At first I thought I would prepend the reviewable type to the ID, but this doesn't work well because the ID is used on the server-side to determine which actions are possible, and these need to be shared between different reviewables. Hence the introduction of server_action, which now serves that purpose.

I also thought about changing the way that the bundle indexes into the bag of actions, but this is happening through some EmberJS mechanism, so we don't own that code.
This commit is contained in:
Ted Johansson 2023-09-06 10:57:30 +08:00 committed by GitHub
parent 2829898951
commit e74560f062
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 23 additions and 6 deletions

View File

@ -154,7 +154,7 @@ export default Component.extend({
});
return ajax(
`/review/${reviewable.id}/perform/${performableAction.id}?version=${version}`,
`/review/${reviewable.id}/perform/${performableAction.server_action}?version=${version}`,
{
type: "PUT",
data,
@ -286,7 +286,8 @@ export default Component.extend({
const requireRejectReason = performableAction.get(
"require_reject_reason"
);
const actionModalClass = actionModalClassMap[performableAction.id];
const actionModalClass =
actionModalClassMap[performableAction.server_action];
if (message) {
this.dialog.confirm({

View File

@ -7,6 +7,7 @@ class ReviewableActionSerializer < ApplicationSerializer
:label,
:confirm_message,
:description,
:server_action,
:client_action,
:require_reject_reason
@ -22,6 +23,10 @@ class ReviewableActionSerializer < ApplicationSerializer
I18n.t(object.description, default: nil)
end
def server_action
object.server_action
end
def include_description?
description.present?
end

View File

@ -4,7 +4,7 @@ require "reviewable/collection"
class Reviewable < ActiveRecord::Base
class Actions < Reviewable::Collection
attr_reader :bundles
attr_reader :bundles, :reviewable
def initialize(reviewable, guardian, args = nil)
super(reviewable, guardian, args)
@ -46,6 +46,10 @@ class Reviewable < ActiveRecord::Base
super(id)
@icon, @button_class, @label = icon, button_class, label
end
def server_action
id.split("-").last
end
end
def add_bundle(id, icon: nil, label: nil)
@ -55,6 +59,7 @@ class Reviewable < ActiveRecord::Base
end
def add(id, bundle: nil)
id = [reviewable.target_type&.underscore, id].compact_blank.join("-")
action = Actions.common_actions[id] || Action.new(id)
yield action if block_given?
@content << action

View File

@ -18,8 +18,8 @@ class Reviewable < ActiveRecord::Base
@content = []
end
def has?(id)
@content.any? { |a| a.id.to_s == id.to_s }
def has?(action_id)
@content.any? { |a| a.server_action.to_s == action_id.to_s }
end
def blank?

View File

@ -9,6 +9,10 @@ class Reviewable < ActiveRecord::Base
super(id)
@type = type
end
def server_action
id
end
end
def add(id, type)

View File

@ -49,7 +49,9 @@ RSpec.describe ReviewableUser, type: :model do
def assert_require_reject_reason(id, expected)
actions = reviewable.actions_for(Guardian.new(moderator))
expect(actions.to_a.find { |a| a.id == id }.require_reject_reason).to eq(expected)
expect(actions.to_a.find { |a| a.server_action.to_sym == id }.require_reject_reason).to eq(
expected,
)
end
end