Skip to content

LG-9371 Add flow_path back to upload submitted analytics#8528

Merged
soniaconnolly merged 3 commits intomainfrom
sonia-lg-9371-hybrid-handoff-redirect-fixes
Jun 2, 2023
Merged

LG-9371 Add flow_path back to upload submitted analytics#8528
soniaconnolly merged 3 commits intomainfrom
sonia-lg-9371-hybrid-handoff-redirect-fixes

Conversation

@soniaconnolly
Copy link
Contributor

@soniaconnolly soniaconnolly commented Jun 1, 2023

In HybridHandoffController, analytics are sent separately from hybrid and standard code paths, and there were no specs on the standard path. Add flow_path back in on the standard path.

Use the telephony_form_response for analytics on the hybrid path. I checked in events.log for UploadStep, and that's what it does.

Analytics are sent separately from hybrid and standard code paths, and there were no specs on the standard path

[skip changelog]
@soniaconnolly soniaconnolly requested a review from a team June 1, 2023 23:25
…osen

Confirmed by looking at UpdateStep logs that the telephony form response is added to analytics
analytics.idv_doc_auth_upload_submitted(
**analytics_arguments.merge(response.to_h),
)
analytics_args = analytics_arguments.merge(form_response(destination: :document_capture).to_h)
Copy link
Contributor

Choose a reason for hiding this comment

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

question: could analytics_args be named to better suggest how it now differs from analytics_arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in bf0985d by removing the temp variable.

**analytics_arguments.merge(response.to_h),
)
analytics_args = analytics_arguments.merge(form_response(destination: :document_capture).to_h)
analytics_args[:flow_path] = flow_session[:flow_path]
Copy link
Contributor

Choose a reason for hiding this comment

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

question: if flow_session[:flow_path] couldn't be anything other than 'standard' as assigned in line 121 above, could you assign 'standard' here directly? (right now what's being assigned here is a bit obfuscated, but you may genuinely not know)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rearranged the code so the assignments are further apart. My sense is that the code is more resilient if the value is only assigned at decision points, and then accessed elsewhere. Could go either way though.

analytics_id: 'Doc Auth',
irs_reproofing: false,
skip_upload_step: false }
end
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpicks:

  1. preserve the key order in analytics_args assignment for easier comparison across contexts.
  2. standardize the indentation:
{
  success: true,
  errors: { message: nil },
  analytics_id: 'Doc Auth',
  destination: :link_sent,
  flow_path: 'hybrid',
  irs_reproofing: false,
  step: 'upload',
  telephony_response: {
    errors: {},
    message_id: 'fake-message-id',
    request_id: 'fake-message-request-id',
    success: true
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Addressed in bf0985d

Copy link
Contributor

@kbighorse kbighorse left a comment

Choose a reason for hiding this comment

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

any suggestions made can be ignored at developer's discretion.

Remove analytics args variable, re-order spec hashes to match
Copy link
Contributor

@eric-gade eric-gade left a comment

Choose a reason for hiding this comment

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

LGTM!

@soniaconnolly soniaconnolly merged commit 30ca396 into main Jun 2, 2023
@soniaconnolly soniaconnolly deleted the sonia-lg-9371-hybrid-handoff-redirect-fixes branch June 2, 2023 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants