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

[Gutenberg] Support POST requests #20428

Merged
merged 12 commits into from
Apr 5, 2023
Merged

Conversation

fluiddot
Copy link
Contributor

@fluiddot fluiddot commented Mar 29, 2023

Related PRs:

Description

This PR adds support for POST requests coming from the editor.

To test

The editor doesn't make POST requests yet, hence this change can only be tested with a test PR: #20430

Alternatively, we can also follow testing instructions from Automattic/jetpack#29756 to test a POST request in the VideoPress block.

Regression Notes

  1. Potential unintended areas of impact
    This change shouldn't impact any unintended area. The only area that could be impacted is the network logic used in the editor.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    I tested that GET requests coming from the editor work as expected.

  3. What automated tests I added (or what prevented me from doing so)
    N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Sorry, something went wrong.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 29, 2023

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr20428-879b1a1
Version22.1
Bundle IDorg.wordpress.alpha
Commit879b1a1
App Center BuildWPiOS - One-Offs #5417
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 29, 2023

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr20428-879b1a1
Version22.1
Bundle IDcom.jetpack.alpha
Commit879b1a1
App Center Buildjetpack-installable-builds #4446
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@fluiddot fluiddot modified the milestones: 22.0 ❄️, 22.2 Mar 30, 2023
@fluiddot fluiddot requested review from SiobhyB and jhnstn March 30, 2023 09:39
Copy link
Contributor

@SiobhyB SiobhyB left a comment

Choose a reason for hiding this comment

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

Similar to the review I left on the Android PR, I'm approving this code based on the following:

👏 👏 🚀

@fluiddot fluiddot requested a review from twstokes March 31, 2023 09:16
fluiddot and others added 2 commits March 31, 2023 11:18
# Conflicts:
#	Podfile
#	Podfile.lock
Notice that the `Podfile` requirement has been updated instead of using
`pod update` because the code now depends on a method introduced in
version 7.1.0.
@mokagio
Copy link
Contributor

mokagio commented Apr 2, 2023

I just shipped WordPressKit 7.1.0 and updated the Podfile accordingly.

Enabling auto-merge as this PR has already been approved to get ahead with the work for the 22.2 code freeze.

@mokagio mokagio enabled auto-merge April 2, 2023 11:38
@mokagio
Copy link
Contributor

mokagio commented Apr 3, 2023

@fluiddot @SiobhyB sorry for the spam folks. For some reason, I convinced myself 22.2 was the next release, while it's actually 22.1.

I did all the work to get this ready to merge into trunk from a dependencies point of view, but didn't account for changes in what the code would have to do after updating to the latest Gutenberg:

Screenshot 2023-04-03 at 10 31 26 am

The method Xcode suggests to insert is

func gutenbergDidRequestFetch(path: String, completion: @escaping (Result<Any, NSError>) -> Void)

But I'm not sure what to write into it.

However, given this PR was intended for 22.2, I think it's fine for me to wait for your input on how to get this to compile and merge. Thanks!

@fluiddot
Copy link
Contributor Author

fluiddot commented Apr 3, 2023

@fluiddot @SiobhyB sorry for the spam folks. For some reason, I convinced myself 22.2 was the next release, while it's actually 22.1.

No worries, I set this PR for the next release because I wasn't sure that we could merge it on time for 22.1.

I did all the work to get this ready to merge into trunk from a dependencies point of view, but didn't account for changes in what the code would have to do after updating to the latest Gutenberg:

Screenshot 2023-04-03 at 10 31 26 am

The method Xcode suggests to insert is

func gutenbergDidRequestFetch(path: String, completion: @escaping (Result<Any, NSError>) -> Void)

But I'm not sure what to write into it.

Thanks @mokagio for working on the PR to have it ready 🙇 . This PR depends on the following Gutenberg changes:

We'd need first to merge those in order to solve the build failures. Once they are merged, we'll bump the Gutenberg Mobile reference and we could consider this PR ready to merge.

However, given this PR was intended for 22.2, I think it's fine for me to wait for your input on how to get this to compile and merge. Thanks!

Yep, we can wait, especially as the Gutenberg PRs aren't approved yet. I hope to have them approved by this week to include them in 22.2 🤞 .

@mokagio
Copy link
Contributor

mokagio commented Apr 3, 2023

This PR depends on the following Gutenberg changes:

🤦‍♂️ ... It was written in the description.

Thanks! @fluiddot

@fluiddot
Copy link
Contributor Author

fluiddot commented Apr 3, 2023

🤦‍♂️ ... It was written in the description.

Yep, although I think I could have been more explicit in saying that this PR depends on those ones 😅 .

@fluiddot fluiddot disabled auto-merge April 3, 2023 16:22
Copy link
Contributor

@twstokes twstokes left a comment

Choose a reason for hiding this comment

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

LGTM @fluiddot! 🚀

I ran the .com test and it worked great.

@fluiddot fluiddot merged commit 0057eca into trunk Apr 5, 2023
@fluiddot fluiddot deleted the gutenberg/support-post-request branch April 5, 2023 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants