Skip to content

Fix FormResponse to pick error details as type #9570

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
aduth wants to merge 1 commit into from
Closed

Fix FormResponse to pick error details as type #9570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/services/form_response.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ def merge_arrays(_key, first, second)
end

def flatten_details(details)
details.transform_values { |errors| errors.pluck(:error) }
details.transform_values { |errors| errors.flat_map { |error| error[:type] || error[:error] } }
end

attr_reader :success
Expand Down
19 changes: 19 additions & 0 deletions spec/services/form_response_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,25 @@
expect(combined_response.to_h).to eq response_hash
end

context 'with error detail symbol defined as type option on error' do
it 'returns a hash with success, errors, and error_details keys' do
errors = ActiveModel::Errors.new(build_stubbed(:user))
errors.add(:email_language, 'Language cannot be blank', type: :blank)
Copy link
Copy Markdown
Contributor Author

@aduth aduth Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be misinterpreting, but in trying to refamiliarize myself with the errors API (source, docs), I think this usage isn't actually "standard", and the sort of thing we're trying to enforce with type is probably better articulated by always specifying type as the second argument, and providing a message option instead.

# Instead of...
errors.add(:email_language, 'Language cannot be blank', type: :blank)

# ...do this:
errors.add(:email_language, :blank, message: 'Language cannot be blank')

However, at this point, we already have a linter to enforce the former, and it's pretty widespread throughout the application. Supporting it this way gets us to the same net result, but we may also want to consider migrating to the more "standard" option.

response = FormResponse.new(success: false, errors: errors)
response_hash = {
success: false,
errors: {
email_language: ['Language cannot be blank'],
},
error_details: {
email_language: [:blank],
},
}

expect(response.to_h).to eq response_hash
end
end

context 'with serialize_error_details_only' do
it 'excludes errors from the hash' do
errors = ActiveModel::Errors.new(build_stubbed(:user))
Expand Down