FEATURE: the ability to change the order of flags (#27269)

Continued work on moderate flags UI.
In this PR admins are allowed to change the order of flags. The notify user flag is always on top but all other flags can be moved.

app/assets/javascripts/admin/addon/components/admin-flag-item.gjs

@@ -4,13 +4,22 @@ import { fn } from "@ember/helper";
 import { on } from "@ember/modifier";
 import { action } from "@ember/object";
 import { htmlSafe } from "@ember/template";
+import DButton from "discourse/components/d-button";
 import DToggleSwitch from "discourse/components/d-toggle-switch";
+import DropdownMenu from "discourse/components/dropdown-menu";
 import { ajax } from "discourse/lib/ajax";
 import { popupAjaxError } from "discourse/lib/ajax-error";
+import { SYSTEM_FLAG_IDS } from "discourse/lib/constants";
+import i18n from "discourse-common/helpers/i18n";
+import DMenu from "float-kit/components/d-menu";
 
 export default class AdminFlagItem extends Component {
   @tracked enabled = this.args.flag.enabled;
 
+  get canMove() {
+    return this.args.flag.id !== SYSTEM_FLAG_IDS.notify_user;
+  }
+
   @action
   toggleFlagEnabled(flag) {
     this.enabled = !this.enabled;
@@ -23,8 +32,25 @@ export default class AdminFlagItem extends Component {
     });
   }
 
+  @action
+  onRegisterApi(api) {
+    this.dMenu = api;
+  }
+
+  @action
+  moveUp() {
+    this.args.moveFlagCallback(this.args.flag, "up");
+    this.dMenu.close();
+  }
+
+  @action
+  moveDown() {
+    this.args.moveFlagCallback(this.args.flag, "down");
+    this.dMenu.close();
+  }
+
   <template>
-    <tr class="admin-flag-item">
+    <tr class="admin-flag-item {{@flag.name_key}}">
       <td>
         <p class="admin-flag-item__name">{{@flag.name}}</p>
         <p class="admin-flag-item__description">{{htmlSafe
@@ -32,11 +58,46 @@ export default class AdminFlagItem extends Component {
           }}</p>
       </td>
       <td>
-        <DToggleSwitch
-          @state={{this.enabled}}
-          class="admin-flag-item__toggle {{@flag.name_key}}"
-          {{on "click" (fn this.toggleFlagEnabled @flag)}}
-        />
+        <div class="admin-flag-item__options">
+          <DToggleSwitch
+            @state={{this.enabled}}
+            class="admin-flag-item__toggle {{@flag.name_key}}"
+            {{on "click" (fn this.toggleFlagEnabled @flag)}}
+          />
+          {{#if this.canMove}}
+            <DMenu
+              @identifier="flag-menu"
+              @title={{i18n "admin.flags.more_options.title"}}
+              @icon="ellipsis-v"
+              @onRegisterApi={{this.onRegisterApi}}
+            >
+              <:content>
+                <DropdownMenu as |dropdown|>
+                  {{#unless @isFirstFlag}}
+                    <dropdown.item>
+                      <DButton
+                        @label="admin.flags.more_options.move_up"
+                        @icon="arrow-up"
+                        @class="btn-transparent move-up"
+                        @action={{this.moveUp}}
+                      />
+                    </dropdown.item>
+                  {{/unless}}
+                  {{#unless @isLastFlag}}
+                    <dropdown.item>
+                      <DButton
+                        @label="admin.flags.more_options.move_down"
+                        @icon="arrow-down"
+                        @class="btn-transparent move-down"
+                        @action={{this.moveDown}}
+                      />
+                    </dropdown.item>
+                  {{/unless}}
+                </DropdownMenu>
+              </:content>
+            </DMenu>
+          {{/if}}
+        </div>
       </td>
     </tr>
   </template>

app/assets/javascripts/admin/addon/components/admin-flags.gjs

@@ -1,11 +1,50 @@
 import Component from "@glimmer/component";
+import { tracked } from "@glimmer/tracking";
+import { action } from "@ember/object";
 import { inject as service } from "@ember/service";
+import { ajax } from "discourse/lib/ajax";
+import { popupAjaxError } from "discourse/lib/ajax-error";
 import i18n from "discourse-common/helpers/i18n";
+import { bind } from "discourse-common/utils/decorators";
 import AdminFlagItem from "admin/components/admin-flag-item";
 
 export default class AdminFlags extends Component {
   @service site;
-  flags = this.site.flagTypes;
+  @tracked flags = this.site.flagTypes;
+
+  @bind
+  isFirstFlag(flag) {
+    return this.flags.indexOf(flag) === 1;
+  }
+
+  @bind
+  isLastFlag(flag) {
+    return this.flags.indexOf(flag) === this.flags.length - 1;
+  }
+
+  @action
+  moveFlagCallback(flag, direction) {
+    const fallbackFlags = [...this.flags];
+
+    const flags = this.flags;
+
+    const flagIndex = flags.indexOf(flag);
+    const targetFlagIndex = direction === "up" ? flagIndex - 1 : flagIndex + 1;
+
+    const targetFlag = flags[targetFlagIndex];
+
+    flags[flagIndex] = targetFlag;
+    flags[targetFlagIndex] = flag;
+
+    this.flags = flags;
+
+    return ajax(`/admin/config/flags/${flag.id}/reorder/${direction}`, {
+      type: "PUT",
+    }).catch((error) => {
+      this.flags = fallbackFlags;
+      return popupAjaxError(error);
+    });
+  }
 
   <template>
     <div class="container admin-flags">
@@ -17,7 +56,12 @@ export default class AdminFlags extends Component {
         </thead>
         <tbody>
           {{#each this.flags as |flag|}}
-            <AdminFlagItem @flag={{flag}} />
+            <AdminFlagItem
+              @flag={{flag}}
+              @moveFlagCallback={{this.moveFlagCallback}}
+              @isFirstFlag={{this.isFirstFlag flag}}
+              @isLastFlag={{this.isLastFlag flag}}
+            />
           {{/each}}
         </tbody>
       </table>

app/assets/javascripts/discourse/app/lib/constants.js

@@ -80,3 +80,13 @@ export const TOPIC_VISIBILITY_REASONS = {
   bulk_action: 5,
   unknown: 99,
 };
+
+export const SYSTEM_FLAG_IDS = {
+  like: 2,
+  notify_user: 6,
+  notify_moderators: 7,
+  off_topic: 3,
+  inappropriate: 4,
+  spam: 8,
+  illegal: 10,
+};

app/assets/stylesheets/common/admin/flags.scss

@@ -8,4 +8,14 @@
   &__description {
     margin-top: 0.5em;
   }
+  &__options {
+    display: flex;
+    align-items: center;
+  }
+  .d-toggle-switch--label {
+    margin-bottom: 0;
+  }
+  .flag-menu-trigger {
+    padding: 0.25em 0.325em;
+  }
 }

app/assets/stylesheets/mobile/_index.scss

@@ -1,5 +1,6 @@
 @import "admin_badges";
 @import "admin_customize";
+@import "admin_flags";
 @import "admin_report_counters";
 @import "admin_report_table";
 @import "admin_report";

app/assets/stylesheets/mobile/admin_flags.scss

@@ -0,0 +1,3 @@
+.admin-contents table.grid tr.admin-flag-item {
+  grid-template-columns: auto min-content;
+}

app/controllers/admin/config/flags_controller.rb

@@ -18,4 +18,20 @@ class Admin::Config::FlagsController < Admin::AdminController
 
   def index
   end
+
+  def reorder
+    with_service(ReorderFlag) do
+      on_success do
+        Discourse.request_refresh!
+        render(json: success_json)
+      end
+      on_failure { render(json: failed_json, status: 422) }
+      on_model_not_found(:message) { raise Discourse::NotFound }
+      on_failed_policy(:invalid_access) { raise Discourse::InvalidAccess }
+      on_failed_policy(:invalid_move) { render_json_error(I18n.t("flags.errors.wrong_move")) }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
+    end
+  end
 end

app/models/flag.rb

@@ -10,7 +10,7 @@ class Flag < ActiveRecord::Base
   after_save :reset_flag_settings!
   after_destroy :reset_flag_settings!
 
-  default_scope { order(:position) }
+  default_scope { order(:position).where(score_type: false) }
 
   def used?
     PostAction.exists?(post_action_type_id: self.id) ||

app/models/post_action_type.rb

@@ -45,7 +45,7 @@ class PostActionType < ActiveRecord::Base
     end
 
     def all_flags
-      @all_flags ||= Flag.all
+      @all_flags ||= Flag.unscoped.order(:position).all
     end
 
     def auto_action_flag_types

app/services/reorder_flag.rb

@@ -0,0 +1,60 @@
+# frozen_string_literal: true
+
+VALID_DIRECTIONS = %w[up down]
+
+class ReorderFlag
+  include Service::Base
+
+  contract
+  model :flag
+  policy :invalid_access
+  policy :invalid_move
+
+  transaction do
+    step :move
+    step :log
+  end
+
+  class Contract
+    attribute :flag_id, :integer
+    attribute :direction, :string
+    validates :flag_id, presence: true
+    validates :direction, inclusion: { in: VALID_DIRECTIONS }
+  end
+
+  private
+
+  def fetch_flag(flag_id:)
+    Flag.find(flag_id)
+  end
+
+  def invalid_access(guardian:, flag:)
+    guardian.can_reorder_flag?(flag)
+  end
+
+  def all_flags
+    @all_flags ||= Flag.where.not(name_key: "notify_user").order(:position)
+  end
+
+  def invalid_move(flag:, direction:)
+    return false if all_flags.first == flag && direction == "up"
+    return false if all_flags.last == flag && direction == "down"
+    true
+  end
+
+  def move(flag:, direction:)
+    old_position = flag.position
+    index = all_flags.index(flag)
+    target_flag = all_flags[direction == "up" ? index - 1 : index + 1]
+
+    flag.update!(position: target_flag.position)
+    target_flag.update!(position: old_position)
+  end
+
+  def log(guardian:, flag:, direction:)
+    StaffActionLogger.new(guardian.user).log_custom(
+      "move_flag",
+      { flag: flag.name, direction: direction },
+    )
+  end
+end

config/locales/client.en.yml

@@ -5033,6 +5033,10 @@ en:
         title: "Moderation Flags"
         description: "Description"
         enabled: "Enabled?"
+        more_options:
+          title: "More options"
+          move_up: "Move up"
+          move_down: "Move down"
 
       groups:
         new:
@@ -6093,6 +6097,7 @@ en:
             update_watched_word_group: "update watched word group"
             delete_watched_word_group: "delete watched word group"
             toggle_flag: "toggle flag"
+            move_flag: "move flag"
         screened_emails:
           title: "Screened Emails"
           description: "When someone tries to create a new account, the following email addresses will be checked and the registration will be blocked, or some other action performed."

config/locales/server.en.yml

@@ -1250,6 +1250,7 @@ en:
   flags:
     errors:
       already_handled: "Flag was already handled"
+      wrong_move: "Flag cannot be moved"
   reports:
     default:
       labels:

config/routes.rb

@@ -388,6 +388,7 @@ Discourse::Application.routes.draw do
       namespace :config, constraints: StaffConstraint.new do
         resources :flags, only: %i[index] do
           put "toggle"
+          put "reorder/:direction" => "flags#reorder"
         end
 
         resources :about, constraints: AdminConstraint.new, only: %i[index] do

db/fixtures/003_flags.rb

@@ -54,7 +54,7 @@ Flag.seed do |s|
   s.custom_type = true
   s.applies_to = %w[Post Topic Chat::Message]
 end
-Flag.seed do |s|
+Flag.unscoped.seed do |s|
   s.id = 9
   s.name = "needs_approval"
   s.position = 6

lib/guardian/flag_guardian.rb

@@ -8,4 +8,8 @@ module FlagGuardian
   def can_toggle_flag?
     @user.admin?
   end
+
+  def can_reorder_flag?(flag)
+    @user.admin? && flag.name_key != "notify_user"
+  end
 end

lib/tasks/javascript.rake

@@ -164,6 +164,8 @@ task "javascript:update_constants" => :environment do
     export const MAX_NOTIFICATIONS_LIMIT_PARAMS = #{NotificationsController::INDEX_LIMIT};
 
     export const TOPIC_VISIBILITY_REASONS = #{Topic.visibility_reasons.to_json};
+
+    export const SYSTEM_FLAG_IDS = #{PostActionType.types.to_json}
   JS
 
   pretty_notifications = Notification.types.map { |n| "  #{n[0]}: #{n[1]}," }.join("\n")

spec/lib/guardian/flag_guardian_spec.rb

@@ -41,4 +41,14 @@ RSpec.describe FlagGuardian do
       expect(Guardian.new(user).can_toggle_flag?).to eq(false)
     end
   end
+
+  describe "#can_reorder_flag?" do
+    it "returns true for admin and false for regular user and notify_user" do
+      expect(Guardian.new(admin).can_reorder_flag?(Flag.system.last)).to eq(true)
+      expect(
+        Guardian.new(admin).can_reorder_flag?(Flag.system.find_by(name_key: "notify_user")),
+      ).to eq(false)
+      expect(Guardian.new(user).can_reorder_flag?(Flag.system.last)).to eq(false)
+    end
+  end
 end

spec/services/reorder_flag_spec.rb

@@ -0,0 +1,56 @@
+# frozen_string_literal: true
+
+RSpec.describe(ReorderFlag) do
+  subject(:result) do
+    described_class.call(flag_id: flag.id, guardian: current_user.guardian, direction: direction)
+  end
+
+  let(:flag) { Flag.order(:position).last }
+  let(:direction) { "up" }
+
+  context "when user is not allowed to perform the action" do
+    fab!(:current_user) { Fabricate(:user) }
+
+    it { is_expected.to fail_a_policy(:invalid_access) }
+  end
+
+  context "when direction is invalid" do
+    fab!(:current_user) { Fabricate(:admin) }
+    let(:direction) { "side" }
+
+    it { is_expected.to fail_a_contract }
+  end
+
+  context "when move is invalid" do
+    fab!(:current_user) { Fabricate(:admin) }
+    let(:direction) { "down" }
+
+    it { is_expected.to fail_a_policy(:invalid_move) }
+  end
+
+  context "when user is allowed to perform the action" do
+    fab!(:current_user) { Fabricate(:admin) }
+
+    it "sets the service result as successful" do
+      expect(result).to be_a_success
+    end
+
+    it "moves the flag" do
+      expect(Flag.order(:position).map(&:name)).to eq(
+        %w[notify_user notify_moderators off_topic inappropriate spam illegal],
+      )
+      result
+      expect(Flag.order(:position).map(&:name)).to eq(
+        %w[notify_user notify_moderators off_topic inappropriate illegal spam],
+      )
+    end
+
+    it "logs the action" do
+      expect { result }.to change { UserHistory.count }.by(1)
+      expect(UserHistory.last).to have_attributes(
+        custom_type: "move_flag",
+        details: "flag: #{result[:flag].name}\ndirection: up",
+      )
+    end
+  end
+end

spec/system/admin_flags_spec.rb

@@ -26,4 +26,47 @@ describe "Admin Flags Page", type: :system do
       ["Something Else", "It's Inappropriate", "It's Illegal"],
     )
   end
+
+  it "allows admin to change order of flags" do
+    topic_page.visit_topic(post.topic)
+    topic_page.open_flag_topic_modal
+    expect(all(".flag-action-type-details strong").map(&:text)).to eq(
+      ["Something Else", "It's Inappropriate", "It's Spam", "It's Illegal"],
+    )
+
+    visit "/admin/config/flags"
+    admin_flags_page.move_down("spam")
+
+    topic_page.visit_topic(post.topic)
+    topic_page.open_flag_topic_modal
+    expect(all(".flag-action-type-details strong").map(&:text)).to eq(
+      ["Something Else", "It's Inappropriate", "It's Illegal", "It's Spam"],
+    )
+
+    visit "/admin/config/flags"
+    admin_flags_page.move_up("spam")
+
+    topic_page.visit_topic(post.topic)
+    topic_page.open_flag_topic_modal
+    expect(all(".flag-action-type-details strong").map(&:text)).to eq(
+      ["Something Else", "It's Inappropriate", "It's Spam", "It's Illegal"],
+    )
+  end
+
+  it "does not allow to move notify user flag" do
+    visit "/admin/config/flags"
+    expect(page).not_to have_css(".notify_user .flag-menu-trigger")
+  end
+
+  it "does not allow bottom flag to move down" do
+    visit "/admin/config/flags"
+    admin_flags_page.open_flag_menu("illegal")
+    expect(page).not_to have_css(".dropdown-menu__item .move-down")
+  end
+
+  it "does not allow top flag to move up" do
+    visit "/admin/config/flags"
+    admin_flags_page.open_flag_menu("notify_moderators")
+    expect(page).not_to have_css(".dropdown-menu__item .move-up")
+  end
 end

spec/system/page_objects/pages/admin_flags.rb

@@ -6,6 +6,20 @@ module PageObjects
       def toggle(key)
         PageObjects::Components::DToggleSwitch.new(".admin-flag-item__toggle.#{key}").toggle
       end
+
+      def open_flag_menu(key)
+        find(".#{key} .flag-menu-trigger").click
+      end
+
+      def move_down(key)
+        open_flag_menu(key)
+        find(".dropdown-menu__item .move-down").click
+      end
+
+      def move_up(key)
+        open_flag_menu(key)
+        find(".dropdown-menu__item .move-up").click
+      end
     end
   end
 end