Add "Create a new campaign" modal #37044

ecgan · 2023-03-02T18:44:39Z

All Submissions:

Have you followed the WooCommerce Contributing guideline?
Does your code follow the WordPress' coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Closes #34909. Previous draft PR and discussions are in PR #36130.

This PR implements the "Create a new campaign" modal.

The implemented modal here is different from the figma designs in #34909. Specifically, there are no borders within the modal component. This is to be more consistent with the WordPress Modal component which no longer has border under the modal title since PR WordPress/gutenberg#40781.

The new campaign types data is loaded from GET /marketing/campaign-types API. The API is called when the Campaigns card is rendered, so that when users click on the "Create new campaign" button, the data is already there and no additional waiting is required.

How to test the changes in this Pull Request:

Have you followed the Writing high-quality testing instructions guide?

Pre-conditions:

Enable WooCommerce Multichannel Marketing experience in WooCommerce Settings > Advanced > Features page: /wp-admin/admin.php?page=wc-settings&tab=advanced&section=features
Installed and activated a marketing channel, such as:
- Google Listings and Ads (Note: you may need to use a filter, see PR Add feature flag for Multichannel Marketing Integration google-listings-and-ads#1904).
- Pinterest for WooCommerce (see PR Multichannel Marketing Integration - Basic details and methods to report feed status pinterest-for-woocommerce#705).

Go to WooCommerce Marketing page: /wp-admin/admin.php?page=wc-admin&path=%2Fmarketing
Click on the "Create new campaign" button in the Campaigns card.
You should see the modal like the screenshot above.

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you created a changelog file for each project being changed, ie pnpm --filter=<project> changelog add?
Have you included testing instructions?

FOR PR REVIEWER ONLY:

I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

…igin.

…name.

…eader.

…aign modal.

…paigns-card' into feature/34903-multichannel-marketing-frontend/34909-create-campaign-modal Conflicts: plugins/woocommerce-admin/client/marketing/hooks/index.ts plugins/woocommerce-admin/client/marketing/overview-multichannel/Campaigns/Campaigns.tsx plugins/woocommerce-admin/client/marketing/overview-multichannel/Channels/Channels.tsx plugins/woocommerce-admin/client/marketing/overview-multichannel/Channels/RecommendedChannels.tsx plugins/woocommerce-admin/client/marketing/overview-multichannel/Channels/useChannels.ts

…909-marketing-create-campaign-modal Conflicts: plugins/woocommerce-admin/client/marketing/components/RecommendedChannelsList.tsx plugins/woocommerce-admin/client/marketing/overview-multichannel/Campaigns/Campaigns.scss plugins/woocommerce-admin/client/marketing/overview-multichannel/Campaigns/Campaigns.tsx plugins/woocommerce-admin/client/marketing/overview-multichannel/Channels/Channels.tsx plugins/woocommerce-admin/client/marketing/overview-multichannel/Channels/RecommendedChannels.tsx

…909-marketing-create-campaign-modal Conflicts: plugins/woocommerce-admin/client/marketing/data-multichannel/action-types.ts plugins/woocommerce-admin/client/marketing/data-multichannel/actions.ts plugins/woocommerce-admin/client/marketing/data-multichannel/resolvers.ts

The previous text "Create a new campaign" can be a bit confusing with the other text "Create new campaign" in the test.

The code is copied from the Channels component, but in CreateNewCampaignModal, we don't need to display the divider, so we can just remove the divider code.

This helps us save indentation and make the code few lines shorter.

…nModal.

ecgan · 2023-03-09T18:58:11Z

@eason9487 , thanks for the thorough review, great eyes as always! 😀 👍 I have addressed your code review comments in this PR, this is ready for another round of review. 🙏

The following is about the 6 points mentioned in your code review summary in #37044 (review). Thanks so much for bringing them up!

It will show a "0" when no campaigns.

I'll fix it in a separate PR. I'm thinking to check through the marketing code base to make sure this is handled correctly. We recently just fixed a similar issue #37083.

Same as the getCampaignTypes reducer, the other three reducers also have the same concern of re-throwing errors.

This is related to #37044 (comment). The problem is I don't see the issue that you are seeing. We can talk about it in that comment thread.

Although not causing actual problems at this moment, there is a concern about incorrect state caching within useCampaigns, which could lead to incorrect results in the future for standalone components like Campaigns.

Agree. I actually thought of doing some changes with useCampaigns. I feel like useCampaigns is doing too much with useRegisteredChannels in it, I want to make it leaner and remove "channel" things. I'm thinking to work with what we have now and change it later.

After installing or activating an extension in MarketingOverviewMultichannel, the refetch doesn't trigger the data refetching for getRegisteredChannels.

I couldn't reproduce this, it works as expected for me. 🤔 See my demo video below.

demo-pr-activate-channel-refetch.mp4

In the relevant processing of getCampaigns in wp-data store and useCampaigns hook, they don't include perPage as a part of the data key. It will lead to incorrect data and states if other implementations use a different perPage to request data someday.

Good point! I guess I can make the perPage as a part of the key, or don't expose perPage as a parameter 😂 . I'll create a separate issue and label it as low priority since this isn't a major issue now.

After activating GLA via the Channels card, the GLA item in the Installed extensions card is still remaining unactivated.

This should not be a problem with issue #34907 and its PR #37126, because the extension should only appear in one card, it shouldn't appear in both Channels card and Installed extensions card.

eason9487 · 2023-03-10T08:53:28Z

4. After installing or activating an extension in MarketingOverviewMultichannel, the refetch doesn't trigger the data refetching for getRegisteredChannels.

I couldn't reproduce this, it works as expected for me.

Just realized it's because of different WP versions. See #37044 (comment) and #37044 (comment).

Considering the latest WC still states it supports WP 5.9, I guess it would still need to add the second parameter to the invalidateResolution call in useRegisteredChannels at this moment.

eason9487

Thanks for the updates.

The two remaining concerns are about the compatibility with WP 5.9:

About the remaining issues mentioned in the main content of this #37044 (review), I also think they could be addressed in separate PRs. 👍

In WP 5.9, if we throw error, there would be an uncaught promise, and it causes UI to break.

This is to make it work with WP 5.9. See #37044 (comment).

ecgan · 2023-03-10T18:46:58Z

After installing or activating an extension in MarketingOverviewMultichannel, the refetch doesn't trigger the data refetching for getRegisteredChannels.

I couldn't reproduce this, it works as expected for me.

Just realized it's because of different WP versions. See #37044 (comment) and #37044 (comment).

Considering the latest WC still states it supports WP 5.9, I guess it would still need to add the second parameter to the invalidateResolution call in useRegisteredChannels at this moment.

Got it. I have created a separate issue #37176 for this.

It will show a "0" when no campaigns.

I'll fix it in a separate PR. I'm thinking to check through the marketing code base to make sure this is handled correctly. We recently just fixed a similar issue #37083.

Created issue #37175.

In the relevant processing of getCampaigns in wp-data store and useCampaigns hook, they don't include perPage as a part of the data key. It will lead to incorrect data and states if other implementations use a different perPage to request data someday.

Good point! I guess I can make the perPage as a part of the key, or don't expose perPage as a parameter 😂 . I'll create a separate issue and label it as low priority since this isn't a major issue now.

Created issue #37177.

@eason9487 , all the code review comments are addressed. This is ready for another round of review. 🙂 🙏

eason9487

Thanks for the further adjustments! LGTM.

ecgan added 24 commits December 20, 2022 01:17

Add CampaignsCardHeader component with modal.

0866135

Remove unneeded imports in RecommendedChannelsList.

3399874

Move RecommendedChannelsList to shared components directory.

6de95c7

Move useRecommendedChannels to shared hooks directory.

5f50e63

Display recommended channels in create campaign modal.

17f2072

Hide the recommended channels section if it is empty.

24d59f7

Display new campaign types in create campaign modal.

6b3fbdf

Make create campaign button open in new tab depending on createUrl or…

5caffb6

…igin.

Code refactor with CreateNewCampaignModal.

894b77d

Better code clarity with woocommerce-marketing-campaigns-table class …

321145e

…name.

Use perPage variable instead of duplicated number.

a3c8680

Code refactor for Campaigns component, remove unneeded CampaignsCardH…

07894d4

…eader.

Add border and change padding for recommended channels in create camp…

d0de1b5

…aign modal.

Fix build error after merging.

418e4fb

wp.data for getting new campaign types.

9526608

Make UI work with wp.data new campaign types.

d111370

Simplify wp.data for receiveCampaignTypes.

8132add

Simplify receiveCampaignTypes.

51a5a18

Move useNewCampaignTypes to shared hooks directory.

e131128

Load campaign types in Campaigns card.

ec15697

Refetch registered channels after installing channel in Campaigns card.

913f717

ecgan added the focus: marketing Marketing page in WooCommerce Admin, i.e. `/wp-admin/admin.php?page=wc-admin&path=%2Fmarketing`. label Mar 2, 2023

ecgan self-assigned this Mar 2, 2023

github-actions bot added the focus: react admin label Mar 2, 2023

ecgan mentioned this pull request Mar 2, 2023

Add "Create a new campaign" modal #36130

Closed

7 tasks

ecgan requested review from a team March 2, 2023 18:57

ecgan added 12 commits March 9, 2023 21:52

Change NewCampaignTypes to CampaignTypes.

f19969d

Remove unneeded getByText in Campaigns test.

551db21

Make the mocked modal clearer.

e831e89

The previous text "Create a new campaign" can be a bit confusing with the other text "Create new campaign" in the test.

Rename open and setOpen to isModalOpen and setModalOpen in Campaigns.

f5bb849

Make it clear that title and classname cannot be overwritten by props.

c6dcee4

Remove unneeded CardDivider in CreateNewCampaignModal.

e15305b

The code is copied from the Channels component, but in CreateNewCampaignModal, we don't need to display the divider, so we can just remove the divider code.

Remove unneeded return in CreateNewCampaignModal.

9f0326b

This helps us save indentation and make the code few lines shorter.

Refactor test data in CreateNewCampaignModal tests.

29d67a3

Refetch campaign types after installing a channel in CreateNewCampaig…

8b6aad4

…nModal.

Load campaign types in overview and refetch after installeing a channel.

6446053

Display info message when there are no campaign types.

e4da8cb

Remove unneeded div in CreateNewCampaignModal.

0a58ec1

eason9487 reviewed Mar 10, 2023

View reviewed changes

nima-karimi mentioned this pull request Mar 10, 2023

Return the URL to setup ads account in MCM dashboard if it is not already set up woocommerce/google-listings-and-ads#1922

Closed

ecgan added 2 commits March 11, 2023 00:31

Don't throw error in getCampaignTypes.

1fea018

In WP 5.9, if we throw error, there would be an uncaught promise, and it causes UI to break.

Specify empty array dependency in useCampaignTypes.

c6e6f87

This is to make it work with WP 5.9. See #37044 (comment).

This was referenced Mar 10, 2023

Make sure expression before && is always boolean in JSX #37175

Closed

Specify empty array [] as invalidateResolution args to support WP 5.9 #37176

Closed

useCampaigns hook cannot be used with multiple instances #37177

Closed

ecgan requested a review from eason9487 March 10, 2023 18:47

eason9487 approved these changes Mar 13, 2023

View reviewed changes

Merge branch 'trunk' into feature/34909-marketing-create-campaign-modal

0cf5677

ecgan merged commit 7bf2ced into trunk Mar 13, 2023
27 of 29 checks passed

ecgan deleted the feature/34909-marketing-create-campaign-modal branch March 13, 2023 05:24

github-actions bot added this to the 7.6.0 milestone Mar 13, 2023

This was referenced Mar 13, 2023

Specify args for wp.data resolution in marketing page to support WP 5.9 #37198

Merged

Make sure expression before && is always boolean in React rendering in marketing page #37227

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "Create a new campaign" modal #37044

Add "Create a new campaign" modal #37044

ecgan commented Mar 2, 2023 •

edited

ecgan commented Mar 9, 2023

eason9487 commented Mar 10, 2023

eason9487 left a comment

ecgan commented Mar 10, 2023 •

edited

eason9487 left a comment

Add "Create a new campaign" modal #37044

Add "Create a new campaign" modal #37044

Conversation

ecgan commented Mar 2, 2023 • edited

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

FOR PR REVIEWER ONLY:

ecgan commented Mar 9, 2023

eason9487 commented Mar 10, 2023

eason9487 left a comment

Choose a reason for hiding this comment

ecgan commented Mar 10, 2023 • edited

eason9487 left a comment

Choose a reason for hiding this comment

ecgan commented Mar 2, 2023 •

edited

ecgan commented Mar 10, 2023 •

edited