0valt/1732/close reasons

app/controllers/close_reasons_controller.rb

@@ -16,18 +16,21 @@ def edit; end
 
   def update
     before = @close_reason.attributes.map { |k, v| "#{k}: #{v}" }.join(' ')
-    @close_reason.update(close_reason_params)
-    after = @close_reason.attributes.map { |k, v| "#{k}: #{v}" }.join(' ')
+    if @close_reason.update(close_reason_params)
+      after = @close_reason.attributes.map { |k, v| "#{k}: #{v}" }.join(' ')
 
-    AuditLog.moderator_audit(event_type: 'close_reason_update',
-                             related: @close_reason,
-                             user: current_user,
-                             comment: "from <<CloseReason #{before}>>\nto <<CloseReason #{after}>>")
+      AuditLog.moderator_audit(event_type: 'close_reason_update',
+                               related: @close_reason,
+                               user: current_user,
+                               comment: "from <<CloseReason #{before}>>\nto <<CloseReason #{after}>>")
 
-    if @close_reason.community.nil?
-      redirect_to close_reasons_path(global: 1)
+      if @close_reason.community.nil?
+        redirect_to close_reasons_path(global: 1)
+      else
+        redirect_to close_reasons_path
+      end
     else
-      redirect_to close_reasons_path
+      render :edit, status: :bad_request
     end
   end
 
@@ -36,14 +39,12 @@ def new
   end
 
   def create
-    @close_reason = CloseReason.new(name: params[:close_reason][:name],
-                                    description: params[:close_reason][:description],
-                                    requires_other_post: params[:close_reason][:requires_other_post],
-                                    active: params[:close_reason][:active],
-                                    community: params[:global] == '1' ? nil : @community)
+    community_params = { community: params[:global] == '1' ? nil : @community }
+    @close_reason = CloseReason.new(close_reason_params.merge(community_params))
 
     if @close_reason.save
       attr = @close_reason.attributes_print
+
       AuditLog.moderator_audit(event_type: 'close_reason_create',
                                related: @close_reason,
                                user: current_user,
@@ -55,7 +56,7 @@ def create
         redirect_to close_reasons_path
       end
     else
-      render :new
+      render :new, status: :bad_request
     end
   end
 

app/models/close_reason.rb

@@ -3,5 +3,11 @@ class CloseReason < ApplicationRecord
 
   scope :active, -> { where(active: true) }
 
-  validates :name, uniqueness: { scope: [:community_id], case_sensitive: false }
+  validates :name, length: { maximum: 255 },
+                   presence: true,
+                   uniqueness: { scope: [:community_id], case_sensitive: false }
+
+  def global?
+    community.nil?
+  end
 end

test/controllers/close_reasons_controller_test.rb

@@ -33,14 +33,22 @@ class CloseReasonsControllerTest < ActionController::TestCase
     assert_not_nil assigns(:close_reason)
   end
 
-  test 'should create close reason' do
+  test 'should correctly create close reasons' do
     sign_in users(:global_admin)
-    post :create, params: { close_reason: { name: 'test', description: 'test', requires_other_post: true,
-                                            active: true } }
-    assert_response(:found)
-    assert_redirected_to close_reasons_path
-    assert_not_nil assigns(:close_reason)
-    assert_not_nil assigns(:close_reason).id
+
+    [false, true].each do |global|
+      try_create_close_reason(global: global, name: global ? 'all communities' : 'per-community')
+
+      assert_response(:found)
+      assert_redirected_to close_reasons_path(global: global ? '1' : nil)
+      assert_not_nil assigns(:close_reason)&.id
+    end
+  end
+
+  test 'should not create invalid close reasons' do
+    sign_in users(:global_admin)
+    try_create_close_reason(name: '')
+    assert_response(:bad_request)
   end
 
   test 'should get edit' do
@@ -56,14 +64,43 @@ class CloseReasonsControllerTest < ActionController::TestCase
     assert_response(:not_found)
   end
 
-  test 'should update close reason' do
+  test 'should correctly update close reasons' do
     sign_in users(:global_admin)
-    patch :update, params: { id: close_reasons(:duplicate).id, close_reason: { name: 'test', description: 'test',
-                                                                               requires_other_post: true,
-                                                                               active: false } }
-    assert_response(:found)
-    assert_redirected_to close_reasons_path
-    assert_not_nil assigns(:close_reason)
-    assert_equal false, assigns(:close_reason).active
+
+    close_reasons.each do |reason|
+      try_update_close_reason(reason, active: false, name: "#{reason.name} updated")
+
+      assert_response(:found)
+      assert_redirected_to close_reasons_path(global: reason.global? ? '1' : nil)
+
+      @close_reason = assigns(:close_reason)
+
+      assert_not_nil @close_reason
+      assert_equal false, @close_reason.active
+      assert_equal "#{reason.name} updated", @close_reason.name
+    end
+  end
+
+  test 'should not update close reasons to invalid states' do
+    sign_in users(:global_admin)
+    try_update_close_reason(close_reasons(:duplicate), name: '')
+    assert_response(:bad_request)
+  end
+
+  private
+
+  def try_create_close_reason(**opts)
+    global = opts.delete(:global) || false
+
+    post :create, params: { close_reason: { name: 'test',
+                                            description: 'test',
+                                            requires_other_post: true,
+                                            active: true }.merge(opts),
+                            global: global ? '1' : '0' }
+  end
+
+  def try_update_close_reason(reason, **opts)
+    patch :update, params: { id: reason.id,
+                             close_reason: opts }
   end
 end