Fix Product Syncing via Feeds by mshymon · Pull Request #2841 · facebook/facebook-for-woocommerce

mshymon · 2024-12-17T11:38:36Z

Changes proposed in this Pull Request:

The current state of the product syncing via Feeds is broken - while plugin code base has business logic to generate feed file daily and serve the feed file on request, there is no logic for triggering a feed one time upload.

In this PR I am suggesting the following fixes to unblock product sync via feeds:

Trigger a daily feed upload sessions.
- Sending a one time create feed upload request to Meta would happen on feed file generation completion (this is to ensure we upload as feed file fresh data as possible).
- Added logic of choosing or creating the correct feed to which such feed upload would happen. This included local caching of the integrated feed ID, validating it is valid, choosing a feed from catalog or creating a new one.
Update Feed file content by:
- Adding GTIN to the feed file
- Adding quantity_to_sell_on_facebook to the feed file
- Adding rich_text_description to the feed file
Fixed incorrect values in the Feed file content:
- Fixed google_product_category (was empty)
- Fixed product_type (was incorrect, we were populating a GPC here)
- Fixed description (was incorrect values for variants)

Detailed test instructions:

Unit Tests
A. Run new tests:
- ./vendor/bin/phpunit --filter ApiTest
- ./vendor/bin/phpunit --filter fbproductTest
  B. Run all tests:
- npm run test:php
Manual Tests.
- Steps were added via an internal dogfooding doc and requires access to Meta internal tools to validate feed upload sessions status.

Changelog entry

Fix - Fixed feeds by requesting a feed file upload session after feed file is generated and added missing new fields to the feed file.

includes/Products/Feed.php

Jmencab · 2025-01-22T21:00:25Z

includes/Products/Feed.php

+					continue;
+				}
+
+				$woo_feed_name_option_1 = self::FEED_NAME;


suggest factor out to private function named something like private function pattern_match_feed_names()

includes/Products/Feed.php

tests/Unit/ApiTest.php

includes/Products/Feed.php

devbodaghe · 2025-01-24T11:00:27Z

includes/Products/Feed.php

+	private function validate_feed_exists($feed_id) {
+		$catalog_id = facebook_for_woocommerce()->get_integration()->get_product_catalog_id();
+		if ( '' === $catalog_id ) {
+			throw new Error( 'No catalog ID' );


Are the errors stored in the logs or was it a conscious decision to not add any logs here

This is a pattern I saw in the code for checking $catalog_id presence. This is mostly because we should not get into a state with no catalog.
But this is actually possible if user disconnects the plugin from a catalog (basically offboards).
This is a good use case to test our plugin behaviour overall, not just feed.

devbodaghe · 2025-01-24T11:04:41Z

includes/Products/Feed.php

+	 * 
+	 * @internal
+	 */
+	private function query_and_filter_integration_feed_id() {


This method seems like a lot of logic
Can we have it broken out a bit

devbodaghe · 2025-01-24T11:07:43Z

tests/Unit/ApiTest.php

 			$response->data
 		);
 	}
+


Can you add a test to check unhappy path
If errors are thrown or logs are generated

I think it won't bring value in this case, as we just mock response. Test with error mocks were added for other calls. that should be enough I think.

mshymon

Addressed the PR comments. Thanks everyone for reviewing the code!

includes/Products/Feed.php

mshymon · 2025-01-29T16:54:36Z

includes/Products/Feed.php

+	private function validate_feed_exists($feed_id) {
+		$catalog_id = facebook_for_woocommerce()->get_integration()->get_product_catalog_id();
+		if ( '' === $catalog_id ) {
+			throw new Error( 'No catalog ID' );


This is a pattern I saw in the code for checking $catalog_id presence. This is mostly because we should not get into a state with no catalog.
But this is actually possible if user disconnects the plugin from a catalog (basically offboards).
This is a good use case to test our plugin behaviour overall, not just feed.

includes/Products/Feed.php

mshymon · 2025-02-12T20:35:30Z

tests/Unit/ApiTest.php

 			$response->data
 		);
 	}
+


I think it won't bring value in this case, as we just mock response. Test with error mocks were added for other calls. that should be enough I think.

facebook-github-bot · 2025-02-13T16:55:58Z

@mshymon has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2025-02-17T11:37:18Z

@mshymon has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2025-02-17T17:49:25Z

@mshymon has updated the pull request. You must reimport the pull request before landing.

…d for SAOff.

facebook-github-bot · 2025-02-18T15:10:08Z

@mshymon has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2025-02-18T16:13:30Z

@mshymon merged this pull request in 08cb559.

Summary: ## Changelog: * Add - FB product video field to add videos. Also added products sync to support the video field with Batch API by gurtejrehal in #2874 * Tweak - tests for #2874 by gurtejrehal in #2888 * Tweak - tests for Product Update action as ramp up task by nealweiMeta in #2883 * Fix - translations loading before the init hook by iodic in #2866 * Fix - Fixed feeds by requesting a feed file upload session after feed file is generated and added missing new fields to the feed file by mshymon in #2841 Pull Request resolved: #2891 Reviewed By: akpurifb Differential Revision: D69836863 Pulled By: vahidkay-meta fbshipit-source-id: 32408ab4b42f60f921bd599ed982933017185b5e

Summary: ### Changes proposed in this Pull Request: Changes in detail here: https://docs.google.com/document/d/1PaXHKK1PRVtiBEwJtH_8ZP8h0UitDCXZnpFFDWo-eik/edit?tab=t.0#heading=h.m8p14s8600vn The main idea is to add a FeedManager singleton that will manage all the distinct Feed singletons. The Feed singletons will inherit from an AbstractFeed which will delegate the actual file creation to the Feed Handler or Feed Generator given the admin preferences. With that being said, the sequence will look similar to the existing product flow, but with the above simplifications: ![image](https://github.com/user-attachments/assets/1f0b3929-e4b5-4d37-bd53-6c3792e12a93) Closes # . Detailed test instructions: Added an API test as done in this PR: #2841 Integration test not yet possible as dependent on Mice framework addition and Ent creation Added comments for how to test/dogfood as well as included instructions in the above linked doc Successful upload via Feed Handler: https://www.internalfb.com/intern/cat/sessions/642042718535396/ Successful upload via Feed Generator: https://www.internalfb.com/intern/cat/sessions/642101398529528/ **See branch common_framework_test to see an example proof of concept that utilizes the base classes.** This will just include what will be checked in via diff. Unit Tests A. Run new tests: ./vendor/bin/phpunit --filter ApiTest B. Run all tests: npm run test:php Manual Tests. Steps were added via an internal dogfooding doc and requires access to Meta internal tools to validate feed upload sessions status. - [ ] Do the changed files pass `phpcs` checks? Please remove `phpcs:ignore` comments in changed files and fix any issues, or delete if not practical. Pull Request resolved: #2875 Reviewed By: carterbuce, nrostrow-meta Differential Revision: D69409593 Pulled By: Jmencab fbshipit-source-id: 8ec2feb7f1ac54d89c9a84f9e6f7ca31592f3f02

facebook-github-bot added the CLA Signed label Jan 15, 2025

Jmencab reviewed Jan 22, 2025

View reviewed changes

includes/Products/Feed.php Show resolved Hide resolved

Jmencab reviewed Jan 22, 2025

View reviewed changes

includes/Products/Feed.php Outdated Show resolved Hide resolved

Jmencab reviewed Jan 22, 2025

View reviewed changes

includes/Products/Feed.php Show resolved Hide resolved

halilozanakgul suggested changes Jan 23, 2025

View reviewed changes

devbodaghe reviewed Jan 24, 2025

View reviewed changes

includes/Products/Feed.php Show resolved Hide resolved

devbodaghe reviewed Jan 24, 2025

View reviewed changes

includes/Products/Feed.php Outdated Show resolved Hide resolved

devbodaghe reviewed Jan 24, 2025

View reviewed changes

includes/Products/Feed.php Outdated Show resolved Hide resolved

devbodaghe reviewed Jan 24, 2025

View reviewed changes

mshymon commented Feb 12, 2025

View reviewed changes

mshymon force-pushed the fix_feeds branch from 8cea74c to c9a076c Compare February 13, 2025 15:28

mshymon assigned vinkmeta Feb 13, 2025

vinkmeta approved these changes Feb 17, 2025

View reviewed changes

Marian Shymon and others added 11 commits February 18, 2025 15:09

WIP to fix feeds

d85dda8

Added GTIN to feed file.

3f62a54

Implemented creation of a new feed.

1c52118

Added local cache feed ID invalidation logic.

74ee4c6

Added descriptions to new functions.

ff7c5dc

Add quantity_to_sell_on_facebook and woo_product_id to the feed.

9f13ab3

Added tests to ApiTest.php

caeaef1

Added tests to fbproductTest.php

61b762e

Removed woo_product_id field, will be handled as seperate PR if neede…

cba18b9

…d for SAOff.

Fixed product_type and google_product_category

39b304d

Addressed PR comments.

9a8dfe7

mshymon added 4 commits February 18, 2025 15:09

Remove test after rebase.

5a81179

Fixed test test_enhanced_catalog_fields_from_attributes after rebase.

e47eae3

Added to rich_text_description feed.

098d24a

Fixed broken description via feeds for variable products.

51b031e

mshymon force-pushed the fix_feeds branch from aa5cac3 to 51b031e Compare February 18, 2025 15:10

facebook-github-bot closed this in 08cb559 Feb 18, 2025

facebook-github-bot added the Merged label Feb 18, 2025

vahidkay-meta mentioned this pull request Feb 19, 2025

Updating the version number to 3.4.0 #2891

Closed

Jmencab mentioned this pull request Mar 3, 2025

Common Feed Upload Framework #2875

Closed

1 task

Conversation

mshymon commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Detailed test instructions:

Changelog entry

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshymon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Feb 13, 2025

Uh oh!

facebook-github-bot commented Feb 17, 2025

Uh oh!

facebook-github-bot commented Feb 17, 2025

Uh oh!

facebook-github-bot commented Feb 18, 2025

Uh oh!

facebook-github-bot commented Feb 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mshymon commented Dec 17, 2024 •

edited

Loading