-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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
WCA Test helper - add remote inbox notification staging importer #48735
Conversation
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: |
1 similar comment
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: |
…mmerce-beta-tester
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. |
Size Change: +31 B (0%) Total Size: 2.49 MB |
…mmerce-beta-tester, 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.
I encountered an error when trying to delete all notes.
Screen.Recording.2024-06-24.at.16.43.46.mov
Other than that, tested well and the tool is working as expected.
setNotice, | ||
} ) { | ||
const importFromEnv = async ( env ) => { | ||
const urls = { |
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.
It would be helpful if we could also allow users to use a custom URL for the import. This way, we can test some functionalities unavailable in staging or production and fetch notes from a gist URL to test PR changes. What do you think?
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.
Good idea!
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.
Added in 7e628ac
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.
I encountered an error when trying to delete all notes.
It seems like JN does not like DELETE
methods. I've changed DELETE to POST. Please try again.
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.
I think this is a much better solution compared to the previous one, great work
@moon0326!
I have a few comments, but everything is testing well except for the delete all similar to @chihsuan.
I'm curious if difference between button placement is different than in the PR description, is it intended? (screenshot)
...a-tester/api/remote-inbox-notifications/class-wca-test-helper-remote-inbox-notifications.php
Show resolved
Hide resolved
...a-tester/api/remote-inbox-notifications/class-wca-test-helper-remote-inbox-notifications.php
Outdated
Show resolved
Hide resolved
...a-tester/api/remote-inbox-notifications/class-wca-test-helper-remote-inbox-notifications.php
Outdated
Show resolved
Hide resolved
...a-tester/api/remote-inbox-notifications/class-wca-test-helper-remote-inbox-notifications.php
Outdated
Show resolved
Hide resolved
plugins/woocommerce-beta-tester/src/remote-inbox-notifications/data/actions.js
Outdated
Show resolved
Hide resolved
plugins/woocommerce/src/Admin/RemoteSpecs/RuleProcessors/RuleEvaluator.php
Outdated
Show resolved
Hide resolved
…/class-wca-test-helper-remote-inbox-notifications.php Co-authored-by: Ilyas Foo <foo.ilyas@gmail.com>
…valuator.php Co-authored-by: Ilyas Foo <foo.ilyas@gmail.com>
…/class-wca-test-helper-remote-inbox-notifications.php Co-authored-by: Ilyas Foo <foo.ilyas@gmail.com>
…mmerce-beta-tester
…ix DELETE method errors
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 work, tested well. 👍
…mote-notification-importer Fix filename typo
…mmerce-beta-tester
Fix changelog filename typo
…mmerce-beta-tester
It seems the new CI is automatically creating a changelog based on branch name 43d6abe
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.
Awesome, thanks for adding the public function as utilities! Tests well, LGTM! 🚢
Submission Review Guidelines:
Changes proposed in this Pull Request:
This PR adds a new tool to the WCA Test Helper that helps import remote inbox notifications from staging or production for testing purposes. Since this tool cover functionalities from
Admin notes
tool, I've retired it.Import from staging
requires Automattic proxy connected.How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
plugins/woocommerce-beta-tester
and runpnpm run build:zip
woocommerce-beta-tester.zip
Tools -> WCA Test Helper -> Remote Inbox Notifications
Import from staging
Import from staging
again.Changelog entry
Changelog Entry Details
Significance
Type
Message
Adds a new tool to the WCA Test Helper that helps import remote inbox notifications from staging or production for testing purposes
Changelog Entry Comment
Comment