Skip to content
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

MU WPCOM: Port the starter-page-templates feature from ETK #38475

Merged
merged 30 commits into from
Jul 31, 2024

Conversation

miksansegundo
Copy link
Contributor

@miksansegundo miksansegundo commented Jul 23, 2024

Fixes https://github.com/Automattic/dotcom-forge/issues/8242

Proposed changes:

Other information:

  • Have you written new tests for your changes, if applicable?
  • Have you checked the E2E test CI results, and verified that your changes do not break them?
  • Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

  • Follow the instructions below to use this branch in simple or atomic. I have pending to test on atomic
    • If that doesn't work on simple, make sure to run bin/jetpack-downloader reset jetpack-mu-wpcom-plugin first.
    • Make sure the files are uploaded to the right folder (it can be moon, or production, or sun).
    • Alternatively, you can use the rsync approach from local. Read more PCYsg-Osp-p2
  • Go to { SITE }/wp-admin/post-new.php?post_type=page
  • Verify the assets are served from /vendor/automattic/jetpack-mu-wpcom/src/build/
  • Verify the feature works as before
Screenshot 2567-07-25 at 18 58 16

Copy link
Contributor

github-actions bot commented Jul 23, 2024

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖


The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.


Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team Review" label and ask someone from your team review the code. Once reviewed, it can then be merged.
If you need an extra review from someone familiar with the codebase, you can update the labels from "[Status] Needs Team Review" to "[Status] Needs Review", and in that case Jetpack Approvers will do a final review of your PR.


Mu Wpcom plugin:

  • Next scheduled release: WordPress.com Simple releases happen daily.

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.


Wpcomsh plugin:

  • Next scheduled release: on demand (usually Mondays if not sooner).

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

Copy link
Contributor

github-actions bot commented Jul 23, 2024

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

  • To test on WoA, go to the Plugins menu on a WordPress.com Simple site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin, and enable the port/starter-page-templates branch.

    • For jetpack-mu-wpcom changes, also add define( 'JETPACK_MU_WPCOM_LOAD_VIA_BETA_PLUGIN', true ); to your wp-config.php file.
  • To test on Simple, run the following command on your sandbox:

    bin/jetpack-downloader test jetpack port/starter-page-templates
    
    bin/jetpack-downloader test jetpack-mu-wpcom-plugin port/starter-page-templates
    

Interested in more tips and information?

  • In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
  • Read more about our development workflow here: PCYsg-eg0-p2
  • Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

@okmttdhr
Copy link
Member

FYI: It seems that @automattic/onboarding is not used in editing-toolkit-plugin/starter-page-templates as well as page-pattern-modal: Automattic/wp-calypso#92871 (review).

@miksansegundo
Copy link
Contributor Author

miksansegundo commented Jul 25, 2024

I'd appreciate some help with the failing tests.

PHP Tests are failing in wpcomsh:

  1. FunctionsTest::test_wpcomsh_get_atomic_client_id_defined

PHPUnit\Framework\Exception: PHP Warning: filemtime(): stat failed for /tmp/wordpress-latest/src/wp-content/plugins/wpcomsh/vendor/automattic/jetpack-mu-wpcom/src/features/starter-page-templates/../../build/starter-page-templates/starter-page-templates.js in /tmp/wordpress-latest/src/wp-content/plugins/wpcomsh/vendor/automattic/jetpack-mu-wpcom/src/features/starter-page-templates/class-starter-page-templates.php on line 74

Maybe that's related to the path /../../ here. Any ideas to fix this? It seems to fail for the .js file but not for the .css.

The filemtime function seems to be working well. See the params in the file request:

Screenshot 2567-07-25 at 19 00 41

Some Static Analysis tests are also failing:

  • Check failure on line 165 in projects/plugins/jetpack/tests/e2e/specs/sync/sync.test.js here
  • Check failure on line 320 in tools/e2e-commons/pages/page-actions.js here

These don't seem related. Any ideas?

The diff D156481-code is meant to fix the error "UndefError PhanUndeclaredFunction Call to undeclared function \gutenberg_dir_path()" here

The check "TypeError PhanTypeArraySuspicious Suspicious array access to $editor_settings of type object" here is explained here

Copy link
Contributor

@fushar fushar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the functionalities (without looking at the source code) and it seems to work properly. Tested on a TT4 theme. The default TT4 page layout selector modal is not shown (expected).

I also compared it with another test site which uses default ETK plugin and I didn't find any difference in functionalities.

@arthur791004
Copy link
Contributor

Tested and the functionalities work well 👍

@miksansegundo
Copy link
Contributor Author

miksansegundo commented Jul 26, 2024

Remove unused code related to "side-loading images" because it's Automattic/wp-calypso#34603 for #18362 (comment) that is not reproducible (I just reproduced it and will put back that code tomorrow. You have to click on the Gallery category to see the images broken in the patterns using the Gallery block).

About that issue, it's on production so it's not related to this PR.

The following screenshot is on production without sandboxing.

Screenshot 2567-07-26 at 13 01 14 Screenshot 2567-07-26 at 13 02 37

The block error happens only in the modal, not when it is inserted.

Screenshot 2567-07-26 at 13 04 21

@miksansegundo
Copy link
Contributor Author

I have added back the code for sideloading images just in case. 7ec6c7f

@github-actions github-actions bot added [Plugin] mu wpcom jetpack-mu-wpcom plugin [Plugin] Wpcomsh labels Jul 26, 2024
@miksansegundo miksansegundo force-pushed the port/starter-page-templates branch from 70844c8 to 5c36c4b Compare July 26, 2024 06:59
* Register block editor scripts.
*/
public function register_scripts() {
$script_path = '../../build/starter-page-templates/starter-page-templates.js';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try this to resolve the failing test? 🤔

Suggested change
$script_path = '../../build/starter-page-templates/starter-page-templates.js';
$script_path = include Jetpack_Mu_Wpcom::BASE_DIR . 'build/starter-page-templates/starter-page-templates.js';

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I have tried that in 2a32436 but it didn't work. I still see the error:

FunctionsTest::test_wpcomsh_get_atomic_client_id_defined
  PHPUnit\Framework\Exception: PHP Warning:  filemtime(): stat failed for /tmp/wordpress-latest/src/wp-content/plugins/wpcomsh/vendor/automattic/jetpack-mu-wpcom/src/build/starter-page-templates/starter-page-templates.js in /tmp/wordpress-latest/src/wp-content/plugins/wpcomsh/vendor/automattic/jetpack-mu-wpcom/src/features/starter-page-templates/class-starter-page-templates.php on line 104

@miksansegundo
Copy link
Contributor Author

This is ready for another review.

I'd appreciate if someone can take it over when possible because there are still e2e errors failing that I don't know how to resolve.

@arthur791004
Copy link
Contributor

I'll continue on it next week 🙂

@fushar
Copy link
Contributor

fushar commented Jul 26, 2024

Yes, I will also help take a look next week. Thanks @miksansegundo, we'll take care of this PR!

@arthur791004 arthur791004 force-pushed the port/starter-page-templates branch 2 times, most recently from f3db17b to a5cb965 Compare July 29, 2024 05:54
@arthur791004 arthur791004 force-pushed the port/starter-page-templates branch from a5cb965 to 36e2449 Compare July 29, 2024 08:50
@arthur791004
Copy link
Contributor

All tests are passed successfully now. Re-tested and the starter page templates modal displays as expected when creating the new page.

fushar
fushar previously approved these changes Jul 30, 2024
Copy link
Contributor

@fushar fushar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Smoke-tested again on Simple and Atomic sites. It looks good!

Copy link
Contributor

@arthur791004 arthur791004 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approved.

@arthur791004 arthur791004 merged commit 6c50025 into trunk Jul 31, 2024
72 checks passed
@arthur791004 arthur791004 deleted the port/starter-page-templates branch July 31, 2024 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants