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
Add range operator in Remote Inbox Notification #45201
Conversation
Hi @psealock, @chihsuan, @woocommerce/ghidorah 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: |
Test Results SummaryCommit SHA: 0ce4a35
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. |
…ges/php/remote-specs-validation, woocommerce
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested well. 👍 Just left a minor comment about the code.
Pre-approving!
@@ -64,6 +64,8 @@ public static function compare( $left_operand, $right_operand, $operation ) { | |||
return strpos( $left_operand, $right_operand ) === false; | |||
} | |||
break; | |||
case 'range': | |||
return $left_operand >= $right_operand[0] && $left_operand <= $right_operand[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check if $right_operand
is an array and has at least 2 elements like what we have in other cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chihsuan Good suggestion! I'll make the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in d48f340
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just one minor thing
@@ -183,6 +188,22 @@ onboarding profile: | |||
} | |||
``` | |||
|
|||
#### range | |||
|
|||
`range` operator can check if a number falls within a certain range |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a good place to note its inclusive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@psealock Good suggestion! I'll make the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in f9b3cf2
* Add range rule in Remote Inbox Notification * Add changefile(s) from automation for the following project(s): woocommerce * Add range * Add changefile(s) from automation for the following project(s): packages/php/remote-specs-validation, woocommerce * Update README.md * Fix lint errors * Make sure right-hand is an array with two elemetns -- range * Update README * Lint fixes --------- Co-authored-by: github-actions <github-actions@github.com>
Submission Review Guidelines:
Changes proposed in this Pull Request:
Closes #44787
This PR adds
range
operation in Remote Inbox Notification to support testing a number's range. This PR also updates Remote Spec Validation schema files to supportrange
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
./vendor/bin/phpunit --testsuite shard2
Tools -> WCA Test Helper -> Remote Spec Rule Validator
woocommerce_remote_variant_assignment
option.value
for testing.Changelog entry
Significance
Type
Message
Support range operator in Remote Inbox Notification
Comment