site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

Update `Conversion_Reporting_Events_Sync` to Store detected and lost events

Open 10upsimon opened this issue 1 year ago • 3 comments

Feature Description

The previously implemented CRON sync within the Conversion_Reporting_Events_Sync class will be updated so that once events are detected, the final array of detected events will be compared against the currently saved values. If there is a difference between the arrays (i.e the newly detected events array contains values not present in the currently saved events array, or saved array has more events than newly detected one), the transients should be updated accordingly.

The saved data in googlesitekit_conversion_reporting_detected_events and googlesitekit_conversion_reporting_lost_events should be included in Analytics_4 inline module data

See Implementation - New Events Detection Following Initial Events Detection section of the design doc


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

Acceptance criteria

  • Conversion_Reporting_Events_Sync::sync_detected_events is updated in following way:
    • Final array of detected events, before stored to the setting, should be checked for difference with current value of detectedEvents setting
    • If there are events in newly detected events array that are not present in currently saved setting, store that event difference in googlesitekit_conversion_reporting_detected_events transient
    • If there are events in currently saved setting, that are not present in the newly detected events array, store that event difference in googlesitekit_conversion_reporting_lost_events transient
  • Analytics_4 module inline data is updated to include the values from above created transients

Implementation Brief

  • [x] Update Conversion_Reporting_Events_Sync::sync_detected_events method
    • Pull the detectedEvents setting from the settings (eg $this->settings->get())
    • Before the final array in $detected_events is saved to the settings, check for the difference agains value of the current detectedEvents that was previously retrieved. If there are items present $detected_events but not in the retrieved setting, save the difference in the googlesitekit_conversion_reporting_detected_events
    • Do the same, but in the reversed order, so if there are items present in retrieved setting, but not in the $detected_events array, save the difference in the googlesitekit_conversion_reporting_lost_events
    • If this is being saved for the first time - current value for detectedEvents setting is empty array, update googlesitekit_conversion_reporting_detected_events to have the same value as $detected_events that will be saved in the setting
  • [x] In Google\Site_Kit\Modules\Analytics_4 class
    • Add inline_conversion_reporting_events_detection method:
      • Retrieve the values from googlesitekit_conversion_reporting_detected_events and googlesitekit_conversion_reporting_lost_events transients and save them under their respective array keys newEvents and lostEvents, example $modules_data['analytics-4']['newEvents'].

Test Coverage

  • Update existing tests in tests/phpunit/integration/Modules/Analytics_4/Conversion_Reporting/Conversion_Reporting_Events_SyncTest.php to account for new changes - testing that the new and lost events are properly sorted and stored in their respective transients

QA Brief

  • This feature can be considered working provided the PHPUnit tests pass, as manually testing this would require the exact same changes made by the unit tests.

Changelog entry

  • Update conversion reporting events synchronization to save detected and lost events.

10upsimon avatar Sep 12 '24 09:09 10upsimon

AC ✔️

eugene-manuilov avatar Sep 20 '24 14:09 eugene-manuilov

@zutigrm IB is ✅ , moving to EB

10upsimon avatar Oct 01 '24 14:10 10upsimon

No QA is needed because it was completed during CR/MR. Hence, moving ticket directly to approval.

kelvinballoo avatar Oct 23 '24 07:10 kelvinballoo