Skip to content
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

subscription period #256

Merged
merged 38 commits into from
Jun 5, 2020
Merged

subscription period #256

merged 38 commits into from
Jun 5, 2020

Conversation

aboedo
Copy link
Member

@aboedo aboedo commented May 27, 2020

Adds subscripion period to the POST receipt request.

Changes summary:

  • moved the test files into groups that mimic the ones for the main repo, for organization purposes.
  • added the fields from https://github.com/RevenueCat/khepri/pull/1539 to the post receipt request:
    • normal_duration
    • intro_duration
    • trial_duration
  • The number of parameters for the post receipt endpoint was getting pretty unwieldy, and would change any time we read a new field from SKProducts. I extracted a few of those fields into a new class, RCProductInfo. This class also knows how to convert itself into a backend-compatible dictionary, and how to generate a cache key.
  • moved the logic that reads individual fields from SKProducts and does whatever transformations are needed for the backend endpoint into RCProductInfoExtractor. This also abstracts the logic of which fields can be read in which iOS versions.
  • Some minor refactors here and there. But the main changes are the ones described above.

@aboedo aboedo self-assigned this May 27, 2020
@aboedo aboedo force-pushed the feature/subscription_period branch from b121e8a to 110a9b8 Compare May 29, 2020 23:47
Comment on lines -12 to +16
Purchases: 0ed262862309e8aa3348b525755c8e84f53b1924
Purchases: 6bf68ed9926fefa775649bec025878c4de9175b5

PODFILE CHECKSUM: b6ad00ca1e6fc400e0898ad2ac4a293958616d2e

COCOAPODS: 1.8.4
COCOAPODS: 1.9.1
Copy link
Member Author

Choose a reason for hiding this comment

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

just the result of upgrading cocoapods and running pod install.

Comment on lines -16 to +18
2D8DB3502407333100BE3D31 /* NSError+RCExtensionsTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 37E3550459807FF5636B2667 /* NSError+RCExtensionsTests.swift */; };
2DD448FF24088473002F5694 /* RCPurchases+SubscriberAttributes.h in Headers */ = {isa = PBXBuildFile; fileRef = 2DD448FD24088473002F5694 /* RCPurchases+SubscriberAttributes.h */; settings = {ATTRIBUTES = (Private, ); }; };
2DD4490024088473002F5694 /* RCPurchases+SubscriberAttributes.m in Sources */ = {isa = PBXBuildFile; fileRef = 2DD448FE24088473002F5694 /* RCPurchases+SubscriberAttributes.m */; };
2DEB9767247DB46900A92099 /* RCISOPeriodFormatter.h in Headers */ = {isa = PBXBuildFile; fileRef = 2DEB9766247DB46900A92099 /* RCISOPeriodFormatter.h */; settings = {ATTRIBUTES = (Private, ); }; };
Copy link
Member Author

Choose a reason for hiding this comment

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

two main things happening in this file:

  • moved test files intro groups, essentially a follow-up of Reorganize folders #242
  • added new files: RCISOPeriodFormatter, RCProductInfo, RCProductInfoExtractor

Comment on lines -19 to -24
typedef NS_ENUM(NSInteger, RCPaymentMode) {
RCPaymentModeNone = -1,
RCPaymentModePayAsYouGo = 0,
RCPaymentModePayUpFront = 1,
RCPaymentModeFreeTrial = 2
};
Copy link
Member Author

Choose a reason for hiding this comment

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

moved to RCProductInfo

Comment on lines -57 to +48
productIdentifier:(nullable NSString *)productIdentifier
price:(nullable NSDecimalNumber *)price
paymentMode:(RCPaymentMode)paymentMode
introductoryPrice:(nullable NSDecimalNumber *)introductoryPrice
currencyCode:(nullable NSString *)currencyCode
subscriptionGroup:(nullable NSString *)subscriptionGroup
discounts:(nullable NSArray<RCPromotionalOffer *> *)discounts
productInfo:(nullable RCProductInfo *)productInfo
Copy link
Member Author

Choose a reason for hiding this comment

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

abstracted parameters into object

extern NSErrorUserInfoKey const RCSuccessfullySyncedKey;
extern NSString * const RCAttributeErrorsKey;
extern NSString * const RCAttributeErrorsResponseKey;

API_AVAILABLE(ios(11.2), macos(10.13.2))
RCPaymentMode RCPaymentModeFromSKProductDiscountPaymentMode(SKProductDiscountPaymentMode paymentMode);
Copy link
Member Author

Choose a reason for hiding this comment

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

moved to RCProductInfo.

I'm not a huge fan of the format, I was going to turn it into a static method instead but changing it didn't seem to add much value anyway

Comment on lines +123 to +136
- (NSDictionary *)productDurationsAsDictionary {
NSMutableDictionary *durations = [[NSMutableDictionary alloc] init];

if (self.normalDuration) {
durations[@"normal_duration"] = self.normalDuration;
}
if (self.introDurationType == RCIntroDurationTypeIntroPrice) {
durations[@"intro_duration"] = self.introDuration;
}
if (self.introDurationType == RCIntroDurationTypeFreeTrial) {
durations[@"trial_duration"] = self.introDuration;
}

return durations;
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the only big new thing here

return self;
}

- (RCProductInfo *)extractInfoFromProduct:(SKProduct *)product {
Copy link
Member Author

Choose a reason for hiding this comment

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

most of this file was extracted from RCPurchases and cleaned up so that it's easier to follow

return introPrice;
}

- (NSString *)extractNormalDurationForProduct:(SKProduct *)product {
Copy link
Member Author

Choose a reason for hiding this comment

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

this and the method after it are the real new things here

@@ -4,16 +4,54 @@
//

class MockSKProduct: SKProduct {
Copy link
Member Author

Choose a reason for hiding this comment

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

we had two separate mocks for SKProduct, MockSKProduct and MockProduct.
I unified them, updated tests and added a whole bunch of fields / mock stuff


var receivedProductInfo = productInfoExtractor.extractInfo(from: product)

expect(receivedProductInfo.currencyCode) == "UYU"
Copy link
Member Author

Choose a reason for hiding this comment

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

Uruguayan Peso 💵 🇺🇾

@aboedo aboedo marked this pull request as ready for review June 4, 2020 00:19
@aboedo aboedo requested a review from vegaro June 4, 2020 00:20
Copy link
Contributor

@vegaro vegaro left a comment

Choose a reason for hiding this comment

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

This PR looks great. Left a couple minor comments regarding the tests.

Thanks so much for doing all that cleanup!!!!

Purchases/Networking/RCBackend.m Outdated Show resolved Hide resolved
error:error];
}];
}
}];
Copy link
Contributor

Choose a reason for hiding this comment

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

wow this cleanup. awesome thanks!!!!

let productInfo = RCProductInfo(productIdentifier: "product_id",
paymentMode: .none,
currencyCode: "UYU",
price: 15.99,
Copy link
Contributor

Choose a reason for hiding this comment

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

not a biggie, but I would still have these as nil so that the only difference between the productInfos is the discounts

Copy link
Member Author

@aboedo aboedo Jun 5, 2020

Choose a reason for hiding this comment

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

yeah, I did think about that when writing. the tests are definitely more noisy this way.
The reason I went this way was:

  • I wanted only arguments that were truly nullable (i.e.: you might not be able to extract them from an SKProduct) to be nullable.
  • I didn't want properties to be editable unless they needed to (this part could be done by doing a protected header, though)
  • Even though you can initialize RCProductInfo with no arguments (because it inherits from NSObject), that can lead to some pretty bad gotchas with swift:

When you initialize with no arguments, everything is nil. However, swift will assume that the object can't be nil unless nullable is specified (or no nullability is specified in the entire file).

For example, if I do:

let productInfo = RCProductInfo()
let price = productInfo.price

That will crash, because price is nil, but swift didn't expect that. I won't be able to even check for it, since doing

let productInfo = RCProductInfo()
if productInfo.price != nil {

will raise a compiler warning saying that price can't be nil.

To get around this, I usually disable the no-params constructor, I forgot about it this time.
To disable it, you do NS_UNAVAILABLE (example here which will raise a compiler warning when using that constructor.

That way you have control over the properties to make sure that they're never called from swift without having initialized them properly.

Thinking about possible fixes for this, I think the best workaround is to have a protected header that allows for these properties to be modified, and then a single function that always creates a valid ProductInfo object, so that I can do:

let productInfo = createProductInfo()
productInfo.whateverProperty = "the value I want to test"

I'll do that refactor

Copy link
Member Author

Choose a reason for hiding this comment

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

done!

PurchasesTests/Purchasing/ProductInfoExtractorTests.swift Outdated Show resolved Hide resolved
PurchasesTests/Purchasing/ProductInfoExtractorTests.swift Outdated Show resolved Hide resolved
PurchasesTests/Purchasing/ProductInfoTests.swift Outdated Show resolved Hide resolved
PurchasesTests/Purchasing/ProductInfoTests.swift Outdated Show resolved Hide resolved
PurchasesTests/Purchasing/ProductInfoTests.swift Outdated Show resolved Hide resolved
@aboedo aboedo merged commit bc55495 into develop Jun 5, 2020
@aboedo aboedo deleted the feature/subscription_period branch June 5, 2020 20:11
@aboedo aboedo mentioned this pull request Jul 13, 2021
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.

2 participants