Conversation
jessieay
left a comment
jessieay
left a comment
There was a problem hiding this comment.
Two minor comments from me. Looks good!
app/services/analytics.rb
Outdated
There was a problem hiding this comment.
Might consider putting these "other cleanup" bits in a separate commit or even separate PR to keep refactor and cleanup separate in git history
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Can you be more specific, please? What does re-entry mean in this scenario?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
this tracks whether phone confirmation is required, but not whether it succeeded or not.
i.e. IdV is not complete until the phone is confirmed, so maybe we need a second analytics call after that step.
There was a problem hiding this comment.
OK, so it looks like there are 2 separate IdV events we need to track. One is whether the initial resolution passed or not (from the Review Controller). The other is whether they completed IdV after this initial resolution, which may or may not involve a phone confirmation, and may or may not involve KBV.
There was a problem hiding this comment.
That sounds right.
The current implementation does this:
whether they completed IdV after this initial resolution, which may or may not involve a phone confirmation, and may or may not involve KBV.
you are essentially adding coverage for whether initial resolution succeeded or not.
monfresh
force-pushed
the
mb-analytics-cleanup
branch
2 times, most recently
from
522a89d to
378a1c1
Compare
monfresh
changed the title
|
PR revised with updated commit message. PTAL. |
pkarman
left a comment
pkarman
left a comment
There was a problem hiding this comment.
I like that you have moved all the KBV actions into the questions controller, rather than splitting them between questions and confirmations.
Just one naming suggestion, otherwise lgtm.
There was a problem hiding this comment.
what do you think about renaming submit_answers because it no longer actually submits? The actual submission happens in the track_kbv_event and all submit_answers does is handle the response.
There was a problem hiding this comment.
Yeah, I was thinking about that. I'll make it better.
**Why**: - To better track IdV analytics. Previously, we were only tracking failures in the KBV scenario. **How**: - Add an analytics call to ReviewController to track success and failure for the initial IdV submission (prior to KBV and/or phone confirmation) - Move most of the code in ConfirmationsController to QuestionsController since it only applies when KBV is on - Check if KBV is on in QuestionsController#index. If not, redirect directly to ConfirmationsController. Previously, we were checking if questions were available even when KBV was turned off. - Add an analytics call in QuestionsController to track success and failure when KBV is on - Only call analytics in ConfirmationsController if KBV is off to prevent duplicate events when KBV is on.
monfresh
force-pushed
the
mb-analytics-cleanup
branch
from
46eef5d to
7a8f8ef
Compare
**Why**: - To better track IdV analytics. Previously, we were only tracking failures in the KBV scenario. **How**: - Add an analytics call to ReviewController to track success and failure for the initial IdV submission (prior to KBV and/or phone confirmation) - Move most of the code in ConfirmationsController to QuestionsController since it only applies when KBV is on - Check if KBV is on in QuestionsController#index. If not, redirect directly to ConfirmationsController. Previously, we were checking if questions were available even when KBV was turned off. - Add an analytics call in QuestionsController to track success and failure when KBV is on - Only call analytics in ConfirmationsController if KBV is off to prevent duplicate events when KBV is on.
**Why**: - To better track IdV analytics. Previously, we were only tracking failures in the KBV scenario. **How**: - Add an analytics call to ReviewController to track success and failure for the initial IdV submission (prior to KBV and/or phone confirmation) - Move most of the code in ConfirmationsController to QuestionsController since it only applies when KBV is on - Check if KBV is on in QuestionsController#index. If not, redirect directly to ConfirmationsController. Previously, we were checking if questions were available even when KBV was turned off. - Add an analytics call in QuestionsController to track success and failure when KBV is on - Only call analytics in ConfirmationsController if KBV is off to prevent duplicate events when KBV is on.
3 participants
Why:
failures in the KBV scenario.
How:
failure for the initial IdV submission (prior to KBV and/or phone
confirmation)
QuestionsController since it only applies when KBV is on
directly to ConfirmationsController. Previously, we were checking if
questions were available even when KBV was turned off.
failure when KBV is on
prevent duplicate events when KBV is on.
commit 378a1c191f3d7d342bdf4295153c6fa271fad9ec