-
Notifications
You must be signed in to change notification settings - Fork 121
Require selected site for AlamofireNetwork
#16124
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
Require selected site for AlamofireNetwork
#16124
Conversation
| public init(siteID: Int64, | ||
| credentials: Credentials?) { | ||
| credentials: Credentials?, | ||
| selectedSite: AnyPublisher<JetpackSite?, Never>? = nil) { |
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.
This has nil as the default value to avoid updating all related unit tests.
|
|
Generated by 🚫 Danger |
|
@iamgabrielma I see that you're already reviewing this PR. I just added some changes above to inject app password support availability (from remote feature flag and experimental feature flag) to POS's networks. This PR is ready for a new testing round 🙇 |
Great, thanks for the updates! I tested all POS flows and I see no issues, when logging into the site via a JP-connected account and enabling application passwords in Settings > Experimental features, all requests appear to be done through direct requests rather than tunneled through the JP proxy 👍
Aside from POS, I smoke tested a bit the app and requests for Blaze and some parts of Analytics (analytics reports) do seem to go through a tunneled request and return a 404 for some endpoints. Is this expected? I believe it is since are routes for gifts and bundles, which I do not have on the test site, but just to be sure.
ie: |
|
Thanks @iamgabrielma for the review!
Yeah these are expected. Blaze endpoints requires DotCom requests instead of direct requests. Analytics for gifts and bundles analytics fails if you don't have those plugins on your sites, and requests are retried with JP proxy to ensure the failures were not from the direct requests. I noticed that the app does not switch network requests immediately upon toggling the experimental flag. I added a commit 43e4592 to persist |
|
Works great, toggling the option switches between tunneled/direct requests at runtime 🚀 |
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues:
|



Closes WOOMOB-1332
Description
This PR removes the default nil value for
selectedSiteinAlamofireNetwork. It is now required to explicitly decide if network switching should be supported when the network is created.Network switching is disabled for the following flows:
For POS flows, since
AlamofireNetworkis created separately and there were no clear disagreement on the network switching in p91TBi-dve-p2, I updated all the services to inject selected site and app password support availability to their networks.Testing steps
Testing information
Screenshots
N/A
RELEASE-NOTES.txtif necessary.