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

Update modified date when a metadata is saved for HPOS. #39911

Merged
merged 5 commits into from Aug 29, 2023

Conversation

vedanshujain
Copy link
Contributor

@vedanshujain vedanshujain commented Aug 25, 2023

Submission Review Guidelines:

Changes proposed in this Pull Request:

When using a $order->save_meta_data() method, we have a gap in our sync logic that we do not update the date_updated_gmt column. The consequence here is that it will become non-deterministic for code to figure whether posts or order is more updated when there is a difference between them. To prevent this, we also update the modified date when updating the metadata via save_metadata.

How to test the changes in this Pull Request:

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

Note that we will be using wp shell to test this as it's not possible in the UI only to reproduce the bug.

  1. Set HPOS as authoritative. Create a new order and note it's ID (or use an existing order with ID XXX).
  2. Open a new wp shell and test following:
// Let's fetch the order 
$order = wc_get_order( XXX );

// Note it's current modified date
$order->get_date_modified();

// Add a new meta.
$order->add_meta_data( 'test1', 'new_value');
$order->save_meta_data();

// Note the current modified date, it should be updated.
$order->get_date_modified();

// Update the metadata.
$order->update_meta_data( 'test1', 'updated_value');
$order->save_meta_data();

// Note the current modified again, it should be updated
$order->get_date_modified();

// Delete the metadata
$order->delete_meta_data( 'test1' );
$order->save_meta_data();

// Note the current modified again, it should be updated
$order->get_date_modified();

Changelog entry

  • Automatically create a changelog entry from the details below.

Significance

  • Patch
  • Minor
  • Major

Type

  • Fix - Fixes an existing bug
  • Add - Adds functionality
  • Update - Update existing functionality
  • Dev - Development related task
  • Tweak - A minor adjustment to the codebase
  • Performance - Address performance issues
  • Enhancement - Improvement to existing functionality

Message

Comment

@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Aug 25, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 25, 2023

Test Results Summary

Commit SHA: 9b10f04

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202611m 20s
E2E Tests1950015021014m 14s

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.

Additionally, move the `type` column from always change to optional, by moving it so that its applied if there is atleast one other change.
@vedanshujain vedanshujain marked this pull request as ready for review August 28, 2023 12:06
@vedanshujain vedanshujain requested review from a team and coreymckrill and removed request for a team August 28, 2023 13:19
@github-actions
Copy link
Contributor

Hi @coreymckrill,

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
Collaborator

@coreymckrill coreymckrill left a comment

Choose a reason for hiding this comment

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

👍 Works as described testing with wp shell. Noting that I also tested running $order->save_meta_data() without first adding/updating/deleting any meta data, and it did not change the date modified value, which is as expected.


// Prevent this happening multiple time in same request.
if ( $order->get_date_modified() < $current_date_time && empty( $order->get_changes() ) ) {
$order->set_date_modified( current_time( 'mysql' ) );
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking that the call to current_time here should also have the 1 gmt switch, like above, but I guess that set_date_modified handles that already?

@coreymckrill coreymckrill added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Aug 28, 2023
@rodelgc rodelgc added needs: internal testing Indicates if the PR requires further testing conducted by Solaris status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Aug 29, 2023
@nigeljamesstevenson nigeljamesstevenson merged commit f53e5f7 into trunk Aug 29, 2023
33 checks passed
@nigeljamesstevenson nigeljamesstevenson deleted the fix/cache_data_store branch August 29, 2023 10:18
@github-actions github-actions bot added this to the 8.2.0 milestone Aug 29, 2023
@ObliviousHarmony ObliviousHarmony modified the milestones: 8.2.0, 8.1.0 Sep 8, 2023
ObliviousHarmony pushed a commit that referenced this pull request Sep 8, 2023
* Update modified date when a metadata is saved for HPOS. (#39911)

* Prep for cherry pick 39911

---------

Co-authored-by: nigeljamesstevenson <105309450+nigeljamesstevenson@users.noreply.github.com>
Co-authored-by: WooCommerce Bot <no-reply@woocommerce.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: internal testing Indicates if the PR requires further testing conducted by Solaris plugin: woocommerce Issues related to the WooCommerce Core plugin. status: analysis complete Indicates if a PR has been analysed by Solaris
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants