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.

Sign up for GitHub

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

Jump to bottom

Allow /wp/v2/sites/1234/batch endpoint to process widget updates #21549

Merged
merged 2 commits into from
Oct 29, 2021
Merged

Allow /wp/v2/sites/1234/batch endpoint to process widget updates #21549

merged 2 commits into from
Oct 29, 2021

Conversation

mreishus
Copy link
Contributor

@mreishus mreishus commented Oct 26, 2021 •
edited by jeherve
Loading

This was made to fix an issue discovered on WPCOM, but the source of truth is on Jetpack, and we should also ensure this doesn't affect jetpack negatively. The core issue was that when saving widgets on widgets.php, widgets with visibility rules could be deleted upon save.

Changes proposed in this Pull Request:

Allow /wp/v2/sites/1234/batch endpoint to process widget updates

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

no

Testing instructions:

  1. Apply D69021-code
  2. Enable the seedlet theme on your test blog.
  3. In customizer, remove all the footer widgets
  4. In customizer, add a paragraph block widget to footer, and set the visibility to 404 page only.
  5. In customizer, add another paragraph block widget
  6. Save the changes
  7. Navigate to wp-admin/widgets.php
  8. Add just another paragraph block widget. Hit "Save". Do not leave the page.
  9. You should notice an error while saving.
  10. Re-save the changes - hit "Save" again.
  11. Add just another paragraph block widget. Hit "Save".
  12. Notice the error while saving again.

Repeat the steps with this patch applied. No errors while saving should be visible.

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello mreishus! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D69054-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

@mreishus mreishus self-assigned this Oct 26, 2021
@github-actions github-actions bot added [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Feature] Widget Visibility labels Oct 26, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Oct 26, 2021 •
edited
Loading

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: November 2, 2021.
  • Scheduled code freeze: October 25, 2021.

@github-actions github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Oct 26, 2021
@mreishus mreishus requested a review from david-binda October 26, 2021 18:55
@mreishus mreishus added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Status] Needs Team Review and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 26, 2021
@mreishus mreishus requested a review from sixhours October 26, 2021 18:57
@sixhours
Copy link
Contributor

This works in testing on my WP.com simple site!

we should also ensure this doesn't affect jetpack negatively.

What do you think about testing it on a local Jetpack test site, too? Or a JN test site. Just wondering based on the comment above.

@mreishus
Copy link
Contributor Author

mreishus commented Oct 26, 2021 •
edited
Loading

What do you think about testing it on a local Jetpack test site, too? Or a JN test site. Just wondering based on the comment above.

Testing on either of those is a good idea; I don't have test steps other than "Try Widget visibility on customizer and widgets.php and check that it's still working". I tried on my local JP docker site and everything looks good to me.

@mreishus mreishus force-pushed the fix/widget-visibility-wpcom branch from 77719ac to 50cc577 Compare October 27, 2021 21:55
@mreishus mreishus mentioned this pull request Oct 28, 2021
Closed
@sixhours
Copy link
Contributor

I'm not able to reproduce the error locally, but everything still works locally with this patch, so 👍

@mreishus mreishus added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Team Review labels Oct 28, 2021
david-binda
david-binda approved these changes Oct 29, 2021
View reviewed changes
Copy link
Contributor

@david-binda david-binda left a comment

Choose a reason for hiding this comment

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

Thanks for opening the PR. The code tests well for me :shipit:

@jeherve jeherve added this to the jetpack/10.4 milestone Oct 29, 2021
jeherve
jeherve approved these changes Oct 29, 2021
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

LGTM!

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 29, 2021
@mreishus mreishus merged commit 1c3f1e0 into master Oct 29, 2021
@mreishus mreishus deleted the fix/widget-visibility-wpcom branch October 29, 2021 15:01
@github-actions github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 29, 2021
@github-actions
Copy link
Contributor

Great news! One last step: head over to your WordPress.com diff, D69054-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

@mreishus
Copy link
Contributor Author

r234192

@sixhours sixhours mentioned this pull request Oct 29, 2021
Closed
davidlonjon added a commit that referenced this pull request Nov 2, 2021
@davidlonjon
[not verified] Merge branch 'master' into fix/boost-and-general-linti…
cf79f69 
…ng-formatting

* master:
  docker: Add script to create "fake" translations for testing i18n (#21407)
  Add webpack-config package (#21482)
  Update dependency composer/semver to v3.2.6 (#21608)
  Update dependency eslint-plugin-import to v2.25.2 (#21607)
  Update JS unit testing packages (#21605)
  Admin page: Fix pricing/currency displayed on the upgrade page (#21594)
  Update dependency @rollup/plugin-node-resolve to v13.0.6 (#21599)
  Publicize: Add new publicize icon toggle component (#20957)
  Search: hide search menu for atomic sites (#21565)
  Allow /wp/v2/sites/1234/batch endpoint to process widget updates (#21549)
  tooling: Add `.mjs` as a JavaScript extension (#21589)
  Jetpack: PHPCS src/class-tracking (#21583)
  Publicize: make the default social media message blank (#21477)
davidlonjon added a commit that referenced this pull request Nov 2, 2021
@davidlonjon
Merge branch 'master' into add/boost-e2e-ci-integration
515610a 
* master:
  docker: Add script to create "fake" translations for testing i18n (#21407)
  Add webpack-config package (#21482)
  Update dependency composer/semver to v3.2.6 (#21608)
  Update dependency eslint-plugin-import to v2.25.2 (#21607)
  Update JS unit testing packages (#21605)
  Admin page: Fix pricing/currency displayed on the upgrade page (#21594)
  Update dependency @rollup/plugin-node-resolve to v13.0.6 (#21599)
  Publicize: Add new publicize icon toggle component (#20957)
  Search: hide search menu for atomic sites (#21565)
  Allow /wp/v2/sites/1234/batch endpoint to process widget updates (#21549)
  tooling: Add `.mjs` as a JavaScript extension (#21589)
  Jetpack: PHPCS src/class-tracking (#21583)
  Publicize: make the default social media message blank (#21477)
davidlonjon added a commit that referenced this pull request Nov 2, 2021
@davidlonjon
Merge branch 'master' into fix/explicitly-hide-admin-bar
8c032d9 
* master: (40 commits)
  docker: Add script to create "fake" translations for testing i18n (#21407)
  Add webpack-config package (#21482)
  Update dependency composer/semver to v3.2.6 (#21608)
  Update dependency eslint-plugin-import to v2.25.2 (#21607)
  Update JS unit testing packages (#21605)
  Admin page: Fix pricing/currency displayed on the upgrade page (#21594)
  Update dependency @rollup/plugin-node-resolve to v13.0.6 (#21599)
  Publicize: Add new publicize icon toggle component (#20957)
  Search: hide search menu for atomic sites (#21565)
  Allow /wp/v2/sites/1234/batch endpoint to process widget updates (#21549)
  tooling: Add `.mjs` as a JavaScript extension (#21589)
  Jetpack: PHPCS src/class-tracking (#21583)
  Publicize: make the default social media message blank (#21477)
  Photon: do not serve Wikipedia images from CDN. (#21572)
  Publicize: update endpoint (#21510)
  [Plugin] Backup: Update initial backup screen (#21559)
  RNA Connect Screen: Remove unused files (#21570)
  RNA Connection: Add ConnectScreenRequiredPlan Component (#21521)
  Update PHPUnit coverage configs (#21557)
  cli: Fix skeleton phpunit config (#21555)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david-binda david-binda david-binda approved these changes

@sixhours sixhours sixhours approved these changes

@jeherve jeherve jeherve approved these changes

Assignees

@mreishus mreishus

Labels
[Feature] Widget Visibility [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ Touches WP.com Files
Projects
None yet
Milestone
jetpack/10.4
Development

Successfully merging this pull request may close these issues.
5 participants
@mreishus @matticbot @sixhours @jeherve @david-binda