Skip to content

Fix FormResponse to pick error details as type#9570

Closed
aduth wants to merge 1 commit intomainfrom
aduth-form-response-error-detail
Closed

Fix FormResponse to pick error details as type#9570
aduth wants to merge 1 commit intomainfrom
aduth-form-response-error-detail

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Nov 9, 2023

🛠 Summary of changes

Improves FormResponse's serialization of error_details to consistently opt for the symbol form of the individual error.

The specs already largely asserted for this behavior, but only when calling errors.add with a symbol as the second argument (see related #9569).

For context, error_details was added in #5048 as a way to support querying for errors within logs irrespective of locale by assigning a locale-independent symbol type. Therefore, including human-readable strings in error_details is counter to the original goal of the property.

Extracted from #9564

📜 Testing Plan

rspec spec/services/form_response_spec.rb

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
Contributor Author

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.

@aduth
Copy link
Contributor Author

aduth commented Nov 9, 2023

There's a good chance the build may fail if we've been expecting this form of error_details in e.g. controller or forms specs.

@aduth
Copy link
Contributor Author

aduth commented Nov 9, 2023

There's indeed a lot of test failures now.

I wonder if this is also a good opportunity to revive #7454 if we're going to be changing the expected value structure anyways.

@aduth
Copy link
Contributor Author

aduth commented Nov 9, 2023

Considering #9572 as a replacement for this.

@aduth
Copy link
Contributor Author

aduth commented Nov 9, 2023

Closing in favor of #9572

@aduth aduth closed this Nov 9, 2023
@aduth aduth deleted the aduth-form-response-error-detail branch November 9, 2023 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants