Render incorrect verify-by-mail OTP error inline#10329
Conversation
Prior to this commit the verify-by-mail OTP submission would add errors to the flash and redirect to the OTP entry path. This lead to a couple problems: 1. The error for the OTP appeared in the flash instead of on the OTP field 2. The `did_not_receive_letter` param was not sticky i.e. when a user submitted the OTP it would switch back to the “Welcome back” UI. This was because the param was not present on the redirected URL. This commit changes the implementation to closer match what we would expect from a controller using the [Form Object Pattern](https://dev.to/drbragg/rails-design-patterns-form-object-4d47). Specifically the `#create` action no longer redirects, but renders the `index` template with the `GpoVerifyForm` that was used for submission and thus has all of the eros and context for the submission in the `@gpo_verify_form` ivar. One wrinkle with this approach is I had to set the `@can_request_another_letter`, `@user_did_not_receive_letter`, and `@last_date_letter_was_sent` ivars in the create action and in the index action. I opted to add a `#render_enter_code_form` method which handles setting up these ivars and rendering the index template. [skip changelog]
jmhooper
force-pushed
the
jmhooper-render-gpo-otp-error-correctly
branch
from
f851d7a to
f9a627a
Compare
|
Love the use of the form object here to automatically show errors on related fields 👍 Visually, I'd wonder if there'd be any UX feedback about the width constraints resulting in line breaks. This is some custom code we added a while back that I've been eyeing to remove since it's weird to apply the styling in JavaScript, and IMO doesn't always look great in practice. |
Are you suggesting I remove this JS here or is there an alternative approach I can take to tweak the styling? Now that you point it out I can't unsee it. |
I don't think we need to do it here, but in case it's noted as a point of feedback, that'd be the easiest solution. It'd apply everywhere so we might want to think through potential impact or look back at the initial context where it was added, but I feel like it'd be generally safe since I think it's cosmetic at-worst. |
|
@aduth: I just pushed a tweak to make the error message shorter so it won't wrap in most cases. I think that should do it. |
3 participants
Prior to this commit the verify-by-mail OTP submission would add errors to the flash and redirect to the OTP entry path. This lead to a couple problems:
did_not_receive_letterparam was not sticky i.e. when a user submitted the OTP it would switch back to the “Welcome back” UI. This was because the param was not present on the redirected URL.This commit changes the implementation to closer match what we would expect from a controller using the Form Object Pattern. Specifically the
#createaction no longer redirects, but renders theindextemplate with theGpoVerifyFormthat was used for submission and thus has all of the errors and context for the submission.One wrinkle with this approach is I had to set the
@can_request_another_letter,@user_did_not_receive_letter, and@last_date_letter_was_sentivars in the create action and in the index action. I opted to add a#render_enter_code_formmethod which handles setting up these ivars and rendering the index template.I also had to add a hidden input to the form with the
did_not_receive_letterparam so it appears in the params on submission.Here is what the form looked like before:
Here is what it looks like now: