`IntroEligibilityCalculator`: fixed logic for subscriptions in same group by NachoSoto · Pull Request #2174 · RevenueCat/purchases-ios

NachoSoto · 2022-12-20T00:08:51Z

Fixes TRIAGE-221 for SK1.
Filed FB11889732 for SK2.

Changes:

Added 2 IntroEligibilityCalculatorTests unit tests to cover this scenario
Added 1 IntroEligibilityCalculatorTests unit tests to cover the equivalent of the testIneligibleForIntroAfterPurchaseExpires integration test
Added 2 StoreKitIntegrationTests to cover this scenario (one of them disabled in SK2)
Updated IntroEligibilityCalculator logic to fix the new tests.
Refactored IntroEligibilityCalculatorTests to avoid duplication and allow adding new cases more easily.
Added a few more tests to ensure other cases are correct.

This simplifies the `+=` implementation by avoiding an intermediate dictionary, and allows a simpler way of merging dictionaries. Will be used in #2174.

NachoSoto · 2022-12-22T21:50:08Z

Simplified this using the MergeStrategy (see #2190)

how does this differ from result += eligibility?

Oh it's the same, I'll change it, thanks!
I improved this from the previous implementation after bae4a00 but didn't realize it can be even simpler now.

NachoSoto · 2022-12-22T21:50:38Z

Disabled for now.

it's crazy that this fails. this is with timeRate > the minimum? just wondering if it's a bug related to the thing where subs would immediately expire

The time rate is the minimum, but it's not a weekly subscription, it's monthly, so it's not the "immediate expiration" bug.

NachoSoto · 2022-12-22T21:51:01Z

This test used to pass before too, but it helped me make sure I didn't break this scenario, thanks for the suggestion @aboedo.

NachoSoto · 2022-12-22T21:51:09Z

This test was failing before this fix.

NachoSoto · 2022-12-22T21:51:22Z

These 2 tests were failing before the fix.

NachoSoto · 2022-12-22T21:51:42Z

@aboedo this is ready now, I think the logic is correct?

NachoSoto · 2022-12-22T23:00:33Z

Thanks to all our test coverage I realized that the new logic wasn't quite right. I've added another failing unit test, but now I have to figure out how to reconcile both scenarios.

aboedo · 2023-01-17T13:04:38Z

how does this differ from result += eligibility?

aboedo · 2023-01-17T13:09:06Z

+            let expiredSubscriptionForProduct = expiredSubscriptionProducts
+                .lazy
+                .map(\.productIdentifier)
+                .contains(candidate.productIdentifier)


We could potentially have a false negative here:
Expired subs for the product only matter if you have used a free trial for the product.

For example, if you subscribe to yearly 1, and it has no free trial available:

you're not eligible while the subscription to yearly 1 is active

if the developer then adds a free trial to yearly 1 and you cancel the subscription, you're eligible because you haven't used a free trial for this product (or its group), and one is available

https://developer.apple.com/documentation/storekit/in-app_purchase/original_api_for_in-app_purchase/subscriptions_and_offers/implementing_introductory_offers_in_your_app

all in all, this means that this check isn't needed:
check is

!activeSubscriptionInGroup and !expiredTrialInGroup.
There isn't anything specific to the product itself, but... if arguably the product didn't have a subs group, the same would apply to it. In practice the field is nullable but I don't think you can have products without subs groups anymore, but it can't hurt to still check for it, i.e.:
!activeSubscriptionForProductOrSubsGroup and !expiredTrialForProductOrSubsGroup.

Thanks a lot for the thorough review!

I think my TDD approach has kinda failed me here. I’ve been writing tests and finding the correct implementation that satisfies all tests, but it led me to the wrong implementation.

I misinterpreted the is_in_trial_period property in the receipt, I figured that wouldn't be true for an expired subscription, but now I see that it is (and verified through integration tests).
So your suggested implementation works after I fix the incorrect unit test 👍🏻

I've also add a new unit test to cover this false positive you described, which failed before the fix:

func testCheckTrialOrIntroDiscountEligibilityReturnsEligibleForPreviouslyOwnedSubscriptionWithUnusedTrial() { self.testEligibility( purchaseExpirationsByProductIdentifier: [ ("com.revenuecat.product1", Date().addingTimeInterval(-1000), false) ], productsInGroups: [ "com.revenuecat.product1": (groupID: "group1", hasTrial: true) ], expectedResult: [ "com.revenuecat.product1": .eligible ] ) }

NachoSoto · 2023-01-17T21:15:17Z

@aboedo third time is the charm hopefully! Let me know if this is good now 🙏🏻

…fy behavior with subscription group See [TRIAGE-221].

codecov · 2023-01-21T16:57:17Z

Codecov Report

Merging #2174 (7d91eda) into main (4533538) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2174      +/-   ##
==========================================
+ Coverage   85.79%   85.84%   +0.04%     
==========================================
  Files         183      183              
  Lines       12077    12105      +28     
==========================================
+ Hits        10362    10391      +29     
+ Misses       1715     1714       -1

Impacted Files	Coverage Δ
.../LocalReceiptParsing/BasicTypes/AppleReceipt.swift	`100.00% <100.00%> (ø)`
...LocalReceiptParsing/BasicTypes/InAppPurchase.swift	`100.00% <100.00%> (ø)`
...ources/Purchasing/IntroEligibilityCalculator.swift	`100.00% <100.00%> (ø)`
Sources/Logging/Strings/StoreKitStrings.swift	`91.17% <0.00%> (+1.47%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

aboedo

Looks great!
I left a comment about an edge case that I'm not sure is possible to hit anymore, but it'd be nice to still consider.

aboedo · 2023-01-24T20:08:24Z

+            let activeSubscriptionInGroup = (
+                candidate.subscriptionGroupIdentifier != nil &&
+                activeSubscriptionsProducts.contains {
+                    $0.subscriptionGroupIdentifier == candidate.subscriptionGroupIdentifier
                }
+            )
+            let expiredTrialInGroup = (
+                candidate.subscriptionGroupIdentifier != nil &&
+                expiredTrialProducts.contains {
+                    $0.subscriptionGroupIdentifier == candidate.subscriptionGroupIdentifier


this would miss eligibility for a product that doesn't have a subscription. In practice I'm not sure it's possible to have one anymore, but it'd be nice to cover for it.
i.e.:

purchase product1, subsGroupIdentifier = nil

eligibility should be: ineligible if product1 is active subscription or expired but had trial, but this would return unknown

We're talking about this offline. I'll merge this for now since it's over a month old and it already fixes the main issue.
I'll that those 2 new tests in a separate PR.

aboedo · 2023-01-24T20:11:38Z

+    func testIneligibleForIntroForDifferentProductInSameSubscriptionGroupAfterPurchase() async throws {
+        try XCTSkipIf(
+            Self.storeKit2Setting == .enabledForCompatibleDevices,
+            "This test currently does not pass with SK2 (see FB11889732)"


is this one under the company account? I can't find it

It is. Click on "Inbox" instead of "Submitted" because it got a response so it's there.

Thanks!! That's exactly what happened 🤦 I was only checking "submitted"

aboedo · 2023-01-24T20:12:17Z

it's crazy that this fails. this is with timeRate > the minimum? just wondering if it's a bug related to the thing where subs would immediately expire

aboedo · 2023-01-24T20:13:55Z

+        let productWithNoTrial = try await self.product(Self.group3MonthlyNoTrialProductID)
+        let productWithTrial = try await self.annualPackage.storeProduct
+
+        let customerInfo = try await Purchases.shared.purchase(product: productWithNoTrial).customerInfo


for some reason I never realized that you can do this syntax (i.e.: customerInfo = try await purchase().customerinfo. That's awesome

Yeah I love these async tests :D

…o subscription group Follow up to #2174.

…o subscription group (#2247) Follow up to #2174.

NachoSoto added Investigating test labels Dec 20, 2022

NachoSoto requested a review from a team December 20, 2022 00:08

NachoSoto commented Dec 20, 2022

View reviewed changes

Comment thread Tests/BackendIntegrationTests/StoreKitIntegrationTests.swift

NachoSoto force-pushed the trial-checker-intergration-test-sub-group branch from f8561c2 to d37ac1b Compare December 21, 2022 17:13

vegaro approved these changes Dec 21, 2022

View reviewed changes

NachoSoto force-pushed the trial-checker-intergration-test-sub-group branch from d37ac1b to 94ddd67 Compare December 22, 2022 20:03

NachoSoto added a commit that referenced this pull request Dec 22, 2022

Added Dictionary.merge

27b1e95

This simplifies the `+=` implementation by avoiding an intermediate dictionary, and allows a simpler way of merging dictionaries. Will be used in #2174.

NachoSoto mentioned this pull request Dec 22, 2022

Added Dictionary.merge #2190

Merged

NachoSoto removed the Investigating label Dec 22, 2022

NachoSoto changed the base branch from main to dictionary-merge December 22, 2022 21:47

NachoSoto force-pushed the trial-checker-intergration-test-sub-group branch from 20dc327 to d10bd9f Compare December 22, 2022 21:47

NachoSoto requested a review from aboedo December 22, 2022 21:47

NachoSoto added pr:fix A bug fix and removed test labels Dec 22, 2022

NachoSoto changed the title ~~TrialOrIntroPriceEligibilityChecker: added integration test to verify behavior with subscription group~~ IntroEligibilityCalculator: fixed logic for subscriptions in same group Dec 22, 2022

NachoSoto commented Dec 22, 2022

View reviewed changes

NachoSoto requested review from a team and vegaro December 22, 2022 21:59

NachoSoto added the WIP label Dec 22, 2022

NachoSoto commented Dec 22, 2022

View reviewed changes

Comment thread Sources/Purchasing/IntroEligibilityCalculator.swift Outdated

NachoSoto commented Dec 22, 2022

View reviewed changes

Comment thread Sources/Purchasing/IntroEligibilityCalculator.swift Outdated

Base automatically changed from dictionary-merge to main December 27, 2022 17:47

NachoSoto force-pushed the trial-checker-intergration-test-sub-group branch from 1706286 to a6257ea Compare January 12, 2023 18:24

aboedo requested changes Jan 17, 2023

View reviewed changes

NachoSoto force-pushed the trial-checker-intergration-test-sub-group branch from 83373ae to ae5fa63 Compare January 17, 2023 21:13

NachoSoto requested a review from aboedo January 17, 2023 21:15

NachoSoto added 15 commits January 21, 2023 08:46

TrialOrIntroPriceEligibilityChecker: added integration test to veri…

d46d50c

…fy behavior with subscription group See [TRIAGE-221].

Added new test and skip other one in SK2

c6124a3

Added new test

5788b8a

Fixed IntroEligibilityCalculator logic

b28308d

Removed unused method

7f8131b

Added failing test and tried to fix logic

b09fcbf

Refactor

88e408d

FIxed implementation

46dd524

More lenient subscriptions

29a8037

Fixed test

f05994d

Added more tests with mixing groups

a1653fd

Refactored tests

f4c32b7

Added 2 more failing tests and fixed them

3b3c1c8

Simplified implementation from PR feedback

73cd1d5

Added new failing test

7d91eda

NachoSoto force-pushed the trial-checker-intergration-test-sub-group branch from ae5fa63 to 7d91eda Compare January 21, 2023 16:46

NachoSoto requested a review from a team January 21, 2023 16:49

aboedo approved these changes Jan 24, 2023

View reviewed changes

NachoSoto merged commit fdd7874 into main Jan 25, 2023

NachoSoto deleted the trial-checker-intergration-test-sub-group branch January 25, 2023 16:40

RCGitBot mentioned this pull request Jan 25, 2023

[AUTOMATIC] Release/4.17.3 #2246

Merged

NachoSoto added a commit that referenced this pull request Jan 25, 2023

IntroEligibilityCalculator: changed logic to handle products with n…

1131957

…o subscription group Follow up to #2174.

NachoSoto mentioned this pull request Jan 25, 2023

IntroEligibilityCalculator: changed logic to handle products with no subscription group #2247

Merged

NachoSoto added a commit that referenced this pull request Feb 2, 2023

IntroEligibilityCalculator: changed logic to handle products with n…

2e689b1

…o subscription group (#2247) Follow up to #2174.

Conversation

NachoSoto commented Dec 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes:

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NachoSoto commented Dec 22, 2022

Uh oh!

NachoSoto commented Dec 22, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NachoSoto commented Jan 17, 2023

Uh oh!

codecov Bot commented Jan 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aboedo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

NachoSoto commented Dec 20, 2022 •

edited

Loading

codecov Bot commented Jan 21, 2023 •

edited

Loading