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 Ads SetupMainPAX component #8558

Closed
3 tasks done
aaemnnosttv opened this issue Apr 15, 2024 · 7 comments
Closed
3 tasks done

Create Ads SetupMainPAX component #8558

aaemnnosttv opened this issue Apr 15, 2024 · 7 comments
Labels
Module: Ads Google Ads module related issues P0 High priority 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

The Ads module's main setup component should be replaced by an alternate, new component specific to the PAX-capable experience when the feature is enabled. This can be registered in place of the current SetupComponent depending on the state of the feature.

It should make it possible to choose the optional path to launch PAX for Ads onboarding instead of manually configuring the conversion tracking ID as today.

As the main setup component, it should wrap the entire setup experience, including PAX (created in #8557), while preserving the conventions for layout and setup in SK as much as possible.


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

Acceptance criteria

  • A new SetupMainPAX component should be created that will replace the existing Ads SetupComponent when the adsPAX feature flag is enabled.
  • This should only be applicable for new Ads module set up cases, not for editing existing Ads module settings
  • Initially, the component should look the same as the current SetupMain with the addition of a CTA to set up a new account (no designs for this yet)
  • Following/invoking the CTA to set up the new account should either take the user to OAuth or launch PAX depending the granted state of the adwords scope
    • If the user has not granted the adwords scope, they should be sent to OAuth to grant it and upon returning proceed automatically to the next step
      • In this scenario the CTA should include a small disclaimer below: "You’ll first need to give permission for Site Kit to connect to Ads on your behalf" (temp/working language)
    • If the user already has the scope granted, the PAX setup (via the main PAX component created in Create PAX component #8557) should be launched in place of the "body" (form) of the setup component, while preserving the main wrapping elements such as "Connect module", Ads icon, title, and submit button (i.e. nothing related to manual conversion ID input)
  • When setting up Ads via PAX, the field validation should be altered to no longer require the conversionID setting, but require + validate the paxConversionID and extCustomerID settings instead
  • Upon completing the set up in PAX, at this stage, the signal from the app will not be established yet, so clicking the module's main submit button to "Submit changes" should set the required values from the app and then save + navigate as standard for module setup in general
  • The Ads module setup should be deemed complete at this stage and the Ads module should be connected

Implementation Brief

  • Add assets/js/modules/ads/setup/SetupMainPAX.js
    • Use assets/js/modules/ads/components/setup/SetupMain.js as a starting point
      • Check if the current user has the adwords scope granted using (core/user).hasScope(adwords)
      • Add a CTA below the conversion ID form with a button that has conditional behavior based on the state of the adwords scope including the secondary disclaimer text if the scope is not yet granted
      • Use (core/user).getConnectURL to get the URL to request the scope with if needed while adding a parameter to the URL (e.g. pax=1)
      • If the adwords scope is granted and the pax=1 parameter is present, render the PAXEmbeddedApp component in place of the main body of the component
        • Render PAXEmbeddedApp using the displayMode=setup prop
        • In the case that the scope was already granted, push the URL change when the user clicks the button to avoid a full-page reload (see also useQueryArg)
      • When rendering PAXEmbeddedApp also pass an onLaunch function prop to receive a reference to the app instance on successful launch (for use later)
      • Add the SpinnerButton from the main setup form below the PAX app
        • When clicked, use the app instance returned the PAX onLaunch to get access to the Ads services (via app.getServices() (example))
        • For now, the availability of the app instance can be used to conditionally render/enable the submit button. This could allow it to be enabled/usable before the setup is completed, but this will be enhanced/addressed in a follow-up issue
        • With the ads services, use the accountService to get the external customer ID (example), and set the value in settings state
        • Do the same with the conversionTrackingService to get the conversion tracking ID (example), and set the value in settings state
        • Finally, dispatch submitChanges() and finishSetup() similar to how it is done in SetupForm
  • Update assets/js/modules/ads/index.js
    • For SetupComponent check if adsPax feature flag is enabled to render either SetupMainPAX or current SetupMain component
  • Update includes/Modules/Ads.php
    • In is_connected method
      • Use alternate logic when the adsPax feature is enabled to return true if either $options['conversionID'] OR $options['paxConversionID'] are not empty

Test Coverage

  • Update is_connected tests for Ads module
  • Add Storybook stories for SetupMainPAX states as reasonable (it's expected PAX can't actually launch in this context but we could use some kind of simple placeholder for it as the other parts are still useful)

QA Brief

  • Setup Site Kit with Proxy plugin
  • Enable adsModule and adsPax feature flags
  • Go to Ads module setup screen. Verify there is a new Create an account button
    • If you do not have adwords scope granted, it should display permission error description bellow the button
    • If you have adwords scope, there should be no text showing
  • If you do not have adwords scope, when button is clicked, it should redirect you to the OAuth flow, and upon returning back it should launch the PAX app. The complete setup button should setup and connect Ads module and you should be redirected to the dashboard with success subtle notification showing
  • If you do have adwords scope, clicking on the button should launch PAX app, and like in previous point, clicking the complete setup button will connect Ads module..
  • In both cases once PAX app is launched, the manual conversion ID input field should not be visibe

Changelog entry

  • Introduce initial setup experience for Ads via 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
@mxbclang mxbclang added the Team S Issues for Squad 1 label Apr 16, 2024
@10upsimon 10upsimon assigned 10upsimon and unassigned 10upsimon Apr 18, 2024
@tofumatt tofumatt self-assigned this Apr 18, 2024
@tofumatt
Copy link
Collaborator

The SetupMainPAX component should render/call the main PAX component (created in #8557) and should provide a displayMode prop of "default" as we expect the full PAX experience here.

Is this correct? Wouldn't we want some sort of "setup" component/display mode here? Or is the idea that the PAX app will show a "setup" flow if the user doesn't have PAX setup.

I think that's right, actually, but we should clarify that in the ACs so it's clear. 🤔

Otherwise looks good 👍🏻

@tofumatt tofumatt assigned 10upsimon and unassigned tofumatt Apr 18, 2024
@10upsimon
Copy link
Collaborator

Is this correct? Wouldn't we want some sort of "setup" component/display mode here? Or is the idea that the PAX app will show a "setup" flow if the user doesn't have PAX setup.

@tofumatt I believe (base on this design doc comment thread) that the PAX app will only support 2 views: Setup and reporting. There is no support for existing accounts (yet) so we'd not re-launch PAX in an existing user state, and I think they will likely determine the status of existing campaign status from the user token anyway, but this does not exist yet regardless.

The naming of the actual config value may change, but I think we can agree on the component prop name for now, I am not precious about displayMode though :)

@tofumatt
Copy link
Collaborator

Sounds good then, that's what I figured.

Moving this to IB 👍🏻

@aaemnnosttv
Copy link
Collaborator Author

Updated this one a bit to rework the scope a bit and remove the bit about using the campaign service since we'll be adding that in after this rather than before now.

AC + IB ✅

@zutigrm zutigrm self-assigned this Apr 25, 2024
@zutigrm zutigrm removed their assignment May 2, 2024
@tofumatt tofumatt assigned tofumatt and zutigrm and unassigned tofumatt May 2, 2024
@zutigrm zutigrm assigned tofumatt and unassigned zutigrm May 7, 2024
@tofumatt tofumatt assigned zutigrm and unassigned tofumatt May 7, 2024
@zutigrm zutigrm assigned aaemnnosttv and unassigned zutigrm May 8, 2024
@aaemnnosttv aaemnnosttv removed their assignment May 8, 2024
@tofumatt tofumatt assigned aaemnnosttv and zutigrm and unassigned tofumatt and aaemnnosttv May 8, 2024
@zutigrm zutigrm assigned aaemnnosttv and unassigned zutigrm May 9, 2024
@aaemnnosttv aaemnnosttv removed their assignment May 9, 2024
@mohitwp mohitwp self-assigned this May 10, 2024
@mohitwp
Copy link
Collaborator

mohitwp commented May 15, 2024

QA Update ⚠️

  • Testing on dev environment using staging proxy.
  • Verified if user do not have adwords scope, when button is clicked, it redirect user to the OAuth flow, and upon returning back it should launch the PAX app. The complete setup button setup and connect Ads module and user redirected to the dashboard with success subtle notification showing.
  • Verified If user do have adwords scope, clicking on the button launch PAX app, and like in previous point, clicking the complete setup button will connect Ads module.
  • Verified that when user do not have adwords scope small disclaimer below: "You’ll first need to give permission for Site Kit to connect to Ads on your behalf" appears.
  • Verified the pax app flow.

@zutigrm

Q1) Currently, create account button have different color but I have not tested design as part of QA of this ticket. Just want to confirm again that it is not under the scope of this ticket.

image

Q2) Also, when pax app gets launch I saw 2 console errors. Is this expected ?
Update > I reported console error here #8693. #8693 (comment)

image

When user don't have adwords scope

Recording.937.mp4
Recording.939.mp4

When user have adswords scope

Recording.940.mp4

@zutigrm
Copy link
Collaborator

zutigrm commented May 15, 2024

Hey @mohitwp Thanks for your observations.

Q1) Currently, create account button have different color but I have not tested design as part of QA of this ticket. Just want to confirm again that it is not under the scope of this ticket.

Yes, color is correct - we do not have a design for this yet, still early stage. So we use inverse variation to differentiate it from complete setup

Also, when pax app gets launch I saw 2 console errors. Is this expected ?

These are coming from PAX embedded app itself, they don't show always, it is something Ads team will handle on their side. They do not interupt the flow, as PAX is working event when they show up

@mohitwp
Copy link
Collaborator

mohitwp commented May 15, 2024

QA Update ✅

  • Thanks @zutigrm
  • Testing on dev environment using staging proxy.
  • Verified if user do not have adwords scope, when button is clicked, it redirect user to the OAuth flow, and upon returning back it should launch the PAX app. The complete setup button setup and connect Ads module and user redirected to the dashboard with success subtle notification showing.
  • Verified If user do have adwords scope, clicking on the button launch PAX app, and like in previous point, clicking the complete setup button will connect Ads module.
  • Verified that when user do not have adwords scope small disclaimer below: "You’ll first need to give permission for Site Kit to connect to Ads on your behalf" appears.
  • Verified the pax app flow.

When user don't have adwords scope

Recording.937.mp4
Recording.939.mp4

When user have adswords scope

Recording.940.mp4

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 Team S Issues for Squad 1 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

6 participants