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

Create Scheduled_Events_Check Class #9130

Closed
4 tasks
10upsimon opened this issue Aug 5, 2024 · 8 comments
Closed
4 tasks

Create Scheduled_Events_Check Class #9130

10upsimon opened this issue Aug 5, 2024 · 8 comments
Labels
Module: Analytics Google Analytics module related issues P0 High priority PHP Team S Issues for Squad 1 Type: Feature New feature

Comments

@10upsimon
Copy link
Collaborator

10upsimon commented Aug 5, 2024

Feature Description

As part of the initial body of work for the ACR epic is the requirement to ascertain which events are detected within a users GA4 report data, in order to understand the metrics we should make available to them. In order to obtain this data, a single daily event will be scheduled in response to dashboard use.

Said scheduled event will make a request via the GA4 Data API, asking for data over the past 90 days containing either of the supported event metrics:

  • addToCarts
  • ecommercePurchases
  • submit_lead_form
  • generate_lead
  • contact

Based on the results of the report response, an applicable array of events present in the users report data over the past 90 days will be stored in the existing GA4 settings option against a recentEvents key.

The value of this option (obtained from the recentEvents key therein) will be used in various areas of the code to determine what we do and do not show to the end user regarding the Analytics Conversion Reporting metrics.

See the relevant section of the design document for more information.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • A new key detectedEvents which will hold array type value is added to the existing Analytics settings
  • A new Cron class is added:
    • It should be scheduled as single event every 24h, only when user is on the main Site Kit dashboard
    • Callback function should run a GA4 report to check for the presence of any of the following event names (over the last 90 days) and save found events in the recentEvents setting:
      • add_to_cart
      • purchase
      • submit_lead_form
      • generate_lead
      • contact

Implementation Brief

  • Note: Even though issue title mentions the creation of a Scheduled_Events_Check, the implementation doesn't have to be done via a single class as long as the event is logically and correctly scheduled.

  • Update includes/Modules/Analytics_4/Settings.php

    • In get_default method include recentEvents key with an empty array
  • Add includes/Modules/Analytics_4/Conversion_Reporting/Conversion_Reporting_Cron.php

    • Use the same approach implemented for includes/Core/Remote_Features/Remote_Features_Cron.php
      • Schedule single event instead (use wp_schedule_single_event)
      • Use DAY_IN_SECONDS for time
  • Add includes/Modules/Analytics_4/Conversion_Reporting/Conversion_Reporting_Events_Sync.php

    • Pass the Analytics Settings and Analytics_4 module instances in constructor and assign it to the class property
    • Add check_for_events method
      • Create a report options array and pass it to the get_data method of analytics_4 instance ($this->analytics_4->get_data( 'report', $options )). Report options should:
        • Include eventName for dimensions array
        • Use gmdate( 'Y-m-d' ) for endDate
        • Use gmdate( 'Y-m-d', strtotime( '-90 days' ) ) for startDate
        • Include dimensionFilters for eventName and use inListFilter to filter for the event names array listed in the AC
      • Iterate through the results, and push found event names to the final array that should be used to update recentEvents settings key. Results will contain event names that are present under each returned dimensionValues in resulting rows property
  • Add includes/Modules/Analytics_4/Conversion_Reporting/Conversion_Reporting_Provider.php

    • It should use the similar approach implemented for includes/Core/Remote_Features/Remote_Features_Provider.php
    • Pass the Analytics Settings, and analytics_4 instances in constructor, and assign them to the class properties
      • Instantiate Conversion_Reporting_Events_Sync and Conversion_Reporting_Cron classes and for Conversion_Reporting_Cron use check_for_events method from Conversion_Reporting_Events_Sync as a callback argument.
    • Add register method and invoke register method of Conversion_Reporting_Cron instance, and hook into the dashboard screen load-toplevel_page_googlesitekit-dashboard. As callback invoke maybe_schedule_cron method from Conversion_Reporting_Cron class.
  • Update includes/Modules/Analytics_4.php - instantiate new Conversion_Reporting_Provider class and invoke it's register method in Analytics_4::register if conversionReporting feature flag is enabled

Test Coverage

  • Add basic test coverage for new classes where applicable. You can refer to the Remote_Features_* classes for ideas

QA Brief

  • Setup Site Kit with Analytics module (contact Aleksej for access to the GA4 property), and conversionReporting feature flag enabled
  • Install WP Crontrol plugin and trigger googlesitekit_cron_conversion_reporting_events event
  • Go back to the Site Kit dashboard and past this into the console: await googlesitekit.api.get('modules', 'analytics-4', 'settings', {}, {useCache: false});
  • Look for detectedEvents key in the outputted array, it should contain ['purchase', 'submit_lead_form'] as a value
  • Go to the settings, edit Analytics property/or account, and verify that going back to the Site Kit dashboard and pasting the above mentioned snippet recentEvents has empty array [] as a value
  • Edit Analytics settings again, by switching back to the account/property with ACR data, and verify that data is showing as initially in first steps ['purchase', 'submit_lead_form']

Changelog entry

  • Add a cron task to fetch Analytics report data for conversion events reporting.
@10upsimon
Copy link
Collaborator Author

AC Looks good, moving to IB, thanks @zutigrm

@10upsimon 10upsimon removed their assignment Aug 15, 2024
@zutigrm zutigrm self-assigned this Aug 15, 2024
@10upsimon 10upsimon assigned 10upsimon and unassigned zutigrm Aug 15, 2024
@zutigrm zutigrm assigned zutigrm and unassigned 10upsimon and zutigrm Aug 15, 2024
@eugene-manuilov eugene-manuilov self-assigned this Aug 15, 2024
@eugene-manuilov
Copy link
Collaborator

IB looks to be okay-ish, but the ticket says that we need to create the Scheduled_Events_Check class that we don't mention in the ticket description at all. @10upsimon, do we really need to have that class or we can use the approach defined in IB? Also I have slightly updated AC not to mention the specific class name.

@binnieshah binnieshah added the P0 High priority label Aug 19, 2024
@zutigrm
Copy link
Collaborator

zutigrm commented Aug 20, 2024

@eugene-manuilov I can hop in to answer this, the mentioned class is included in the issue, since when we discussed the initial plan of implementation we though of going with one class to handle the logic. But I split this in IB, to propose using single purpose lean classes, to align with the approach of having leaner classes similarly to what @aaemnnosttv used in Remote Features classes recently.

We do not mandatorily need to use the Scheduled_Events_Check class, I can rename the issue, but also I will pick this up for execution when it lands in EB

@10upsimon
Copy link
Collaborator Author

@zutigrm this LGTM, thanks for adding the line indicating that we do not inherently need to implement the Scheduled_Events_Check class, I agree that if it is not necessary hen it's pointless adding it.

Moving this to EB.

@10upsimon 10upsimon removed their assignment Aug 20, 2024
@zutigrm zutigrm self-assigned this Aug 20, 2024
@binnieshah binnieshah removed the Next Up Issues to prioritize for definition label Aug 20, 2024
@zutigrm zutigrm mentioned this issue Aug 22, 2024
18 tasks
@zutigrm zutigrm removed their assignment Aug 26, 2024
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Aug 26, 2024
@zutigrm
Copy link
Collaborator

zutigrm commented Aug 29, 2024

@mohitwp I updated QAB, to reflect recent change - recentEvents option is renamed to the detectedEvents

@mohitwp
Copy link
Collaborator

mohitwp commented Aug 30, 2024

QA Update ✅

  • Tested on dev environment.
  • Verified A new key detectedevents which will hold array type value is added to the existing Analytics settings
  • Verified a new cron is added.
  • Verified detectedEvents key in the outputted array, contain ['purchase', 'submit_lead_form' ]as a value if connected GA4 account/property has ACR data.
  • Verified detectedEvents has empty array [] as a value if connected GA4 account/property don't have ACR data.

account/property with ACR data,

image

When different analytics a/c connected. detectedevents array is empty.

image

switching back to the account/property with ACR data,

image

@mohitwp mohitwp removed their assignment Aug 30, 2024
@aaemnnosttv
Copy link
Collaborator

⚠️ Approval

@mohitwp I updated QAB, to reflect recent change - recentEvents option is renamed to the detectedEvents

@zutigrm this is okay since we discussed it, but it's important to make sure this is updated in the AC since it specifically mentions the key name to use there.

Also, there are a few references to the old name in properties/docs throughout which should be updated from "check" to "sync". E.g.
image

Please address these with a quick follow-up for the release, thanks.

@10upsimon 10upsimon self-assigned this Sep 5, 2024
@10upsimon
Copy link
Collaborator Author

10upsimon commented Sep 5, 2024

@aaemnnosttv I've created a follow up PR at #9310 which addresses the code inconsistencies from a check vs sync naming point of view.

I've also updated both the AC and IB of the issue to correctly reference events_sync as a property name, and correctly reference Conversion_Reporting_Events_Sync in var declarations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: Analytics Google Analytics module related issues P0 High priority PHP Team S Issues for Squad 1 Type: Feature New feature
Projects
None yet
Development

No branches or pull requests

7 participants