Skip to content

Kmas lg 10488 add missing analytics#8940

Merged
kevinsmaster5 merged 9 commits intomainfrom
kmas-lg-10488-add-missing-analytics
Aug 10, 2023
Merged

Kmas lg 10488 add missing analytics#8940
kevinsmaster5 merged 9 commits intomainfrom
kmas-lg-10488-add-missing-analytics

Conversation

@kevinsmaster5
Copy link
Contributor

🎫 Ticket

LG-10488

🛠 Summary of changes

Added analytics methods for when a user visits Add Email Address, Edit Password, and Regenerate Backup Codes pages

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Create an account and log in
  • From the user dashboard visit any of those pages
  • Event log should show those visits.

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Can we add some test coverage for places where we're expecting analytics to be logged?

@kevinsmaster5 kevinsmaster5 marked this pull request as draft August 4, 2023 15:47

def edit; end
def edit
analytics.backup_code_regenerate_visit
Copy link
Contributor

Choose a reason for hiding this comment

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

This screen can be visited in the account creation flow. Per the AC of the ticket, can we ensure that we have a way to differentiate whether the user is regenerating backup codes from the account dashboard vs. account creation?

Account Creation:

  1. Go to http://localhost:3000
  2. Click Create an account
  3. Create an account up to MFA selection
  4. Choose Backup codes
  5. Continue up to "You’ve added your first authentication method!" screen
  6. Click "Skip for now"
  7. Click "I need new backup codes"
  8. This event would be logged, and I should be able to query distinct from existing account

Existing Account:

  1. Go to http://localhost:3000
  2. Sign in with an existing account
  3. From the account dashboard, click "Get backup codes"
  4. This event would be logged, and I should be able to query distinct from account creation

Copy link
Contributor Author

@kevinsmaster5 kevinsmaster5 Aug 9, 2023

Choose a reason for hiding this comment

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

That should now be attached to the event by way of
{"event_properties":{"request_came_from":"accounts#show"}...}
{"event_properties":{"request_came_from":"users/backup_code_setup#confirm_backup_codes"}

@kevinsmaster5 kevinsmaster5 marked this pull request as ready for review August 10, 2023 14:49
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Couple minor notes, but overall LGTM


private

def properties
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we prefix this with analytics_ to be clear that these values are related to analytics? Also for consistency with how similar methods are named elsewhere (though there's a fair bit of existing inconsistency between calling it analytics_arguments vs. analytics_properties 😅 ).

end

# Tracks when the user visits the Backup Code Regenerate page.
def backup_code_regenerate_visit(**properties)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we list out the specific properties we're expecting here, and document them in the method (YARD) documentation? So that it's clear what it's logging and can be included in the handbook analytics events documentation.

Suggested change
def backup_code_regenerate_visit(**properties)
# @param [String] request_came_from the controller/action the request came from
def backup_code_regenerate_visit(request_came_from:, **extra)

get :edit
expect(@analytics).to have_logged_event(
'Backup Code Regenerate Visited',
hash_including(request_came_from: anything),
Copy link
Contributor

@aduth aduth Aug 10, 2023

Choose a reason for hiding this comment

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

Can we have a real string here rather than anything ? I'd only expect to see anything if the value was rather unpredictable.

@kevinsmaster5 kevinsmaster5 merged commit 1340bf4 into main Aug 10, 2023
@kevinsmaster5 kevinsmaster5 deleted the kmas-lg-10488-add-missing-analytics branch August 10, 2023 20:01
@jmdembe jmdembe mentioned this pull request Aug 15, 2023
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.

2 participants