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

Couple bug fixes for ESP sync CLI command #3615

Merged
merged 1 commit into from
Dec 11, 2024
Merged

Conversation

claudiulodro
Copy link
Contributor

All Submissions:

Changes proposed in this Pull Request:

Note: This is a hotfix PR.

This PR fixes a couple bugs in the ESP sync CLI command.

  1. The parser did not like the ellipses in the docs like [--subscription-ids=<id1,id2...>], so those parameters weren't getting recognized.
  2. When an error was thrown during sync, the command threw a fatal error because the $customer object didn't exist.

How to test the changes in this Pull Request:

  1. Before applying this PR try e.g. wp newspack esp sync --user-ids=1. Observe the following output:
Warning: The `wp newspack esp sync` command has an invalid synopsis part: [--subscription-ids=<id1,id2...>]
Warning: The `wp newspack esp sync` command has an invalid synopsis part: [--user-ids=<id1,id2...>]
Warning: The `wp newspack esp sync` command has an invalid synopsis part: [--order-ids=<id1,id2...>]
Error: Parameter errors:
 unknown --user-ids parameter
  1. Add the following to the top of ESP_Sync::sync_contact so an error is always thrown:
	return new \WP_Error(
		'newspack_esp_sync_contact',
		sprintf(
		// Translators: %d is the user ID.
			__( 'Customer with ID %d does not exist.', 'newspack-plugin' ),
			1
		)
	);
  1. Run wp newspack esp sync. Observe output like the following:
[11-Dec-2024 00:10:05 UTC] PHP Fatal error:  Uncaught Error: Call to a member function get_email() on null in /Users/claudiulodro/Local Sites/newspackdev/app/public/wp-content/plugins/newspack-plugin/includes/cli/class-ras-esp-sync.php:229
...
  1. Apply this patch. Verify that the --user-ids flag works and that when an error is thrown you now get something like Error syncing contact info for user ID 3. Customer with ID 1 does not exist instead of a fatal error.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@claudiulodro claudiulodro added [Type] Bug Incorrect behavior or functionality [Status] Needs Review The issue or pull request needs to be reviewed labels Dec 11, 2024
@claudiulodro claudiulodro requested a review from a team as a code owner December 11, 2024 00:29
@github-actions github-actions bot added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Dec 11, 2024
@claudiulodro claudiulodro merged commit 4557dda into release Dec 11, 2024
10 checks passed
@claudiulodro claudiulodro deleted the fix/esp-sync branch December 11, 2024 15:08
matticbot pushed a commit that referenced this pull request Dec 11, 2024
## [5.9.1](v5.9.0...v5.9.1) (2024-12-11)

### Bug Fixes

* couple bug fixes for esp sync cli ([#3615](#3615)) ([4557dda](4557dda))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 5.9.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released [Status] Approved The pull request has been reviewed and is ready to merge [Type] Bug Incorrect behavior or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants