-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add Android Request Interception #30
Conversation
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.
I attempted testing this quickly by rebasing it atop #29 and using wordpress-mobile/WordPress-Android#21223 to build WordPress-Android with these changes.
When testing it this way—and without conforming to GutenbergRequestInterceptor
—I experience CORS errors for all requests for the site's REST API. Below are screenshots of the network failures and one of the requests with and without these changes applied.
I didn't spend time debugging it aside from inspecting the requests. Maybe some of the headers are erroneously discarded by the interception logic? WDYT?
Hopefully #29 and wordpress-mobile/WordPress-Android#21223 merge later today, which will simplify further testing. Update: The referenced pull requests are now merged.
Address the lint warning.
ac9d15f
to
8ed393e
Compare
Friendly note: I rebased this atop the latest |
* fix: Rely upon the WebView for completing non-intercepted requests Previously, a manual request was created for _all_ requests. This was unnecessary and resulted in header values being unexpectedly discarded. * refactor: Remove unused imports * refactor: Remove redundant semicolon * refactor: Remove unused import * refactor: Remove unused view parameter This was added to mirror the `shouldInterceptRequest` method signature, but is unnecessary complexity. * refactor: Separate check and handle request methods Mirror the iOS method structure.
The networking logic that required these dependencies was hoisted to the host app.
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.
Changes look sound, testing plan succeeds for me. 🚀
Noting that I removed (69b8e54) the previously added okhttp
dependencies from this change set, as the networking logic was hoisted to the host app.
This initial implementation allows us to inject authentication at runtime if needed.
You can test it by making the demo app activity conform to
GutenbergRequestInterceptor
, then adding:Once you've added a bearer token, copy the URL to an image on a private blog, and insert a new image block with that URL. The editor will now display it.