Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions app/controllers/api/verify/password_confirm_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ def completion_url(result, user)
idv_come_back_later_url
elsif in_person_enrollment?(user)
idv_in_person_ready_to_verify_url
elsif device_profiling_failed?(user)
idv_come_back_later_url
elsif current_sp
sign_up_completed_url
else
Expand All @@ -63,6 +65,14 @@ def in_person_enrollment?(user)
ProofingComponent.find_by(user: user)&.document_check == Idp::Constants::Vendors::USPS
end

def device_profiling_failed?(user)
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.

To me it feels like a confusing double negative to say that the user passes the test if device_profiling_failed is false. It could be named device_profiling_passed? and return true when the user passes. Or, I would also find the name blocked_by_device_profiling? clearer if you still want it to return false on a pass.

return false unless IdentityConfig.store.proofing_device_profiling_decisioning_enabled
proofing_component = ProofingComponent.find_by(user: user)
# pass users who are inbetween feature flag being enabled and have not had a check run.
return false if proofing_component.threatmetrix_review_status.nil?
proofing_component.threatmetrix_review_status != 'pass'
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.

ditto about checking if this is nil?

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.

Do you mean checking if proofing_component is nil or if threatmetrix_review_status is nil?

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.

I meant threatmetrix_review_status is nil

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 have a test for review status being nil and it returns true . I feel like an extra line checking for nil is unnecessary unless we change threatmetrix_review_status from 'pass' or 'fail' to a boolean in the future.

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.

ah I see, so my expectation is that review status being nil is not a failure, that this method should return falsy in that case? Becaue we don't know that it's a for sure negative? and there may be users who fit into a gap of "the feature flag was enabled but we didn't run the check for them" so they don't have data

Copy link
Copy Markdown
Contributor Author

@theabrad theabrad Aug 24, 2022

Choose a reason for hiding this comment

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

ah so if it is nil let them pass?

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 that's my expectation of how we could ensure the safest/smoothest launch

Copy link
Copy Markdown
Contributor

@jskinne3 jskinne3 Aug 24, 2022

Choose a reason for hiding this comment

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

I'd like to add my support to the idea that a missing review_status in the API response (which I assume is what a nil value for proofing_component.threatmetrix_review_status indicates) should have (for now) the same behavior as pass.

end

def handle_request_enroll_exception(err)
analytics.idv_in_person_usps_request_enroll_exception(
context: context,
Expand Down
10 changes: 10 additions & 0 deletions app/controllers/idv/personal_key_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ def next_step
idv_come_back_later_url
elsif in_person_enrollment?
idv_in_person_ready_to_verify_url
elsif device_profiling_failed?
Copy link
Copy Markdown
Contributor

@aduth aduth Aug 24, 2022

Choose a reason for hiding this comment

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

Note that there are currently two implementations of the personal key step, controlled by the idv_api_enabled_steps feature flag. I'd expect we'd update both to keep the behaviors in sync.

The other:

def completion_url(result, user)
if result.extra[:profile_pending]
idv_come_back_later_url
elsif in_person_enrollment?(user)
idv_in_person_ready_to_verify_url
elsif current_sp
sign_up_completed_url
else
account_url
end
end

idv_come_back_later_url
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.

we'll change this once we have real content, right?

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.

I feel like this PR should also include a new stub page like /verify/setup_error that we'll use for the failure messaging (it can just say something like "Please contact support to continue account setup" or something)

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.

yep

elsif session[:sp] && !pending_profile?
sign_up_completed_url
else
Expand Down Expand Up @@ -82,5 +84,13 @@ def in_person_enrollment?
def pending_profile?
current_user.pending_profile?
end

def device_profiling_failed?
return false unless IdentityConfig.store.proofing_device_profiling_decisioning_enabled
proofing_component = ProofingComponent.find_by(user: current_user)
# pass users who are inbetween feature flag being enabled and have not had a check run.
return false if proofing_component.threatmetrix_review_status.nil?
proofing_component.threatmetrix_review_status != 'pass'
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.

I think we need to make sure it's also not nil

end
end
end
29 changes: 29 additions & 0 deletions spec/controllers/api/verify/password_confirm_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,35 @@ def stub_idv_session
end
end

context 'with device profiling decisioning enabled' do
before do
ProofingComponent.create(user: user, threatmetrix: true, threatmetrix_review_status: nil)
allow(IdentityConfig.store).
to receive(:proofing_device_profiling_decisioning_enabled).
and_return(true)
end

it 'redirects to come back later path when threatmetrix review status is nil' do
post :create, params: { password: password, user_bundle_token: jwt }

expect(JSON.parse(response.body)['completion_url']).to eq(account_url)
end

it 'redirects to account path when device profiling passes' do
ProofingComponent.find_by(user: user).update(threatmetrix_review_status: 'pass')
post :create, params: { password: password, user_bundle_token: jwt }

expect(JSON.parse(response.body)['completion_url']).to eq(account_url)
end

it 'redirects to come back later path when device profiling fails' do
ProofingComponent.find_by(user: user).update(threatmetrix_review_status: 'fail')
post :create, params: { password: password, user_bundle_token: jwt }

expect(JSON.parse(response.body)['completion_url']).to eq(idv_come_back_later_url)
end
end

context 'with gpo_code returned from form submission and reveal gpo feature enabled' do
let(:gpo_code) { SecureRandom.hex }

Expand Down
28 changes: 28 additions & 0 deletions spec/controllers/idv/personal_key_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -204,5 +204,33 @@ def index
expect(response).to redirect_to idv_in_person_ready_to_verify_url
end
end

context 'with device profiling decisioning enabled' do
before do
ProofingComponent.create(user: user, threatmetrix: true, threatmetrix_review_status: nil)
allow(IdentityConfig.store).
to receive(:proofing_device_profiling_decisioning_enabled).and_return(true)
end

it 'redirects to account path when threatmetrix review status is nil' do
patch :update

expect(response).to redirect_to account_path
end

it 'redirects to account path when device profiling passes' do
ProofingComponent.find_by(user: user).update(threatmetrix_review_status: 'pass')
patch :update

expect(response).to redirect_to account_path
end

it 'redirects to come back later path when device profiling fails' do
ProofingComponent.find_by(user: user).update(threatmetrix_review_status: 'fail')
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.

can we get some tests with statuses like pass or nil?

patch :update

expect(response).to redirect_to idv_come_back_later_path
end
end
end
end