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

WC Telemetry: add first_used and installation_date data to WCTracker #39605

Merged
merged 10 commits into from Aug 11, 2023

Conversation

jaclync
Copy link
Contributor

@jaclync jaclync commented Aug 7, 2023

Submission Review Guidelines:

Changes proposed in this Pull Request:

Part of woocommerce/woocommerce-ios#10406

After the initial implementation of the telemetry endpoint #30415, this PR adds tracking for first_used and installation_date that are sent to WCTracker for a better understanding of the Woo mobile merchants.

first_used

When saving the mobile usage data for the first time, first_used field is set to be the same as last_used (the time of the request). For stores that already have the mobile usage data (last_used has been set), they won't have a first_used date so that we can track the usage more accurately.

installation_date

This is an optional field since earlier mobile versions don't track and send the installation date. It's set once when the field hasn't been set before, whether the store has existing mobile usage data or not.

💬 @shiki feel free to suggest a different definition based on the business requirements.

How to test the changes in this Pull Request:

Prerequisite: the site hasn't had any mobile usage data tracked yet (like a brand new site), and supports https so that it can use the REST API key & secret to make API requests. I personally use Local for development for the ease of adding SSL to the site.

  1. Send a POST request to the /wp-json/wc-telemetry/tracker endpoint with body: { "platform": "ios", "version": "1.2", "installation_date": "2023-08-08T03:48:50Z" }. The response should have a 200 status, but no response body.
  2. Via WP-CLI or similar, execute WC_Tracker::get_tracking_data() --> wc_mobile_usage should show an entry for ios, and the data underneath should include version: 1.2, installation_date: 2023-08-08T03:48:50+00:00, and the same first_used and last used dates in ISO 8601 format
    • Example: wp eval "var_dump(WC_Tracker::get_tracking_data()['wc_mobile_usage']);"
  3. Send another POST request to the telemetry endpoint with body: { "platform": "ios", "version": "1.2", "installation_date": "2023-08-10T03:48:50Z" } --> in WCTracker from WP-CLI, wc_mobile_usage should show an entry for ios, and the data underneath should include version: 1.2, installation_date: 2023-08-08T03:48:50+00:00 (same as the previous value), and the first_used date should not be changed from the previous value while last used should be updated to the latest request time.
  4. You can "reset" the mobile usage data for further testing by deleting the relevant option:
    • wp option delete woocommerce_mobile_app_usage

Note: If using wp shell to test, you may have to close the shell session and re-open if the option value is cached.

@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Aug 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2023

Test Results Summary

Commit SHA: c8fbccb

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202611m 1s
E2E Tests1950015021017m 36s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

@jaclync jaclync force-pushed the wcios/10406-telemetry-add-metrics branch from cf1e4e6 to 45422f9 Compare August 9, 2023 01:40
@jaclync jaclync requested a review from shiki August 9, 2023 07:47
@jaclync jaclync marked this pull request as ready for review August 9, 2023 07:47
@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2023

Hi @coreymckrill,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

Copy link
Member

@shiki shiki left a comment

Choose a reason for hiding this comment

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

Thank you for working on this, @jaclync. It looks like this is generally want we want. I have one question.

I haven't really set up my development environment yet. But even if I did, I would ask help from the dev team to help review this. 😁

@coreymckrill would you be able to review this PR? 🙏

'platform' => sanitize_text_field( $platform ),
'version' => sanitize_text_field( $version ),
'last_used' => gmdate( 'c' ),
'installation_date' => get_gmt_from_date( $installation_date, 'c' ),
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what will happen if the $installation_date is null. It looks like it will return a date regardless. We probably want to return a null instead.

I'm mentioning this because on the following code, we just copy over everything that is in $new. And I believe that includes $new['installation_date']. 🤔 So even if the client doesn't send installation_date, it will still be set?

} else {
// Only sets `first_used` when the platform usage data hasn't been set before.
$new['first_used'] = $new['last_used'];
$data[ $platform ] = $new;
}

Maybe we also want to add a unit test that the endpoint behaves as expected if the iOS and Android clients are old (they only pass version and platform). Maybe confirm that installation_date is not set in that scenario. I think we don't have a test for this yet. But please let me know if I just missed it. 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

So even if the client doesn't send installation_date, it will still be set?

Just tried this, and it's true. It just set the current date/time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch on this case that I missed! I was able to reproduce it in a test case and API testing like Corey shared. Updated in f22cff1 for another look!

@shiki shiki requested a review from coreymckrill August 9, 2023 17:17
Copy link
Collaborator

@coreymckrill coreymckrill left a comment

Choose a reason for hiding this comment

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

👍 Worked as described in local testing. I also tried:

  • Sending a request with an installation_date key but no value (400 Bad Request)
  • Sending a second request with a different installation_date value from the first one (The first value is not overwritten)

I updated the testing instructions slightly to describe how to "reset" the usage data using WP-CLI.

@coreymckrill coreymckrill added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Aug 10, 2023
@rodelgc
Copy link
Contributor

rodelgc commented Aug 11, 2023

Thanks for your code review and for adding an additional "reset" step, @coreymckrill. Approving and merging this one now.

@rodelgc rodelgc merged commit f9b68e2 into trunk Aug 11, 2023
26 checks passed
@rodelgc rodelgc deleted the wcios/10406-telemetry-add-metrics branch August 11, 2023 06:32
@github-actions github-actions bot added this to the 8.1.0 milestone Aug 11, 2023
@rodelgc rodelgc added needs: internal testing Indicates if the PR requires further testing conducted by Solaris status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: internal testing Indicates if the PR requires further testing conducted by Solaris plugin: woocommerce Issues related to the WooCommerce Core plugin. status: analysis complete Indicates if a PR has been analysed by Solaris
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants