Skip to content

Commit

Permalink
Set errors instead of exceptions in WorkflowStepsValidator
Browse files Browse the repository at this point in the history
Set validation errors, so the user can know all the non-valid
attributes.

Also improve error messages and adapt specs.
  • Loading branch information
saraycp committed Sep 10, 2021
1 parent 2acf054 commit 4f62b2e
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 24 deletions.
4 changes: 0 additions & 4 deletions src/api/app/models/token/errors.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,4 @@ class SCMTokenInvalid < APIError
class WorkflowsYamlNotParsable < APIError
setup 400
end

class InvalidWorkflowStepDefinition < APIError
setup 403
end
end
19 changes: 12 additions & 7 deletions src/api/app/validators/workflow_steps_validator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@ def validate(record)
private

def valid_steps?
raise Token::Errors::InvalidWorkflowStepDefinition, 'Invalid workflow. Steps are not present.' if no_steps?
raise Token::Errors::InvalidWorkflowStepDefinition, "Invalid workflow step definition: #{errors.to_sentence}" if
unsupported_steps.present? || invalid_steps.present?
@workflow.errors.add(:base, 'Invalid workflow. Steps are not present.') if no_steps?
@workflow.errors.add(:base, "Invalid workflow step definition: #{errors.to_sentence}") if unsupported_steps.present? || invalid_steps.present?
end

def unsupported_steps
Expand All @@ -29,15 +28,21 @@ def no_steps?
end

def errors
acc = []
error_messages = []
step_names = []
unsupported_steps.each do |step_definition|
step_definition.each do |step_name, _|
acc << "'#{step_name}' is not a supported step"
step_names << step_name
end
end

if step_names.size.positive?
error_messages << "#{step_names.to_sentence} #{step_names.size > 1 ? 'are not supported steps' : 'is not a supported step'}"
end

invalid_steps.each do |step|
acc << step.errors.full_messages.to_sentence
error_messages << step.errors.full_messages
end
acc
error_messages.flatten
end
end
50 changes: 37 additions & 13 deletions src/api/spec/models/workflow_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,10 @@
end

describe '#valid' do
before do
subject.valid?
end

# Steps validations

context 'with a supported step' do
Expand All @@ -106,24 +110,28 @@
it { expect(subject).to be_valid }
end

context 'with several unsupported steps' do
context 'with unsupported steps' do
let(:yaml) do
{ 'steps' => [{ 'unsupported_step' => {} },
{ 'steps' => [{ 'unsupported_step_1' => {} },
{ 'unsupported_step_2' => {} },
{ 'branch_package' => { source_project: 'project',
source_package: 'package' } }] }
end

it { expect { subject.valid? }.to raise_error("Invalid workflow step definition: 'unsupported_step' is not a supported step") }
it 'sets validation errors' do
expect(subject.errors.full_messages).to match_array(
['Invalid workflow step definition: unsupported_step_1 and unsupported_step_2 are not supported steps']
)
end
end

context 'when steps are not provided' do
let(:yaml) do
{ 'steps' => [{}] }
end

it 'raises an exception for non-present steps' do
expect { subject.valid? }.to raise_error(Token::Errors::InvalidWorkflowStepDefinition,
'Invalid workflow. Steps are not present.')
it 'sets validation errors' do
expect(subject.errors.full_messages).to match_array(['Invalid workflow. Steps are not present.'])
end
end

Expand All @@ -132,10 +140,11 @@
{ 'steps' => [{ 'branch_package' => {} }] }
end

it 'raises an exception for non-present instructions' do
expect { subject.valid? }.to raise_error(Token::Errors::InvalidWorkflowStepDefinition,
"Invalid workflow step definition: Source project name can't be blank, The 'source_project' key is missing, The 'source_package'
key is missing, and Source package name can't be blank".squish)
it 'sets validation errors' do
expect(subject.errors.full_messages).to match_array(
["Invalid workflow step definition: Source project name can't be blank, The 'source_project' key is missing, The 'source_package' key is missing, \
and Source package name can't be blank"]
)
end
end

Expand All @@ -145,9 +154,24 @@
source_package: nil } }] }
end

it 'raises an exception for invalid instructions' do
expect { subject.valid? }.to raise_error(Token::Errors::InvalidWorkflowStepDefinition,
"Invalid workflow step definition: Source project name can't be blank and Source package name can't be blank")
it 'sets validation errors' do
expect(subject.errors.full_messages).to match_array(
["Invalid workflow step definition: Source project name can't be blank and Source package name can't be blank"]
)
end
end

context 'with a combination of unsupported steps and invalid step configuration' do
let(:yaml) do
{ 'steps' => [{ 'unsupported_step_1' => {} },
{ 'branch_package' => { source_project: nil } }] }
end

it 'sets validation errors' do
expect(subject.errors.full_messages).to match_array(
["Invalid workflow step definition: unsupported_step_1 is not a supported step, Source project name can't be blank, \
The 'source_package' key is missing, and Source package name can't be blank"]
)
end
end

Expand Down

0 comments on commit 4f62b2e

Please sign in to comment.