diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index 17c98b2d31e..05831871f1a 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 7ced962b3d8..a02ab11dace 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 9699b218696..8481531b641 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 506eb8f9e98..38619988127 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') - 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 diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 238746ffb06..fe399566d2c 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 da89df53bc6..3c232b6547e 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 727119a6276..043ede89b59 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 2685292e69c..0d60af6ae44 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 ea7e77bbbda..7ddc1562fed 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 8d52b133f83..cb654a08c6c 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! + + 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) - g = Fabricate(:group, users: [u]) - u.update!(primary_group_id: g.id) - delete "/admin/users/#{u.id}/groups/#{g.id}.json" + group = Fabricate(:group, users: [user]) + user.update!(primary_group_id: group.id) + delete "/admin/users/#{user.id}/groups/#{group.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 diff --git a/spec/requests/categories_controller_spec.rb b/spec/requests/categories_controller_spec.rb index 8451936ccc7..91b1e4ebbb7 100644 --- a/spec/requests/categories_controller_spec.rb +++ b/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 diff --git a/spec/requests/published_pages_controller_spec.rb b/spec/requests/published_pages_controller_spec.rb index 6b3a6894f46..ed50ed88116 100644 --- a/spec/requests/published_pages_controller_spec.rb +++ b/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 diff --git a/spec/requests/reviewables_controller_spec.rb b/spec/requests/reviewables_controller_spec.rb index e42d67ae404..3763b725e93 100644 --- a/spec/requests/reviewables_controller_spec.rb +++ b/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 diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index c5ff3ecc7aa..751c8bf88b8 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/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