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

Add new resolver to the conversion reporting datastore partial to retrieve detected and lost events from inline module data #9379

Open
1 task
zutigrm opened this issue Sep 19, 2024 · 6 comments
Labels
P0 High priority Team S Issues for Squad 1 Type: Enhancement Improvement of an existing feature

Comments

@zutigrm
Copy link
Collaborator

zutigrm commented Sep 19, 2024

Feature Description

Conversion reporting datastore partial should be updated to include new resolver which would retrieve detected and lost events from inline module data (implemented in #9342), and pass them to an action which should receive passed data and store them to the state.

See Implementation - Datastore Partial section of the design doc


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

Acceptance criteria

  • New resolver getConversionReportingEventsChange is added to the conversion-reporting datastore partial
  • New action receiveConversionReportingInlineData is implemented, which yields passed object to the reducer
    • Reducer should store this data in the state under detectedEventsChange property
  • New selectors hasNewConversionReportingEvents and hasLostConversionReportingEvents are added
    • They should extract their respective events from detectedEventsChange state property by using getConversionReportingEventsChange selector

Implementation Brief

  • Update assets/js/modules/analytics-4/datastore/conversion-reporting.js
    • Add the initial state object, containing detectedEventsChange, undefined by default
    • Add getConversionReportingEventsChange resolver
    • Retrieve the values from newEvents and lostEvents from Analytics inline module data like global._googlesitekitModulesData?.[ 'analytics-4' ]?.newEvents, etc.
    • Group them under one object and pass it to the receiveConversionReportingInlineData action
    • You can check an example in assets/js/googlesitekit/datastore/site/info.js, or any other datastore partial resolving inline data.
    • Add receiveConversionReportingInlineData action
      • It should extract newEvents and lostEvents from the passed argument, and yield them as an object to the reducer. Then in the reducer, update detectedEventsChange object in the state to hold newEvents and lostEvents as properties
    • Add getConversionReportingEventsChange selector
      • It should return value from the state state.detectedEventsChange
    • Include hasNewConversionReportingEvents and hasLostConversionReportingEvents selectors
      • They should use getConversionReportingEventsChange selector to retrieve the inline data (or resolve it if not yet available), and then extract their respective properties from the returned value. You can add a helper function to extract the correct property, like for example, how it is used in info datastore
        function getSiteInfoProperty( propName ) {
        return createRegistrySelector( ( select ) => () => {
        const siteInfo = select( CORE_SITE ).getSiteInfo() || {};
        return siteInfo[ propName ];
        } );
        }
        and then invoke it here in each selector passing either newEvents or lostEvents for the argument

Test Coverage

  • Update assets/js/modules/analytics-4/datastore/conversion-reporting.test.js to include test coverage for new resolver, action and selectors

QA Brief

  • Setup Site Kit with Analytics module and conversionReporting feature flag enabled
  • Paste this snippet in the console await googlesitekit.data.select('modules/analytics-4').getConversionReportingEventsChange()
    • Initially it will output undefined
    • On the second attempt it will resolve the data and output {newEvents: Array(0), lostEvents: Array(0)}
    • Note: when feature flag is disabled, this selector will still be accessible but will return undefined for both newEvents and lostEvents
  • Paste this snippet in the console await googlesitekit.data.select('modules/analytics-4').hasNewConversionReportingEvents()
    • Same as above, firs call will output undefined, afterwards should show false
    • Also same applies regarding the feature flag, when disabled selector will still work, but always return undefined value
  • Paste the snippet await googlesitekit.data.select('modules/analytics-4').hasLostConversionReportingEvents()
    • It should behave same as await googlesitekit.data.select('modules/analytics-4').hasNewConversionReportingEvents() above
  • Paste this snippet: await googlesitekit.data.dispatch('modules/analytics-4').receiveConversionReportingInlineData({newEvents: ['contact'], lostEvents: ['purchase']})
    • Then afterwards, without reloading, test the previous three snippets:
      • await googlesitekit.data.select('modules/analytics-4').getConversionReportingEventsChange() should return whole object passed above: {lostEvents: ['purchase'], newEvents: ['contact']}
      • await googlesitekit.data.select('modules/analytics-4').hasNewConversionReportingEvents() should return true
      • await googlesitekit.data.select('modules/analytics-4').hasLostConversionReportingEvents() should return true

Changelog entry

  • Add support for lost events to Conversion Reporting events datastore.
@zutigrm zutigrm added P0 High priority Type: Enhancement Improvement of an existing feature Team S Issues for Squad 1 labels Sep 19, 2024
@zutigrm zutigrm assigned zutigrm and unassigned zutigrm Sep 19, 2024
@eugene-manuilov eugene-manuilov self-assigned this Sep 20, 2024
@eugene-manuilov
Copy link
Collaborator

AC ✔️

@10upsimon
Copy link
Collaborator

@zutigrm IB ✅ , moving to EB.

@zutigrm zutigrm assigned aaemnnosttv and unassigned zutigrm Oct 30, 2024
@aaemnnosttv aaemnnosttv assigned zutigrm and unassigned aaemnnosttv Oct 31, 2024
@zutigrm zutigrm assigned aaemnnosttv and unassigned zutigrm Oct 31, 2024
@aaemnnosttv aaemnnosttv assigned zutigrm and unassigned aaemnnosttv Oct 31, 2024
@zutigrm zutigrm assigned aaemnnosttv and unassigned zutigrm Nov 4, 2024
@tofumatt tofumatt assigned tofumatt and unassigned aaemnnosttv Nov 4, 2024
@tofumatt
Copy link
Collaborator

tofumatt commented Nov 4, 2024

Assigning this to myself to get this moved through and unblock other issues 🙂

@tofumatt tofumatt assigned zutigrm and unassigned tofumatt Nov 4, 2024
@zutigrm zutigrm assigned tofumatt and unassigned zutigrm Nov 5, 2024
@tofumatt tofumatt removed their assignment Nov 5, 2024
@kelvinballoo kelvinballoo self-assigned this Nov 6, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ⚠

Going through the QAB, I have a couple of things to highlight:

ITEM 1:

  • Pasted the snippet
    await googlesitekit.data.select('modules/analytics-4').hasNewConversionReportingEvents()

    First call outputs undefined, afterwards we are expecting it to show empty array []
    However, instead of showing empty array, it's returning as 'False' ❌

    Image

    When feature flag is off, it always returned undefined value, this is expected.
    Image


ITEM 2:

  • Pasted the snippet
    await googlesitekit.data.select('modules/analytics-4').hasLostConversionReportingEvents()

    We are expecting it to behave the same as It should behave same as .hasNewConversionReportingEvents() above.
    Indeed, it behaves the same but given that it returns false instead of empty array, this needs to be double-checked. ⚠

    Image

    When feature flag is off, it always returned undefined value. This is expected.
    Image


ITEM 3:

  • Pasted the snippet:
    await googlesitekit.data.dispatch('modules/analytics-4').receiveConversionReportingInlineData({newEvents: ['contact'], lostEvents: ['purchase']})
    Then afterwards, without reloading, I tested the 3 snippets:
    • getConversionReportingEventsChange() - returns whole object passed above: {lostEvents: ['purchase'], newEvents: ['contact']} ✅

    • hasNewConversionReportingEvents() - we are expecting it to return only newEvents value, in this case ['contact'] but it's returning as 'true' ❌

    • hasLostConversionReportingEvents()- we are expecting it to return only lostEvents value, in this case: ['purchase'] but it's returning as 'true' ❌

      Image


Other than that .getConversionReportingEventsChange() worked well:

  • Pasted the snippet
    await googlesitekit.data.select('modules/analytics-4').getConversionReportingEventsChange()
    Initially, it outputs undefined ✅
    On the second attempt, it will resolve the data and outputs {newEvents: Array(0), lostEvents: Array(0)}

    Image

    When feature flag is disabled, returns undefined for both newEvents and lostEvents

    Image

@zutigrm
Copy link
Collaborator Author

zutigrm commented Nov 7, 2024

@kelvinballoo Thanks for you observations. Sorry about that, I update QAB now, we changed in the last CR round, what you see is correct output, hasNewConversionReportingEvents and hasLostConversionReportingEvents return true, or false, or undefined, they will not return arrays any more

@zutigrm zutigrm removed their assignment Nov 7, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ✅

Thanks @zutigrm , going through the new QAB, everything is checking out.
Moving ticket to Approval.

  • getConversionReportingEventsChange() worked well:

    • Pasted the snippet
      await googlesitekit.data.select('modules/analytics-4').getConversionReportingEventsChange()
      Initially, it outputs undefined ✅
      On the second attempt, it will resolve the data and outputs {newEvents: Array(0), lostEvents: Array(0)} ✅

      Image

      When feature flag is disabled, returns undefined for both newEvents and lostEvents

      Image


  • hasNewConversionReportingEvents() worked well:
    • Pasted the snippet
      await googlesitekit.data.select('modules/analytics-4').hasNewConversionReportingEvents()

      First call outputs undefined, afterwards it returns 'False' ✅
      When feature flag is off, it always returned undefined value, this is expected. ✅

      Image

      When feature flag is off, it always returned undefined value, this is expected.
      Image


  • hasLostConversionReportingEvents() worked well:
    • Pasted the snippet
      await googlesitekit.data.select('modules/analytics-4').hasLostConversionReportingEvents()

      We are expecting it to behave the same as .hasNewConversionReportingEvents() above.
      Indeed, it behaves the same. First call is undefined, followed by 'False' ✅
      When feature flag is off, it always returned undefined value. ✅

      Image

      When feature flag is off, it always returned undefined value. This is expected.
      Image


  • .receiveConversionReportingInlineData worked well:
    • Pasted the snippet:
      await googlesitekit.data.dispatch('modules/analytics-4').receiveConversionReportingInlineData({newEvents: ['contact'], lostEvents: ['purchase']})
      Then afterwards, without reloading, I tested the 3 snippets:

      • getConversionReportingEventsChange() - returns whole object passed above: {lostEvents: ['purchase'], newEvents: ['contact']} ✅
      • hasNewConversionReportingEvents() - returns as true ✅
      • hasLostConversionReportingEvents()- returns as true ✅

      Image

@kelvinballoo kelvinballoo removed their assignment Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Team S Issues for Squad 1 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

8 participants