Skip to content

OfferingsManager/OfferingsFactory: using OfferingsResponse#1435

Merged
NachoSoto merged 1 commit into
mainfrom
offerings-response
Apr 22, 2022
Merged

OfferingsManager/OfferingsFactory: using OfferingsResponse#1435
NachoSoto merged 1 commit into
mainfrom
offerings-response

Conversation

@NachoSoto

@NachoSoto NachoSoto commented Mar 31, 2022

Copy link
Copy Markdown
Contributor

Depends on #1437.
This takes full advantage of Decodable when decoding and creating Offerings.

@taquitos taquitos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. 🥅 ⚽ 🐐 🎉

@NachoSoto NachoSoto force-pushed the wip-decodable-3-rebased branch from 4e0dfa8 to c53e94b Compare April 1, 2022 00:31
@NachoSoto NachoSoto force-pushed the offerings-response branch from c6c0680 to 957cb7b Compare April 1, 2022 00:31
@NachoSoto NachoSoto force-pushed the wip-decodable-3-rebased branch from c53e94b to 7bfbd17 Compare April 1, 2022 20:52
@NachoSoto NachoSoto force-pushed the offerings-response branch from 957cb7b to 8d75181 Compare April 1, 2022 21:01
let request = HTTPRequest(method: .get, path: .getOfferings(appUserID: appUserID))

httpClient.perform(request, authHeaders: self.authHeaders) { (response: HTTPResponse<[String: Any]>.Result) in
httpClient.perform(request, authHeaders: self.authHeaders) { (response: HTTPResponse<OfferingsResponse>.Result) in

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all it takes to get HTTPClient to deserialize the response 🤓

@NachoSoto NachoSoto force-pushed the wip-decodable-3-rebased branch 2 times, most recently from 0e34e67 to 87a2c90 Compare April 5, 2022 18:57
NachoSoto added a commit that referenced this pull request Apr 5, 2022
This will become the response type used everywhere by `OfferingsManager` in #1435.
@NachoSoto NachoSoto force-pushed the offerings-response branch from 8d75181 to 743c458 Compare April 5, 2022 19:10
@NachoSoto NachoSoto force-pushed the wip-decodable-3-rebased branch from 87a2c90 to 27a3ea3 Compare April 6, 2022 21:35
@NachoSoto NachoSoto force-pushed the offerings-response branch from 743c458 to 8430670 Compare April 6, 2022 21:36
@NachoSoto NachoSoto force-pushed the wip-decodable-3-rebased branch from a6e6d43 to 826abb8 Compare April 8, 2022 01:23
@NachoSoto NachoSoto force-pushed the offerings-response branch from 8430670 to 921d8a8 Compare April 8, 2022 18:16
@NachoSoto NachoSoto force-pushed the wip-decodable-3-rebased branch from 4a6ba22 to 7445957 Compare April 8, 2022 21:36
@NachoSoto NachoSoto force-pushed the offerings-response branch from 921d8a8 to da91c0c Compare April 8, 2022 21:40
@NachoSoto NachoSoto force-pushed the wip-decodable-3-rebased branch from 7445957 to 8e0c0be Compare April 8, 2022 22:49
@NachoSoto NachoSoto force-pushed the offerings-response branch 4 times, most recently from ad774e8 to 7456ec6 Compare April 11, 2022 22:29
@NachoSoto NachoSoto changed the title [WIP] Using OfferingsResponse in OfferingsManager OfferingsManager/OfferingsFactory: using OfferingsResponse Apr 11, 2022
@NachoSoto NachoSoto changed the base branch from wip-decodable-3-rebased to http-error April 11, 2022 22:30
@NachoSoto NachoSoto requested review from a team and taquitos April 11, 2022 22:30
@NachoSoto

Copy link
Copy Markdown
Contributor Author

This is finished now 🎉

@@ -17,66 +17,66 @@ import StoreKit

class OfferingsFactory {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No more magic strings in this file 😄

}

func offerings(appUserID: String, completion: ((Offerings?, Error?) -> Void)?) {
func offerings(appUserID: String, completion: ((Result<Offerings, Error>) -> Void)?) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finished using Result around OfferingsManager too.

}

func extractProductIdentifiers(fromOfferingsData offeringsData: [String: Any]) -> Set<String> {
// Fixme: parse Data directly instead of converting from Data to Dictionary back to Data

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Comment on lines -87 to -93
expect(offeringA["identifier"] as? String) == "offering_a"
expect(offeringA["description"] as? String) == "This is the base offering"
expect(packageA["identifier"]) == "$rc_monthly"
expect(packageA["platform_product_identifier"]) == "monthly_freetrial"
expect(packageB["identifier"]) == "$rc_annual"
expect(packageB["platform_product_identifier"]) == "annual_freetrial"
expect(result?.value?["current_offering_id"] as? String) == "offering_a"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No more magic strings

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉🎉🎉

@@ -202,11 +199,15 @@ class OfferingsTests: XCTestCase {
}

func testOfferingsIsNilIfNoOfferingCanBeCreated() throws {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kind of an end-to-end test including deserialization too.

@NachoSoto NachoSoto force-pushed the offerings-response branch from 7456ec6 to 62eaa0e Compare April 12, 2022 19:46
Base automatically changed from http-error to wip-decodable-3-rebased April 12, 2022 20:58
@NachoSoto NachoSoto force-pushed the offerings-response branch from 62eaa0e to 88eb5be Compare April 12, 2022 21:06
Base automatically changed from wip-decodable-3-rebased to main April 12, 2022 22:11
NachoSoto added a commit that referenced this pull request Apr 12, 2022
…rkError` and `BackendError` (#1425)

Closes #695 and finishes [CF-195]. 
Depends on ~~#1431, #1432, #1433, #1437~~.

## Goals:

- [x] Handle all requests / response / deserialization errors in `HTTPClient`. Clients of `HTTPClient` will only have to handle the "happy" path: #1431
- [x] `HTTPClient` shouldn't return `Result.success` with failed responses, forcing clients to verify the response is actually a failure: #1431
- [x] Abstract error response deserialization / error creation: #1427
- [x] Abstract attribute error parsing: #1427 
- [x] Move response deserialization to `HTTPClient` based on a `ResponseType: Decodable` type, so the completion block will simply return a `Result<HTTPResponse<ResponseType>, Error>`
- [x] `ETagManager` should store response `Data` instead of a deserialized `[String: Any]`
- [x] Improved error types in `HTTPClient` to fix tests: #1437
- [x] Dealt with non-backwards compatible `ETagManager: #1438

## Changes:
- Replaced `HTTPResponse`'s body from `[String: Any]` to a generic `HTTPResponseBody`.
- Created `HTTPResponseBody` to abstract `Decodable` and provide some default implementations for types like `Data,` `[String: Any]` (for backwards compatibility to types that aren't `Decodable` yet), and `Decodable` itself.
- New `HTTPResponse.Result` typealias (`Result<HTTPResponse<HTTPResponseBody>, Error>`) used everywhere. This will allow replacing `Error` with a more specific `Error` so we can forward known typed errors, and make sure that we don't end up with the wrong error type, or with a very complex error hierarchy and the details buried in `underlyingError`.
- Each layer (only a few for now) has its own error type: `NetworkError`, `BackendError`, `OfferingsManager.Error`.
- `HTTPClient` for example has to produce `NetworkError`, operations produce `BackendError`
- The parent `BackendError` can have specific errors like `.missingAppUserID`, but also be simply a child error `case networkError(NetworkError)`
- All of these conform to a `ErrorCodeConvertible`, so there is a single point of code that converts from simple and readable errors (like `BackendError.emptySubscriberAttributes`, `.unexpectedBackendResponse(.loginResponseDecoding)`) into errors with all the context using `ErrorUtils`
- Converted `DNSError` into `NetworkError.dnsError`. Its functionality remains unchanged.
- Removed `Backend.RCSuccessfullySyncedKey` and `ErrorDetails.finishableKey` in favor of tested properties on `NetworkError`

### Leftovers

There's a few things I've decided to not finish for now:
- [ ] `CustomerInfo` still does't conform to `Decodable` (manual deserialization is still supported by the current system): #1496
- `SubscriberAttributeDict` could also be `Codable`
- [x] Replace `OfferingsFactory` with `Decodable`: #1435

[CF-195]: https://revenuecats.atlassian.net/browse/CF-195?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
@NachoSoto NachoSoto force-pushed the offerings-response branch 3 times, most recently from 97fb1d0 to 80e56be Compare April 14, 2022 12:37
@NachoSoto

Copy link
Copy Markdown
Contributor Author

This is still waiting for review 🤓

@NachoSoto NachoSoto force-pushed the offerings-response branch from 80e56be to 1af39a9 Compare April 20, 2022 20:26
This takes full advantage of `Decodable` when decoding and creating `Offerings`.
@NachoSoto NachoSoto force-pushed the offerings-response branch from 1af39a9 to 7aafc8f Compare April 21, 2022 17:07
Comment on lines -87 to -93
expect(offeringA["identifier"] as? String) == "offering_a"
expect(offeringA["description"] as? String) == "This is the base offering"
expect(packageA["identifier"]) == "$rc_monthly"
expect(packageA["platform_product_identifier"]) == "monthly_freetrial"
expect(packageB["identifier"]) == "$rc_annual"
expect(packageB["platform_product_identifier"]) == "annual_freetrial"
expect(result?.value?["current_offering_id"] as? String) == "offering_a"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉🎉🎉

Comment on lines +18 to +38
struct OfferingsResponse {

struct Offering {

struct Package {

let identifier: String
let platformProductIdentifier: String

}

let identifier: String
let description: String
let packages: [Package]

}

let currentOfferingId: String?
let offerings: [Offering]

}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍

@NachoSoto NachoSoto merged commit 7530a67 into main Apr 22, 2022
@NachoSoto NachoSoto deleted the offerings-response branch April 22, 2022 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants