google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.25k stars 289 forks source link

Create `Scheduled_Events_Check` Class #9130

Closed 10upsimon closed 1 month ago

10upsimon commented 2 months ago

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:

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

Implementation Brief

Test Coverage

QA Brief

Changelog entry

10upsimon commented 2 months ago

AC Looks good, moving to IB, thanks @zutigrm

eugene-manuilov commented 2 months ago

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.

zutigrm commented 2 months ago

@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 commented 2 months ago

@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.

zutigrm commented 2 months ago

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

mohitwp commented 2 months ago

QA Update ✅

**account/property with ACR data,** ![image](https://github.com/user-attachments/assets/a5d390ca-7d43-4183-b0ea-09ec209b2960) **When different analytics a/c connected. `detectedevents` array is empty.** ![image](https://github.com/user-attachments/assets/493abfb0-6a0e-4971-a430-c07c854c62f8) **switching back to the account/property with ACR data,** ![image](https://github.com/user-attachments/assets/b3c33d67-0dbd-4223-9613-917bfc516db7)
aaemnnosttv commented 1 month ago

⚠️ 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 commented 1 month ago

@aaemnnosttv I've created a follow up PR at https://github.com/google/site-kit-wp/pull/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.