Closed kt-12 closed 3 weeks ago
codecov[bot]
commented
1 month ago Attention: Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.
Project coverage is 63.5%. Comparing base (
3553804) to head (616756f). Report is 195 commits behind head on feature/2460-google-ads-value-prop.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| ...product-statistics/create-campaign-notice/index.js | 77.8% | 2 Missing :warning: |
ankitguptaindia
commented
1 month ago Testing Environment -
Test Results -
When No Paid Campaign available -
When Paid Campaign Available -
Paid campaign creation flow from Notice button:
https://github.com/user-attachments/assets/15a66c7e-d27e-4bb8-bf5f-75517245bfac
joemcgill
commented
1 month ago @eason9487 thanks for the review, we're working through the feedback. Regarding this:
In terms of the requirement, does
useAdsCampaignsneed to be changed?
I do think that useAdsCampaigns needs to be changed for this requirement.
The purpose of this issue is to encourage merchants to create their first campaign but only if they have approved products and do not already have any ads campaigns created. To verify whether ads campaigns are running we need to check for existing campaigns on the account, which is what this hook seems to be for. However, if someone connects an existing account that already has ads and finishes onboarding by skipping creating a new ad, then the request to see if the account already has ads will never run.
This can be verified by connecting an account and creating a campaign.
Then from the settings screen, disconnect the account and reconnect it through the setup-ads flow and close without creating a new campaign. At this point, no more campaigns will show on the dashboard because no ads will be returned from the useAdsCampaigns hook, even though the account does have campaigns running.
This does not seem like expected behavior to me.
eason9487
commented
1 month ago Hi @joemcgill
I do think that
useAdsCampaignsneeds to be changed for this requirement.
Other side effects from changing useAdsCampaigns will need to be reviewed one by one to see if they need to be adjusted together, otherwise there may be inconsistent presentation at a high level.
For example, the campaign list is displayed on the Dashboard page, but the Performance card on the same page still doesn't show campaign performance data.
It's also possible that a merchant has connected to a Google Ads account with existing campaigns but has not completed the Ads Setup flow for a long time. The adsSetupComplete will be always false and ADS_SETUP_COMPLETED_AT doesn't exist. The frontend and backend logic associated with these two values will also be affected and may need to be reviewed one by one as well.
joemcgill
commented
1 month ago Other side effects from changing useAdsCampaigns will need to be reviewed one by one to see if they need to be adjusted together, otherwise there may be inconsistent presentation at a high level.
That makes sense. I've reviewed the codebase and am documenting my observations here for posterity.
It looks like all of the functionality that depends on the gla.adsSetupComplete global, including the useAdsCampaigns() hook assume that there are no campaigns to display until after the Ads account has been fully set up via the onboarding process or the secondary "Set up paid ads" flow. In both cases, "fully set up" means that an ad campaign has been created (not just that the Ads account has been connected).
The only way that global is getting set is firing the useAdsSetupCompleteCallback() hook here and here. Which make a POST request to the ads/setup/complete endpoint, resulting in the gla_ads_setup_completed_at option in the DB being set. Then, on a new page load, that option is checked to set the global via an inline script set here.
This means that as far as the plugin is concerned, you're not able to see and manage campaigns until you've created a new campaign after connecting the account (even if you reconnect an account you were previously using).
That limitation should probably be resolved, IMO, but that is well beyond the scope of what we're trying to solve here or in #2538. So for consistency, I think we'll keep useAdsCampaigns() hook as it was prior to our changes and make our new functionality for these issues work from the same assumption—i.e., that no campaigns are known until after a new campaign is created after connecting the Ads account.
I can log a new issue for the broader updates that we can look at coming back to.
Changes proposed in this Pull Request:
Closes #2539
Replace this with a good description of your changes & reasoning.
Screenshots:
When there is Product but no campaign: ( Show the notice )
When there is Product and a campaign: ( Don't show the notice )
Detailed test instructions:
Additional details:
Further more this ticket address a bug in
useAdsCampaignhttps://github.com/woocommerce/google-listings-and-ads/issues/2539#issuecomment-2397847396, which required certain E2E to be adjusted.Changelog entry