Merge pull request #1733 from codidact/0valt/1717/close-reasons

Minor improvements to close reasons handling

Author: ArtOfCode-

app/controllers/close_reasons_controller.rb

@@ -1,10 +1,11 @@
 class CloseReasonsController < ApplicationController
   before_action :verify_moderator
+  before_action :check_create_access, only: [:new, :create]
   before_action :set_close_reason, only: [:edit, :update]
   before_action :verify_admin_for_global_reasons, only: [:edit, :update]
 
   def index
-    @close_reasons = if current_user.is_global_admin && params[:global] == '1'
+    @close_reasons = if current_user.global_admin? && params[:global] == '1'
                        CloseReason.unscoped.where(community_id: nil)
                      else
                        CloseReason.unscoped.where(community_id: @community.id)
@@ -15,9 +16,12 @@ def edit; end
 
   def update
     before = @close_reason.attributes.map { |k, v| "#{k}: #{v}" }.join(' ')
-    @close_reason.update close_reason_params
+    @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,
+
+    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?
@@ -28,29 +32,23 @@ def update
   end
 
   def new
-    if !current_user.is_global_admin && params[:global] == '1'
-      not_found!
-      return
-    end
-
-    @close_reason = CloseReason.new
+    @close_reason = CloseReason.new(community: params[:global] == '1' ? nil : @community)
   end
 
   def create
-    if !current_user.is_global_admin && params[:global] == '1'
-      not_found!
-      return
-    end
-
     @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)
+
     if @close_reason.save
       attr = @close_reason.attributes_print
-      AuditLog.moderator_audit(event_type: 'close_reason_create', related: @close_reason, user: current_user,
+      AuditLog.moderator_audit(event_type: 'close_reason_create',
+                               related: @close_reason,
+                               user: current_user,
                                comment: "<<CloseReason #{attr}>>")
+
       if @close_reason.community.nil?
         redirect_to close_reasons_path(global: 1)
       else
@@ -71,6 +69,10 @@ def set_close_reason
     @close_reason = CloseReason.unscoped.find(params[:id])
   end
 
+  def check_create_access
+    not_found! unless current_user&.global_admin? || params[:global] != '1'
+  end
+
   def verify_admin_for_global_reasons
     if !current_user.global_admin? && @close_reason.community.nil?
       not_found!

app/views/close_reasons/_form.html.erb

@@ -1,3 +1,11 @@
+<%#
+    Instance variables:
+      @close_reason : close reason to edit
+
+    Variables:
+      submit_url : route URL to submit the form to
+%>
+
 <%= form_for @close_reason, url: submit_url do |f| %>
   <div class="form-group">
     <%= f.label :name, "Name", class: "form-element" %>

test/controllers/close_reasons_controller_test.rb

@@ -3,8 +3,6 @@
 class CloseReasonsControllerTest < ActionController::TestCase
   include Devise::Test::ControllerHelpers
 
-  PARAM_LESS_ACTIONS = [:index, :new].freeze
-
   test 'should get index' do
     sign_in users(:admin)
     get :index
@@ -14,15 +12,15 @@ class CloseReasonsControllerTest < ActionController::TestCase
 
   test 'should deny anonymous users access' do
     sign_out :user
-    PARAM_LESS_ACTIONS.each do |path|
+    [:index, :new].each do |path|
       get path
       assert_response(:not_found)
     end
   end
 
   test 'should deny standard users access' do
     sign_in users(:standard_user)
-    PARAM_LESS_ACTIONS.each do |path|
+    [:index, :new].each do |path|
       get path
       assert_response(:not_found)
     end