Fix inconsistency between enhanced and legacy flow#3285
Closed
jczhuoMeta wants to merge 6 commits intomainfrom
Closed
Fix inconsistency between enhanced and legacy flow#3285jczhuoMeta wants to merge 6 commits intomainfrom
jczhuoMeta wants to merge 6 commits intomainfrom
Conversation
Contributor
📦 Latest Plugin BuildBuilt at: 2025-05-30T19:38:07.007Z Download: Click here to download the plugin To download: Click the link above → Scroll to bottom → Download "facebook-for-woocommerce" artifact |
Contributor
|
@jczhuoMeta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Contributor
|
@jczhuoMeta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Contributor
|
@jczhuoMeta merged this pull request in 17c63e4. |
carterbuce
pushed a commit
that referenced
this pull request
May 30, 2025
Summary: ## TL;DR We found some issue caused by the inconsistency between the legacy and enhanced flow. The real world uses are unlikely to have the same serious issues. This PR put out a graceful fix it them. ## Description During dogfooding and QA testing, we get reports of: 1. Should show legacy view but rendered with BROKEN MiCE page 2. Used the enhanced flow onboard but seeing the legacy view after onboarding. The causes has been identified as: 1. The CPI ID didn't get cleared out when off boarding using legacy flow. This has been fixed in https://github.com/facebook/facebook-for-woocommerce/pull/3262/files but for existing instances off boarded before the fix landed, it's still experience problems 2. Some fields(e.g. business id) were missed during saving the onboarding result, causing MiCE failed to render. ### Issue 1 It's mainly impacting QA and internal users, for public users, the only chance they have a stale CPI ID and seeing this issue only if the performed the exact actions: 1. They upgraded to 3.4.8, the CPI was repaired in the background. 2. They disconnected their shop, staled CPI stay in their DB 3. They reconnected their shop 4. They upgraded 3.5.0, staled CPI causing their MiCE rendering to be all broken However, since this will cause broken MiCE page, this PR introduces graceful and robust logic to ensure the CPI is correct: a. Using MBE response as source of truth, if the response from Meta does not have CPI ID, then clear the local CPI ID. This ensures no stale CPI ID. b. The CPI repair endpoint will fetch or repair, get the correct CPI ID The logic is implemented in refresh_installation_data heartbeat will fix the data inconsistency automatically. ### Issue 2 It wasn't surfaced frequently as the refresh_installation_data is also repairing the missing IDs in the background. This PR added the saving action of those missing fields. ### Type of change - Bug fix (non-breaking change which fixes an issue) ## Checklist - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have confirmed that my changes do not introduce any new PHPCS warnings or errors. - [x] I have checked plugin debug logs that my changes do not introduce any new PHP warnings or FATAL errors. - [x] I followed general Pull Request best practices. Meta employees to follow this [wiki]([url](https://fburl.com/wiki/2cgfduwc)). - [x] I have added tests (if necessary) and all the new and existing unit tests pass locally with my changes. - [x] I have completed dogfooding and QA testing, or I have conducted thorough due diligence to ensure that it does not break existing functionality. - [x] I have updated or requested update to plugin documentations (if necessary). Meta employees to follow this [wiki]([url](https://fburl.com/wiki/nhx73tgs)). ## Changelog entry Fix onboarding and offboarding data inconsistency between legacy and enhanced connection view. Pull Request resolved: #3285 Test Plan: No unit test can cover it since it's caused by a sequence of actions. All existing tests passes: Time: 00:28.917, Memory: 121.00 MB OK (324 tests, 859 assertions) ### Manual test it to cover all the cases. #### Before Verify on 3.5.0 branch, the business manager id was missing: <img width="1004" alt="image" src="https://github.com/user-attachments/assets/3f153d93-8d01-415e-bd15-ffef00ef7230" /> The cpi id exists: <img width="807" alt="image" src="https://github.com/user-attachments/assets/6b2abf18-bf61-4151-9168-2a97593bd6c6" /> and should be cleaned during uninstall since current 3.5.0 already included the cleanup fix(https://github.com/facebook/facebook-for-woocommerce/pull/3262/files), however, still manually make a copy so we can verify a lingering cpi_id won't break things. #### After Disconnect and switch to the current branch: After onboarding, we now have business ID populated: <img width="955" alt="image" src="https://github.com/user-attachments/assets/c9e16e73-1334-4e12-a50d-4249e70a9a96" /> Now use the wrong CPI id to make sure the installation can recover: 1. Use the previously saved, 2153153801813487 to overwrite wc_facebook_commerce_partner_integration_id, 729123966146427, intentionally put a stall cpi_id in the field. <img width="799" alt="image" src="https://github.com/user-attachments/assets/3a511552-7513-452f-a53f-0c0a43cda2b1" /> 6. Go to Meta side and manually delete the correct CPI 729123966146427 The Overview page broken as expected, mimicking the staled CPI ID cases <img width="944" alt="image" src="https://github.com/user-attachments/assets/a4c507df-e0bb-417b-941b-1817545b7c44" /> 7. Run update_installation_data <img width="575" alt="image" src="https://github.com/user-attachments/assets/2efe2ec6-e313-4d2b-85d6-9dd1f4fc2905" /> <img width="780" alt="image" src="https://github.com/user-attachments/assets/8f7bfd4b-d7d1-4be4-926a-47bdca6aadd2" /> This removed staled CPI ID <img width="952" alt="image" src="https://github.com/user-attachments/assets/3a43875d-4bfc-4a8b-a5d3-0afba7df7173" /> The legacy view is showing as expected 8. Run repair to create a new CPI <img width="655" alt="image" src="https://github.com/user-attachments/assets/7b8f9809-4276-4a5d-8af9-554dd9dfda3c" /> <img width="936" alt="image" src="https://github.com/user-attachments/assets/85ec3fff-f260-443f-9377-19ba5b8584c9" /> Now the MiCE is rendering poperly Reviewed By: carterbuce, nrostrow-meta Differential Revision: D75704293 Pulled By: jczhuoMeta fbshipit-source-id: c35608e45d47d261d14cad3b8daf495330eca321 (cherry picked from commit 17c63e4)
carterbuce
added a commit
that referenced
this pull request
May 30, 2025
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
We found some issue caused by the inconsistency between the legacy and enhanced flow. The real world uses are unlikely to have the same serious issues. This PR put out a graceful fix it them.
Description
During dogfooding and QA testing, we get reports of:
The causes has been identified as:
This has been fixed in https://github.com/facebook/facebook-for-woocommerce/pull/3262/files but for existing instances off boarded before the fix landed, it's still experience problems
Issue 1
It's mainly impacting QA and internal users, for public users, the only chance they have a stale CPI ID and seeing this issue only if the performed the exact actions:
However, since this will cause broken MiCE page, this PR introduces graceful and robust logic to ensure the CPI is correct:
a. Using MBE response as source of truth, if the response from Meta does not have CPI ID, then clear the local CPI ID. This ensures no stale CPI ID.
b. The CPI repair endpoint will fetch or repair, get the correct CPI ID
The logic is implemented in refresh_installation_data heartbeat will fix the data inconsistency automatically.
Issue 2
It wasn't surfaced frequently as the refresh_installation_data is also repairing the missing IDs in the background. This PR added the saving action of those missing fields.
Type of change
Checklist
Changelog entry
Fix onboarding and offboarding data inconsistency between legacy and enhanced connection view.
Test Plan
No unit test can cover it since it's caused by a sequence of actions.
All existing tests passes:
Time: 00:28.917, Memory: 121.00 MB
OK (324 tests, 859 assertions)
Manual test it to cover all the cases.
Before
Verify on 3.5.0 branch, the business manager id was missing:

The cpi id exists:

and should be cleaned during uninstall since current 3.5.0 already included the cleanup fix(https://github.com/facebook/facebook-for-woocommerce/pull/3262/files), however, still manually make a copy so we can verify a lingering cpi_id won't break things.
After
Disconnect and switch to the current branch:

After onboarding, we now have business ID populated:
Now use the wrong CPI id to make sure the installation can recover:
The Overview page broken as expected, mimicking the staled CPI ID cases