Skip to content

LG-11427 Frontend Logging for phone_with_camera#9481

Merged
charleyf merged 35 commits intomainfrom
charley/lg-11427-fe-log-phone-with-camera
Nov 3, 2023
Merged

LG-11427 Frontend Logging for phone_with_camera#9481
charleyf merged 35 commits intomainfrom
charley/lg-11427-fe-log-phone-with-camera

Conversation

@charleyf
Copy link
Contributor

🎫 Ticket

https://cm-jira.usa.gov/browse/LG-11427

🛠 Summary of changes

This PR sends the new phone_with_camera logging value to the FE so we can include it in FE logs. This work can only be merged after the work in #9461 is merged and will need cleanup once that work merges. This work is based on #9400 which added the other values we need to the FE logging.

📜 Testing Plan

Same testing plan as in #9400

  • Checkout this branch
  • In your local developer setup remove the logs by running make setup
  • Log in and go through the whole IDV process locally.
  • Look at the logs you just generated locally in log/events.log
    • Verify that the Frontend: IdV: Acuant SDK loaded event has a phone_question_ab_test_bucket property.
    • Verify that the Frontend: IdV: front image clicked event has a phone_question_ab_test_bucket property.
    • Verify that the Frontend: IdV: front image added event has a phone_question_ab_test_bucket property.
    • Verify that the Frontend: IdV: back image clicked event has a phone_question_ab_test_bucket property.
    • Verify that the Frontend: IdV: back image clicked event has a phone_question_ab_test_bucket property.
    • Verify that the Frontend: IdV: Link sent capture doc polling started event has a phone_question_ab_test_bucket property.
    • Verify that the Frontend: IdV: Link sent capture doc polling complete event has a phone_question_ab_test_bucket property.

@charleyf charleyf changed the base branch from main to amirbey/LG-11427-be-log-phone-with-camera October 30, 2023 14:33
Base automatically changed from amirbey/LG-11427-be-log-phone-with-camera to main October 30, 2023 17:16
@charleyf charleyf marked this pull request as ready for review October 31, 2023 21:53
{ phone_question_ab_test_bucket: 'bypass_phone_question' },
{
phone_question_ab_test_bucket: 'bypass_phone_question',
phone_with_camera: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will need to be added to the attributes in the events where it's used

def idv_link_sent_capture_doc_polling_started(**_extra)
track_event(
'Frontend: IdV: Link sent capture doc polling started', # rubocop:disable IdentityIdp/AnalyticsEventNameLinter
)
end

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should have a "hybrid" path or branch in spec/fetaures/idv/analytics_spec.rb to check for a more "real" usage. Between this and #9481 (comment), it doesn't seem like our current tests are giving us much assurance that these events are actually getting logged as expected.

Copy link
Contributor

@amirbey amirbey left a comment

Choose a reason for hiding this comment

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

blocking to prevent approval and merge
phone_with_camera is not logging ... i have added logging for image clicked ... still debugging log requests

@mitchellhenke
Copy link
Contributor

blocking to prevent approval and merge phone_with_camera is not logging ... i have added logging for image clicked ... still debugging log requests

I suspect it is due to the comment here

acuant_sdk_upgrade_a_b_testing_variables,
phone_question_ab_test_analytics_bucket,
)
).merge(phone_with_camera)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
).merge(phone_with_camera)
phone_with_camera,
)

ab_test_analytics_buckets,
phone_with_camera,
)
).merge(phone_with_camera)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
).merge(phone_with_camera)
phone_with_camera,
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed that that makes sense, but when I tried this it caused tests to fail. I'm not sure why.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm ...that's odd 🤔

@amirbey amirbey self-requested a review November 3, 2023 14:48
Copy link
Contributor

@amirbey amirbey left a comment

Choose a reason for hiding this comment

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

LGTM

@charleyf charleyf merged commit 7d0179c into main Nov 3, 2023
@charleyf charleyf deleted the charley/lg-11427-fe-log-phone-with-camera branch November 3, 2023 18:06
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.

4 participants