DEV: Use more specific error responses (#9472)

DEV: Use more specific error responses (#9472)

  • DEV: Use render_json_error (Adds specs for Admin::GroupsController)
  • DEV: Use a specific error on blank category slug (Fixes a render_json_error warning)
  • DEV: Use a specific error on reviewable claim conflict (Fixes a render_json_error warning)
  • DEV: Use specific errors in Admin::UsersController (Fixes render_json_error warnings)
  • FIX: PublishedPages error responses
  • FIX: TopicsController error responses (There was an issue of two separate Topic instances for the same record. This makes sure there‚Äôs only one up-to-date instance.)
diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb
index 17c98b2..0583187 100644
--- a/app/controllers/admin/groups_controller.rb
+++ b/app/controllers/admin/groups_controller.rb
@@ -124,7 +124,7 @@ class Admin::GroupsController < Admin::AdminController
   protected
 
   def can_not_modify_automatic
-    render json: { errors: I18n.t('groups.errors.can_not_modify_automatic') }, status: 422
+    render_json_error(I18n.t('groups.errors.can_not_modify_automatic'))
   end
 
   private
diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index 7ced962..a02ab11 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -197,7 +197,9 @@ class Admin::UsersController < Admin::AdminController
 
   def add_group
     group = Group.find(params[:group_id].to_i)
-    return render_json_error group unless group && !group.automatic
+
+    raise Discourse::NotFound unless group
+    return render_json_error(I18n.t('groups.errors.can_not_modify_automatic')) if group.automatic
 
     group.add(@user)
     GroupActionLogger.new(current_user, group).log_add_user_to_group(@user)
@@ -207,7 +209,9 @@ class Admin::UsersController < Admin::AdminController
 
   def remove_group
     group = Group.find(params[:group_id].to_i)
-    return render_json_error group unless group && !group.automatic
+
+    raise Discourse::NotFound unless group
+    return render_json_error(I18n.t('groups.errors.can_not_modify_automatic')) if group.automatic
 
     group.remove(@user)
     GroupActionLogger.new(current_user, group).log_remove_user_from_group(@user)
diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb
index 9699b21..8481531 100644
--- a/app/controllers/categories_controller.rb
+++ b/app/controllers/categories_controller.rb
@@ -179,7 +179,10 @@ class CategoriesController < ApplicationController
 
     custom_slug = params[:slug].to_s
 
-    if custom_slug.present? && @category.update(slug: custom_slug)
+    if custom_slug.blank?
+      error = @category.errors.full_message(:slug, I18n.t('errors.messages.blank'))
+      render_json_error(error)
+    elsif @category.update(slug: custom_slug)
       render json: success_json
     else
       render_json_error(@category)
diff --git a/app/controllers/reviewables_controller.rb b/app/controllers/reviewables_controller.rb
index 506eb8f..3861998 100644
--- a/app/controllers/reviewables_controller.rb
+++ b/app/controllers/reviewables_controller.rb
@@ -227,11 +227,12 @@ protected
     return if SiteSetting.reviewable_claiming == "disabled" || reviewable.topic_id.blank?
 
     claimed_by_id = ReviewableClaimedTopic.where(topic_id: reviewable.topic_id).pluck(:user_id)[0]
+
     if SiteSetting.reviewable_claiming == "required" && claimed_by_id.blank?
-      return I18n.t('reviewables.must_claim')
+      I18n.t('reviewables.must_claim')
+    elsif claimed_by_id.present? && claimed_by_id != current_user.id
+      I18n.t('reviewables.user_claimed')
     end
-
-    claimed_by_id.present? && claimed_by_id != current_user.id
   end
 
   def find_reviewable
diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb
index 238746f..fe39956 100644
--- a/app/controllers/topics_controller.rb
+++ b/app/controllers/topics_controller.rb
@@ -368,7 +368,7 @@ class TopicsController < ApplicationController
 
     if changes.length > 0
       first_post = topic.ordered_posts.first
-      success = PostRevisor.new(first_post).revise!(current_user, changes, validate_post: false)
+      success = PostRevisor.new(first_post, topic).revise!(current_user, changes, validate_post: false)
     end
 
     # this is used to return the title to the client as it may have been changed by "TextCleaner"
diff --git a/app/models/published_page.rb b/app/models/published_page.rb
index da89df5..3c232b6 100644
--- a/app/models/published_page.rb
+++ b/app/models/published_page.rb
@@ -24,6 +24,8 @@ class PublishedPage < ActiveRecord::Base
   end
 
   def self.publish!(publisher, topic, slug)
+    pp = nil
+
     transaction do
       pp = find_or_initialize_by(topic: topic)
       pp.slug = slug.strip
diff --git a/config/initializers/012-web_hook_events.rb b/config/initializers/012-web_hook_events.rb
index 727119a..043ede8 100644
--- a/config/initializers/012-web_hook_events.rb
+++ b/config/initializers/012-web_hook_events.rb
@@ -26,7 +26,7 @@ end
 end
 
 DiscourseEvent.on(:post_edited) do |post, topic_changed|
-  if post.topic
+  unless post.topic&.trashed?
     WebHook.enqueue_post_hooks(:post_edited, post)
 
     if post.is_first_post? && topic_changed
diff --git a/lib/post_revisor.rb b/lib/post_revisor.rb
index 2685292..0d60af6 100644
--- a/lib/post_revisor.rb
+++ b/lib/post_revisor.rb
@@ -45,9 +45,12 @@ class PostRevisor
 
   attr_reader :category_changed
 
-  def initialize(post, topic = nil)
+  def initialize(post, topic = post.topic)
     @post = post
-    @topic = topic || post.topic
+    @topic = topic
+
+    # Make sure we have only one Topic instance
+    post.topic = topic
   end
 
   def self.tracked_topic_fields
@@ -383,7 +386,7 @@ class PostRevisor
         .where(action_type: UserAction::WAS_LIKED)
         .update_all(user_id: new_owner.id)
 
-      private_message = @post.topic.private_message?
+      private_message = @topic.private_message?
 
       prev_owner_user_stat = prev_owner.user_stat
       unless private_message
diff --git a/spec/requests/admin/groups_controller_spec.rb b/spec/requests/admin/groups_controller_spec.rb
index ea7e77b..7ddc156 100644
--- a/spec/requests/admin/groups_controller_spec.rb
+++ b/spec/requests/admin/groups_controller_spec.rb
@@ -95,6 +95,33 @@ RSpec.describe Admin::GroupsController do
       expect(group.group_users.where(owner: true).map(&:user))
         .to contain_exactly(user, admin)
     end
+
+    it 'returns not-found error when there is no group' do
+      group.destroy!
+
+      put "/admin/groups/#{group.id}/owners.json", params: {
+        group: {
+          usernames: user.username
+        }
+      }
+
+      expect(response.status).to eq(404)
+    end
+
+    it 'does not allow adding owners to an automatic group' do
+      group.update!(automatic: true)
+
+      expect do
+        put "/admin/groups/#{group.id}/owners.json", params: {
+          group: {
+            usernames: user.username
+          }
+        }
+      end.to_not change { group.group_users.count }
+
+      expect(response.status).to eq(422)
+      expect(response.parsed_body["errors"]).to eq(["You cannot modify an automatic group"])
+    end
   end
 
   describe '#remove_owner' do
@@ -108,6 +135,27 @@ RSpec.describe Admin::GroupsController do
       expect(response.status).to eq(200)
       expect(group.group_users.where(owner: true)).to eq([])
     end
+
+    it 'returns not-found error when there is no group' do
+      group.destroy!
+
+      delete "/admin/groups/#{group.id}/owners.json", params: {
+        user_id: user.id
+      }
+
+      expect(response.status).to eq(404)
+    end
+
+    it 'does not allow removing owners from an automatic group' do
+      group.update!(automatic: true)
+
+      delete "/admin/groups/#{group.id}/owners.json", params: {
+        user_id: user.id
+      }
+
+      expect(response.status).to eq(422)
+      expect(response.parsed_body["errors"]).to eq(["You cannot modify an automatic group"])
+    end
   end
 
   describe "#bulk_perform" do
diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb
index 8d52b13..cb654a0 100644
--- a/spec/requests/admin/users_controller_spec.rb
+++ b/spec/requests/admin/users_controller_spec.rb
@@ -342,17 +342,54 @@ RSpec.describe Admin::UsersController do
 
       expect(response.status).to eq(200)
     end
+
+    it 'returns not-found error when there is no group' do
+      group.destroy!
+

[... diff too long, it was truncated ...]

GitHub sha: 17cf300b

This commit appears in #9472 which was approved by eviltrout. It was merged by CvX.