2504: Add skip paid ads confirmation modal. #2563

asvinb · 2024-08-23T13:34:25Z

Changes proposed in this Pull Request:

Closes #2504

Replace this with a good description of your changes & reasoning.

Screenshots:

Detailed test instructions:

Go through the onboarding flow.
On the fourth step, click on the "Skip this step for now" button.
There should be a modal asking the user to confirm if they want to skip.
If the user clicks "Yes", the onboarding flow continues and the user is redirected to the dashboard.
If the user clicks "No", the modal is no longer displayed and the user stays on the page.

Additional details:

Changelog entry

Add - Confirmation modal when user skips without creating a campaign during onboarding.

codecov · 2024-08-23T13:37:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.8%. Comparing base (315fce2) to head (5ec87d6).
Report is 11 commits behind head on feature/2459-campaign-creation-flow.

Additional details and impacted files

@@                           Coverage Diff                            @@
##             feature/2459-campaign-creation-flow   #2563      +/-   ##
========================================================================
- Coverage                                   65.0%   63.8%    -1.2%     
========================================================================
  Files                                        475     326     -149     
  Lines                                      17900    5089   -12811     
  Branches                                       0    1232    +1232     
========================================================================
- Hits                                       11640    3247    -8393     
+ Misses                                      6260    1674    -4586     
- Partials                                       0     168     +168

Flag	Coverage Δ
js-unit-tests	`63.8% <ø> (?)`
php-unit-tests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

see 801 files with indirect coverage changes

asvinb · 2024-08-23T13:41:43Z

js/src/setup-mc/setup-stepper/setup-paid-ads/setup-paid-ads.js

+						? finishOnboardingSetup
+						: handleShowSkipPaidAdsConfirmationModal
+				}
+				// TODO: might want to review eventName and eventProps


@joemcgill Shall we keep those props as is?

Good question. I think we'll want to move that eventName to the new "Complete setup" button in the AYS modal, but the event name for this will likely need an update. Leave the TODO: comment for now and we can address that before the feature is merged.

asvinb · 2024-08-23T13:42:02Z

js/src/setup-mc/setup-stepper/setup-paid-ads/skip-paid-ads-confirmation-modal.js

+ * Triggered when the skip button is clicked
+ * // TODO: to review
+ *
+ * @event gla_skip_paid_ads_modal_confirm_button_click


@joemcgill The event name TBC.

asvinb · 2024-08-23T13:42:18Z

js/src/setup-mc/setup-stepper/setup-paid-ads/skip-paid-ads-confirmation-modal.js

+				<AppButton
+					key="yes"
+					// TODO: confirm the eventName
+					eventName="gla_skip_paid_ads_modal_confirm_button_click"


@joemcgill If you can confirm please :)

asvinb · 2024-08-23T13:42:30Z

js/src/setup-mc/setup-stepper/setup-paid-ads/skip-paid-ads-confirmation-modal.js

+						Link: (
+							<AppDocumentationLink
+								href="https://support.google.com/google-ads/answer/10724817"
+								// TODO: review context and linkId values


@joemcgill Same here.

joemcgill

The functionality is working correctly when the "Skip..." button you've targeted is clicked, but this needs to be updated so this applies to all "Skip" buttons. Left feedback inline.

joemcgill · 2024-08-26T22:01:50Z

js/src/setup-mc/setup-stepper/setup-paid-ads/setup-paid-ads.js

+					showPaidAdsSetup
+						? finishOnboardingSetup
+						: handleShowSkipPaidAdsConfirmationModal


Oh, it looks like there was confusion here. We're removing the initial "Skip this step for now" button from the PaidAdsFeaturesSection in #2500. This AYS modal should apply to any skip button (although there will only be the "Skip paid ads creation" button once that PR is merged).

👍 I've added TODOs to review the code once #2500 has been merged.

joemcgill · 2024-08-26T22:06:47Z

js/src/setup-mc/setup-stepper/setup-paid-ads/setup-paid-ads.js

+						? finishOnboardingSetup
+						: handleShowSkipPaidAdsConfirmationModal
+				}
+				// TODO: might want to review eventName and eventProps


Good question. I think we'll want to move that eventName to the new "Complete setup" button in the AYS modal, but the event name for this will likely need an update. Leave the TODO: comment for now and we can address that before the feature is merged.

joemcgill · 2024-08-26T22:08:34Z

js/src/setup-mc/setup-stepper/setup-paid-ads/skip-paid-ads-confirmation-modal.js

+			title={ __( 'Skip setting up ads?', 'google-listings-and-ads' ) }
+			buttons={ [
+				<AppButton key="no" isSecondary onClick={ onRequestClose }>
+					{ __( 'No', 'google-listings-and-ads' ) }


Reference the Modal Content in the issue description, these buttons should "Cancel" and "Complete setup". I've updated the A/C to make that more clear.

joemcgill

This looks like it's working well. I'm not 100% confident in the way events need to be recorded, so we can confirm with @eason9487 and either update this PR after his review or spin up a follow-up issue to update all event tracking code for #2459.

ankitguptaindia · 2024-08-30T13:30:40Z

Suggested some QA feedback on the connected issue here: #2504 (comment)

Next Step:
Moving PR to in-progress for feedback.

joemcgill · 2024-09-01T18:13:57Z

Thanks @ankitguptaindia I've left a response to your feedback in #2504 (comment). @asvinb Can you make the suggested text change to the modal button?

asvinb · 2024-09-02T08:50:52Z

Sure thing @joemcgill . Label updated @ankitguptaindia . Can you kindly check?

ankitguptaindia · 2024-09-03T15:28:27Z

QA/Test Report- ✅

Testing Environment -

WordPress: 6.6.1
Theme active on store: Twenty Twenty-Four Version: 1.2
WooCommerce - Version 9.2.3
PHP: 8.3
Web Server: Nginx
Browser: Chrome - Version 127
OS: macOS Sonoma 14.6.1

Test Results - The changes have been implemented as described. A confirmation modal will appear when the user clicks on ski ads. If the user clicks 'Cancel,' they remain on the same page. If the user clicks 'Complete setup without setting up ads,' the onboarding flow continues.

Functional Demo / Screencast -

Recording.811.mp4

eason9487

Thanks for the work.

Left some suggestions about reducing the complexity. Besides, the e2e testing couldn't pass - https://github.com/woocommerce/google-listings-and-ads/actions/runs/10697346716/job/29654336938

js/src/setup-mc/setup-stepper/setup-paid-ads/setup-paid-ads.js

js/src/setup-mc/setup-stepper/setup-paid-ads/skip-paid-ads-confirmation-modal.js

:

asvinb · 2024-09-04T15:43:31Z

Thanks for the review @eason9487 . I fixed the failing E2E test which was after we changed the button label in the modal. Should be ok now:
https://github.com/woocommerce/google-listings-and-ads/actions/runs/10704832892

eason9487

Thanks for reducing the complexity. LGTM.

Left a few minor things to be tweaked and I believe it should not need another round of code reviews after adjusting them.

tests/e2e/utils/pages/setup-mc/step-4-complete-campaign.js

js/src/setup-mc/setup-stepper/setup-paid-ads/setup-paid-ads.js

asvinb · 2024-09-05T13:00:14Z

@eason9487 PR has been updated and ready for your final 👍

eason9487

LGTM

asvinb added 4 commits August 22, 2024 19:59

Add SkipPaidAdsConfirmationModal component.

c035bdc

Use correct function to complete onboarding.

0bea89e

Rename functions.

4d81a64

Update e2e tests.

d1182b5

asvinb requested a review from joemcgill August 23, 2024 13:34

github-actions bot added type: enhancement The issue is a request for an enhancement. changelog: add A new feature, function, or functionality was added. labels Aug 23, 2024

asvinb commented Aug 23, 2024

View reviewed changes

joemcgill linked an issue Aug 26, 2024 that may be closed by this pull request

Campaign Creation Overview #2459

Open

12 tasks

joemcgill requested changes Aug 26, 2024

View reviewed changes

joemcgill linked an issue Aug 27, 2024 that may be closed by this pull request

Campaign Creation: Add confirmation modal when user skips without creating a campaign #2504

Open

3 tasks

joemcgill removed a link to an issue Aug 27, 2024

Campaign Creation Overview #2459

Open

12 tasks

Address code review feedback.

2180c5f

joemcgill approved these changes Aug 28, 2024

View reviewed changes

ankitguptaindia self-requested a review August 30, 2024 11:18

Update label.

bdb300f

ankitguptaindia approved these changes Sep 3, 2024

View reviewed changes

eason9487 requested changes Sep 4, 2024

View reviewed changes

asvinb added 3 commits September 4, 2024 19:09

Address CR feedback.

afc0554

Remove comment.

7c4df64

:

Fix E2E tests.

b884b97

eason9487 approved these changes Sep 5, 2024

View reviewed changes

tests/e2e/utils/pages/setup-mc/step-4-complete-campaign.js Outdated Show resolved Hide resolved

tests/e2e/utils/pages/setup-mc/step-4-complete-campaign.js Outdated Show resolved Hide resolved

js/src/setup-mc/setup-stepper/setup-paid-ads/setup-paid-ads.js Show resolved Hide resolved

macka61 assigned asvinb and unassigned asvinb Sep 5, 2024

Fix unecessary page wait.

5ec87d6

eason9487 approved these changes Sep 6, 2024

View reviewed changes

joemcgill merged commit 057ff88 into feature/2459-campaign-creation-flow Sep 9, 2024
8 checks passed

joemcgill deleted the add/2504-skip-confirmation-modal branch September 9, 2024 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2504: Add skip paid ads confirmation modal. #2563

2504: Add skip paid ads confirmation modal. #2563

asvinb commented Aug 23, 2024 •

edited by eason9487

Loading

codecov bot commented Aug 23, 2024 •

edited

Loading

asvinb Aug 23, 2024

joemcgill Aug 26, 2024

asvinb Aug 28, 2024

asvinb Aug 23, 2024

asvinb Aug 23, 2024

asvinb Aug 23, 2024

joemcgill left a comment

joemcgill Aug 26, 2024

asvinb Aug 28, 2024

joemcgill Aug 26, 2024

joemcgill Aug 26, 2024

asvinb Aug 28, 2024

joemcgill left a comment

ankitguptaindia commented Aug 30, 2024

joemcgill commented Sep 1, 2024

asvinb commented Sep 2, 2024

ankitguptaindia commented Sep 3, 2024

eason9487 left a comment

asvinb commented Sep 4, 2024

eason9487 left a comment

asvinb commented Sep 5, 2024

eason9487 left a comment

2504: Add skip paid ads confirmation modal. #2563

2504: Add skip paid ads confirmation modal. #2563

Conversation

asvinb commented Aug 23, 2024 • edited by eason9487 Loading

Changes proposed in this Pull Request:

Screenshots:

Detailed test instructions:

Additional details:

Changelog entry

codecov bot commented Aug 23, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

ankitguptaindia commented Aug 30, 2024

joemcgill commented Sep 1, 2024

asvinb commented Sep 2, 2024

ankitguptaindia commented Sep 3, 2024

QA/Test Report- ✅

eason9487 left a comment

Choose a reason for hiding this comment

asvinb commented Sep 4, 2024

eason9487 left a comment

Choose a reason for hiding this comment

asvinb commented Sep 5, 2024

eason9487 left a comment

Choose a reason for hiding this comment

asvinb commented Aug 23, 2024 •

edited by eason9487

Loading

codecov bot commented Aug 23, 2024 •

edited

Loading