Migrates RCEntitlementInfo #596

taquitos · 2021-06-25T23:49:35Z

Fixes #553
Blocked on #595

… tester for EntitlementInfo

…r now. - Issue #597 filed for when we should re-enable them.

aboedo · 2021-06-28T15:01:49Z

@taquitos is this ready for review?

taquitos · 2021-06-28T16:59:53Z

@taquitos is this ready for review?

Just resolved conflicts, and the PR it was blocked on has merged, we're good to review 🎉

taquitos · 2021-06-28T17:18:24Z

PurchasesCoreSwift/Public/EntitlementInfo.swift

+        self.productIdentifier = productIdString!
+
+        let isSandbox: Bool
+        if maybeSandbox?.responds(to: #selector(getter: NSNumber.boolValue)) ?? false,


Here's a fun one, NSNumber and NSString have the same -boolValue method so in ObjC the message passing worked regardless of type because types are just a suggestion anyways, right? In swift, I had to decide what type it was. First I was like "yeah, this is probably a string" but that didn't work. I'm not going to assume it's an NSNumber or an NSString, so I'll just check for NSNumber first (because that's the most likely and I like to pre-optimize).

added a comment before reading this. I think we should be able to figure out its concrete type since it's coming from our own rest api

Just asked in our company chat.

taquitos · 2021-06-28T17:45:08Z

You'll notice a few TODOs, #599

aboedo · 2021-06-28T17:20:23Z

Purchases.xcodeproj/project.pbxproj

-				37E35D29AD7BE9B81DBF6907 /* RCPromotionalOffer+Protected.h */,
+				3589D15524C21DBD00A65CBB /* RCAttributionTypeFactory+Protected.h */,


is this only alphabetization? or are there files added / removed aside from RCEntitlementInfo headers?

Alphabetization. It's an old habit of mine... everything gets alphabetized.

I love having stuff alphabetized, was just asking because it's hard to figure out what the actual diff is, though.

aboedo · 2021-06-28T17:22:18Z

PurchasesCoreSwift/Public/EntitlementInfo.swift

+@objc(RCStore) public enum Store: Int {
+    /// For entitlements granted via Apple App Store.
+    @objc(RCAppStore) case appStore = 0
+    /// For entitlements granted via Apple Mac App Store.
+    @objc(RCMacAppStore) case macAppStore = 1
+    /// For entitlements granted via Google Play Store.
+    @objc(RCPlayStore) case playStore = 2
+    /// For entitlements granted via Stripe.
+    @objc(RCStripe) case stripe = 3
+    /// For entitlements granted via a promo in RevenueCat.
+    @objc(RCPromotional) case promotional = 4
+    /// For entitlements granted via an unknown store.
+    @objc(RCUnknownStore) case unknownStore = 5
+}
+
+/**
+ Enum of supported period types for an entitlement.
+ */
+@objc(RCPeriodType) public enum PeriodType: Int {
+    /// If the entitlement is not under an introductory or trial period.
+    @objc(RCNormal) case normal = 0
+    /// If the entitlement is under a introductory price period.
+    @objc(RCIntro) case intro = 1
+    /// If the entitlement is under a trial period.
+    @objc(RCTrial) case trial = 2
+}


should we take the opportunity to move these out to their own files?

See: #596 (comment)

aboedo · 2021-06-28T17:23:11Z

PurchasesCoreSwift/Public/EntitlementInfo.swift

+    @objc(RCTrial) case trial = 2
+}
+
+@objcMembers @objc(RCEntitlementInfo) public class EntitlementInfo: NSObject {


nice, didn't know about @objcMembers but it seems super useful

I assume it only applies to the current declaration? i.e.: it wouldn't apply for extensions unless we also mark those as @objcMembers?

also, perhaps we should strive to only add in this declaration the stuff we actually want to expose to obj-c, and move the class funcs to an extension, since they don't even need to be obj-c compatible

@aboedo what wouldn't need to be exposed to obj-c? anything we wouldn't want exposed to users? in that case, could we not just make it private?

like with everything, we should only expose what we absolutely need to.
In this case, we need to expose whatever was a part of the original API in objective-c.
and yes, making it either private or internal would do the trick, I left a comment related to that in one of the methods that might not need to be exposed

@objcMembers:
Apply this attribute to a class declaration, to implicitly apply the objc attribute to all Objective-C compatible members of the class, its extensions, its subclasses, and all of the extensions of its subclasses.

Yeah, so maybe I'll do it member-wise, instead 😄

aboedo · 2021-06-28T17:24:27Z

PurchasesCoreSwift/Public/EntitlementInfo.swift

+    /**
+     The latest purchase or renewal date for the entitlement.
+     */
+    public let latestPurchaseDate: Date? // TODO: This used to be NON-NULL


could you elaborate on why they're optional now?

Our date parsing code returns either a date or a nil

ok, this is a tricky case, but we might as well solve it since it's bound to come up again:

in obj-c, we're declaring this as non-null, but it's actually nullable.

it's nullable in the rest API, so we should try to reflect that (i.e.: the bug is in the declaration, not the implementation)

Currently, this means:

for obj-c clients, this is bad, but it's objc, what's nullability anyway in obj-c land.

for swift clients, this is terrible. The property will show up as non-null, they won't be able to check for nullability, but if it's nil, app crashes.

So I'd say we should just go ahead and make it optional, without even the comment. It's technically a change of the API, but it's more of a fix than an actual change.
We had a similar thing happen with the shared instance of purchases recently

I 100% agree with this approach. It changes the Swift API, but it is a fix. I also think we should talk about revving RC a major version, but with enough comms that folks understand it was additive for ObjC, and some small API fixes around nullablility for swift.

aboedo · 2021-06-28T17:26:23Z

PurchasesCoreSwift/Public/EntitlementInfo.swift

+     The expiration date for the entitlement, can be `nil` for lifetime access.
+     If the `periodType` is `trial`, this is the trial expiration date.
+     */
+    public private(set) var expirationDate: Date?


expirationDate is only set at init, right? I don't think this needs to be var

same for unsubscribeDetectedAt and billingIssueDetectedAt

aboedo · 2021-06-28T17:42:33Z

PurchasesCoreSwift/Public/EntitlementInfo.swift

+        let originalPurchaseDateString = productData["original_purchase_date"] as? String
+        let productExpiresDateString = productData["expires_date"] as? String
+        let storeString = productData["store"] as? String
+        let maybeSandbox = productData["is_sandbox"] as? NSObject // This could be a String or NSNumber


preeetty sure we can trace it back to find out its actual type. This is a value that comes from our REST API.
So if you debug the value as it comes in you can see whether it gets translated into String or NSNumber (I don't recall which one it is tbh)

aboedo · 2021-06-28T17:44:16Z

PurchasesCoreSwift/Public/EntitlementInfo.swift

+        let isSandbox: Bool
+        if maybeSandbox?.responds(to: #selector(getter: NSNumber.boolValue)) ?? false,
+           let unwrapped = maybeSandbox as? NSNumber {
+            isSandbox = unwrapped.boolValue
+        } else if maybeSandbox?.responds(to: #selector(getter: NSString.boolValue)) ?? false,
+                  let unwrapped = maybeSandbox as? NSString {
+            isSandbox = unwrapped.boolValue
+        } else {
+            isSandbox = false
+        }


although I love how thoughtful this translation is, and it is absolutely correct, I think we should just figure out which type it is so that we can remove the complexity here, since it's a value that is under our control (if this was coming from a third-party API, or something outside our control, I'd advocate for keeping this logic)

Will update since we can see it's definitely returned as a bool and not a string 🎉

aboedo · 2021-06-28T17:46:03Z

PurchasesCoreSwift/Public/EntitlementInfo.swift

+        return description
+    }
+
+    public override func isEqual(_ object: Any?) -> Bool {


not needed for this PR, but we might as well add conformity to Equatable and use this method for both

aboedo · 2021-06-28T17:48:07Z

PurchasesCoreSwift/Public/ReOrgLater/EntitlementInfoEnums.swift

-//  EntitlementInfoEnums.swift
-//  PurchasesCoreSwift
-//
-//  Created by Joshua Liebowitz on 6/24/21.


is the intent of moving this into the other file to keep the same file structure that we had before? I actually liked having these separate, since the other file is subject to grow

The intent is to maintain the same file structure as before. There's enough moving pieces I want to be sure our first pass is just migration. We definitely should consider organization as an optimization step 😄

aboedo · 2021-06-28T17:48:29Z

PurchasesTests/Purchasing/EntitlementInfosTests.swift

@@ -441,6 +441,30 @@ class EntitlementInfosTests: XCTestCase {
        verifyProduct()
    }

+    func testSubscriptionIsSandboxString() {


removed since we now are sure we're getting a bool and not a string for this value.

taquitos · 2021-06-28T20:10:00Z

@RevenueCat/sdk ready for re-review

taquitos · 2021-06-28T21:31:08Z

Getting into git rebase trouble with some review chains. Merging, but feel free to leave any more comments and I'll continue to address them.

aboedo · 2021-06-29T18:40:27Z

PurchasesCoreSwift/Public/EntitlementInfo.swift

+        return !(isPromo || isLifetime || hasUnsubscribed || hasBillingIssues)
+    }
+
+    public override var description: String {


I usually try to put public stuff at the top, then internal, then private, so that most readers get to all the public stuff right away.
For swift I usually make an exception for protocol conformance, which I do in an extension, to keep the protocol methods in one place.

And for purely private stuff I usually end up moving it to a private extension as well for the sake of keeping the main declaration as clean as possible... with the obvious limitation being that stored properties have to be in the main declaration as well, so it ends up being a bit mixed.

I don't think it's urgent, but perhaps we could come up with a consistent pattern and use that going forward for swift files?

aboedo · 2021-06-29T18:43:02Z

PurchasesTests/Purchasing/PurchaserInfoTests.swift

-        let entitlements: [String : Purchases.EntitlementInfo] = purchaserInfoWithoutRequestData!.entitlements.active
+        let entitlements: [String : EntitlementInfo] = purchaserInfoWithoutRequestData!.entitlements.active


I'm somewhat concerned about this bit - we have a bit of a conflict where both the framework and its main class are called Purchases, so folks had to disambiguate using Purchases., which won't be needed anymore... and I'm not sure it would compile if you do add it? We should try to figure out what implications this'll have (if any) for devs

taquitos added 2 commits June 25, 2021 15:07

Add support for RCEntitlementInfo API testing

65ac701

Migrates RCEntitlementInfo

04a93ea

taquitos added the blocked label Jun 25, 2021

taquitos linked an issue Jun 26, 2021 that may be closed by this pull request

Migrate EntitlementInfo #553

Closed

Added RCLogLevel check, moved EntitlementInfo enum tests into the API…

834065d

… tester for EntitlementInfo

taquitos mentioned this pull request Jun 26, 2021

Migrate logger #592

Merged

Disable todo, cyclomatic_complexity, and function_body_length rule fo…

0948fca

…r now. - Issue #597 filed for when we should re-enable them.

Base automatically changed from entitlementinfo_tester to swift_migration June 28, 2021 16:52

Merge branch 'swift_migration' into entitlementinfo_migration

24f577f

taquitos removed the blocked label Jun 28, 2021

taquitos requested a review from a team June 28, 2021 16:59

taquitos commented Jun 28, 2021

View reviewed changes

aboedo reviewed Jun 28, 2021

View reviewed changes

taquitos added 2 commits June 28, 2021 11:39

Updated for comments

0340ca6

Make some things private, and fix some indentation

b7e4cb1

taquitos merged commit db98a4a into swift_migration Jun 28, 2021

taquitos deleted the entitlementinfo_migration branch June 28, 2021 21:31

aboedo reviewed Jun 29, 2021

View reviewed changes

taquitos mentioned this pull request Jul 1, 2021

Migrate RCBackend #581

Closed

8 tasks

		37E35D29AD7BE9B81DBF6907 /* RCPromotionalOffer+Protected.h */,
		3589D15524C21DBD00A65CBB /* RCAttributionTypeFactory+Protected.h */,

		let entitlements: [String : Purchases.EntitlementInfo] = purchaserInfoWithoutRequestData!.entitlements.active
		let entitlements: [String : EntitlementInfo] = purchaserInfoWithoutRequestData!.entitlements.active

Migrates RCEntitlementInfo #596

Migrates RCEntitlementInfo #596

Conversation

taquitos commented Jun 25, 2021

aboedo commented Jun 28, 2021

taquitos commented Jun 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taquitos commented Jun 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aboedo Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taquitos commented Jun 28, 2021

taquitos commented Jun 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aboedo Jun 28, 2021 •

edited

Loading