-
Notifications
You must be signed in to change notification settings - Fork 480
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.
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
feat: add DCDO flag for posting email param to finish signup page #58319
Conversation
else | ||
render 'new' # Re-render form to display validation errors | ||
render 'new' unless is_signup_post_enabled # Re-render form to display validation errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this else case, we don't render new if is_signup_post_enabled
, but then won't we still render new on line 71 because it has the opposite condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If is_signup_post_enabled
is true, then it will re-render new
if there are errors via line 71. If there are no errors, it will still render new
via line 71.
If is_signup_post_enabled
is false, then it will re-render new via line 68 if there are errors. If no errors, it will redirect via line 66. Line 71 is skipped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, why do we need the else? If the conditional is removed from line 71, it should handle the case where there are errors and is_signup_post_enabled
is false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The else is needed to prevent the redirect and render, rails will error if you do both. However I cleaned this up a bit to be a little cleaner, let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I didn't catch that, good to know. That looks a lot cleaner with the and return
instead of the else! It's probably a PEBKAC situation, but I find it hard to read Ruby control flow that includes if/elses with unless
and if
keywords inside.
This PR adds scaffolding to enable email addresses to bypass the partial registration cache and go directly into the finish signup page. This is accomplished by adding a new DCDO flag `student-email-post-enabled` which is off by default.
7916f4b
to
a083f6b
Compare
This PR adds scaffolding to enable email addresses to bypass the partial registration cache and go directly into the finish signup page. This is accomplished by adding a new DCDO flag
student-email-post-enabled
which is off by default.Links
Testing story
Deployment strategy
This will be deployed via a DCDO feature flag
Follow-up work
Privacy
Security
Caching
PR Checklist: