Skip to content

Refactor IdV analytics #790

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
monfresh merged 1 commit into from
Dec 1, 2016
Merged

Refactor IdV analytics #790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
54 changes: 9 additions & 45 deletions app/controllers/idv/confirmations_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,9 @@ class ConfirmationsController < ApplicationController
before_action :confirm_idv_vendor_session_started

def index
if idv_questions && idv_questions.any?
handle_kbv
else
handle_without_kbv
end
track_final_idv_event unless FeatureManagement.proofing_requires_kbv?

finish_proofing_success
end

def continue
Expand All @@ -19,45 +17,12 @@ def continue

private

def idv_questions
idv_session.questions
end

def handle_kbv
if idv_session.question_number >= idv_questions.count
submit_answers
else
redirect_to idv_questions_path
end
end

def handle_without_kbv
# should we do further interrogate idv_resolution?
# see https://github.com/18F/identity-private/issues/485
finish_proofing_success
end

def submit_answers
@idv_vendor = idv_session.vendor
resolution = idv_session.resolution
@confirmation = idv_agent.submit_answers(resolution.questions, resolution.session_id)
if @confirmation.success?
finish_proofing_success
else
finish_proofing_failure
end
end

def finish_proofing_failure
# do not store PII that failed.
idv_session.profile.destroy
idv_session.clear
analytics.track_event(Analytics::IDV_FAILED)
if idv_attempter.exceeded?
redirect_to idv_fail_url
else
redirect_to idv_retry_url
end
def track_final_idv_event
result = {
success: true,
new_phone_added: idv_session.params['phone_confirmed_at'].present?
}
analytics.track_event(Analytics::IDV_FINAL, result)
end

def finish_proofing_success
Expand All @@ -66,7 +31,6 @@ def finish_proofing_success
idv_session.complete_profile
idv_session.clear
flash[:allow_confirmations_continue] = true
analytics.track_event(Analytics::IDV_SUCCESSFUL)
end
end
end
61 changes: 55 additions & 6 deletions app/controllers/idv/questions_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@ class QuestionsController < ApplicationController
before_action :confirm_idv_vendor_session_started

def index
render_next_question
if FeatureManagement.proofing_requires_kbv?
render_next_question
else
redirect_to idv_confirmations_path
end
end

def create
Expand All @@ -17,20 +21,65 @@ def create
private

def render_next_question
questions = idv_session.resolution.questions
question_number = idv_session.question_number
if more_questions?
@question_sequence = question_number + 1
@question = questions[question_number]
else
redirect_to idv_confirmations_path
submit_answers
track_kbv_event
process_submission
end
end

def more_questions?
questions = idv_session.questions
question_number = idv_session.question_number
questions && question_number < questions.count
end

def questions
idv_session.questions
end

def question_number
idv_session.question_number
end

def submit_answers
@_submission ||= begin
resolution = idv_session.resolution
idv_agent.submit_answers(resolution.questions, resolution.session_id)
end
end

def track_kbv_event
result = {
kbv_passed: correct_answers?,
idv_attempts_exceeded: idv_attempter.exceeded?,
new_phone_added: idv_session.params['phone_confirmed_at'].present?
}
analytics.track_event(Analytics::IDV_FINAL, result)
end

def correct_answers?
submit_answers.success?
end

def process_submission
if correct_answers?
redirect_to idv_confirmations_path
else
finish_proofing_failure
end
end

def finish_proofing_failure
# do not store PII that failed.
idv_session.profile.destroy
idv_session.clear
if idv_attempter.exceeded?
redirect_to idv_fail_url
else
redirect_to idv_retry_url
end
end
end
end
12 changes: 11 additions & 1 deletion app/controllers/idv/review_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,21 @@ def new

def create
resolution = start_idv_session
track_idv_event(resolution)
process_resolution(resolution)
end

private

def track_idv_event(resolution)
result = {
success: resolution.success,
idv_attempts_exceeded: idv_attempter.exceeded?
}

analytics.track_event(Analytics::IDV_INITIAL, result)
end

def process_resolution(resolution)
if resolution.success
init_questions_and_profile(resolution)
Expand Down Expand Up @@ -63,7 +73,7 @@ def idv_params
end

def phone_confirmation_required?
!idv_params[:phone_confirmed_at] || idv_params[:phone] != current_user.phone
idv_params[:phone] != current_user.phone
Copy link
Contributor

@jessieay jessieay Nov 30, 2016

Choose a reason for hiding this comment

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

👏 much better!

Copy link
Contributor

@pkarman pkarman Nov 30, 2016

Choose a reason for hiding this comment

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

hmm. maybe we don't have proper test coverage around this, but I believe the or condition is necessary for re-entry of the idv flow.

Copy link
Contributor Author

@monfresh monfresh Nov 30, 2016

Choose a reason for hiding this comment

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

Can you be more specific, please? What does re-entry mean in this scenario?

Copy link
Contributor

@pkarman pkarman Nov 30, 2016

Choose a reason for hiding this comment

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

Good question.

As I read Idv::PhoneForm.update_idv_params it seems like this check is, indeed, superfluous. I think what I was originally imagining was the edge case where the 2fa phone number was set but not confirmed (somehow) on the User. I don't think it is possible to reach the IdV process, though, w/o first having confirmed 2fa, so your change seems ok as-is.

end

def start_idv_session
Expand Down
4 changes: 2 additions & 2 deletions app/services/analytics.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,12 @@ def uuid
EMAIL_CHANGE_REQUEST = 'Email Change Request'.freeze
EMAIL_CONFIRMATION = 'Email Confirmation'.freeze
IDV_BASIC_INFO_VISIT = 'IdV: basic info visited'.freeze
IDV_FAILED = 'IdV: failed'.freeze
IDV_INITIAL = 'IdV: initial resolution'.freeze
IDV_FINAL = 'IdV: final resolution'.freeze
IDV_FINANCE_VISIT = 'IdV: finance visited'.freeze
IDV_INTRO_VISIT = 'IdV: intro visited'.freeze
IDV_PHONE_RECORD_VISIT = 'IdV: phone of record visited'.freeze
IDV_REVIEW_VISIT = 'IdV: review info visited'.freeze
IDV_SUCCESSFUL = 'IdV: successful'.freeze
INVALID_AUTHENTICITY_TOKEN = 'Invalid Authenticity Token'.freeze
INVALID_SERVICE_PROVIDER = 'Invalid Service Provider'.freeze
OTP_DELIVERY_SELECTION = 'OTP: Delivery Selection'.freeze
Expand Down
Loading