[ABW-3259] NPS survey upload using body params #1200
Conversation
|
|
||
| do { | ||
| _ = try await httpClient.executeRequest(urlRequest) | ||
| _ = try await httpClient.executeRequest(urlRequest) { statusCode in | ||
| [200, 202].contains(statusCode) |
There was a problem hiding this comment.
This request returns a 202 response code. For other requests, we want to be more restrictive and accept only a 200 response code.
| let (data, urlResponse) = try await session.data(for: request) | ||
|
|
||
| guard let httpURLResponse = urlResponse as? HTTPURLResponse else { | ||
| throw ExpectedHTTPURLResponse() | ||
| } | ||
|
|
||
| guard httpURLResponse.statusCode == BadHTTPResponseCode.expected else { | ||
| let isSuccessStatusCode = isStatusCodeValid?(httpURLResponse.statusCode) ?? (httpURLResponse.statusCode == BadHTTPResponseCode.expected) |
There was a problem hiding this comment.
not part of this PR, but wondering why are we checking the success code from something like BadHTTPResponseCode 😅
There was a problem hiding this comment.
Good question :)
@GhenadieVP do you think we can move the expected success code elsewhere?
There was a problem hiding this comment.
I would say we can completely remove expected and butExpected from BadHTTPResponseCode
| @@ -5,5 +5,5 @@ public struct HTTPClient: Sendable, DependencyKey { | |||
|
|
|||
| // MARK: HTTPClient.ExecuteRequest | |||
| extension HTTPClient { | |||
| public typealias ExecuteRequest = @Sendable (URLRequest) async throws -> Data | |||
| public typealias ExecuteRequest = @Sendable (_ request: URLRequest, _ isStatusCodeValid: ((Int) -> Bool)?) async throws -> Data | |||
There was a problem hiding this comment.
instead of a closure to determine whether a status code is considered valid, I would have it receive an array of accepted status codes. We could use just Int or define an enum with the predefined values (example).
There was a problem hiding this comment.
Either what @matiasbzurovski suggested, or return the status code along with the Data and let the consumer decide if it is valid or not.
There was a problem hiding this comment.
@matiasbzurovski not sure if we need such an ample enum, but an array of Int sounds good
There was a problem hiding this comment.
we may not want to add all the cases (since we are not mapping the int value to the corresponding case anyway), but if define at least the expected ones (200 & 202 for now) we could refer it directly (HttpStatusCode.ok vs BadHTTPResponseCode.expected)
| @@ -70,7 +70,7 @@ extension GatewayAPIClient { | |||
| urlRequest.timeoutInterval = timeoutInterval | |||
| } | |||
|
|
|||
| let data = try await httpClient.executeRequest(urlRequest) | |||
| let data = try await httpClient.executeRequest(urlRequest, nil) | |||
There was a problem hiding this comment.
We can define a helper method to avoid sending nil most of the times
extension HTTPClient {
func executeRequest(_ request: URLRequest) async throws -> Data {
try await executeRequest(request, nil)
}
}or if you follow my previous comment:
extension HTTPClient {
func executeRequest(_ request: URLRequest) async throws -> Data {
try await executeRequest(request, [.ok])
}
}
| @@ -33,3 +31,18 @@ public struct ResponseDecodingError: Swift.Error { | |||
| self.error = error | |||
| } | |||
| } | |||
|
|
|||
| // MARK: - HTTPStatusCode | |||
| public enum HTTPStatusCode: Int, Error { | |||
Jira ticket: ABW-3259
Description
This PR changes the NPS survey upload from using query parameters to body parameters. Refiner requires this to ensure the NPS score is treated as a number instead of a string.