-
Notifications
You must be signed in to change notification settings - Fork 69
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
Remove Woo plugin header #8050
Remove Woo plugin header #8050
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
Size Change: 0 B Total Size: 1.27 MB ℹ️ View Unchanged
|
4c8a0ab
to
355241b
Compare
Note for the first reviewer: I think the code is not in the right place, but should work. Please focus on testing and validating solution at this point. I'd appreciate the following recommendations:
|
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, the code looks good, and works as advertised! 👌
Keep in mind that this is not a final review.
…ill lots of pointless work to cover it with tests
@RadoslavGeorgiev I think it's ready for a final round of review and testing with fresh eyes before merging. FYI, it's not urgent, so can easily wait a bit longer than usual PRs. |
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.
A couple of comments for now, I'll test it thoroughly later today or on Monday.
Notes on failing tests. None of the failures seem to be related to PR changes. More notes in slack: p1706873714162189-slack-CGGCLBN58 |
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.
The changes look great, and translations are functioning. Thanks for working on this!
Merging since failing checks are not related to this PR. Fix for E2E tests is in #8123. |
Testing instructions for 7.2.0 are updated: https://github.com/Automattic/woocommerce-payments/wiki/Release-testing-instructions-for-WC-Payments-7.2.0 No update for critical flows needed. Nothing is changed in UI, plugin behavior, so not updating release notes. |
There are some internal problems caused by existence of Woo header in the plugin file.
However, removing Woo header will break mechanism used for loading translations from translate.wordpress.com.
This PR removes Woo header and fixes the problem with loading translations.
More context in p1705063122937369-slack-CGGCLBN58
Changes proposed in this Pull Request
Test coverage exception
Most of the code interacts directly with WordPress plugin update process, so there is very little value in covering it with automated tests, because it'll require lots of mocking and code without really testing the behavior.
This decision was discussed with @RadoslavGeorgiev in a call (p1706252630150489-slack-CGGCLBN58).
Testing instructions
If using dev tools plugin, disable proxy for wordpress.com. Server is not needed in these tests, but downloading files from translate.wordpress.com is.
Open debug.log file
wp-content/debug.log
to monitor errors in the process.Open plugins translation folder in your file manager. The path depends on your WP installation (local or within docker), bur the relative path is
wp-content/languages/plugins
Remove all the existing translations files (it's pretty safe, new translations will be loaded).
Go to WP admin dashboard > Settings > General, and change language to one of the languages supported by WooPayments: https://woo.com/document/woopayments/customization-and-translation/translations/
Go to WP updates page:
/wp-admin/update-core.php
, at the bottom of the page a button to update translations should appear, click it to load new packages.Once update process is completed check that translations for WooPayments appeared in translation folder mentioned above.
Check that plugin pages have translations.
Compare the translation files with files from wordpress.com. For this:
7_0_0.*fr_fr.zip
. Note, do not use files from test builds (they havetest
in file name).https://translate.files.wordpress.com/<file name starting from date>
. Example:https://translate.files.wordpress.com/2024/01/woocommerce-payments-7_0_0-1704390844-es_es.zip
.Try the same steps by selecting new language in Settings > General.
Try removing some .po files from WP translations folder, and go back to WP updates page:
/wp-admin/update-core.php
, new translations should be offered again.Try breaking the flow either by enabling local server proxy, or introducing some fake exceptions/errors. There should be no errors logged in debug.log from WooPayments, but error message should be logged in WooPayments log file in Woo > Sytem > Logs.
npm run changelog
to add a changelog file, choosepatch
to leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge