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

Scaffold PAX launch dependencies #8556

Closed
2 tasks done
aaemnnosttv opened this issue Apr 15, 2024 · 4 comments
Closed
2 tasks done

Scaffold PAX launch dependencies #8556

aaemnnosttv opened this issue Apr 15, 2024 · 4 comments
Labels
Module: Ads Google Ads module related issues P0 High priority Squad 1 (Team S) Issues for Squad 1 Type: Enhancement Improvement of an existing feature

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Apr 15, 2024

Feature Description

This issue is for adding the minimal infra needed to be able to launch PAX, particularly the token and base infrastructure for partner services.

The minimum requirements for launching PAX can be found in the "Launching the App" section of the design document.

PAX has a few minimum requirements in order to successfully launch the application, namely the following:

  • Loading the PAX JS source file (https://www.gstatic.com/pax/${version}/pax_integrator.js)
  • A config property containing the oAuth token and DOM selector of the element in which it renders PAX (see design doc here)
  • PAX services that are passed along with the config (see design doc here)
  • Launching the PAX app using google.ads.integration.integrator.launchGoogleAds()

To see a demo/proof-of-concept PAX plugin, check out this Gist: https://gist.github.com/tofumatt/f9ff5ae02b1844cbd62623e88e5dd5e7

It contains a simple PAX plugin that demonstrates the minimal requirements for launching PAX.


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

Acceptance criteria

The following changes should only take effect when the adsPax feature is enabled

  • A new Script Data global for PAX config should be created
    • global _googlesitekitPAXConfig
    • data_callback should return empty array if current user cannot view the main dashboard, otherwise should return the following (dots denote nesting)
      • authAccess.oauthTokenAccess.token: The current user's OAuth access token
      • locale: 2-letter locale identifier, ie en, fr, de etc. Use existing Site Kit utils and helpers to obtain the locale and trim it to the 2 letter identifier.
      • debuggingConfig.env: The environment (default PROD)
  • The PAX JavaScript source script should be loaded using the dev version, eg. <script async src="https://www.gstatic.com/pax/dev/pax_integrator.js"></script>
    • should include the above script data as a dependency
    • The file should be enqueued on Site Kit screens (i.e. CONTEXT_ADMIN_SITEKIT) via WordPress when
      • the Ads module is active
      • the user can view the main dashboard
  • A synchronous function/helper should be created in JS that returns an object with the services required for the PAX application. The service functions themselves are async functions. These services (properties within said services object) should be as follows:
    • businessService: Used to provide information about the account being set up in PAX, contains two properties:
      • getBusinessInfo: an async function that returns an object containing info about the site. The returned object from said async function should return the following properties:
        • businessName: A string representation of the business/site name. Existing datastore selectors can be used to get the site name for this value
        • businessUrl: The URL of the site. Existing Site Kit utils should be used to obtain this value as it is consistently obtained throughout the code base
      • fixBusinessInfo: Acts similar to a catch scenario, where an error object may be returned. See fix property above in authenticationService for more info. (Does not seem to be required)
    • conversionTrackingService: Used to provide information about the supported conversion event types, used to inform the user during campaign setup. While we will eventually integrate with the Conversion Event Infrastructure work currently being identified, an empty labels object should be provided for now. Contains 1 property:
      • getSupportedConversionLabels: async function that should returns an an object containing an array of support conversion type labels. This should be defaulted to an empty array for now, { conversionLabels: [] }
    • termsAndConditionsService: required but not needed for our use
      • notify - empty function

Implementation Brief

  • Update Google\Site_Kit\Modules\Ads class
    • Implement Module_With_Assets interface, and use Module_With_Assets_Trait
      • Implement setup_assets conditionally returning new assets according to AC
        • a new Script_Data asset googlesitekit-ads-pax-config
        • a new Script asset for googlesitekit-ads-pax-integratorpax_integrator.js depending on googlesitekit-ads-pax-config, and googlesitekit-modules-data
    • In register method, hook into the googlesitekit_inline_modules_data and conditionally add $modules_data['ads'] holding an array with following items:
      • supportedConversionEvents with value of an empty array for now
  • Add ./modules/ads/pax/services.js
    • export helper function createPaxServices(registry)
      • It should return an object with following properties:
        • businessService nested object as defined in AC
          • getBusinessInfo: async( () => { businessName, businessUrl } ) - source businessName from getSiteName selector of the core/info datastore, and businessUrl from the getHomeURL selector of the same datastore (be sure to make sure values are available/resolved before returning)
          • fixBusinessInfo: return { retryReady: true }
        • conversionTrackingService nested object with following property:
          • getSupportedConversionLabels: returns { conversionLabels: string[] } - with inner value sourced from module data (supportedConversionEvents)
          • termsAndConditionsService
            • notify as defined in AC

Test Coverage

  • Add test for the method returning the inline data added in Ads class
  • Add test for createPaxServices

QA Brief

  • Setup Site Kit without feature flags enabled
  • On site kit dashboard or settings screen, open developer inspector
    • Search for ads-pax, you should not see any inline JS
  • Enable adsModule and adsPax feature flags
  • On site kit dashboard or settings screen, open developer inspector
    • Search for ads-pax again, you should see only inline JS script
  • Setup Ads module
    • Search again, you should see inline script and https://www.gstatic.com/pax/dev/pax_integrator.js script included

Changelog entry

  • Scaffold dependencies for launching PAX.
@aaemnnosttv aaemnnosttv added P0 High priority Type: Enhancement Improvement of an existing feature Module: Ads Google Ads module related issues labels Apr 15, 2024
@bethanylang bethanylang added Squad 1 (Team S) Issues for Squad 1 Next Up Issues to prioritize for definition labels Apr 16, 2024
@10upsimon 10upsimon assigned 10upsimon and unassigned 10upsimon Apr 17, 2024
@tofumatt tofumatt self-assigned this Apr 18, 2024
@10upsimon
Copy link
Collaborator

10upsimon commented Apr 18, 2024

@aaemnnosttv @tofumatt I know we had the termsAndConditionsService present in the demo plugin, but looking at https://github.com/google/partner-ads-experience/blob/main/sdk/service/partner/terms-and-conditions-service.d.ts I am not sure we require this for launch, so I've left it out. It appears to mostly be about notifying when the user accepts TOC via NotifyResponse service, and I have not identified a need for this value, at least not for the PAX Pilot MVP.

@tofumatt
Copy link
Collaborator

@10upsimon Thanks so much for the details on this one. 🙂

I think the ACs here are too hard to scan and easily check off. There's extra context around the criteria that is better-placed in the issue description. It's a minor thing, but having the ACs as more of a "checklist" and the description being the context/background means it's easier to see if the issue is complete according to ACs or not.

The ACs here are too long and varied to reasonably consider them "met".


Words like "probably" are dangerous in ACs too, so things like:

  • Loading/printing of the PAX Javascript source file at https://www.gstatic.com/pax/${version}/pax_integrator.js where ${version} may be a reference to a tagged version or similar, but we'll likely implement latest, i,.e https://www.gstatic.com/pax/latest/pax_integrator.js

should be more concrete. I'd rather this be:

  • The PAX JavaScript source file should be loaded using the latest version, eg. https://www.gstatic.com/pax/${version}/pax_integrator.js.

I'm happy to give it a go and filter things down too, after you run through it again 🙂

@aaemnnosttv
Copy link
Collaborator Author

I've revised the approach here a bit to be simplified and leverage WP assets and pass the base config from the backend which eliminates the need for another helper and related datastore bits for now.

IB ✅

@zutigrm zutigrm self-assigned this Apr 22, 2024
@eclarke1 eclarke1 removed the Next Up Issues to prioritize for definition label Apr 22, 2024
@zutigrm zutigrm removed their assignment Apr 22, 2024
@mohitwp
Copy link
Collaborator

mohitwp commented Apr 29, 2024

QA Update ✅

  • Tested on dev environment.
  • Verified when Ads Pax Featured flag is not enabled.
  • Verified when Ads Pax Featured flag is not enabled and Ads Module is setup.
  • Verified when Ads Pax feature flag is enabled and Ads module is not setup.
  • Verified when Ads Pax feature flag is enabled and Ads module is setup.
  • Verified AdsPax inline script is appearing as per QAB
  1. When Ads Pax Featured flag is not enabled.

image

image

  1. When Ads Pax Featured flag is not enabled and Ads Module is setup.

image

  1. When Ads Pax feature flag is enabled and Ads module is not setup.

image

  1. When Ads Pax feature flag is enabled and Ads module is setup.

image

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: Ads Google Ads module related issues P0 High priority Squad 1 (Team S) Issues for Squad 1 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

8 participants