-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
Delete coupons instead of trashing them #30696
Conversation
369b935
to
ce1b1a6
Compare
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.
Thanks for working on this @rrennick! These ran great for me locally.
Only a few minor comments on text changes and a question. Otherwise this looks good to merge once those are addressed.
tests/e2e/utils/CHANGELOG.md
Outdated
## Added | ||
|
||
- `utils.waitForTimeout( delay )` pause processing for `delay` milliseconds | ||
- `AdminEdit`, `OrderEdit` classes with utility functions for the respctive edit screens |
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.
Minor typo:
- `AdminEdit`, `OrderEdit` classes with utility functions for the respctive edit screens | |
- `AdminEdit`, `OrderEdit` classes with utility functions for the respective edit screens |
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.
Nice catch, fixed in 5ccf3a5
tests/e2e/utils/README.md
Outdated
@@ -153,6 +153,34 @@ This package provides support for enabling retries in tests: | |||
| `updatePaymentGateway`| `paymentGatewayId`, `payload` | Update a payment gateway | | |||
| `getSystemEnvironment` | | Get the current environment from the WooCommerce system status API. | |||
|
|||
### Classes | |||
|
|||
The package includes the following page specific utility classes |
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.
Just a minor suggestion to add a colon at the end:
The package includes the following page specific utility classes | |
The package includes the following page specific utility classes: |
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.
Fixed in 5ccf3a5
await expect( page ).toMatchElement( | ||
'#woocommerce-order-notes .note_content', | ||
{ | ||
text: 'Order status changed from Pending payment to Processing.', |
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.
Do we always expect the Processing
status and note status text to be present (e.g., Order status changed from Pending payment to Processing.
) when using verifyPublish()
here, or do we need to make them more dynamic?
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.
@zhongruige Thanks for pointing this out. That test is specific to our test flow so I moved that test to there. That eliminated the need for the OrderEdit
class so I removed it.
This is ready for another review.
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.
Thanks for working on this @rrennick!
The changes look good and the tests ran great for me locally.
Hi @zhongruige, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:
|
Changes proposed in this Pull Request:
This PR updates the new coupon test to delete the created coupon instead of trashing it. It adds a
waitForTimeout
utility function andAdminEdit
,OrderEdit
utility classes.Closes #30371 .
How to test the changes in this Pull Request: