[SK2] Enable Observer Mode for StoreKit 2 #3580

MarkVillacampa · 2024-01-17T17:49:27Z

No description provided.

… product id and add tests

…hrow if observer mode is not enabled

MarkVillacampa · 2024-01-17T19:07:54Z

Sources/Purchasing/Purchases/Purchases.swift

+        purchaseResult: StoreKit.Product.PurchaseResult
+    ) async throws -> StoreTransaction? {
+        guard !self.systemInfo.observerMode else {
+            throw NewErrorUtils.observerModeNotEnabledError()


Throw early, throw often. 😄

I would prevent creating yet another error for this. That's yet another error that users need to handle not just on this API, but in all of our APIs, and maybe localize to different languages, etc.
I would instead throw unsupportedError or configurationError.

MarkVillacampa · 2024-01-17T19:10:02Z

Sources/Purchasing/Purchases/Purchases.swift

+        let (userCancelled, transaction) = try await self.purchasesOrchestrator.storeKit2TransactionListener.handle(
+            purchaseResult: purchaseResult, fromTransactionUpdate: true)
+
+        if userCancelled, self.systemInfo.dangerousSettings.customEntitlementComputation {


I'm not entirely sure about the customEntitlementComputation check here. I don't think this method should be used if it's enabled anyway? This snippet was taken from PurchasesOrchestrator:

https://github.com/RevenueCat/purchases-ios/blob/main/Sources/Purchasing/Purchases/PurchasesOrchestrator.swift#L501-L502

Removed it for now

NachoSoto · 2024-01-17T19:40:18Z

Sources/Purchasing/Purchases/PurchasesType.swift

+     */
+    @available(iOS 15.0, macOS 12.0, tvOS 15.0, watchOS 8.0, *)
+    func handleObserverModeTransaction(
+        purchaseResult: StoreKit.Product.PurchaseResult


I think the swifty way would be

Suggested change

purchaseResult: StoreKit.Product.PurchaseResult

_ purchaseResult: StoreKit.Product.PurchaseResult

NachoSoto · 2024-01-17T19:41:09Z

Tests/BackendIntegrationTests/StoreKitObserverModeIntegrationTests.swift

@@ -81,6 +81,33 @@ class StoreKit2ObserverModeIntegrationTests: StoreKit1ObserverModeIntegrationTes

 }

+class StoreKit2ObserverModeDisabledIntegrationTests: StoreKit1ObserverModeIntegrationTests {


This is going to inherit and run every single test in the super class.

Ugh. This can be tested as unit in StoreKitTests (because we need a Transaction object) but we don't have any test that instantiates a Purchases object. Is it worth it creating one?

but we don't have any test that instantiates a Purchases object. Is it worth it creating one?

Yeah I think it would be useful, but also mock all the dependencies. So I think we can extract the Purchases setup from BasePurchasesTest for that.

But we can put this in the regular SK2 test class, right?

true! I moved it to StoreKit2IntegrationTests and created StoreKit2NotEnabledObserverModeIntegrationTests for the sk2 not enabled one.

NachoSoto · 2024-01-17T19:42:21Z

Sources/Purchasing/Purchases/Purchases.swift

+    @available(iOS 15.0, macOS 12.0, tvOS 15.0, watchOS 8.0, *)
+    func handleObserverModeTransaction(
+        purchaseResult: StoreKit.Product.PurchaseResult
+    ) async throws -> StoreTransaction? {


Note that this needs to call throw NewErrorUtils.error().asPublicError.

NachoSoto · 2024-01-17T19:50:25Z

Tests/APITesters/SwiftAPITester/SwiftAPITester/PurchasesAPI.swift

@@ -263,6 +263,11 @@ private func checkAsyncMethods(purchases: Purchases) async {
        let _: CustomerInfo = try await purchases.restorePurchases()
        let _: CustomerInfo = try await purchases.syncPurchases()

+        if #available(iOS 15.0, macOS 12.0, tvOS 15.0, watchOS 8.0, *) {
+            let result = try await StoreKit.Product.products(for: [""]).first!.purchase()


I like that you did it this way to ensure that whatever we pass to the next method is the result of this.

joshdholtz · 2024-01-17T19:50:22Z

Sources/Purchasing/Purchases/Purchases.swift

+    func handleObserverModeTransaction(
+        purchaseResult: StoreKit.Product.PurchaseResult
+    ) async throws -> StoreTransaction? {


I know this is Observer Mode but does it matter what they have set for the StoreKit version? Like... should this also require .with(storeKitVersion: .storeKit2)?

Good catch. Indeed that's a requirement.

NachoSoto · 2024-01-17T19:51:12Z

Sources/Purchasing/Purchases/Purchases.swift

+        let (_, transaction) = try await self.purchasesOrchestrator.storeKit2TransactionListener.handle(
+            purchaseResult: purchaseResult, fromTransactionUpdate: true)


I wish we linted this stuff 😇
I'd format it with symmetrical parenthesis:

Suggested change

let (_, transaction) = try await self.purchasesOrchestrator.storeKit2TransactionListener.handle(

purchaseResult: purchaseResult, fromTransactionUpdate: true)

let (_, transaction) = try await self.purchasesOrchestrator.storeKit2TransactionListener.handle(

purchaseResult: purchaseResult, fromTransactionUpdate: true

)

NachoSoto · 2024-01-17T19:51:36Z

Sources/Purchasing/Purchases/PurchasesType.swift

+     * Purchases.shared.handleObserverModeTransaction(purchaseResult: result)
+     * ```
+     *
+     * - Note: You need to finish the transaction yourself after calling this method.


Maybe turn this into a Warning since it's pretty important? Maybe put that in the code example above too?

NachoSoto · 2024-01-17T19:54:15Z

Sources/Purchasing/Purchases/PurchasesType.swift

+     *
+     * - Note: You need to finish the transaction yourself after calling this method.
+     *
+     * - Parameter purchaseResult: The ``StoreKit.Product.PurchaseResult`` of the product that was just purchased.


I don't think you can reference types outside the module with "``", does this not give a warning when building?

I haven't noticed any warning, but replaced with single ticks since it cannot link anywhere ayway.

NachoSoto · 2024-01-17T19:55:37Z

Tests/UnitTests/Purchasing/ConfigurationTests.swift

-        expect(configuration.observerMode) == true
-        expect(configuration.storeKitVersion) == .storeKit2
-
-        self.logger.verifyMessageWasLogged(Strings.configure.observer_mode_with_storekit2,


Maybe instead we could log a different message now pointing out that transactions need to be handled manually?

NachoSoto · 2024-01-17T19:56:16Z

Sources/Logging/Strings/ConfigureStrings.swift

@@ -187,6 +185,8 @@ extension ConfigureStrings: LogMessage {
            return "Purchases is not configured with StoreKit 2 enabled. This is required in order to detect " +
            "transactions coming from SwiftUI paywalls. You must use `.with(storeKitVersion: .storeKit2)` " +
            "when configuring the SDK."
+        case .sk2_required_observer_mode:
+            return "Observer mode must be enabled. You must use `.with(observerMode: true)` when configuring the SDK."


Maybe:

Suggested change

return "Observer mode must be enabled. You must use `.with(observerMode: true)` when configuring the SDK."

return "Attempted to manually handle transactions with observer mode not enabled. You must use `.with(observerMode: true)` when configuring the SDK."

codecov · 2024-01-18T00:11:01Z

Codecov Report

Attention: 25 lines in your changes are missing coverage. Please review.

Comparison is base (ce9f13c) 85.87% compared to head (97bd115) 85.86%.

Files	Patch %	Lines
Sources/Purchasing/Purchases/Purchases.swift	0.00%	20 Missing ⚠️
Sources/Logging/Strings/ConfigureStrings.swift	66.66%	4 Missing ⚠️
Sources/Logging/Strings/PurchaseStrings.swift	50.00%	1 Missing ⚠️

Additional details and impacted files

@@                Coverage Diff                @@
##           sk2-full-flow    #3580      +/-   ##
=================================================
- Coverage          85.87%   85.86%   -0.02%     
=================================================
  Files                242      242              
  Lines              17613    17645      +32     
=================================================
+ Hits               15125    15150      +25     
- Misses              2488     2495       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

NachoSoto

Looks good, just one more question about the removed tests.

NachoSoto · 2024-01-18T00:17:34Z

Tests/UnitTests/Purchasing/ConfigurationTests.swift

+
+        expect(configuration.observerMode) == true
+        expect(configuration.storeKitVersion) == .storeKit2
+


Nit: extra space

NachoSoto · 2024-01-18T00:18:09Z

Tests/UnitTests/Purchasing/ConfigurationTests.swift

@@ -31,50 +31,6 @@ class ConfigurationTests: TestCase {
        expect(Configuration.validate(apiKey: "swRTCezdEzjnJSxdexDNJfcfiFrMXwqZ")) == .legacy
    }

-    func testNoObserverModeWithStoreKit1() {


You brought back observer_mode_with_storekit2. Can you bring back these tests instead? I think you can just reset this file, right?

MarkVillacampa added 3 commits January 17, 2024 12:45

Add method to process initial sk2 transactions in observer mode

f1e49d1

Improve error handling, add method for processing last transaction by…

e988db6

… product id and add tests

process -> handle

762df7d

MarkVillacampa added the pr:feat A new feature label Jan 17, 2024

NachoSoto added the feat:StoreKit 2 label Jan 17, 2024

MarkVillacampa added 3 commits January 17, 2024 19:32

add back removed tests

0b62e26

Remove method to handle purchases by product id, add documentation, t…

d2dba88

…hrow if observer mode is not enabled

remove unused string

7a274b6

MarkVillacampa commented Jan 17, 2024

View reviewed changes

MarkVillacampa added 3 commits January 17, 2024 20:23

do not throw purchaseCancelledError

ebb32b6

typos & docs

87a431a

docs

8c05b52

MarkVillacampa mentioned this pull request Jan 17, 2024

[WIP] Add support for StoreKit 2 full flow #3529

Closed

7 tasks

remove unneeded tests

2a9de8a

NachoSoto reviewed Jan 17, 2024

View reviewed changes

put back space

1b7a1fe

NachoSoto reviewed Jan 17, 2024

View reviewed changes

MarkVillacampa added 2 commits January 17, 2024 20:46

swiftify api

e52b1d1

fix test

48a84d9

NachoSoto reviewed Jan 17, 2024

View reviewed changes

joshdholtz reviewed Jan 17, 2024

View reviewed changes

NachoSoto reviewed Jan 17, 2024

View reviewed changes

do not create new error

f0e4b22

NachoSoto reviewed Jan 17, 2024

View reviewed changes

NachoSoto changed the title ~~[SK2] Enable Obserber Mode for StoreKit 2~~ [SK2] Enable Observer Mode for StoreKit 2 Jan 17, 2024

MarkVillacampa added 5 commits January 17, 2024 23:58

fix build

7b34817

simplify test

f7d0a1f

make extension private again

3fc2dac

fix test

b00ef6a

space

eb83202

NachoSoto reviewed Jan 18, 2024

View reviewed changes

MarkVillacampa added 2 commits January 18, 2024 11:35

reset tests

8e68c20

add tests

97bd115

MarkVillacampa merged commit ce434bc into sk2-full-flow Jan 18, 2024
26 checks passed

MarkVillacampa deleted the sk2-observer-mode branch January 18, 2024 11:02

MarkVillacampa added a commit that referenced this pull request Jan 26, 2024

[SK2] Enable Observer Mode for StoreKit 2 (#3580)

7ce72e8

MarkVillacampa added a commit that referenced this pull request Feb 6, 2024

[SK2] Enable Observer Mode for StoreKit 2 (#3580)

c10c73f

MarkVillacampa added a commit that referenced this pull request Feb 6, 2024

[SK2] Enable Observer Mode for StoreKit 2 (#3580)

b714965

MarkVillacampa added a commit that referenced this pull request Feb 6, 2024

[SK2] Enable Observer Mode for StoreKit 2 (#3580)

cf4bb42

MarkVillacampa added a commit that referenced this pull request Feb 6, 2024

[SK2] Enable Observer Mode for StoreKit 2 (#3580)

da266a2

MarkVillacampa added a commit that referenced this pull request Feb 7, 2024

[SK2] Enable Observer Mode for StoreKit 2 (#3580)

0ad61e8

NachoSoto pushed a commit that referenced this pull request Feb 9, 2024

[SK2] Enable Observer Mode for StoreKit 2 (#3580)

68b4145

NachoSoto pushed a commit that referenced this pull request Feb 12, 2024

[SK2] Enable Observer Mode for StoreKit 2 (#3580)

6ff2f06

NachoSoto pushed a commit that referenced this pull request Feb 12, 2024

[SK2] Enable Observer Mode for StoreKit 2 (#3580)

07d561c

NachoSoto pushed a commit that referenced this pull request Feb 14, 2024

[SK2] Enable Observer Mode for StoreKit 2 (#3580)

ee0a38a

MarkVillacampa added a commit that referenced this pull request Feb 21, 2024

[SK2] Enable Observer Mode for StoreKit 2 (#3580)

f570eab

joshdholtz pushed a commit that referenced this pull request Feb 23, 2024

[SK2] Enable Observer Mode for StoreKit 2 (#3580)

18e6d5e

MarkVillacampa added a commit that referenced this pull request Mar 6, 2024

[SK2] Enable Observer Mode for StoreKit 2 (#3580)

9eae91e

MarkVillacampa added a commit that referenced this pull request Mar 6, 2024

[SK2] Enable Observer Mode for StoreKit 2 (#3580)

1aecbaa

MarkVillacampa added a commit that referenced this pull request Apr 9, 2024

[SK2] Enable Observer Mode for StoreKit 2 (#3580)

2ecbd3c

fire-at-will pushed a commit that referenced this pull request Apr 15, 2024

[SK2] Enable Observer Mode for StoreKit 2 (#3580)

971ea7c

fire-at-will pushed a commit that referenced this pull request May 13, 2024

[SK2] Enable Observer Mode for StoreKit 2 (#3580)

06d3e8d

fire-at-will pushed a commit that referenced this pull request Jun 24, 2024

[SK2] Enable Observer Mode for StoreKit 2 (#3580)

8c0fbab

fire-at-will pushed a commit that referenced this pull request Jun 28, 2024

[SK2] Enable Observer Mode for StoreKit 2 (#3580)

160ee40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SK2] Enable Observer Mode for StoreKit 2 #3580

[SK2] Enable Observer Mode for StoreKit 2 #3580

MarkVillacampa commented Jan 17, 2024

MarkVillacampa Jan 17, 2024

NachoSoto Jan 17, 2024

MarkVillacampa Jan 17, 2024 •

edited

Loading

MarkVillacampa Jan 17, 2024

NachoSoto Jan 17, 2024

NachoSoto Jan 17, 2024

MarkVillacampa Jan 17, 2024

NachoSoto Jan 17, 2024

NachoSoto Jan 17, 2024

MarkVillacampa Jan 17, 2024

NachoSoto Jan 17, 2024

NachoSoto Jan 17, 2024

joshdholtz Jan 17, 2024 •

edited

Loading

MarkVillacampa Jan 17, 2024

NachoSoto Jan 17, 2024

NachoSoto Jan 17, 2024

NachoSoto Jan 17, 2024

MarkVillacampa Jan 17, 2024

NachoSoto Jan 17, 2024

NachoSoto Jan 17, 2024

NachoSoto Jan 17, 2024

codecov bot commented Jan 18, 2024 •

edited

Loading

NachoSoto left a comment

NachoSoto Jan 18, 2024

NachoSoto Jan 18, 2024

	purchaseResult: StoreKit.Product.PurchaseResult
	_ purchaseResult: StoreKit.Product.PurchaseResult

		@@ -81,6 +81,33 @@ class StoreKit2ObserverModeIntegrationTests: StoreKit1ObserverModeIntegrationTes

		}

		class StoreKit2ObserverModeDisabledIntegrationTests: StoreKit1ObserverModeIntegrationTests {

		let (_, transaction) = try await self.purchasesOrchestrator.storeKit2TransactionListener.handle(
		purchaseResult: purchaseResult, fromTransactionUpdate: true)

	return "Observer mode must be enabled. You must use `.with(observerMode: true)` when configuring the SDK."
	return "Attempted to manually handle transactions with observer mode not enabled. You must use `.with(observerMode: true)` when configuring the SDK."


		expect(configuration.observerMode) == true
		expect(configuration.storeKitVersion) == .storeKit2

[SK2] Enable Observer Mode for StoreKit 2 #3580

[SK2] Enable Observer Mode for StoreKit 2 #3580

Conversation

MarkVillacampa commented Jan 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkVillacampa Jan 17, 2024 • 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

joshdholtz Jan 17, 2024 • 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

codecov bot commented Jan 18, 2024 • edited Loading

Codecov Report

NachoSoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkVillacampa Jan 17, 2024 •

edited

Loading

joshdholtz Jan 17, 2024 •

edited

Loading

codecov bot commented Jan 18, 2024 •

edited

Loading