Fix RIM rules processor #38312
Fix RIM rules processor #38312
Conversation
github-actions
bot
added
focus: react admin [team:Ghidorah]
plugin: woocommerce
Test Results SummaryCommit SHA: 7057b54
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. |
octaedro
added
focus: product management
|
Hi @mattsherman, @louwie17, 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: |
There was a problem hiding this comment.
I didn't test this yet, but I did review the code...
I think that we should make use of the transformer support in RIM processing.
And, some unit tests surrounding this would be nice, in:
- https://github.com/woocommerce/woocommerce/blob/trunk/plugins/woocommerce/tests/legacy/unit-tests/woocommerce-admin/remote-inbox-notifications/transformer-service.php
- https://github.com/woocommerce/woocommerce/blob/trunk/plugins/woocommerce/tests/legacy/unit-tests/woocommerce-admin/remote-inbox-notifications/option-rule-processor.php
plugins/woocommerce/src/Admin/RemoteInboxNotifications/OptionRuleProcessor.php
Outdated
Show resolved
Hide resolved
plugins/woocommerce/src/Admin/RemoteInboxNotifications/OptionRuleProcessor.php
Outdated
Show resolved
Hide resolved
octaedro
force-pushed
the
fix/38167_RIM_rules_processor
branch
from
63f6cd7
to
5423e4c
Compare
There was a problem hiding this comment.
Nice work @octaedro, this is testing well, and the code changes mostly look good, I did leave one suggestion/comment in the option rule processor condition, as it looks a bit confusing, and I am not sure what test cases we could use to better test this.
| @@ -25,7 +25,7 @@ public function process( $rule, $stored_state ) { | |||
| $default = isset( $rule->default ) ? $rule->default : $default_value; | |||
| $option_value = get_option( $rule->option_name, $default ); | |||
|
|
|||
| if ( $is_contains && ! is_array( $option_value ) ) { | |||
| if ( $is_contains && ! is_array( $option_value ) && ( ! is_string( $option_value ) || ! is_string( $rule->value ) ) ) { | |||
There was a problem hiding this comment.
This additional condition is really confusing to me, maybe it would be worth adding a comment here, or is there a way to simplify this?
Why are we also checking if rule->value is not a string? Given the condition only sets the option value to an empty array.
Would $is_contains && ! ( is_array( $option_value ) || is_string( $option_value ) ) be enough?
There was a problem hiding this comment.
Maybe it be worth writing some tests around this, to make sure the scenarios are covered.
plugins/woocommerce/src/Admin/RemoteInboxNotifications/OptionRuleProcessor.php
Outdated
Show resolved
Hide resolved
|
@louwie17 I just addressed the changes you mentioned. Could you take another look at this PR? |
Submission Review Guidelines:
Changes proposed in this Pull Request:
This PR introduces code to accurately parse the
siteurlduring the execution of the remote inbox messages rules processor.Closes #38167.
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
Tools > WCA Test Helper > Optionsand delete the options:_transient_timeout_woocommerce_admin_remote_inbox_notifications_specsand_transient_woocommerce_admin_remote_inbox_notifications_specsTools > WCA Test Helper > Toolsand press theRunbutton next toRun wc_admin_daily jobIncrease conversions with WooPay — our fastest checkout yetshould be visible four times under the Inbox section.I'm going to add some unit tests in a follow-up PR.