Application password experiment: Update error handling for direct requests #16075
Conversation
|
|
RafaelKayumov
left a comment
RafaelKayumov
left a comment
There was a problem hiding this comment.
LGTM. Testing steps work as described. Tested all snippets for 4th scenario. Requests fell back to JP proxy and reloaded orders list. For the 5th scenario snippet it took a few order list load re-triggers to completely fallback to JP.
| private var appPasswordFailures: [Int64: Int] = [:] | ||
|
|
||
| /// Keeps track of retried requests when direct requests fail | ||
| private var retriedJetpackRequests: [RetriedJetpackRequest] = [] |
There was a problem hiding this comment.
Is it planned to clean-up the collection of retried requests? At the moment I see that specific retried requests only get removed in flagSiteAsUnsupportedForAppPasswordIfNeeded. I wonder if we should also remove those upon successful completion after a retry?
There was a problem hiding this comment.
In flagSiteAsUnsupportedForAppPasswordIfNeeded the sites are flagged when retry succeeds (hence the check if failure == nil). The cleanup is done outside of the if check, meaning it's always done disregard of the result of the retry. @RafaelKayumov please let me know if there's still something missing.
There was a problem hiding this comment.
@itsmeichigo Ok, makes sense now. I missed IfNeeded part of naming in my logic. So the flagSiteAsUnsupportedForAppPasswordIfNeeded is executed in the happy case as well skipping the failure handling and going directly to request removal.
Ship it
Part of WOOMOB-1126
Description
This PR adds retry logic for REST requests in
AlamofireNetworkto fall back to Jetpack proxied requests and mark sites as unsupported for application passwords following the plan in pe5sF9-4ye-p2#comment-4787.Most changes come from unit tests.
Testing steps
Run test cases 4 & 5 in this test plan: pe5sF9-4Am-p2.
Testing information
Screenshots
No UI changes.
RELEASE-NOTES.txtif necessary.