chore: apply API v8 changes - WPB-15840 #2570
Conversation
…ement-v8-changes-WPB-15840
Test Results5 230 tests 5 228 ✅ 9m 26s ⏱️ Results for commit 2936cd7. ♻️ This comment has been updated with latest results. |
Datadog ReportBranch report: ✅ 0 Failed, 5041 Passed, 2 Skipped, 4m 52.99s Total Time |
wire-ios-mocktransport/Tests/Source/Search/MockTransportSessionAddressBookTests.m
Show resolved
Hide resolved
caldrian
changed the title
| @@ -39,6 +39,7 @@ class CountSelfMLSKeyPackagesActionHandler: ActionHandler<CountSelfMLSKeyPackage | |||
| return nil | |||
| } | |||
|
|
|||
| // TODO: .v8 requires ciphersuite or ciphersuites! | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes it looks like it. The optional query parameter was added from v5 but we never used it. I think it's fine to start using if from v8. The question is... which cipher suite should we pass?
| @@ -39,6 +39,7 @@ class CountSelfMLSKeyPackagesActionHandler: ActionHandler<CountSelfMLSKeyPackage | |||
| return nil | |||
| } | |||
|
|
|||
| // TODO: .v8 requires ciphersuite or ciphersuites! | |||
There was a problem hiding this comment.
Yes it looks like it. The optional query parameter was added from v5 but we never used it. I think it's fine to start using if from v8. The question is... which cipher suite should we pass?
| if apiVersion < .v8 { | ||
| path = "/conversations/\(identifier)/message-timer" | ||
| } else { | ||
| guard let domain = conversation.domain else { fatal("conversation has no domain") } // TODO: unit test? |
There was a problem hiding this comment.
I would suggest to fallback to the local domain, which there should be (even though it's optional 🙃).
There was a problem hiding this comment.
I wouldn't crash the app if there was no domain
| @@ -49,9 +49,17 @@ public extension ZMConversation { | |||
| guard let conversationId = remoteIdentifier?.transportString() | |||
| else { return completion(.failure(ReadReceiptModeError.noConversation)) } | |||
|
|
|||
| let path: String | |||
| if apiVersion >= .v8 { | |||
| guard let domain else { return completion(.failure(ReadReceiptModeError.noConversation)) } | |||
There was a problem hiding this comment.
suggestion: also here it'd be good to fallback on the local domain.
| @@ -60,7 +60,7 @@ extension RegistrationStrategy: ZMSingleRequestTranscoder { | |||
| userInfoParser?.upgradeToAuthenticatedSession(with: $0) | |||
| } | |||
| registrationStatus.success() | |||
| } else { | |||
| } else { // TODO: handle new errors? 400: "invalid-domain" and 403: "condition-failed" | |||
There was a problem hiding this comment.
If we don't specifically handle these new errors, does the registration status still consider it an error? The login and registration flows are being re-written so probably doesn't make much sense to handle these errors if not absolutely necessary.
| @@ -77,7 +77,7 @@ public final class TeamInvitationRequestStrategy: AbstractRequestStrategy { | |||
| ) | |||
|
|
|||
| request.add(ZMCompletionHandler(on: managedObjectContext, block: { [weak self] response in | |||
| self?.processResponse(response, for: email) | |||
| self?.processResponse(response, for: email) // TODO: handle new error? 403 "condition-failed" | |||
There was a problem hiding this comment.
Is there something to handle or would it end up as an error log?
There was a problem hiding this comment.
Agree, I don't think we have tickets or design changes to handle this error
…ement-v8-changes-WPB-15840
Issue
A few endpoints changed with v8, this PR make the adjustments.
Checklist
[WPB-XXX].UI accessibility checklist
If your PR includes UI changes, please utilize this checklist: