Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove sync not possible modal. #2665

Merged
merged 2 commits into from
Nov 19, 2023
Merged

Conversation

budzanowski
Copy link
Collaborator

@budzanowski budzanowski commented Nov 17, 2023

Changes proposed in this Pull Request:

Closes #2636

In this PR I remove the modal that warns the merchant before crating a product if certain conditions are met. The modal was shown during the creation or update action of a product when:

  • that product was set to sync
  • product had a category assigned and that category was excluded from Facebook sync.

The original modal was intended to warn the merchant about the excluding sync conditions. The interaction looked as follows:

Screen.Recording.2023-11-17.at.09.00.37.mov

The intended behaviour was to "force" merchant to change the settings and make the product setup consistent. But this has backfired and is causing merchants a lot of problems. The product that is using the excluded from sync category can't be updated in any way.

The simplest fix is to remove the modal entirely. This solution is supported by the fact that the Facebook Product sync section will show what is blocking the sync for that product so no information is being lost. This feature has been implemented some time ago and looks like this:

  • Do the changed files pass phpcs checks? Please remove phpcs:ignore comments in changed files and fix any issues, or delete if not practical.

Screenshots:

Detailed test instructions:

  1. Create a product category
  2. Go to Marketing > Facebook > Product sync > Exclude categories from sync and add that category to the excluded list
  3. Create a new product and assign the excluded category to that product
  4. Try to publish/update that product - it should work
  5. Compare to the develop (rebuild required) branch where this operation should fail

Additional details:

The modal was displayed in two cases:

  1. When the product was created or updated
  2. When the merchant was opening a product editor for a product with the excluded setup

I am removing modal for both cases. For the case 2 the modal was never displayed due to a bug.

The <script> added to the footer was doing an AJAX call with a wrong parameter

sync_enabled: 'enabled',

when a different name was expected by the AJAX handler:

$var_sync_enabled = isset( $_POST['var_sync_enabled'] ) ? (string) wc_clean( wp_unslash( $_POST['var_sync_enabled'] ) ) : '';

This makes the total removal of the popup modal safe.

Changelog entry

Fix - Remove popup modal blocking product edit when product is using category excluded from sync.

@budzanowski
Copy link
Collaborator Author

Zip for testing based on 77d138e:

facebook-for-woocommerce.zip

@budzanowski budzanowski marked this pull request as ready for review November 17, 2023 08:35
@budzanowski budzanowski requested review from a team and sukafia November 17, 2023 08:35
@budzanowski budzanowski self-assigned this Nov 17, 2023
@budzanowski budzanowski added the changelog: fix Took care of something that wasn't working. label Nov 17, 2023
@message-dimke message-dimke requested review from message-dimke and removed request for a team November 17, 2023 08:58
@sukafia
Copy link

sukafia commented Nov 17, 2023

I can't publish drafts or new products when the test copy is installed on my site:

TQ8jwn.png

For the new products, the Publish button just doesn't work.

Here's a screen recording of my test:
https://github.com/woocommerce/facebook-for-woocommerce/assets/60121732/eedd95d6-2d34-4f41-91d2-b59a9b002ca6

@budzanowski
Copy link
Collaborator Author

The problem found by @sukafia has been the result of cached js files. After clearing the cache the issue was no longer there.
When this will be a part of a new release this problem will not happen.

Copy link
Contributor

@message-dimke message-dimke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Thank you, @budzanowski .

@budzanowski budzanowski merged commit 0827fd2 into develop Nov 19, 2023
6 checks passed
@budzanowski budzanowski deleted the remove/sync-not-possible-modal branch November 19, 2023 08:37
@camilolunacom
Copy link

@budzanowski this is amazing, and how things should be like! I know I'm saying this for many WC users like me: THANK YOU!!!

@budzanowski
Copy link
Collaborator Author

@camilolunacom I am happy that you like it.

This should be released on Tuesday.

Cheers!

@ibndawood ibndawood added changelog: none Skip changelog entry for this PR changelog: fix Took care of something that wasn't working. and removed changelog: fix Took care of something that wasn't working. changelog: none Skip changelog entry for this PR labels Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: fix Took care of something that wasn't working.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to create new product in excluded category
5 participants