Skip to content
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

Generate UUID v4 for telemetry ID #1693

Merged
merged 12 commits into from
Jan 2, 2024
Merged

Generate UUID v4 for telemetry ID #1693

merged 12 commits into from
Jan 2, 2024

Conversation

mindctrl
Copy link
Member

@mindctrl mindctrl commented Dec 21, 2023

Tasks

  • If a code change, I have written testing instructions that the whole team & outside contributors can understand.

Description

Generates and saves a uuid for use with anonymous opt-in telemetry requests.

Testing

Check out this branch and do one or both of these:

  • run get_telemetry_client_id() and notice it returns a uuid.
  • make a mock telemetry call to the REST route

Note there is a uuid stored in wp_options.faustwp_settings.

Copy link

changeset-bot bot commented Dec 21, 2023

⚠️ No Changeset found

Latest commit: c3f67ac

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@mindctrl mindctrl marked this pull request as ready for review December 21, 2023 21:42
@mindctrl mindctrl requested a review from a team as a code owner December 21, 2023 21:42
Copy link
Contributor

@blakewilson blakewilson left a comment

Choose a reason for hiding this comment

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

It looks like if the UUID isn't set, it falls back to generate_telemetry_client_id, which is good, but that value is not saved or persisted, so the value will change every request. We'll probably want to save the value if one doesn't exist.

uuid-is-not-persisted.mov

@mindctrl
Copy link
Member Author

@blakewilson haha yeah I pushed a commit right before you posted! Nice catch. 😅

Comment on lines 242 to 249
case 'telemetry_client_id':
if ( $value ) {
$settings[ $name ] = sanitize_text_field( $value );
} else {
unset( $settings[ $name ] );
}
break;

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a duplicate of the enable_telemetry case. But, it's very important we don't mess that one up, so I kept them separate in case we update one or the other.

Copy link
Contributor

@blakewilson blakewilson left a comment

Choose a reason for hiding this comment

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

Nice, I've confirmed that worked and the telemetry id is now persisted.

One other small thing I noticed however. This is up to interpretation, but I noticed when telemetry is disabled, then enabled again, the telemetry id is re-generated. Should this be the case?

The telemetry id is meant to anonymously establish a given site, so in theory if someone has telemetry on, disabled it, then re-enables, to us it would look like 2 seperate sites 🤔

If this ID is missing from the $_POST data when the settings page is saved, it gets removed. This ensures it's always in the saved payload.
@mindctrl
Copy link
Member Author

Good catch once again. This is because the uuid was not present in the $settings array in the sanitizer, because it wasn't in the $_POST data. I've added it as a hidden input so it's always present: 8384b51

Copy link
Contributor

@blakewilson blakewilson left a comment

Choose a reason for hiding this comment

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

Awesome! Works great

@mindctrl mindctrl merged commit b6c1967 into telemetry-updates Jan 2, 2024
16 of 18 checks passed
@mindctrl mindctrl deleted the generate-uuid branch January 2, 2024 13:38
mindctrl added a commit that referenced this pull request Jan 11, 2024
* Create process telemetry rest route (#1684)

* Create tests

* [Merl-1281] Add extra Google Analytics variables to Faust (#1689)

* Feat: Add block_editor_utils and app-router versions in telemetry events.

* Chore: Added changeset

* MERL-1343/MERL-1342: API rewrite to WP plugin vs GA (#1687)

* Removed telemetry from the CLI and moved it into the Faust WordPress plugin.

---------

Co-authored-by: Blake Wilson <blake.wilson@wpengine.com>
Co-authored-by: John Parris <john.parris@wpengine.com>

* MERL-1339: add WP notice for anonymous telemetry opt-in (#1690)

* Added checkbox for 'Enable Faust Telemetry'

* Added telemetry prompt to user page options

* Add `enable_telemetry` and `telemetry_reminder` to `sanitize_faustwp_settings()`

* Load telemetry callbacks file

* test: confirm `show_telemetry_prompt()` is hooked to `admin_notices`

* Sanitize telemetry reminder settings value by ensuring it's an integer

* Test telemetry prompt behavior

* test: confirm behavior of `should_show_telemetry_prompt()`

* fix: adjust CSS button styles to not style buttons in the telemetry prompt

* chore: adjust button classes and add aria-label attributes

* Register, enqueue, and localize telemetry script

* Register telemetry decision REST route

* test: confirm telemetry script is registered

* UX: Toggle checkbox on settings page to match user's decision.

Since this happens over REST via JS and we don't reload the page, this provides a better UX.

* test: confirm `should_show_telemetry_prompt()` returns false when telemetry disabled

* chore: add changeset

---------

Co-authored-by: John Parris <john.parris@wpengine.com>

* Generate UUID v4 for telemetry ID (#1693)

* chore: change test namespace from Unit to Integration

* chore: fix test class name to match file name

* fix: return 204 if telemetry not enabled

* test: confirm behavior of `generate_telemetry_client_id()`

* Support `telemetry_client_id` in settings sanitization function

* test: confirm generated and saved id matches retrieved id.


---------

Co-authored-by: Blake Wilson <blake.wilson@wpengine.com>

---------

Co-authored-by: Blake Wilson <blake.wilson@wpengine.com>
Co-authored-by: Theofanis Despoudis <328805+theodesp@users.noreply.github.com>
Co-authored-by: Matthew Wright <1815200+matthewguywright@users.noreply.github.com>
Co-authored-by: Teresa Gobble <teresagobble@gmail.com>
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.

None yet

2 participants