-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
Adds tracking for all of the site settings areas #17545
Conversation
… field is being changed
You can trigger an installable build for these changes by visiting CircleCI here. |
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emilylaguna I didn't see any tracking for the Privacy and Language settings under Site Settings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! As Momo mentioned, I also don't see tracking for Privacy or Language.
Under Discussion, the events for Sending and Receiving pingbacks seem to be reversed (toggling sending pingbacks fires the 'receive' event and vice versa).
I also added a code level comment as I feel most of these tracking calls should use constant values for the events.
@@ -268,6 +277,7 @@ open class DiscussionSettingsViewController: UITableViewController { | |||
} | |||
|
|||
self?.settings.commentsThreading = newThreadingDepth | |||
WPAnalytics.trackSettingsChange("site_settings_discussion", fieldName: "comments_threading", value: selected as Any) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think most of these settings keys should be stored as constants somewhere (and likely field names), as the same literals are reproduced in many places and a typo could mean we end up tracking the wrong thing. This comment applies to basically every trackSettingsChange
call 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see this point, but all of them are stored as private constants in BlogServiceRemoteREST and it feels like overkill to expose these constants, or duplicate them in a public fashion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I guess I mean if nothing else we could have a constant in DiscussionSettingsViewController
for let tracksDiscussionSettingsKey = "site_settings_discussion"
and so forth?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored the tracking a bit more on that view, added a constant, and moved all of the tracking to a helper func.
Oops, my sites didn't have this appear. Fixed now, and added tracking for some other options as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Project: #17503
View only this PRs changes here
Description
Adds tracking for every option in Site Settings and its encompassing settings
To test:
This affects every item on Site Settings that did not already have tracks
🔵 Tracked: settings_did_change <field_name: FIELD_NAME, page: PAGE_NAME>
Note: Areas with existing tracks were not changed, and tracking was not added to those areas
Regression Notes
Potential unintended areas of impact
None, adding tracking.
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual testing
What automated tests I added (or what prevented me from doing so)
None.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.