Skip to content

LG-8577: Prefill user's mobile number for hybrid docauth #7762

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

Merged
matthinz merged 5 commits into from
Feb 3, 2023
Merged

LG-8577: Prefill user's mobile number for hybrid docauth #7762

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6c356de
LG-8577: Prefill mobile number for hybrid docauth
matthinz Feb 3, 2023
5c61745
Update spec/features/idv/doc_auth/send_link_step_spec.rb
matthinz Feb 3, 2023
edc26d5
Update app/services/idv/steps/send_link_step.rb
matthinz Feb 3, 2023
339abf0
create_form -> build_form
matthinz Feb 3, 2023
b683228
Fix line length issue
matthinz Feb 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions app/services/idv/steps/send_link_step.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,22 @@ def call
build_telephony_form_response(telephony_result)
end

def extra_view_variables
{
idv_phone_form: create_form,
}
end
Comment on lines +31 to +35

@matthinz matthinz Feb 3, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This feels kind of yucky, but I think is the way to do this until we pull the hybrid flow out of the FSM?

@aduth aduth Feb 3, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, I think if we were invested in improving FSM, it could be nice to somehow provide a form object through the FSM machinery, but it seems like we're moving away from that, and this is the otherwise standard way to make values available to the view, so seems fine to me.


private

def create_form
Comment thread
matthinz marked this conversation as resolved.
Outdated
Idv::PhoneForm.new(
previous_params: {},
user: current_user,
delivery_methods: [:sms],
)
end

def build_telephony_form_response(telephony_result)
FormResponse.new(
success: telephony_result.success?,
Expand Down Expand Up @@ -76,11 +90,7 @@ def sp_or_app_name
def form_submit
params = permit(:phone)
params[:otp_delivery_preference] = 'sms'
Idv::PhoneForm.new(
previous_params: {},
user: current_user,
delivery_methods: [:sms],
).submit(params)
create_form.submit(params)
end

def formatted_destination_phone
Expand Down
3 changes: 2 additions & 1 deletion app/views/idv/doc_auth/send_link.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@

<p><%= t('doc_auth.instructions.send_sms') %></p>
<%= simple_form_for(
:doc_auth,
idv_phone_form,

@zachmargolis zachmargolis Feb 3, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

just to check: the only reason we're building the form is so we can pass it something that responds to #phone right? Could we pass a different object or like a simple PORO/Struct that has a #phone method?

@matthinz matthinz Feb 3, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I based this usage on Idv::PhoneController which also uses Idv::PhoneForm. The form has some logic already built into it to pre-fill the user's MFA phone number.

as: :doc_auth,
url: url_for,
method: 'PUT',
html: { autocomplete: 'off' },
Expand Down
4 changes: 4 additions & 0 deletions spec/features/idv/doc_auth/send_link_step_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@
let(:fake_analytics) { FakeAnalytics.new }
let(:fake_attempts_tracker) { IrsAttemptsApiTrackingHelper::FakeAttemptsTracker.new }

it "defaults phone to user's 2fa phone numebr" do
expect(page.find('input[name="doc_auth[phone]"]').value).to eql('+1 202-555-1212')
Comment thread
matthinz marked this conversation as resolved.
Outdated
end

it 'proceeds to the next page with valid info' do
expect_any_instance_of(IrsAttemptsApi::Tracker).to receive(:track_event).with(
:idv_phone_upload_link_sent,
Expand Down
10 changes: 10 additions & 0 deletions spec/services/idv/steps/send_link_step_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,16 @@
and_return(irs_attempts_api_tracker)
end

describe '#extra_view_variables' do
it 'includes form' do
expect(step.extra_view_variables).to include(
{
idv_phone_form: be_an_instance_of(Idv::PhoneForm),
},
Comment on lines +74 to +76

@aduth aduth Feb 3, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Technically the curly braces could be optional, though I don't have a strong preference in case you think they help readability.

Suggested change
{
idv_phone_form: be_an_instance_of(Idv::PhoneForm),
},
idv_phone_form: be_an_instance_of(Idv::PhoneForm),

)
end
end

describe 'the return value from #call' do
let(:response) { step.call }

Expand Down