-
Notifications
You must be signed in to change notification settings - Fork 166
Remove unused EmailSent step #8158
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
Changes from all commits
cf613a9
bb35f2f
6580cd2
6ec3d8b
cf5b1ab
65e0d97
6bda5bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,28 +19,23 @@ def call | |
|
|
||
| # See the simple_form_for in | ||
| # app/views/idv/doc_auth/upload.html.erb | ||
|
Comment on lines
20
to
21
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: Is this comment still relevant for the updated code?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's still relevant because that's where either the |
||
| if params[:type] == 'desktop' | ||
| handle_desktop_selection | ||
| else | ||
| return bypass_send_link_steps if mobile_device? | ||
| if hybrid_flow_chosen? | ||
| handle_phone_submission | ||
| else | ||
| bypass_send_link_steps | ||
| end | ||
| end | ||
|
|
||
| def hybrid_flow_chosen? | ||
| params[:type] != 'desktop' && !mobile_device? | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps an edge case, but curious: Should we be so restrictive here with device? If a user chooses to send a link to a phone number, should we just let that happen? Seems like that'd make the code a little simpler and respect the user's choice.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe we should be able to remove the mobile device check. Users on mobile devices should not be getting to this step/screen in the first place.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eric-gade and I discussed this and the logic is arcanely complicated, so we're leaving further refactoring for the upcoming FSM extraction. |
||
| end | ||
|
|
||
| def extra_view_variables | ||
| { idv_phone_form: build_form } | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def handle_desktop_selection | ||
| if mobile_device? | ||
| send_user_to_email_sent_step | ||
| else | ||
| bypass_send_link_steps | ||
| end | ||
| end | ||
|
|
||
| def build_form | ||
| Idv::PhoneForm.new( | ||
| previous_params: {}, | ||
|
|
@@ -72,8 +67,6 @@ def handle_phone_submission | |
| failure_reason: failure_reason, | ||
| ) | ||
|
|
||
| mark_step_complete(:email_sent) | ||
|
|
||
| build_telephony_form_response(telephony_result) | ||
| end | ||
|
|
||
|
|
@@ -89,17 +82,8 @@ def application | |
| identity&.friendly_name || APP_NAME | ||
| end | ||
|
|
||
| def send_user_to_email_sent_step | ||
| mark_step_complete(:link_sent) | ||
| UserMailer.with( | ||
| user: current_user, email_address: current_user.confirmed_email_addresses.first, | ||
| ).doc_auth_desktop_link_to_sp(application, link).deliver_now_or_later | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like we could remove everything to do with this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed, thanks! |
||
| form_response(destination: :email_sent) | ||
| end | ||
|
|
||
| def bypass_send_link_steps | ||
| mark_step_complete(:link_sent) | ||
| mark_step_complete(:email_sent) | ||
| if IdentityConfig.store.doc_auth_document_capture_controller_enabled | ||
| flow_session[:flow_path] = @flow.flow_path | ||
| redirect_to idv_document_capture_url | ||
|
|
||
This file was deleted.
This file was deleted.
This file was deleted.
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.
Should we also remove
email_sent_view_atandemaiil_sent_view_countcolumns from the correspondingdoc_auth_logsdatabase table?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.
we can't drop the columns in the same PR, but we could mark them as ignored in this one
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.
I have my eye on cleaning up the doc_auth_logs table in LG-9334.
Meanwhile, marked the email_sent columns as ignored and did the same for the send_link columns which are also awaiting deletion. Thanks for the pointer!