Skip to content

Conversation

@fullofcaffeine
Copy link
Contributor

@fullofcaffeine fullofcaffeine commented Jul 26, 2022

Changes proposed in this Pull Request:

Gutenberg 13.7 deprecates the old themes.php?page=gutenberg-edit-site and admin.php?page=gutenberg-edit-site in favor of WP core's site-editor.php. This PR updates the route in the admin bar so that the update_submenus logic keeps working and replaces the wp-admin variant with the "calipsoified" one, that opens the editor in the Gutenframe. It also keeps the old rule for backward compatibility with GB < 13.7.

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?

Jetpack product discussion

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

Testing instructions:

The easiest way is to create a WoA dev site, install the Jetpack Beta Tester plugin in it and use it to install a dev version of the Jetpack plugin from this branch. From there, you can also modify the source-code directly through ssh using vim.

Test that the Appearance->Editor menu item links to the "Calipsoified" Site Editor

With Gutenberg 13.6
  1. Make sure Gutenberg 13.6 is active on your WoA site. At the time I write this, it should come by default;
  2. Load your site from Calypso (i.e https://wordpress.com/home/myatsite.wpcomstaging.com) and verify that the Appearance->Editor links to https://wordpress.com/site-editor/myatsite.wpcomstaging.com.
  3. Load your site from WPadmin (i.e https://myatsite.wpcomstaging.com/wp-admin/) and verify that the Appearance->Editor links to https://wordpress.com/site-editor/myatsite.wpcomstaging.com.
With Gutenberg 13.7.3
  1. Remove the Gutenberg symlink from your Woa box: rm -rf /srv/htdocs/wp-content/plugins/gutenberg
  2. Download Gutenberg 13.7.3 from the WP plugins repo
  3. Manually upload it to your WoA dev site using the plugins installation UI
  4. Follow the steps from the 13.6 test above; they should have the same results.
Without Gutenberg (WP 6.0)
  1. Remove the Gutenberg directory from your Woa box: rm -rf /srv/htdocs/wp-content/plugins/gutenberg
  2. Follow the same steps above. The results should be the same.

IMPORTANT: The "Calipsofied" link will not work in the block editor from WP until Automattic/wp-calypso#65997 is merged. If you try to open it, it will redirect to themes.php which will finally show a 403 HTTP error page.

Without Gutenberg (WP 5.9)
  1. Run wp core update --version=5.9 --force to downgrade your box to WordPress 5.9
  2. Follow the same steps above. The results should be the same.

IMPORTANT: The "Calipsofied" link will not work in the block editor from WP until Automattic/wp-calypso#65997 is merged. If you try to open it, it will redirect to themes.php which will finally show a 403 HTTP error page.

Test changes in projects/plugins/jetpack/class.jetpack-gutenberg.php

With Gutenberg 13.6
  1. Make sure Gutenberg 13.6 is active on your WoA site. At the time I write this, it should come by default;
  2. Add an error_log('ping') here. You can do that by editing the file using vim in your WoA dev box: vim /srv/htdocs/wp-content/plugins/jetpack-dev/class.jetpack-gutenberg.php
  3. tail -f /tmp/php-errors
  4. Load the Site Editor
  5. You should see a 'ping` message
With Gutenberg 13.7.3
  1. Remove the Gutenberg symlink from your Woa box: rm -rf /srv/htdocs/wp-content/plugins/gutenberg
  2. Download Gutenberg 13.7.3 from the WP plugins repo
  3. Manually upload it to your WoA dev site using the plugins install UI
  4. Follow the steps from the 13.6 test above; they should have the exact same results.
Without Gutenberg (WP 6.0)
  1. Remove the Gutenberg directory from your Woa box: rm -rf /srv/htdocs/wp-content/plugins/gutenberg
  2. Follow the same steps above. The results should be the same.
Without Gutenberg (WP 5.9)
  1. Run wp core update --version=5.9 --force to downgrade your box to WordPress 5.9
  2. Follow the same steps above. The results should be the same.

Test changes in projects/plugins/jetpack/modules/notes.php

  1. Make sure Gutenberg 13.6 is active on your WoA site. At the time I write this, it should come by default;
  2. Add a error_log('pong') here and a error_log('returning') here before the return. You can do that by editing the file using vim in your WoA dev box: vim /srv/htdocs/wp-content/plugins/jetpack-dev/modules/notes.php
  3. tail -f /tmp/php-errors
  4. Load the Site Editor
  5. You should see a returning message and should not see a pong message.
Without Gutenberg (WP 6.0)
  1. Remove the Gutenberg directory from your Woa box: rm -rf /srv/htdocs/wp-content/plugins/gutenberg
  2. Follow the same steps above. The results should be the same.
Without Gutenberg (WP 5.9)
  1. Run wp core update --version=5.9 --force to downgrade your box to WordPress 5.9
  2. Follow the same steps above. The results should be the same.

Test changes in projects/js-packages/shared-extension-utils/src/plan-utils.js

See https://github.com/Automattic/jetpack/pull/25281/files#r933693930

Further context

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello fullofcaffeine! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D84883-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

@github-actions github-actions bot added [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Feature] Masterbar WordPress.com Toolbar and Dashboard customizations labels Jul 26, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jul 26, 2022

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.
  • ✅ All commits were linted before commit.
  • ✅ 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 (other than "Required review") 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 you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.


Jetpack plugin:

  • Next scheduled release: August 2, 2022.
  • Scheduled code freeze: July 25, 2022.

@github-actions github-actions bot added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Jul 26, 2022
@fullofcaffeine fullofcaffeine changed the title [GB 13.7] Calipsoify the new site-editor.php route in the admin bar [GB 13.7] Gutenberg 13.7 Site Editor route fixes Jul 26, 2022
@fullofcaffeine fullofcaffeine force-pushed the add/calypsoify-support-for-site-editor-in-gb-13.7 branch from 423f9e2 to 3f6abdf Compare July 27, 2022 00:02
@creativecoder
Copy link
Contributor

I'm not quite sure how to test all of the changes here, so let me know when there are testing instructions available.

I can confirm that the changes in projects/plugins/jetpack/modules/masterbar/admin-menu/class-admin-menu.php fix the Site editor sidebar url for simple sites, that I flagged in D84749-code

image

Sandboxing a Simple site with this change that's also running Gutenberg edge, the url correctly shows as /site-editor/{site}

@fullofcaffeine
Copy link
Contributor Author

fullofcaffeine commented Jul 27, 2022

I'm not quite sure how to test all of the changes here, so let me know when there are testing instructions available.

I'm not either :/ (apart from the menu href change you mention, that is). I'd expect E2E or unit-tests to cover them? I haven't had the chance to try to test them yet (currently fixing core tests in D84588-code). I think it's worth asking the Jetpack Crew (👋🏻 ) for insights here before going any further. I'll mark this with the "Needs Review" flag.

@fullofcaffeine fullofcaffeine added the [Status] Needs Review This PR is ready for review. label Jul 27, 2022
@fullofcaffeine fullofcaffeine force-pushed the add/calypsoify-support-for-site-editor-in-gb-13.7 branch 2 times, most recently from c88ec29 to 9a95950 Compare July 28, 2022 00:25
@fullofcaffeine fullofcaffeine force-pushed the add/calypsoify-support-for-site-editor-in-gb-13.7 branch from 4fbc60c to a085f31 Compare July 28, 2022 01:03
@sdixon194 sdixon194 removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. [Status] In Progress labels Jul 28, 2022
@coder-karen
Copy link
Contributor

I'm not quite sure how to test all of the changes here

In terms of just the menu changes on WoA sites, I've been able to test Appearance -> Editor menu href changes by using the Beta tester plugin. Under 'Search for a Jetpack Feature Branch' input 25281 and then click Activate. By testing, I just mean that on an WoA site I can see the URLs correctly showing /site-editor/ in that Appearance -> Editor path. I'm not too sure if this is part of what you are looking for in terms of testing, to be able to test on those sites.

In terms of other ways to test this I'm not too sure (though I'm aware the existing tests are all passing for this PR). Seeing the comments on this post - p9dueE-5tE-p2 - I'm wondering if this is something @anomiex might have more insight on?

@fullofcaffeine
Copy link
Contributor Author

Hi @coder-karen, thanks!

In terms of just the menu changes on WoA sites, I've been able to test Appearance -> Editor menu href

Yeah, we also tested that part, thanks for double-checking, though!

In terms of other ways to test this I'm not too sure (though I'm aware the existing tests are all passing for this PR). Seeing the comments on this post - p9dueE-5tE-p2 - I'm wondering if this is something @anomiex might have more insight on?

Yep! I'd love some advice on how to go about testing those! Thanks!

@anomiex
Copy link
Contributor

anomiex commented Jul 29, 2022

I don't have any insight on how to test the actual things, but I can suggest which environments to test in:

  • With an older version of Gutenberg, e.g. 13.6.
  • With Gutenberg 13.7, of course.
  • WordPress 5.9 without a Gutenberg plugin.
  • WordPress 6.0 without a Gutenberg plugin.

@fullofcaffeine
Copy link
Contributor Author

@anomiex I tested the Site Editor href fix in the following scenarios:

  • With an older version of Gutenberg, e.g. 13.6.
  • With Gutenberg 13.7, of course.

I'll take a look a the others.

@fullofcaffeine fullofcaffeine force-pushed the add/calypsoify-support-for-site-editor-in-gb-13.7 branch from a085f31 to 421d0d6 Compare July 29, 2022 17:22
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think we should try to get the preference from the old route if the one for site-editor-php is null (I don't know what is the actual return value returned when it can't find a persisted preferred view, though, would have to look)?

I think that if we don't, users will lose their preference here, if it was set 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense to me 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you know how to set this preference for the site editor? 🤔 cc @Addison-Stavlo

Copy link
Contributor

@creativecoder creativecoder Jul 29, 2022

Choose a reason for hiding this comment

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

I did a little searching trying to understand this. The "preferred view" comes from the preferred-view query parameter in the url. The only place I can see that we set that in the UI is in the View panel at the top right of some Calypso/wp-admin screens.

image

I'm not seeing a way the preferred view would get set specifically for the Site editor, other than manually appending the query parameter to the wp-admin url. If that's correct, I don't think we need to worry about persisting the preference.

Copy link
Contributor

Choose a reason for hiding this comment

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

I miss-typed that last comment! Edited, it now reads: "I don't think we need to worry about persisting the preference."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @creativecoder, thanks for diving into this! Yeah, I was having a chat with @jeyip about the very same thing and we reached the same conclusion. It seems that we can assume the Site Editor has always been set to load inside the Gutenframe and that users couldn't change that.

Copy link
Contributor Author

@fullofcaffeine fullofcaffeine Jul 29, 2022

Choose a reason for hiding this comment

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

That being said, I'll leave an additional note that - as reported by you here - the Gutenframe isn't able to load the Site Editor in AT sites at the moment in any case, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we'll need to keep checking for the old page here if we want this condition to keep working with Gutenberg 13.6 🤔 the problem is that Gutenberg 13.6 will redirect site-editor.php to the old themes.php?page=gutenberg-edit-site.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the existing code handles both Site editor routes, so you can this leave as is?

Copy link
Contributor Author

@fullofcaffeine fullofcaffeine Jul 29, 2022

Choose a reason for hiding this comment

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

@creativecoder Are you sure? Unless I'm missing something, if 13.6 is active, accessing site-editor.php will redirect to themes.php?page=gutenberg-edit-site, so this logic will not apply.

Copy link
Contributor Author

@fullofcaffeine fullofcaffeine Jul 29, 2022

Choose a reason for hiding this comment

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

Ah, I see what you mean, sorry for the misunderstanding! Yeah, the existing logic does handle both, you're right. However, I think the rewritten one is a bit easier to read and understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one will work fine with 13.6 and 13.7. In envs with 13.6, it will redirect to site-editor.php, which will in turn redirect to themes.php .... If 13.7+ is active instead, it will work as is, as 13.7 uses this route for the Site Editor.

Copy link
Contributor Author

@fullofcaffeine fullofcaffeine Jul 30, 2022

Choose a reason for hiding this comment

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

I'm not sure how to test this change from an integration standpoint, I asked the Jetpack folks here: p1659139972849529-slack-CDLH4C1UZ. However, the change is simple enough that it's feasible to accept it as is.

@fullofcaffeine fullofcaffeine force-pushed the add/calypsoify-support-for-site-editor-in-gb-13.7 branch from c0ec1ca to 39d9de4 Compare July 29, 2022 23:39
@fullofcaffeine fullofcaffeine force-pushed the add/calypsoify-support-for-site-editor-in-gb-13.7 branch from d2c05c9 to 2d03003 Compare August 1, 2022 16:55
@coder-karen coder-karen added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Aug 1, 2022
@kraftbj kraftbj merged commit 57d3568 into trunk Aug 1, 2022
@kraftbj kraftbj deleted the add/calypsoify-support-for-site-editor-in-gb-13.7 branch August 1, 2022 17:50
@github-actions github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Aug 1, 2022
@github-actions github-actions bot added this to the jetpack/11.2 milestone Aug 1, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2022

Great news! One last step: head over to your WordPress.com diff, D84883-code, and deploy it.
Once you've done so, come back to this PR and add a comment with your SVN changeset ID (e.g. r12345-wpcom).

Thank you!

@kraftbj
Copy link
Contributor

kraftbj commented Aug 1, 2022

r249926-wpcom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Masterbar WordPress.com Toolbar and Dashboard customizations [JS Package] Publicize Components [JS Package] Shared Extension Utils [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ RNA Touches WP.com Files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants