Skip to content

Commit

Permalink
Return a "Not found" error instead of "Invalid review"
Browse files Browse the repository at this point in the history
  • Loading branch information
eduardoj committed Apr 18, 2023
1 parent 9007edd commit cc408cb
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 7 deletions.
2 changes: 2 additions & 0 deletions src/api/app/models/bs_request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -736,6 +736,8 @@ def create_new_review(opts)
)
return newreview if newreview.valid?

newreview.check_reviewer!

raise InvalidReview, 'Review invalid: ' + newreview.errors.full_messages.join("\n")
end

Expand Down
5 changes: 5 additions & 0 deletions src/api/app/models/review.rb
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,11 @@ def staging_project?
for_project? && !project&.staging_workflow_id.nil?
end

def check_reviewer!
selected_errors = errors.select { |error| error.attribute.in?([:user, :group, :project, :package]) }
raise ::NotFoundError, selected_errors.map { |error| "#{error.attribute.capitalize} not found" }.to_sentence if selected_errors.any?
end

private

def matches_maintainers?(user)
Expand Down
11 changes: 4 additions & 7 deletions src/api/spec/models/bs_request_spec.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
require 'rails_helper'
# WARNING: If you change tests make sure you uncomment this line
# and start a test backend. Some of the BsRequestAction methods
# require real backend answers for projects/packages.
# CONFIG['global_write_through'] = true

RSpec.describe BsRequest, vcr: true do
let(:user) { create(:confirmed_user, :with_home, login: 'tux') }
let(:target_project) { create(:project, name: 'target_project') }
Expand Down Expand Up @@ -122,10 +119,10 @@
it { expect(request.state).to eq(:review) }
it { expect(request.commenter).to eq(reviewer.login) }

it 'fails with reasonable error' do
it 'fails with not found' do
expect { request.addreview(by_user: 'NOEXIST') }.to raise_error do |exception|
expect(exception).to be_a(BsRequest::InvalidReview)
expect(exception.message.to_s).to eq("Review invalid: User can't be blank")
expect(exception).to be_a(NotFoundError)
expect(exception.message).to eq('User not found')
end
end
end
Expand Down

0 comments on commit cc408cb

Please sign in to comment.