-
Notifications
You must be signed in to change notification settings - Fork 316
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
Better non-subscriptions support #281
Better non-subscriptions support #281
Conversation
Purchases/Public/RCPurchaserInfo.m
Outdated
|
||
@interface RCPurchaserInfo () | ||
|
||
@property (nonatomic) NSDictionary<NSString *, NSDate *> *expirationDatesByProduct; | ||
@property (nonatomic) NSDictionary<NSString *, NSDate *> *purchaseDatesByProduct; | ||
@property (nonatomic) NSSet<NSString *> *nonConsumablePurchases; | ||
@property (nonatomic) NSArray<RCTransaction *> *nonConsumableTransactionsList; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List
is redundant with the type.
|
||
@objc public class PurchaserInfoHelper: NSObject { | ||
|
||
@objc public class func nonConsumableTransactionsList(withNonSubscriptionsTransactionsDictionary data: Dictionary<String, Array<Transaction>>) -> Array<Transaction> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
withNonSubscriptionsTransactionsDictionary
-> withNonSubscriptionTransactionsDictionary
Purchases/Public/RCPurchaserInfo.m
Outdated
@@ -82,7 +85,8 @@ - (void)initializePurchasesAndEntitlementsWithSubscriberData:(NSDictionary *)sub | |||
subscriptions:(NSDictionary *)subscriptions { | |||
NSDictionary<NSString *, NSArray *> *nonSubscriptions = subscriberData[@"non_subscriptions"]; | |||
self.nonConsumablePurchases = [NSSet setWithArray:[nonSubscriptions allKeys]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait. How do we go from "non-subscriptions" to "non-consumables"?
Purchases/Public/RCPurchaserInfo.m
Outdated
@@ -82,7 +85,8 @@ - (void)initializePurchasesAndEntitlementsWithSubscriberData:(NSDictionary *)sub | |||
subscriptions:(NSDictionary *)subscriptions { | |||
NSDictionary<NSString *, NSArray *> *nonSubscriptions = subscriberData[@"non_subscriptions"]; | |||
self.nonConsumablePurchases = [NSSet setWithArray:[nonSubscriptions allKeys]]; | |||
|
|||
self.nonConsumableTransactionsByProduct = [PurchaserInfoHelper nonConsumableTransactionsMapWithNonSubscriptionsDictionary:subscriberData dateFormatter:dateFormatter]; | |||
self.nonConsumableTransactionsList = [PurchaserInfoHelper nonConsumableTransactionsListWithNonSubscriptionsTransactionsDictionary:self.nonConsumableTransactionsByProduct]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Widening from that. How does a list of "non consumable transactions" improve our consumable support?
Purchases/Public/RCPurchaserInfo.m
Outdated
|
||
NSDictionary<NSString *, NSArray *> *nonSubscriptionsData = subscriberData[@"non_subscriptions"]; | ||
self.nonConsumablePurchases = [NSSet setWithArray:[nonSubscriptionsData allKeys]]; | ||
self.nonSubscriptionTransactions = [PurchaserInfoHelper initNonSubscriptionTransactionsWith:nonSubscriptionsData dateFormatter:dateFormatter]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something weird happening in the name here, right? should this be initWithNonSubscriptionTransactions
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't initWithNonSubscriptionTransactions
indicate that a PurchaserInfoHelper
is being initialized? In this case I want that static function to initialize the nonSubscriptionTransactions
of the PurchaserInfo
. What name do you think works better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oohhh I see, my bad.
In that case, I would steer clear of init
so it's not misinterpreted as an initializer (that's what I thought it was).
how about configureNonSubscriptionTransactions:dateFormatter:
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed something again, I need to get more sleep. Just realized that this returns new objects.
In that case, I don't think either init
or configure
really apply - why not just nonSubscriptionTransactions(fromSubscriptionsData:dateFormatter)
or
nonSubscriptionTransactions(withSubscriptionsData:dateFormatter)
(I think this was used at some point?)
|
||
@objc public class PurchaserInfoHelper: NSObject { | ||
|
||
@objc public class func initNonSubscriptionTransactions(with data: Dictionary<String, Array<Dictionary<String, Any>>>, dateFormatter: DateFormatter) -> Array<Transaction> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for something more idiomatic, this should be
@objc public init(withNonSubscriptionTransactions nonSubscriptionTransactions: [String: [[String: Any]],
dateFormatter: DateFormatter)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't that be an init for the PurchaserInfoHelper
? I was thinking about making this a helper class with static functions. Maybe I can rename this function to be extractNonSubscriptionTransactions(from data:
? Or maybe this shouldn't be a static class and it should be similar to what you did in RCProductInfoExtractor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, my bad, I thought this was initializing purchaserInfoHelper.
@objc public class PurchaserInfoHelper: NSObject { | ||
|
||
@objc public class func initNonSubscriptionTransactions(with data: Dictionary<String, Array<Dictionary<String, Any>>>, dateFormatter: DateFormatter) -> Array<Transaction> { | ||
data.flatMap { (productId: String, transactionData: Array<Dictionary<String, Any>>) -> Array<Transaction> in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Array<Dictionary<String, Any>>
-> [[String: Any]]
Array<Transaction>
-> [Transaction]
super.init() | ||
} | ||
|
||
internal init(with data: Dictionary<String, Any>, productId: String, dateFormatter: DateFormatter) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dictionary<String, Any>
-> [String: Any]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we rename data
to something more descriptive of what it should contain?
developers could also expect data
to be of type Data
and not a dict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about serverResponse
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds great
let productId: String | ||
let purchaseDate: Date | ||
|
||
internal init(transactionId: String, productId: String, purchaseDate: Date) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internal is the default, we might want to omit it
|
||
import Foundation | ||
|
||
@objc(RCTransaction) public class Transaction: NSObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all objects that inherit from NSObject
have a public init
with no arguments.
If we want to prevent that from being used, maybe we can do something like
required init?() { fatalError("init() has not been implemented") }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haven't tested that, though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to provide some context on the why:
there's a bit of a gotcha when it comes to swift-objc interoperability and initialization, and nullability.
if you call init() from objc, that will work, because this is an NSObject. however, the object's properties will not have been initialized, so revenuecatId for example will be nil.
but since the type is declared as non-nil, you won't be able to check against that - if you do
if revenuecatId != nil {
the compiler will complain because the object isn't nullable. so you'll be in a bad place.
Here's a blog post that goes into a bit more detail in case you're curious:
https://fabiancanas.com/blog/2020/1/9/swift-undefined-behavior.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation! Unfortunately it doesn't work, it says "Failable initializer 'init()' cannot override a non-failable initializer". Since it throws a fatalError
this is actually not a Failable Initializer right? I think it should be this instead:
required override init() { fatalError("init() has not been implemented") }
Should it be a convenience
initializer too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed the override, but yeah, it should be there.
does failable mean that it's an init?()
vs init()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah failable means init?
, but since this is throwing a fatalError, it is not an init?
because it will actually never finish initializing. If I understand correctly, to make it failable it would have to return nil instead of throwing the error, it needs to return something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, for a failable it'd be best practice to return nil if it fails.
I'm good with anything, as long as we make sure that [[Transaction alloc] init];
can't be used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this is the way to go?
} | ||
|
||
internal init(with data: Dictionary<String, Any>, productId: String, dateFormatter: DateFormatter) { | ||
self.revenuecatId = data["id"] as! String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should avoid force casts since they're pretty annoying to debug, the stack trace isn't ideal.
so I'd do
guard let revenueCatId = data["id"] as? String,
let dateString = data["purchase_date"] as? String,
let purchaseDate = dateFormatter.date(from: dateString) else {
fatalError("couldn't initialize Transaction from dictionary. Reason: unexpected format. Dictionary: \(data).")
}
self.revenuecatId = revenuecatId
self.purchaseDate = purchaseDate
You could also get more granular and do multiple guard lets, of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh didn't know you could chain the conditions of the guard let
} | ||
|
||
func testNonSubscriptionsIsCorrectlyCreated() { | ||
let list: Array<Transaction> = PurchaserInfoHelper.initNonSubscriptionTransactions(with: dict, dateFormatter: dateFormatter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the type can be inferred, so I'd leave it out.
But if you do want it to be explicit, I'd replace Array<Transaction>
-> [Transaction]
Purchases/Public/RCPurchaserInfo.m
Outdated
|
||
NSDictionary<NSString *, NSArray *> *nonSubscriptionsData = subscriberData[@"non_subscriptions"]; | ||
self.nonConsumablePurchases = [NSSet setWithArray:[nonSubscriptionsData allKeys]]; | ||
self.nonSubscriptionTransactions = [PurchaserInfoHelper initNonSubscriptionTransactionsWith:nonSubscriptionsData dateFormatter:dateFormatter]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oohhh I see, my bad.
In that case, I would steer clear of init
so it's not misinterpreted as an initializer (that's what I thought it was).
how about configureNonSubscriptionTransactions:dateFormatter:
?
|
||
import Foundation | ||
|
||
@objc public class PurchaserInfoHelper: NSObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since the class doesn't directly use purchaserInfo, maybe TransactionsFactory
would be more accurate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! I thought about using it to build other stuff in the PurchaserInfo, but we can think about that in the future
|
||
@objc public class PurchaserInfoHelper: NSObject { | ||
|
||
@objc public class func initNonSubscriptionTransactions(with data: [String: [[String: Any]]], dateFormatter: DateFormatter) -> [Transaction] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an instance method might make this easier to replace in testing
|
||
import Foundation | ||
|
||
@objc(RCTransaction) public class Transaction: NSObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed the override, but yeah, it should be there.
does failable mean that it's an init?()
vs init()
?
super.init() | ||
} | ||
|
||
internal init(with data: [String: Any], productId: String, dateFormatter: DateFormatter) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internal
is the default, we don't need the keyword here
…urchase-array # Conflicts: # Purchases.xcodeproj/project.pbxproj
@aboedo I believe all comments have been addressed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good! left a few style comments
Purchases/Public/RCPurchaserInfo.h
Outdated
/// Returns all the non-subscription purchases a user has made. The purchases are ordered being the most recent purchase | ||
/// the last item of the array. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this makes the ordering clearer?
/// Returns all the non-subscription purchases a user has made.
/// The purchases are ordered by purchase date in ascending order.
@available(*, unavailable, message: "Use init(transactionId, productId, purchaseDate) instead") | ||
override init() { | ||
fatalError("init() has not been implemented") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
|
||
@objc(RCTransactionsFactory) public class TransactionsFactory: NSObject { | ||
|
||
@objc public func nonSubscriptionTransactions(with subscriptionsData: [String: [[String: Any]]], dateFormatter: DateFormatter) -> [Transaction] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add a line break in between params to stick to 120 chars per line whenever possible
@objc public func nonSubscriptionTransactions(with subscriptionsData: [String: [[String: Any]]],
dateFormatter: DateFormatter) -> [Transaction] {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that Any
can probably be constrained to AnyObject
, right? since it's meant to be used from obj-c.
The difference is that Any
can also be non-objects (for example, a bool), whereas AnyObject
means anything that's a class.
This is a requirement for objective-c NSDictionaries anyway - if you want to insert a bool, you'd have to wrap it in an NSNumber *.
https://medium.com/@mimicatcodes/any-vs-anyobject-in-swift-3-b1a8d3a02e00
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to delete that comment regarding Any
-> AnyObject
- after thinking about it more, I think it may not be worth it. It would make testing slightly trickier since you have to convert primitive types before calling the method, and it's already enforced for obj-c anyway. so.... nevermind. sorry for the noise.
expect { list.count }.to(equal(4)) | ||
|
||
dict.forEach { productId, transactionsData in | ||
let filteredTransactions: Array<Transaction> = list.filter { (transaction: Transaction) in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the type can be inferred here, no need for the explicit type.
Also, the filter is simple enough that it might not merit the explicit variable naming, so this expression could be simplified to
let filteredTransactions = list.filter { $0.productId == productId }
} | ||
|
||
func testNonSubscriptionsIsCorrectlyCreated() { | ||
let list = transactionsFactory.nonSubscriptionTransactions(with: dict, dateFormatter: dateFormatter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
list
-> nonSubscriptionTransactions
let dateFormatter = DateFormatter() | ||
let transactionsFactory = TransactionsFactory() | ||
|
||
let dict = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sampleTransactions
} | ||
expect { filteredTransactions.count }.to(equal(transactionsData.count)) | ||
transactionsData.forEach { dictionary in | ||
let containsTransaction: Bool = filteredTransactions.contains { (transaction: Transaction) in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could also be simplified to
guard let transactionId = dictionary["id"] as String else { fatalError("incorrect dict format") }
let containsTransaction = filteredTransactions.contains { $0.revenueCatId == transactionId }
let containsTransaction: Bool = filteredTransactions.contains { (transaction: Transaction) in | ||
transaction.revenueCatId == dictionary["id"] as! String | ||
} | ||
expect { containsTransaction }.to(beTrue()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shorthand syntax for this is
expect(containsTransaction) == true
also, the { expression }
is meant for blocks of code, for example if you want to check that running a method won't crash, you use
expect { myMethod() }.notTo(raiseException())
for variables that have already been computed, use expect(myValue)
|
||
func testNonSubscriptionsIsEmptyIfThereAreNoNonSubscriptions() { | ||
let list = transactionsFactory.nonSubscriptionTransactions(with: [:], dateFormatter: dateFormatter) | ||
expect { list.count }.to(equal(0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect(list.count) == 0
a couple of equivalent expressions:
expect(list.isEmpty) == true
expect(list).to(beEmpty())
@objc(RCTransactionsFactory) public class TransactionsFactory: NSObject { | ||
|
||
@objc public func nonSubscriptionTransactions(with subscriptionsData: [String: [[String: Any]]], dateFormatter: DateFormatter) -> [Transaction] { | ||
subscriptionsData.flatMap { (productId: String, transactionData: [[String: Any]]) -> [Transaction] in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually don't specify the types in these since they can be inferred, but In this case I like it - it helps the compiler and the dev not get lost in the nested block.
NSDictionary<NSString *, NSArray *> *nonSubscriptionsData = subscriberData[@"non_subscriptions"]; | ||
self.nonConsumablePurchases = [NSSet setWithArray:[nonSubscriptionsData allKeys]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so non_subscriptions
filters out consumables as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, non_subscriptions
contains both consumables and non-consumables. We've named this (nonConsumablePurchases
) wrong since the beginning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohh, so nonConsumablePurchases
should have been named nonSubscriptions
. Maybe we should add a warning? Seems like a gotcha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in that case, disregard my comment about the sample purchases naming, since consumables would also work
let transactionsFactory = TransactionsFactory() | ||
|
||
let dict = [ | ||
"100_coins": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"100_coins"
would be consumable, right? but the endpoint doesn't return them? if that's the case, we should maybe come up with something else that wouldn't be consumable, like
"sword_of_1000_truths"
and "super_mega_shield_thingy"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The endpoint returns both non-consumables and consumables. I will add that to the tests
@aboedo made the requested changes. I renamed the test file too so it might look like a new file. If you want I can rename it back for easier reviewing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great! 🚢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great! 🚢
Should fix #272
If a user purchases the the same product identifier more than once, Purchaser info won't reflect that, since allPurchasedProductIdentifiers is a Set.
This is a problem for consumables, where if a user purchases the same consumable multiple times (for example, a pack of coins), the app won't have a good way of checking how many times the purchase has been made.