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

Improve hpos cli verbose output #38699

Merged
merged 6 commits into from Jun 23, 2023
Merged

Conversation

rrennick
Copy link
Contributor

@rrennick rrennick commented Jun 13, 2023

Submission Review Guidelines:

Changes proposed in this Pull Request:

Note: This PR is a followup to #38690 which will need to be merged first.

print_r is used when logging warnings/errors during the HPOS CLI migrate command:

Warning: Attempting to remigrate...
Warning: 1 error found: Array
(
    [3304] => Array
        (
            [0] => Array
                (
                    [column] => _order_total
                    [original_value] => 14.29
                    [new_value] => 1429
                )

        )

)
 when re-migrating order. Please review the error above.

Formatting this output as JSON will make it easier to read when there are a significant number of entries output:

Warning: Attempting to remigrate...
Warning: 1 error found: {
    "3304": [
        {
            "column": "_order_total",
            "original_value": "14.29",
            "new_value": "1429"
        }
    ]
} when re-migrating order. Please review the error above.

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

  1. With posts authoritative, enable sync.
  2. Turn off the sync when both HPOS and posts tables are in sync.
  3. Edit any existing order, or add a new order so that the out of sync condition can be simulated.
  4. Run the CLI command like so: wp wc cot verify_cot_data --verbose --re-migrate. In the output, you will first see the order that you have edited shown as error and out of sync. This will be followed by message: Warning: Attempting to remigrate and then order will be re-migrated.
  5. Output will be JSON instead of print_r.

@rrennick rrennick added type: enhancement The issue is a request for an enhancement. focus: wc-cli Issues related to WooCommerce CLI. focus: custom order tables / HPOS Issues related to High-Performance Order Storage (HPOS) née Custom Order Tables. labels Jun 13, 2023
@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Jun 13, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jun 13, 2023

Test Results Summary

Commit SHA: f879ebb

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 51s
E2E Tests1900018020816m 59s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

@rrennick
Copy link
Contributor Author

@vedanshujain Does this look like a good list to add to this PR:

# git grep -n print_r -- '*.php' | grep -v _print_r | grep -v 'tests/' | grep -v phpcs:ign | grep -v src/Admin
includes/wc-core-functions.php:2073:			'func' => 'print_r',
includes/wc-deprecated-functions.php:95:	$message .= '. Args: ' . print_r( $args, true ) . '.';
src/Database/Migrations/CustomOrderTable/CLIRunner.php:498:			$errors = print_r( $failed_ids, true );
src/Internal/Admin/Schedulers/MailchimpScheduler.php:128:		$msg = isset( $extra_msg ) ? 'Incorrect response from Mailchimp API with: ' . print_r( $extra_msg, true ) : 'Error getting a response from Mailchimp API.';
src/Internal/BatchProcessing/BatchProcessingController.php:372:			$batch_detail_string = '\n' . print_r(
src/Internal/DataStores/Orders/OrdersTableDataStore.php:1312:		$this->error_logger->notice( 'Diff found: ' . print_r( $diff, true ) );

@vedanshujain
Copy link
Contributor

I'd keep the changes limited to HPOS related stuff, so only BatchProcessingController, OrdersTableDataStore and CLIRunner. Perhaps we can push rest changes in a different PR.

For rest of the changes, since they are pretty old there may be parsers already configured for print_r output, so we probably format them conditionally (for example if a config constant is set). wdyt?

@rrennick rrennick marked this pull request as ready for review June 22, 2023 18:08
@rrennick rrennick requested review from a team and barryhughes and removed request for a team June 22, 2023 18:08
@github-actions
Copy link
Contributor

Hi @barryhughes,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

1 similar comment
@github-actions
Copy link
Contributor

Hi @barryhughes,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

Copy link
Member

@barryhughes barryhughes left a comment

Choose a reason for hiding this comment

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

Lovely jubbly.

  • Works as described, received nice JSON-formatted output.
  • Left one (optional) suggestion.
  • Approving but not merging so you can review the suggestion and since another piece of work needs to be merged first.

Co-authored-by: Barry Hughes <3594411+barryhughes@users.noreply.github.com>
@barryhughes
Copy link
Member

I see #38690 was merged, will merge this also.

@barryhughes barryhughes merged commit 7ed5f59 into trunk Jun 23, 2023
17 checks passed
@barryhughes barryhughes deleted the improve-hpos-cli-verbose-output branch June 23, 2023 20:40
@github-actions github-actions bot added this to the 8.0.0 milestone Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
focus: custom order tables / HPOS Issues related to High-Performance Order Storage (HPOS) née Custom Order Tables. focus: wc-cli Issues related to WooCommerce CLI. plugin: woocommerce Issues related to the WooCommerce Core plugin. type: enhancement The issue is a request for an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants