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.)

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

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)

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)

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')
-    end
 
-    claimed_by_id.present? && claimed_by_id != current_user.id
+    if SiteSetting.reviewable_claiming == "required" && claimed_by_id.blank?
+      I18n.t('reviewables.must_claim')
+    elsif claimed_by_id.present? && claimed_by_id != current_user.id
+      I18n.t('reviewables.user_claimed')
+    end
   end
 
   def find_reviewable

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"

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

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

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

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

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!
+
+      put "/admin/users/#{user.id}/groups.json", params: {
+        group_id: group.id
+      }
+
+      expect(response.status).to eq(404)
+    end
+
+    it 'does not allow adding users to an automatic group' do
+      group.update!(automatic: true)
+
+      expect do
+        post "/admin/users/#{user.id}/groups.json", params: {
+          group_id: group.id
+        }
+      end.to_not change { 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_group' do
     it "also clears the user's primary group" do
-      u = Fabricate(:user)
+      group = Fabricate(:group, users: [user])
-      g = Fabricate(:group, users: [u])
+      user.update!(primary_group_id: group.id)
-      u.update!(primary_group_id: g.id)
+      delete "/admin/users/#{user.id}/groups/#{group.id}.json"
-      delete "/admin/users/#{u.id}/groups/#{g.id}.json"
 
       expect(response.status).to eq(200)
-      expect(u.reload.primary_group).to eq(nil)
+      expect(user.reload.primary_group).to eq(nil)
+    end
+
+    it 'returns not-found error when there is no group' do
+      delete "/admin/users/#{user.id}/groups/9090.json"
+
+      expect(response.status).to eq(404)
+    end
+
+    it 'does not allow removing owners from an automatic group' do
+      group = Fabricate(:group, users: [user], automatic: true)
+
+      delete "/admin/users/#{user.id}/groups/#{group.id}.json"
+
+      expect(response.status).to eq(422)
+      expect(response.parsed_body["errors"]).to eq(["You cannot modify an automatic group"])
     end
   end
 

spec/requests/categories_controller_spec.rb

@@ -452,8 +452,9 @@ describe CategoriesController do
       end
 
       it 'rejects blank' do
-        put "/category/#{category.id}/slug.json", params: { slug: nil }
+        put "/category/#{category.id}/slug.json", params: { slug: '   ' }
         expect(response.status).to eq(422)
+        expect(response.parsed_body["errors"]).to eq(["Slug can't be blank"])
       end
 
       it 'accepts valid custom slug' do

spec/requests/published_pages_controller_spec.rb

@@ -114,6 +114,7 @@ RSpec.describe PublishedPagesController do
           PublishedPage.create!(slug: 'i-hate-salt', topic: Fabricate(:topic))
           put "/pub/by-topic/#{topic.id}.json", params: { published_page: { slug: 'i-hate-salt' } }
           expect(response).not_to be_successful
+          expect(response.parsed_body['errors']).to eq(['Slug has already been taken'])
         end
 
         it "returns an error if the topic already has been published" do

spec/requests/reviewables_controller_spec.rb

@@ -380,6 +380,7 @@ describe ReviewablesController do
           ReviewableClaimedTopic.create!(topic_id: qp.topic_id, user: Fabricate(:admin))
           put "/review/#{qp.id}/perform/approve_post.json?version=#{qp.version}"
           expect(response.code).to eq("422")
+          expect(response.parsed_body["errors"]).to match_array(["This item has been claimed by another user."])
         end
 
         it "works when claims are optional" do

spec/requests/topics_controller_spec.rb

@@ -989,7 +989,7 @@ RSpec.describe TopicsController do
           }
 
           expect(response.status).to eq(422)
-          expect(JSON.parse(response.body)['errors']).to be_present
+          expect(response.parsed_body['errors']).to match_array([/Title is too short/, /Title seems unclear/])
         end
 
         it "returns errors when the rate limit is exceeded" do