Skip to content
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

Jump to bottom

Validate Email Language on Registration #5948

Merged
merged 1 commit into from
Feb 15, 2022
Merged

Validate Email Language on Registration #5948

merged 1 commit into from
Feb 15, 2022

Conversation

mitchellhenke
Copy link
Contributor

@mitchellhenke mitchellhenke commented Feb 14, 2022 •
edited
Loading

Currently, we do not validate the email language, but the database has a limited size of varchar(10), and invalid submissions longer than that cause a 500

We do validate similarly in

identity-idp/app/forms/update_email_language_form.rb

Line 6 in 9b87bd7

validates_inclusion_of :email_language, in: I18n.available_locales.map(&:to_s)
, so this is mostly copied from there

@mitchellhenke mitchellhenke requested a review from aduth February 14, 2022 18:37
@aduth
Copy link
Member

aduth commented Feb 14, 2022

I'm struggling with getting the error messages to render. I thought it may be due to using a set of radio button input fields with more custom ids, so I tried converting to a more standard collection_radio_buttons.

I think I got the tags converted correctly, but am still not getting any error messages rendered 😞

There's some similar work being done in #5933. Could that help here?

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/fix-invalid-email-language branch 2 times, most recently from 934bd0c to 059f893 Compare February 14, 2022 19:41
zachmargolis
zachmargolis approved these changes Feb 14, 2022
View reviewed changes
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

app/forms/register_user_email_form.rb
@@ -4,6 +4,7 @@ class RegisterUserEmailForm
include FormEmailValidator

validate :validate_terms_accepted
validates_inclusion_of :email_language, in: I18n.available_locales.map(&:to_s).append(nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@mitchellhenke mitchellhenke marked this pull request as ready for review February 14, 2022 20:12
@mitchellhenke mitchellhenke changed the base branch from main to aduth-test-page-layouts February 14, 2022 20:15
aduth
aduth approved these changes Feb 14, 2022
View reviewed changes
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

👍

Base automatically changed from aduth-test-page-layouts to main February 15, 2022 15:29
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/fix-invalid-email-language branch from 059f893 to 5cc7369 Compare February 15, 2022 15:31
@mitchellhenke mitchellhenke merged commit c879351 into main Feb 15, 2022
@mitchellhenke mitchellhenke deleted the mitchellhenke/fix-invalid-email-language branch February 15, 2022 17:07
mitchellhenke added a commit that referenced this pull request Feb 17, 2022
@mitchellhenke
Validate email_language on registration (#5948)
2af3aca
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduth aduth aduth approved these changes

@zachmargolis zachmargolis zachmargolis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mitchellhenke @aduth @zachmargolis