Feature/coupon syncer #1014

chennyxie · 2021-09-17T19:17:04Z

Closes #1008
GooglePromotionService is released in https://github.com/googleapis/google-api-php-client-services (v0.211.0 and above)
Note that createPromotion API works for create or update as long as using same promotion id. In addition, we use create with set effective dates as expired to disable the promotion.

Major files that are added:

Add hooks for listen coupon create/update/trash/delete
Add jobs to update/delete promotion on Google by calling CouponSyncer
Add CouponHelper and CouponMetaHelper to manage coupon sync status
Add CouponSyncer to manage direct call to GooglePromotionService.
Add WCCouponAdapter to map WC_Coupon to Google promotion.
Add coupon visibility metabox to provide visibility setting option in coupon edit page

Screenshots:

Detailed test instructions:

Before mc account setup, go to coupon edit page, check setup is link displayed.
Create or edit a coupon with select channel visibility as show on Google
Sumit the change and wait for sync status shows it started to sync
Trash the coupon and wait to confirm a request sent for disable promotion.

Changelog entry

jconroy · 2021-09-21T04:01:32Z

Thanks @chennyxie !

JPry · 2021-09-21T19:54:47Z

Thanks for the PR @chennyxie!

I noticed in the list of commits, there seem to be numerous additional commits besides the ones that you made. I would recommend rebasing against the develop branch so that only your changes are part of this PR. I'd be happy to help out with that if needed.

chennyxie · 2021-09-22T06:37:30Z

Hi, JPry,
I have rebased my changes based on the current develop branch, and this pull request is requesting to merge into the branch "woocommerce:feature/1008-coupon-integration", which has several commits behind the current "develop".

Do you think if it will help to update this "woocommerce:feature/1008-coupon-integration" branch to the current develop, so that only my commits are shown in this PR? thanks

JPry · 2021-09-22T12:52:20Z

Do you think if it will help to update this "woocommerce:feature/1008-coupon-integration" branch to the current develop, so that only my commits are shown in this PR?

@chennyxie Yes, I think this PR should target the woocommerce:develop branch instead of woocommerce:feature/1008-coupon-integration

jconroy · 2021-09-22T13:13:15Z

@JPry I set up this as a feature branch so that we can choose when to merge to develop and release

JPry · 2021-09-22T13:33:31Z

I set up this as a feature branch so that we can choose when to merge to develop and release

Ah, sorry, I wasn't aware of that. In that case, it definitely sounds like we should stick with the woocommerce:feature/1008-coupon-integration branch as a target.

That also means that the rebase should take place against the woocommerce:feature/1008-coupon-integration branch rather than the woocommerce:develop branch.

… for the triggerings

…d CouponSyncer

fix MC dependency in coupon visibility box fix

chennyxie · 2021-09-22T23:39:37Z

Hi, I have rebased this PR again according to the comments above. Now it should be ready for review. Thanks

…ices

jconroy · 2021-09-23T04:07:23Z

Thanks @chennyxie that looks better! Cheers 🍻

JPry

@chennyxie I've gone through and tested this functionality on my test site. Setting up a coupon is working well, and I can see that they sync to Google is "pending". Do you know how long it would typically take for the sync to occur?

On a first look, the logic looks to be sound in your code. I'll take another pass tomorrow to see if there's anything else to adjust. Until then, the biggest thing I've noticed is related to the project code style. I've added a few suggestions below, but it looks like some of the files need more comprehensive adjustments. Could you please make changes based on running ./vendor/bin/phpcs?

composer.json

src/Admin/MetaBox/CouponChannelVisibilityMetaBox.php

src/Admin/MetaBox/MetaBoxInterface.php

src/Coupon/CouponHelper.php

…ry to start ramup-up from US

… support.

…pter

jconroy · 2021-10-28T00:27:19Z

@JPry can you give the last few commits a once over to check they are all good. I noticed in 9f3b231#diff-55e830624fd3dfe830824f42db8d5cd251b37c1d637d837435cbd48f8d15e1dfL219 we might be losing some debugging

Then I think we can merge and get ready for release on Tuesday

nima-karimi · 2021-11-02T06:30:42Z

src/Coupon/CouponHelper.php

 	 */
 	public function mark_as_synced(
 		WC_Coupon $coupon,
-		string $google_id,
+		?string $google_id,


In which cases this can be a null value? Shouldn't all synced coupons have an ID?

This is to be compatible to the alpha version of content API and the id will be used in later version.

src/Coupon/CouponHelper.php

src/Coupon/WCCouponAdapter.php

src/Jobs/DeleteCoupon.php

src/Product/WCProductAdapter.php

layoutd · 2021-11-02T11:35:41Z

During some usage testing, a 403 error popped up:

This happened when trying to sync a coupon with a linked, existing Merchant Center account (that is, not a new Merchant Center account created under our MCA). My best guess is that the MCA can't manipulate promotions for linked accounts (unlike for sub-accounts), and since the promotions endpoint has "Limited Access" – which the existing accounts don't have – the operations fail.

Does this sound correct? Is there any workaround for this? Or will this access limitation be removed once the Promotions feature goes public?

nima-karimi

The code looks good to merge but I did leave a bunch of comments that need to be addressed in a subsequent PR. Justin has tested this and left a comment with the results here: #1014 (comment)

src/Product/WCProductAdapter.php

nima-karimi · 2021-11-02T12:35:15Z

src/Product/WCProductAdapter.php

+	 *
+	 * @return array
+	 */
+	public static function convert_product_types( $category_ids ): array {


It's probably best to make these methods (this one and get_google_product_offer_id) protected if we're not using them anywhere else. Especially given that this class isn't a utility class.

convert_product_types and get_google_product_offer_id are also used in WCCouponAdapter, the 2 adapters share some product info conversion method to guarantee product attributes matches the coupon restrictions related to product ids or product categories. We can consider to create a utility for adapters in the followup improvements.

Ah, okay then I would definitely vote for a utility class since that's not a functionality specific to the WCProductAdapter.

src/Product/WCProductAdapter.php

src/Jobs/DeleteCoupon.php

src/Google/InvalidCouponEntry.php

src/Coupon/CouponSyncer.php

chennyxie · 2021-11-02T20:45:05Z

Hi, yes, as the Google documentation says, currently promotion API is open for limited access, so a MC account that not managed by WooCommerce MCA are not open to use this alpha version, which should be resolved once we open the API to public.

During some usage testing, a 403 error popped up:

This happened when trying to sync a coupon with a linked, existing Merchant Center account (that is, not a new Merchant Center account created under our MCA). My best guess is that the MCA can't manipulate promotions for linked accounts (unlike for sub-accounts), and since the promotions endpoint has "Limited Access" – which the existing accounts don't have – the operations fail.

Does this sound correct? Is there any workaround for this? Or will this access limitation be removed once the Promotions feature goes public?

src/Product/WCProductAdapter.php

jconroy · 2021-11-02T22:25:35Z

Hi, yes, as the Google documentation says, currently promotion API is open for limited access, so a MC account that not managed by WooCommerce MCA are not open to use this alpha version, which should be resolved once we open the API to public.

Thanks @chennyxie just needed to confirm as the e2e tests note that "we made this case by disabling promotion API access from Google side" and weren't sure if we still had some access disabled etc.

chennyxie · 2021-11-04T14:28:55Z

Hi, yes, as the Google documentation says, currently promotion API is open for limited access, so a MC account that not managed by WooCommerce MCA are not open to use this alpha version, which should be resolved once we open the API to public.

Thanks @chennyxie just needed to confirm as the e2e tests note that "we made this case by disabling promotion API access from Google side" and weren't sure if we still had some access disabled etc.

Hi, the API access issue for the account linking of an existing MC account has been resolved now, be free to retry the test to verify it

layoutd · 2021-11-04T17:31:38Z

Hi, the API access issue for the account linking of an existing MC account has been resolved now, be free to retry the test to verify it

Hi @chennyxie, I just ran a quick test and it looks like the issue with linked accounts is fixed - they go to Sent to Google status. 🎉

However, I'm seeing an issue with coupons created in stores targeting a non-US country. When I set up a store targeting somewhere else (Spain, for example), and then create a coupon, it just sticks on Pending for sync:

In the log, it shows the job running (gla/jobs/update_coupon/process_item) and the log indicates that the coupon is being skipped:

DEBUG Automattic\WooCommerce\GoogleListingsAndAds\Coupon\CouponSyncer::update Skipping coupon (ID: 97) because it is not supported in main target country ES.

Should this message be displayed in the Coupon form (I thought I saw it at some point before)? Or should the coupon be updated even though the target country is non-US?

chennyxie · 2021-11-04T20:22:39Z

Hi, the API access issue for the account linking of an existing MC account has been resolved now, be free to retry the test to verify it

Hi @chennyxie, I just ran a quick test and it looks like the issue with linked accounts is fixed - they go to Sent to Google status. 🎉

However, I'm seeing an issue with coupons created in stores targeting a non-US country. When I set up a store targeting somewhere else (Spain, for example), and then create a coupon, it just sticks on Pending for sync:

In the log, it shows the job running (gla/jobs/update_coupon/process_item) and the log indicates that the coupon is being skipped:
DEBUG Automattic\WooCommerce\GoogleListingsAndAds\Coupon\CouponSyncer::update Skipping coupon (ID: 97) because it is not supported in main target country ES.
Should this message be displayed in the Coupon form (I thought I saw it at some point before)? Or should the coupon be updated even though the target country is non-US?

Hi, thanks for catching this. Seems I missed one commit(during mistakenly click fix on this PR page and which the conflict with my local committed fix) with a force push, I have added the commit back

jconroy · 2021-11-05T01:43:37Z

the API access issue for the account linking of an existing MC account has been resolved now, be free to retry the test to verify it

Thanks @chennyxie 👏

layoutd · 2021-11-08T17:07:21Z

👍🏻 I've done a bit more usage testing and functionally, it looks good:

Coupons targeting US are synced correctly for existing, USA and Global MC accounts
Coupons targeting non-US have syncing disabled
Different coupon types and details (restrictions, limits) are synced correctly
Coupons are deleted correctly
Coupons expire correctly

I'm going to merge this PR into the feature/1008-coupon-integration branch as the first step towards releasing it in version 1.6.0 #1084. Thanks @chennyxie 👏🏻

jconroy requested a review from a team September 21, 2021 04:02

chennyxie force-pushed the feature/coupon-syncer branch 2 times, most recently from 3a5eb0b to e335d33 Compare September 22, 2021 06:19

jconroy assigned chennyxie Sep 22, 2021

chennyxie added 14 commits September 22, 2021 23:32

Register coupon syncerhooks with coupon update job

676c806

impl coupon syncer hooks with triggerring a coupon update job

534f4a9

add unit test for coupon syncerhooks for update and creation

b2d1bf3

add DeleteCoupon job(unimplemented) in SyncerHooks and add unit tests…

6d7a1c0

… for the triggerings

Add DeleteCoupon job(unimplemented)

f351c48

Add and test coupon update job triggered from untrash

8628a68

add CouponChannelVisibilityMetaBox, CouponMetaHandler CouponHepler an…

902ad7d

…d CouponSyncer

add unit tests of couopn meta handler.

739f2a5

add coupon_channel_visibilty view.

e77c140

finish coupon syncer to delete coupon, and add unit tests

237d212

fix unit tests for SyncerHooks and CouponSyncer

64338b6

add unit tests for coupon delete and fix the tests

91fe7d3

Add promotion supported country list

3147061

fix visibility box injection

d51dc81

fix MC dependency in coupon visibility box fix

chennyxie force-pushed the feature/coupon-syncer branch from e335d33 to d51dc81 Compare September 22, 2021 23:38

fix and update the composer version for package:google/apiclient-serv…

4ae0ec3

…ices

chennyxie removed their assignment Sep 23, 2021

JPry suggested changes Sep 23, 2021

View reviewed changes

chennyxie added 4 commits October 7, 2021 07:40

Fix args serialization in delete coupon job

2869752

Fix product inclusion/exclusion for coupon to Google promotion mapping

ceca94e

Fix coupon visibility box sync status style

122c7f4

Config promotion destination ids and update promotion supported count…

e4425ef

…ry to start ramup-up from US

chennyxie force-pushed the feature/coupon-syncer branch from 1441d16 to e4425ef Compare October 7, 2021 08:39

chennyxie added 3 commits October 12, 2021 00:12

fix product restriction setting in promo adapter

cb78a16

Update google lib version and add coupon product category restriction…

37d093d

… support.

refactor to move all disable promotion related logic into WCCouponAda…

4891b19

…pter

chennyxie force-pushed the feature/coupon-syncer branch from 587ae2f to 4891b19 Compare October 14, 2021 17:14

Refactor to optimize product type coversion for GoogleProduct.

9f3b231

nima-karimi reviewed Nov 2, 2021

View reviewed changes

nima-karimi approved these changes Nov 2, 2021

View reviewed changes

jconroy reviewed Nov 2, 2021

View reviewed changes

src/Product/WCProductAdapter.php Show resolved Hide resolved

chennyxie force-pushed the feature/coupon-syncer branch from 238fecf to 4891b19 Compare November 3, 2021 06:13

chennyxie added 2 commits November 3, 2021 07:07

Fix the code syltes and add comments for coupon deletion logic

f3af796

refactor to clean up unnecessary delete flag in WCCouponAdapter

27a0b65

Disable channel visibility for countries not open to upload promotion

2a4def9

layoutd mentioned this pull request Nov 8, 2021

Release 1.6.0 #1084

Merged

layoutd merged commit 23c1d29 into woocommerce:feature/1008-coupon-integration Nov 8, 2021

layoutd mentioned this pull request Nov 8, 2021

Feature/1008 coupon integration #1086

Merged

jconroy mentioned this pull request Nov 18, 2021

WooCommerce Coupon Integration #1008

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/coupon syncer #1014

Feature/coupon syncer #1014

chennyxie commented Sep 17, 2021 •

edited

jconroy commented Sep 21, 2021

JPry commented Sep 21, 2021 •

edited

chennyxie commented Sep 22, 2021

JPry commented Sep 22, 2021

jconroy commented Sep 22, 2021

JPry commented Sep 22, 2021

chennyxie commented Sep 22, 2021

jconroy commented Sep 23, 2021

JPry left a comment

jconroy commented Oct 28, 2021 •

edited

nima-karimi Nov 2, 2021

chennyxie Nov 3, 2021

layoutd commented Nov 2, 2021

nima-karimi left a comment

nima-karimi Nov 2, 2021

chennyxie Nov 3, 2021

nima-karimi Nov 3, 2021

chennyxie commented Nov 2, 2021

jconroy commented Nov 2, 2021

chennyxie commented Nov 4, 2021

layoutd commented Nov 4, 2021

chennyxie commented Nov 4, 2021

jconroy commented Nov 5, 2021

layoutd commented Nov 8, 2021

Feature/coupon syncer #1014

Feature/coupon syncer #1014

Conversation

chennyxie commented Sep 17, 2021 • edited

Screenshots:

Detailed test instructions:

Changelog entry

jconroy commented Sep 21, 2021

JPry commented Sep 21, 2021 • edited

chennyxie commented Sep 22, 2021

JPry commented Sep 22, 2021

jconroy commented Sep 22, 2021

JPry commented Sep 22, 2021

chennyxie commented Sep 22, 2021

jconroy commented Sep 23, 2021

JPry left a comment

Choose a reason for hiding this comment

jconroy commented Oct 28, 2021 • edited

nima-karimi Nov 2, 2021

Choose a reason for hiding this comment

chennyxie Nov 3, 2021

Choose a reason for hiding this comment

layoutd commented Nov 2, 2021

nima-karimi left a comment

Choose a reason for hiding this comment

nima-karimi Nov 2, 2021

Choose a reason for hiding this comment

chennyxie Nov 3, 2021

Choose a reason for hiding this comment

nima-karimi Nov 3, 2021

Choose a reason for hiding this comment

chennyxie commented Nov 2, 2021

jconroy commented Nov 2, 2021

chennyxie commented Nov 4, 2021

layoutd commented Nov 4, 2021

chennyxie commented Nov 4, 2021

jconroy commented Nov 5, 2021

layoutd commented Nov 8, 2021

chennyxie commented Sep 17, 2021 •

edited

JPry commented Sep 21, 2021 •

edited

jconroy commented Oct 28, 2021 •

edited