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

Syncs product with name in ALL CAPS and displays warning #2589

Merged
merged 6 commits into from
Aug 1, 2023

Conversation

krutidugade
Copy link
Contributor

@krutidugade krutidugade commented Jul 20, 2023

Changes proposed in this Pull Request:

This PR enables syncing to Facebook for products with All Caps title. It adds a warning in the Facebook meta box on edit product page mentioning a potential synchronization issue and suggests changing it to sentence case.

Closes #2582.

Replace this with a good description of your changes & reasoning.

  • 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:

Screenshot 2023-07-24 at 8 28 26 PM

Detailed test instructions:

  1. Clone the repo and checkout this branch
  2. Add new product or edit existing product
  3. Enter product name in All Caps
  4. Enter dummy description
  5. Click Update or Publish
  6. You'll see "This product is not yet synced to Facebook." under Facebook meta box
  7. Refresh the page
  8. Verify Facebook meta box shows notice "Facebook ID: xxxxx may have issue with synchronisation as product title is all capital letters. Please change the title to sentence case to ensure product is synchronized."

Additional information:

If you leave the description blank, it will throw an exception showing a different notice. This will be addressed in separate report.

Changelog entry

Fix - Syncs products with All Caps title to Facebook and displays a warning in Facebook meta box.

@github-actions github-actions bot added changelog: fix Took care of something that wasn't working. type: bug The issue is a confirmed bug. labels Jul 20, 2023
Kruti Dugade added 3 commits July 24, 2023 15:01
…ook metabox about potential issue with synchronization of the product.
@krutidugade krutidugade requested a review from a team July 24, 2023 19:38
@krutidugade krutidugade marked this pull request as ready for review July 24, 2023 19:38
Copy link
Contributor

@rawdreeg rawdreeg left a comment

Choose a reason for hiding this comment

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

Thanks @krutidugade . LGTM! 👍🏾

@budzanowski
Copy link
Collaborator

Two things.

First. I think that the sentence that you propose is a bit vague. may have issues does not tell a lot to the merchant about the nature of the product.

Please consider a variant of:
Product title in all capital letters can lead to the server rejecting the product. To ensure successful product synchronization, please convert the product title to a sentence case.

Second.
Why show the sentence in case the product got synced? :) If we have the fb_product_id we should be good.

@budzanowski
Copy link
Collaborator

One more thing. Have you tested how it looks for a variation product with multiple variants?

Copy link
Collaborator

@budzanowski budzanowski left a comment

Choose a reason for hiding this comment

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

Please check the comments and adjust.

@krutidugade
Copy link
Contributor Author

Please consider a variant of:
Product title in all capital letters can lead to the server rejecting the product. To ensure successful product synchronization, please convert the product title to a sentence case.

Sure, this looks neat.

Why show the sentence in case the product got synced? :) If we have the fb_product_id we should be good.

Understood. I was under the impression that we want to sync the product and show the notice irrespective of the product being synced.

One more thing. Have you tested how it looks for a variation product with multiple variants?

No, I missed this part. The Variant IDs aren't generated. I'll look into this.

@budzanowski
Copy link
Collaborator

Understood. I was under the impression that we want to sync the product and show the notice irrespective of the product being synced.

This is my question basically. What should we do? I think that your proposal is the best option. I just want to be sure that we have thought about everything. Don't hesitate to question my thoughts and statements in case you feel or know that they are wrong.

@krutidugade
Copy link
Contributor Author

This is my question basically. What should we do? I think that your proposal is the best option. I just want to be sure that we have thought about everything. Don't hesitate to question my thoughts and statements in case you feel or know that they are wrong.

Noted. I do think it is best we show the notice irrespective of the product being synced. In the following screenshot, we can see one variant isn't synced so the product wasn't synced properly.

Screenshot 2023-07-26 at 4 25 58 PM

I have updated the code. The notice can be seen after FB and variant IDs.

Screenshot 2023-07-26 at 4 21 26 PM Screenshot 2023-07-26 at 4 33 00 PM

@krutidugade krutidugade merged commit 57f52cd into develop Aug 1, 2023
5 checks passed
@krutidugade krutidugade deleted the fix/sync-product-all-caps branch August 1, 2023 13:09
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. type: bug The issue is a confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Product with ALL CAPS name isn't synced with Facebook
3 participants