Add trusted role to new tag moderators (#11242)

* Don't add to mailchimp for N/A circumstance

* Use class << self over self. methods

* Refactor and make add/remove tag mod idempotent

* Add missing param wiki_body_markdown

* Add errors_as_sentence b/c Tag not inheriting from AppRecord

* Refactor and Crayonsify tag page

* Use proper for attr to make label checkable

* Crayonsify moderator section

* Make placeholder white

* Remove remote: false for sync form submit

* Fix more merge conflicts

* Minor styling adjustments

* Fix color field value

* Don't add to Mailchimp community mod list if not enabled

* Add tests for new tag mod routes

* Add missing line oops

* Use self.method over class << self for new Rubyists

* Use more efficient query for getting tag mods

* Use parentheses to follow convention

* Use cleaner way of routing and controller

* Update tests to match new configuration

app/controllers/admin/tags/moderators_controller.rb

@@ -0,0 +1,44 @@
+module Admin
+  module Tags
+    class ModeratorsController < Admin::ApplicationController
+      def authorization_resource
+        Tag
+      end
+
+      after_action only: %i[create destroy] do
+        Audit::Logger.log(:moderator, current_user, params.dup)
+      end
+
+      def create
+        user = User.find_by(id: tag_params[:user_id])
+        if user&.update(email_tag_mod_newsletter: true)
+          AssignTagModerator.add_tag_moderators([user.id], [params[:tag_id]])
+          flash[:success] = "#{user.username} was added as a tag moderator!"
+        else
+          flash[:error] = "Error: User ID ##{tag_params[:user_id]} was not found,
+          or their account has errors: #{user&.errors_as_sentence}"
+        end
+        redirect_to admin_tag_path(params[:tag_id])
+      end
+
+      def destroy
+        user = User.find_by(id: tag_params[:user_id])
+        tag = Tag.find_by(id: params[:tag_id])
+        if user&.update(email_tag_mod_newsletter: false)
+          AssignTagModerator.remove_tag_moderator(user, tag)
+          flash[:success] = "@#{user.username} - ID ##{user.id} was removed as a tag moderator."
+        else
+          flash[:error] = "Error: User ID ##{tag_params[:user_id]} was not found,
+          or their account has errors: #{user&.errors_as_sentence}"
+        end
+        redirect_to admin_tag_path(tag.id)
+      end
+
+      private
+
+      def tag_params
+        params.require(:tag).permit(:user_id)
+      end
+    end
+  end
+end

app/controllers/admin/tags_controller.rb

@@ -20,37 +20,27 @@ module Admin
 
     def show
       @tag = Tag.find(params[:id])
+      @tag_moderators = User.with_role(:tag_moderator, @tag).select(:id, :username)
+      @badges_for_options = Badge.pluck(:title, :id)
     end
 
     def update
       @tag = Tag.find(params[:id])
-      @add_user_id = params[:tag][:tag_moderator_id]
-      @remove_user_id = params[:tag][:remove_moderator_id]
-      add_moderator if @add_user_id
-      remove_moderator if @remove_user_id
-      @tag.update!(tag_params)
-
-      redirect_to "/admin/tags/#{params[:id]}"
+      if @tag.update(tag_params)
+        flash[:success] = "#{@tag.name} tag successfully updated!"
+      else
+        flash[:error] = "The tag update failed: #{@tag.errors_as_sentence}"
+      end
+      redirect_to admin_tag_path(@tag.id)
     end
 
     private
 
-    def remove_moderator
-      user = User.find(@remove_user_id)
-      user.update(email_tag_mod_newsletter: false)
-      AssignTagModerator.remove_tag_moderator(user, @tag)
-    end
-
-    def add_moderator
-      User.find(@add_user_id).update(email_tag_mod_newsletter: true)
-      AssignTagModerator.add_tag_moderators([@add_user_id], [@tag.id])
-    end
-
     def tag_params
       allowed_params = %i[
-        supported rules_markdown short_summary pretty_name bg_color_hex
-        text_color_hex tag_moderator_id remove_moderator_id alias_for badge_id
-        category social_preview_template
+        id supported rules_markdown short_summary pretty_name bg_color_hex
+        text_color_hex user_id alias_for badge_id
+        category social_preview_template wiki_body_markdown
       ]
       params.require(:tag).permit(allowed_params)
     end

app/labor/assign_tag_moderator.rb

@@ -3,9 +3,9 @@ module AssignTagModerator
     return if user.has_role?(:trusted)
     return if user.has_role?(:banned)
 
-    user.add_role :trusted
+    user.add_role(:trusted)
     user.update(email_community_mod_newsletter: true)
-    MailchimpBot.new(user).manage_community_moderator_list
+    MailchimpBot.new(user).manage_community_moderator_list if community_mod_newsletter_enabled?
     Rails.cache.delete("user-#{user.id}/has_trusted_role")
     NotifyMailer.with(user: user).trusted_role_email.deliver_now
   end
@@ -47,17 +47,25 @@ module AssignTagModerator
     user.update(email_tag_mod_newsletter: true) if user.email_tag_mod_newsletter == false
     user.add_role(:tag_moderator, tag)
     Rails.cache.delete("user-#{user.id}/tag_moderators_list")
-    MailchimpBot.new(user).manage_tag_moderator_list
+    MailchimpBot.new(user).manage_tag_moderator_list if tag_mod_newsletter_enabled?
   end
 
   def self.remove_tag_moderator(user, tag)
     user.remove_role(:tag_moderator, tag)
     user.update(email_tag_mod_newsletter: false) if user.email_tag_mod_newsletter == true
     Rails.cache.delete("user-#{user.id}/tag_moderators_list")
-    MailchimpBot.new(user).manage_tag_moderator_list
+    MailchimpBot.new(user).manage_tag_moderator_list if tag_mod_newsletter_enabled?
   end
 
   def self.chat_channel_slug(tag)
     tag.mod_chat_channel&.slug
   end
+
+  def self.community_mod_newsletter_enabled?
+    SiteConfig.mailchimp_api_key.present? && SiteConfig.mailchimp_community_moderators_id.present?
+  end
+
+  def self.tag_mod_newsletter_enabled?
+    SiteConfig.mailchimp_api_key.present? && SiteConfig.mailchimp_tag_moderators_id.present?
+  end
 end

app/models/tag.rb

@@ -89,6 +89,10 @@ class Tag < ActsAsTaggableOn::Tag
     errors.add(:name, "contains non-ASCII characters") unless name.match?(/\A[[a-z0-9]]+\z/i)
   end
 
+  def errors_as_sentence
+    errors.full_messages.to_sentence
+  end
+
   private
 
   def evaluate_markdown

app/views/admin/tags/_form.html.erb

@@ -0,0 +1,57 @@
+<div class="crayons-card crayons-card--content-rows">
+  <h4 class="mb-4">Edit details</h4>
+  <%= form_with model: [:admin, tag], data: { remote: false } do |f| %>
+  <div class="crayons-field crayons-field--checkbox">
+    <%= f.check_box :supported, class: "crayons-checkbox" %>
+    <label class="crayons-field__label" for="tag_supported">
+      Supported
+      <p class="crayons-field__description">Allows the tag to be a searchable result when writing a post or a listing</p>
+    </label>
+  </div>
+  <div class="crayons-field">
+    <%= f.label :badge_id, class: "crayons-field__label" %>
+    <%= f.select(:badge_id, options_for_select(badges_for_options, tag.badge_id), { include_blank: true }, { class: "crayons-select" }) %>
+  </div>
+  <div class="crayons-field">
+    <%= f.label :category, class: "crayons-field__label" %>
+    <%= f.select(:category, options_for_select(Tag.valid_categories, tag.category), {}, { class: "crayons-select" }) %>
+  </div>
+  <div class="crayons-field">
+    <%= f.label :social_preview_template, class: "crayons-field__label" %>
+    <%= f.select(:social_preview_template, Tag.social_preview_templates, {}, { class: "crayons-select" }) %>
+  </div>
+  <div class="crayons-field">
+    <%= f.label :alias_for, class: "crayons-field__label" %>
+    <%= f.text_field :alias_for, value: tag.alias_for, class: "crayons-textfield" %>
+  </div>
+  <div class="crayons-field">
+    <%= f.label :pretty_name, class: "crayons-field__label" %>
+    <%= f.text_field :pretty_name, value: tag.pretty_name, class: "crayons-textfield" %>
+  </div>
+  <div class="crayons-field">
+    <%= f.label :short_summary, class: "crayons-field__label" %>
+    <%= f.text_field :short_summary, value: tag.short_summary, class: "crayons-textfield" %>
+  </div>
+  <div class="crayons-field">
+    <%= f.label :rules_markdown, class: "crayons-field__label" %>
+    <br>
+    <%= f.text_area :rules_markdown, value: tag.rules_markdown, class: "crayons-textfield" %>
+  </div>
+  <div class="crayons-field">
+    <%= f.label :wiki_body_markdown, class: "crayons-field__label" %>
+    <br>
+    <%= f.text_area :wiki_body_markdown, value: tag.wiki_body_markdown, class: "crayons-textfield" %>
+  </div>
+  <%= f.label :bg_color_hex, class: "crayons-field__label" %>
+  <div class="flex items-center w-100 m:w-50">
+    <%= f.text_field :bg_color_hex, class: "crayons-textfield", value: tag.bg_color_hex.presence || "#000000", placeholder: "#000000" %>
+    <%= f.color_field :bg_color_hex, class: "crayons-color-selector ml-2", required: true, value: tag.bg_color_hex.presence || "#000000", placeholder: "#000000" %>
+  </div>
+  <%= f.label :text_color_hex, class: "crayons-field__label" %>
+  <div class="flex items-center w-100 m:w-50">
+    <%= f.text_field :text_color_hex, class: "crayons-textfield", value: tag.text_color_hex.presence || "#ffffff", placeholder: "#ffffff" %>
+    <%= f.color_field :text_color_hex, class: "crayons-color-selector ml-2", required: true, value: tag.text_color_hex.presence || "#ffffff", placeholder: "#ffffff" %>
+  </div>
+  <%= f.submit class: "crayons-btn mt-2" %>
+  <% end %>
+</div>

app/views/admin/tags/show.html.erb

@@ -11,84 +11,35 @@
 
 <div class="grid gap-6">
   <div class="crayons-card p-6">
-    <h4 class="mb-4">Moderators</h4>
-    <% if @tag.tag_moderator_ids.any? %>
-      <ul class="list-group-flush">
-        <% @tag.tag_moderator_ids.each do |id| %>
-            <li class="list-group-item d-flex justify-content-between">
-              <a href="/<%= User.find(id).username %>">@<%= User.find(id).username %></a>
-              <%= form_for [:admin, @tag] do |f| %>
-                <%= f.hidden_field :remove_moderator_id, value: id %>
-                <%= f.submit "Remove", class: "btn btn-danger btn-sm" %>
-              <% end %>
-            </li>
-        <% end %>
-      </ul>
-    <% else %>
-      <p>This tag currently has no moderators.</p>
-    <% end %>
-    <%= form_for [:admin, @tag], html: { class: "form-inline justify-content-between" } do |f| %>
-      <div class="form-group">
-        <%= f.label :tag_moderator_id, "Add Moderator (id):", class: "mr-3" %>
-        <%= f.text_field :tag_moderator_id, class: "form-control" %>
-      </div>
-      <div>
-        <%= f.submit "Add Moderator", class: "btn btn-primary" %>
-      </div>
-    <% end %>
-  </div>
-
-  <div class="crayons-card p-6">
-    <h4 class="mb-4">Edit details</h4>
-    <%= form_for [:admin, @tag] do |f| %>
-      <div class="form-group">
-        <%= f.label :supported %>
-        <%= f.check_box :supported %>
-      </div>
-      <div class="form-group">
-        <%= f.label :badge_id %>
-        <% badges_array = Badge.pluck(:title, :id) %>
-        <%= f.select(:badge_id, options_for_select([["None", nil]] + badges_array, @tag.badge_id)) %>
-      </div>
-      <div class="form-group">
-        <%= f.label :category %>
-        <%= f.select(:category, options_for_select(Tag.valid_categories, @tag.category)) %>
-      </div>
-      <div class="form-group">
-        <%= f.label :social_preview_template %>
-        <%= f.select(:social_preview_template, Tag.social_preview_templates) %>
-      </div>
-      <div class="form-group">
-        <%= f.label :alias_for %>
-        <%= f.text_field :alias_for, class: "form-control" %>
-      </div>
-      <div class="form-group">
-        <%= f.label :pretty_name %>
-        <%= f.text_field :pretty_name, class: "form-control" %>
-      </div>
-      <div class="form-group">
-        <%= f.label :short_summary %>
-        <%= f.text_field :short_summary, class: "form-control" %>
-      </div>
-      <div class="form-group">
-        <%= f.label :rules_markdown %>
-        <br>
-        <%= f.text_area :rules_markdown, class: "form-control" %>
-      </div>
-      <div class="form-group">
-        <%= f.label :wiki_body_markdown %>
-        <br>
-        <%= f.text_area :wiki_body_markdown, class: "form-control" %>
-      </div>
-      <div class="form-group">
-        <%= f.label :bg_color_hex %>
-        <%= f.color_field :bg_color_hex, class: "form-control", required: true %>
-      </div>
-      <div class="form-group">
-        <%= f.label :text_color_hex %>
-        <%= f.color_field :text_color_hex, class: "form-control", required: true, value: @tag.text_color_hex.presence || "#ffffff" %>
-      </div>
-      <%= f.submit class: "btn btn-primary float-right" %>
-    <% end %>
+    <div class="crayons-card__header">
+      <h4 class="crayons-card__headline">Moderators</h4>
+    </div>
+    <div class="crayons-card__body flex flex-col gap-2">
+      <% if @tag_moderators.exists? %>
+        <ul class="list-none">
+          <% @tag_moderators.each do |user| %>
+              <li class="flex justify-between">
+                <a href="/<%= URL.user(user) %>">@<%= user.username %></a>
+                <%= form_with url: admin_tag_moderator_path(@tag.id), model: [:admin, @tag], method: :delete, data: { remote: false } do |f| %>
+                  <%= f.hidden_field :user_id, value: user.id %>
+                  <%= f.submit "Remove", class: "crayons-btn crayons-btn--danger crayons-btn--s" %>
+                <% end %>
+              </li>
+          <% end %>
+        </ul>
+      <% else %>
+        <div class="crayons-notice mb-5">
+          This tag currently has no moderators.
+        </div>
+      <% end %>
+      <%= form_with url: admin_tag_moderator_path(@tag.id), model: [:admin, @tag], method: :post, data: { remote: false } do |f| %>
+        <div class="crayons-field w-50">
+          <%= f.label :user_id, "Add Moderator (ID):", class: "crayons-field__label" %>
+          <%= f.text_field :user_id, class: "crayons-textfield" %>
+          <%= f.submit "Add Moderator", class: "crayons-btn" %>
+        </div>
+      <% end %>
+    </div>
   </div>
+  <%= render "form", tag: @tag, badges_for_options: @badges_for_options %>
 </div>

config/routes.rb

@@ -103,7 +103,9 @@ Rails.application.routes.draw do
           post "save_status"
         end
       end
-      resources :tags, only: %i[index update show]
+      resources :tags, only: %i[index update show] do
+        resource :moderator, only: %i[create destroy], module: "tags"
+      end
       resources :users, only: %i[index show edit update] do
         member do
           post "banish"

spec/requests/admin/tags/moderators_spec.rb

@@ -0,0 +1,35 @@
+require "rails_helper"
+
+RSpec.describe "/admin/tags/:id/moderator", type: :request do
+  let(:super_admin) { create(:user, :super_admin) }
+  let(:user)        { create(:user) }
+  let(:tag)         { create(:tag) }
+
+  describe "POST /admin/tags/:id/moderator" do
+    before { sign_in super_admin }
+
+    it "adds the given user as trusted and as a tag moderator" do
+      post admin_tag_moderator_path(tag.id), params: { tag_id: tag.id, tag: { user_id: user.id } }
+      expect(user.tag_moderator?).to be true
+      expect(user.trusted).to be true
+    end
+  end
+
+  describe "DELETE /admin/tags/:id/moderator" do
+    before do
+      sign_in super_admin
+      user.add_role :trusted
+      user.add_role :tag_moderator, tag
+    end
+
+    it "removes the tag moderator role from the user" do
+      delete admin_tag_moderator_path(tag.id), params: { tag_id: tag.id, tag: { user_id: user.id } }
+      expect(user.tag_moderator?).to be false
+    end
+
+    it "does not remove the trusted role from the user" do
+      delete admin_tag_moderator_path(tag.id), params: { tag_id: tag.id, tag: { user_id: user.id } }
+      expect(user.trusted).to be true
+    end
+  end
+end

spec/requests/admin/tags_spec.rb

@@ -2,23 +2,23 @@ require "rails_helper"
 
 RSpec.describe "/admin/tags", type: :request do
   let(:super_admin) { create(:user, :super_admin) }
-  let(:tag)         { create(:tag) }
+  let(:user)        { create(:user) }
+  let!(:tag) { create(:tag) }
 
   before do
-    tag
     sign_in super_admin
   end
 
   describe "GET /admin/tags" do
     it "responds with 200 OK" do
-      get "/admin/tags"
+      get admin_tags_path
       expect(response.status).to eq 200
     end
   end
 
   describe "GET /admin/tags/:id" do
     it "responds with 200 OK" do
-      get "/admin/tags/#{tag.id}"
+      get admin_tag_path(tag.id)
       expect(response.status).to eq 200
     end
   end