Update OrderCountCache to return values for all previously saved statuses #60209
Conversation
Testing GuidelinesHi @jorgeatorres @joshuatf , Apart from reviewing the code changes, please make sure to review the testing instructions (Guide) and verify that relevant tests (E2E, Unit, Integration, etc.) have been added or updated as needed. Reminder: PR reviewers are required to document testing performed. This includes:
|
📝 WalkthroughWalkthroughThe changes enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant OrderCountCache
Caller->>OrderCountCache: set_multiple(order_type, counts)
OrderCountCache->>OrderCountCache: ensure_statuses_for_type(order_type, statuses)
OrderCountCache->>OrderCountCache: cache counts and update saved statuses
Caller->>OrderCountCache: get(order_type)
OrderCountCache->>OrderCountCache: get_saved_statuses_for_type(order_type)
OrderCountCache->>OrderCountCache: retrieve counts for all saved statuses
OrderCountCache-->>Caller: return counts for all previously saved statuses
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.{php,js,jsx,ts,tsx}📄 CodeRabbit Inference Engine (.cursor/rules/code-quality.mdc)
Files:
**/*.{php,js,ts,jsx,tsx}⚙️ CodeRabbit Configuration File
Files:
🧠 Learnings (2)📚 Learning: in the woocommerce payments settings provider state tracking system (plugins/woocommerce/src/interna...Applied to files:
📚 Learning: in woocommerce codebase, when table names are hardcoded (like `$wpdb->prefix . 'wc_order_coupon_look...Applied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
🔇 Additional comments (9)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
plugins/woocommerce/changelog/fix-WOOSUBS-867-retrieve-previously-cached-order-counts(1 hunks)plugins/woocommerce/src/Caches/OrderCountCache.php(5 hunks)plugins/woocommerce/src/Utilities/OrderUtil.php(2 hunks)plugins/woocommerce/tests/php/src/Caching/OrderCountCacheServiceTest.php(0 hunks)plugins/woocommerce/tests/php/src/Caching/OrderCountCacheTest.php(2 hunks)
💤 Files with no reviewable changes (1)
- plugins/woocommerce/tests/php/src/Caching/OrderCountCacheServiceTest.php
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{php,js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/code-quality.mdc)
**/*.{php,js,jsx,ts,tsx}: Guard against unexpected inputs
Sanitize and validate any potentially dangerous inputs
Ensure code is backwards compatible
Write code that is readable and intuitive
Ensure code has unit or E2E tests where applicable
Files:
plugins/woocommerce/src/Utilities/OrderUtil.phpplugins/woocommerce/tests/php/src/Caching/OrderCountCacheTest.phpplugins/woocommerce/src/Caches/OrderCountCache.php
**/*.{php,js,ts,jsx,tsx}
⚙️ CodeRabbit Configuration File
**/*.{php,js,ts,jsx,tsx}: Don't trust that extension developers will follow the best practices, make sure the code:
- Guards against unexpected inputs.
- Sanitizes and validates any potentially dangerous inputs.
- Is backwards compatible.
- Is readable and intuitive.
- Has unit or E2E tests where applicable.
Files:
plugins/woocommerce/src/Utilities/OrderUtil.phpplugins/woocommerce/tests/php/src/Caching/OrderCountCacheTest.phpplugins/woocommerce/src/Caches/OrderCountCache.php
🧠 Learnings (2)
📚 Learning: in woocommerce core repository, changelog entries for all prs live in `plugins/woocommerce/changelog...
Learnt from: jorgeatorres
PR: woocommerce/woocommerce#59675
File: .github/workflows/release-bump-as-requirement.yml:48-65
Timestamp: 2025-07-15T15:39:21.856Z
Learning: In WooCommerce core repository, changelog entries for all PRs live in `plugins/woocommerce/changelog/` directory and are processed during releases, not at the repository root level.
Applied to files:
plugins/woocommerce/changelog/fix-WOOSUBS-867-retrieve-previously-cached-order-counts
📚 Learning: in the woocommerce payments settings provider state tracking system (plugins/woocommerce/src/interna...
Learnt from: vladolaru
PR: woocommerce/woocommerce#58784
File: plugins/woocommerce/src/Internal/Admin/Settings/Payments.php:484-488
Timestamp: 2025-06-17T11:30:23.806Z
Learning: In the WooCommerce Payments settings provider state tracking system (plugins/woocommerce/src/Internal/Admin/Settings/Payments.php), when an extension is deactivated and its snapshot key disappears, only the 'extension_active' flag should be set to false while keeping other state flags like 'account_connected', 'needs_setup', 'test_mode', etc. unchanged. This is intentional behavior to preserve historical state information.
Applied to files:
plugins/woocommerce/src/Caches/OrderCountCache.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (11)
plugins/woocommerce/changelog/fix-WOOSUBS-867-retrieve-previously-cached-order-counts (1)
1-4: LGTM!The changelog entry follows the correct format and accurately describes the change.
plugins/woocommerce/src/Utilities/OrderUtil.php (2)
209-210: Good defensive programming with explicit type casting.The explicit cast ensures type safety when the parameter is passed to cache methods.
238-238: Performance improvement with batch caching.Using
set_multiple()is more efficient than individualset()calls in a loop, reducing the number of cache operations.plugins/woocommerce/tests/php/src/Caching/OrderCountCacheTest.php (1)
34-59: Comprehensive test coverage for registered and unregistered statuses.The expanded test properly validates that the cache handles both known WooCommerce statuses and third-party unregistered statuses, ensuring backward compatibility as intended.
plugins/woocommerce/src/Caches/OrderCountCache.php (7)
34-41: Well-implemented status retrieval with safe defaults.The method properly handles cache misses by returning an empty array, preventing potential null/false issues.
51-64: Efficient status tracking with optimized cache updates.The method efficiently tracks new statuses with early returns to avoid unnecessary cache operations when no new statuses are added.
128-132: Proper status tracking integration in set method.The method correctly updates the saved statuses list when setting cache values, maintaining consistency.
144-157: Well-implemented batch cache operation.The method efficiently sets multiple cache values in a single operation while properly tracking all statuses and handling edge cases.
167-174: Correct implementation of expanded status retrieval.The method properly retrieves all previously saved statuses when none are specified, addressing the backward compatibility issue described in the PR objectives.
184-187: Clear documentation of cache miss behavior.The updated comment clearly explains why null is returned when any requested status is not found in cache.
229-244: Smart cleanup of saved statuses during full flush.The method correctly removes the saved statuses list when all statuses are flushed, preventing accumulation of stale status entries.
Test using WordPress PlaygroundThe changes in this pull request can be previewed and tested using a WordPress Playground instance. Test this pull request with WordPress Playground. Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit. |
There was a problem hiding this comment.
Pre-approving with some tiny feedback. Tested well. Thanks @prettyboymp!
| /** | ||
| * Get the default statuses. | ||
| * | ||
| * @return string[] | ||
| * | ||
| * @deprecated 10.0.0 This method will be removed in the future. |
There was a problem hiding this comment.
I believe this should be 10.1.0 assuming we'll backport the fix.
| /** | ||
| * Cache prefix. | ||
| * | ||
| * @var string | ||
| */ | ||
| private $cache_prefix = 'order-count'; | ||
|
|
There was a problem hiding this comment.
Any reason to "move" the property declaration here? Not a problem per se, but a bit non-standard?
…deprecation version
…uses (#60209) * Update OrderCountCache to return values for all previously saved statuses. * Switch to set_multiple so that status cache doesn't need to be set multiple times * Fix call to ensure_statuses_for_type within set * Add some type safety * Add test for unregistered cache counts * lint fixes * more lint fixes * another equal alignment * add separator to statuses cache key * Deprecate get_default_statuses() method * remove short array syntax * Address feedback, move private property back to top of class and fix deprecation version
|
IMPORTANT: Merging this PR to the appropriate branches is critical to the release process and ensures that the bug does not cause regressions in the future releases. Cherry picking was successful for |
…r all previously saved statuses (#60236) Update OrderCountCache to return values for all previously saved statuses (#60209) * Update OrderCountCache to return values for all previously saved statuses. * Switch to set_multiple so that status cache doesn't need to be set multiple times * Fix call to ensure_statuses_for_type within set * Add some type safety * Add test for unregistered cache counts * lint fixes * more lint fixes * another equal alignment * add separator to statuses cache key * Deprecate get_default_statuses() method * remove short array syntax * Address feedback, move private property back to top of class and fix deprecation version Co-authored-by: Michael Pretty <prettyboymp@users.noreply.github.com>
Changes proposed in this Pull Request:
This updates the
OrderCountCache::get()method so that it returns all previously stored values for a given order type.Previously, if $order_statuses were not passed in, the list of statuses that would be fetched from cache would be based on the default list of statuses retrieved from
wc_get_order_statuses(). This causes a backward compatibility issue with extensions like WooCommerce Subscriptions which doesn't register all of the statuses it uses to be returned bywc_get_order_statuses().This changes it so that the OrderCountCache will keep it's own list of saved statuses per order type allowing
OrderCountCache->get()to return all previously saved counts for the order type.Partial Fix for WOOSUBS-867.
This specifically fixes an issue with WooCommerce Subscriptions when running object cache.
(For Bug Fixes) Bug introduced in PR #54034.
To See Previous Bug:
Screenshots or screen recordings:
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
Note - The cached count shown within pagination WooCommerce Subscriptions does not get properly updated when a subscription is transitioned from one status to another. This is noted in https://github.com/woocommerce/woocommerce-subscriptions/pull/4973#issuecomment-3154877832. Because Woo Subscriptions overrides the transition method of its extended order, it will need to integrate cache incrementing itself.
Testing that has already taken place:
Changelog entry
Changelog Entry Details
Significance
Type
Message
Changelog Entry Comment
Comment