`TransactionPoster`: don't finish transactions for non-subscriptions if they're not processed by NachoSoto · Pull Request #2841 · RevenueCat/purchases-ios

NachoSoto · 2023-07-19T13:37:44Z

This ensures that if the server returns a 200, but didn't actually process the transaction, we don't lose it forever.

When this happens, a warning will be logged:

[purchases] DEBUG: ℹ️ TransactionPoster: handling transaction for product 'BA92DCD8-74C2-45FF-AEE4-2FFD036AF827'
[purchases] WARN: ⚠️ Transaction '19B3BD23-181B-442A-A5B8-05D686ED98E2' will not be finished: it's a non-subscription and it's missing in CustomerInfo list: []

Dependencies:

NachoSoto · 2023-07-19T13:38:29Z

@tonycosentini this is ready. Just gotta change to use the real store transaction ID not the RC ID.
Also I wonder if we should restrict this by sandbox and store too /cc @aboedo

Does non subscription here mean both a consumable and a non-consumable? 🤔

Can the answer to this be added as a comment to this function?

@NachoSoto is your comment here still true? Or is this ready to go? I think the khepri PR has been merged, not sure whether it's been deployed

Also...
We might wanna do it for non-renewing subscriptions as well, since they have the same characteristics as consumables in that they disappear from the receipt forever.

And lastly, non-consumables don't actually need this, do they get the same treatment?

TODO: we discovered that they're included, so test those too:

Integration test

Sandbox test.

I updated the PR, logs, and code to make this more clear. I was mixing consumables with non-subscriptions everywhere.

We might wanna do it for non-renewing subscriptions as well, since they have the same characteristics as consumables in that they disappear from the receipt forever.

And lastly, non-consumables don't actually need this, do they get the same treatment?

See TransactionPoster.shouldFinish(transaction:for:customerInfo:). That new logic is based on StoreProduct.productCategory (subscription / non-subscriptions).
We can't use ProductType because we can't determine that for SK1 products (see SK1StoreProduct.productType).

Just a note that we could in theory distinguish between non-renewing and auto-renewing subs by mixing in the subs vs non subs distinction + expiration date != nil, but it's a non-trivial change that feels a bit more risky, and it isn't worth the effort.

NachoSoto · 2023-07-19T15:09:53Z

Waiting on @tonycosentini to change transaction identifiers for StoreKit tests so we can verify this in integration tests.

NachoSoto · 2023-08-03T21:21:55Z

This should be ready.

codecov · 2023-08-03T22:08:10Z

Codecov Report

Merging #2841 (3940482) into main (022cdea) will increase coverage by 0.04%.
Report is 2 commits behind head on main.
The diff coverage is 90.62%.

@@            Coverage Diff             @@
##             main    #2841      +/-   ##
==========================================
+ Coverage   86.74%   86.79%   +0.04%     
==========================================
  Files         219      220       +1     
  Lines       15650    15701      +51     
==========================================
+ Hits        13576    13627      +51     
  Misses       2074     2074

Files Changed	Coverage Δ
...urces/Identity/CustomerInfo+NonSubscriptions.swift	`0.00% <0.00%> (ø)`
Sources/Misc/Deprecations.swift	`37.85% <0.00%> (-0.28%)`	⬇️
Sources/Logging/Strings/PurchaseStrings.swift	`89.69% <100.00%> (+0.17%)`	⬆️
...urces/Purchasing/Purchases/TransactionPoster.swift	`100.00% <100.00%> (ø)`
...ing/StoreKitAbstractions/SK1StoreTransaction.swift	`100.00% <100.00%> (ø)`
...ing/StoreKitAbstractions/SK2StoreTransaction.swift	`100.00% <100.00%> (ø)`
...hasing/StoreKitAbstractions/StoreTransaction.swift	`98.14% <100.00%> (-0.04%)`	⬇️

... and 3 files with indirect coverage changes

joshdholtz

This makes sense to me! I think I really have questions about if and how consumables and non-consumables are being treated here 🤷‍♂️ The title of the PR mentions consumables but the code refers to non-subscriptions.

joshdholtz · 2023-08-08T01:36:52Z

Does non subscription here mean both a consumable and a non-consumable? 🤔

Can the answer to this be added as a comment to this function?

joshdholtz · 2023-08-08T01:45:34Z

My head is still getting stuck here if nonSubscription means consumable or non-consumable and if they should be treated the same 🤷‍♂️

yeah... I think ideal behavior is:

auto-renewing subscriptions and non-consumables: business as usual

consumables and non-renewing subscriptions: finish only if part of response

Current is:

auto-renewing subscriptions and non-renewing subscriptions: business as usual

consumables and non-consumables: finish only if part of the response

But we should make sure the backend behaves in the same way

Answer to this (will test to make sure), non_subscriptions include:

Consumables

Non-consumables

Non-renewing subscriptions

Updating on this comment for folks who haven't been following as closely - I believe we do now have integration tests with non-consumables and non-subscriptions, and have used them to confirm that the backend indeed sends them as the same thing, and they're safe to treat the same way.

tonidero · 2023-08-08T08:42:30Z

Should we add this to the PR description as well?

I extracted this to #3009 so we can make that a feature and keep this as a fix.

tonidero

I'm a bit worried about the storeTransactionIdentifier vs transactionIdentifier nomenclature and their differences between subscriptions and non-subscriptions... Other than that, I think this is good

NachoSoto · 2023-08-14T16:45:13Z

I think I really have questions about if and how consumables and non-consumables are being treated here 🤷‍♂️ The title of the PR mentions consumables but the code refers to non-subscriptions.

@joshdholtz I updated the log, PR description, and code to make that more clear.

…packages (#3008) There were some questions about these in #2841, and realized we had no test coverage for them. This also improves the documentation of `CustomerInfo.nonSubscriptions` and `NonSubscriptionTransaction`.

This will be used for #2841, but it can be useful for users as well.

NachoSoto · 2023-08-14T22:33:28Z

This is ready now.

…er` (#3009) This will be used for #2841.

…if they're not processed

This test was added in #2841, but iOS 12 can't detect offline `CustomerInfo`s because those aren't available, so this test isn't relevant there. Fixes https://app.circleci.com/pipelines/github/RevenueCat/purchases-ios/14193/workflows/a3836667-878c-4bdd-939a-e809e9f0ac5a/jobs/107291/tests

@NachoSoto

**This is an automatic release.** ### Bugfixes * `PurchasesOrchestrator`: fixed callback not invoked regression during downgrades (#3028) via NachoSoto (@NachoSoto) * `TransactionPoster`: don't finish transactions for non-subscriptions if they're not processed (#2841) via NachoSoto (@NachoSoto) ### Performance Improvements * `StoreKit 2`: only listen to `StoreKit.Transaction.updates` when SK2 is enabled (#3032) via NachoSoto (@NachoSoto) * `CustomerInfoManager`: post transactions in parallel to POST receipts only once (#2954) via NachoSoto (@NachoSoto) ### Other Changes * `PostedTransactionCache`: remove implementation (#3030) via NachoSoto (@NachoSoto) * `Integration Tests`: improved `testCanPurchaseMultipleSubscriptions` (#3025) via NachoSoto (@NachoSoto) * `GitHub`: improved `ISSUE_TEMPLATE` (#3022) via NachoSoto (@NachoSoto) * `TransactionPoster`: added transaction ID and Date to log (#3026) via NachoSoto (@NachoSoto) * `TransactionPoster`: fix iOS 12 test (#3018) via NachoSoto (@NachoSoto) * `SystemInfo`: added `ClockType` (#3014) via NachoSoto (@NachoSoto) * `Integration Tests`: begin tests with `UIApplication.willEnterForegroundNotification` to simulate a real app (#3015) via NachoSoto (@NachoSoto) * `Integration Tests`: add tests to verify `CustomerInfo`+`Offerings` request de-dupping (#3013) via NachoSoto (@NachoSoto) * `SwiftLint`: disable `unneeded_synthesized_initializer` (#3010) via NachoSoto (@NachoSoto) * Added `internal` `NonSubscriptionTransaction.storeTransactionIdentifier` (#3009) via NachoSoto (@NachoSoto) * `Integration Tests`: added tests for non-renewing and non-consumable packages (#3008) via NachoSoto (@NachoSoto) * Expanded `EnsureNonEmptyArrayDecodable` to `EnsureNonEmptyCollectionDecodable` (#3002) via NachoSoto (@NachoSoto)

…packages (#3008) There were some questions about these in #2841, and realized we had no test coverage for them. This also improves the documentation of `CustomerInfo.nonSubscriptions` and `NonSubscriptionTransaction`.

…er` (#3009) This will be used for #2841.

…if they're not processed (#2841)

@NachoSoto

**This is an automatic release.** ### Bugfixes * `PurchasesOrchestrator`: fixed callback not invoked regression during downgrades (#3028) via NachoSoto (@NachoSoto) * `TransactionPoster`: don't finish transactions for non-subscriptions if they're not processed (#2841) via NachoSoto (@NachoSoto) ### Performance Improvements * `StoreKit 2`: only listen to `StoreKit.Transaction.updates` when SK2 is enabled (#3032) via NachoSoto (@NachoSoto) * `CustomerInfoManager`: post transactions in parallel to POST receipts only once (#2954) via NachoSoto (@NachoSoto) ### Other Changes * `PostedTransactionCache`: remove implementation (#3030) via NachoSoto (@NachoSoto) * `Integration Tests`: improved `testCanPurchaseMultipleSubscriptions` (#3025) via NachoSoto (@NachoSoto) * `GitHub`: improved `ISSUE_TEMPLATE` (#3022) via NachoSoto (@NachoSoto) * `TransactionPoster`: added transaction ID and Date to log (#3026) via NachoSoto (@NachoSoto) * `TransactionPoster`: fix iOS 12 test (#3018) via NachoSoto (@NachoSoto) * `SystemInfo`: added `ClockType` (#3014) via NachoSoto (@NachoSoto) * `Integration Tests`: begin tests with `UIApplication.willEnterForegroundNotification` to simulate a real app (#3015) via NachoSoto (@NachoSoto) * `Integration Tests`: add tests to verify `CustomerInfo`+`Offerings` request de-dupping (#3013) via NachoSoto (@NachoSoto) * `SwiftLint`: disable `unneeded_synthesized_initializer` (#3010) via NachoSoto (@NachoSoto) * Added `internal` `NonSubscriptionTransaction.storeTransactionIdentifier` (#3009) via NachoSoto (@NachoSoto) * `Integration Tests`: added tests for non-renewing and non-consumable packages (#3008) via NachoSoto (@NachoSoto) * Expanded `EnsureNonEmptyArrayDecodable` to `EnsureNonEmptyCollectionDecodable` (#3002) via NachoSoto (@NachoSoto)

Fixes #3636. This was added in #3009. It was not made `public` because it was only used for #2841, but as explained in #3636 it can be useful as `public`.

) Fixes #3636. This was added in #3009. It was not made `public` because it was only used for #2841, but as explained in #3636 it can be useful as `public`.

NachoSoto added the pr:fix A bug fix label Jul 19, 2023

NachoSoto requested review from a team and tonycosentini July 19, 2023 13:37

NachoSoto commented Jul 19, 2023

View reviewed changes

Comment thread Tests/BackendIntegrationTests/BaseStoreKitIntegrationTests.swift Outdated

NachoSoto added the blocked label Jul 20, 2023

NachoSoto force-pushed the nacho/sdk-3220-dont-finish-ios-consumable-transactions-until-we-see-them-in branch from 1def557 to 1e0ad6e Compare July 21, 2023 06:40

NachoSoto commented Jul 21, 2023

View reviewed changes

Comment thread Sources/Purchasing/Purchases/TransactionPoster.swift Outdated

NachoSoto requested a review from aboedo August 3, 2023 20:31

NachoSoto force-pushed the nacho/sdk-3220-dont-finish-ios-consumable-transactions-until-we-see-them-in branch 3 times, most recently from 70ee846 to b6c08ab Compare August 3, 2023 21:21

NachoSoto assigned aboedo Aug 3, 2023

NachoSoto added PostReceipt optimizations and removed blocked labels Aug 3, 2023

NachoSoto force-pushed the nacho/sdk-3220-dont-finish-ios-consumable-transactions-until-we-see-them-in branch from b6c08ab to 00215b7 Compare August 3, 2023 21:45

NachoSoto mentioned this pull request Aug 3, 2023

TransactionPoster: avoid posting transactions multiple times #2914

Closed

4 tasks

tonycosentini removed their request for review August 4, 2023 03:33

NachoSoto force-pushed the nacho/sdk-3220-dont-finish-ios-consumable-transactions-until-we-see-them-in branch from 00215b7 to 556d11e Compare August 7, 2023 23:15

joshdholtz reviewed Aug 8, 2023

View reviewed changes

tonidero reviewed Aug 8, 2023

View reviewed changes

Comment thread Sources/Purchasing/Purchases/TransactionPoster.swift Outdated

NachoSoto force-pushed the nacho/sdk-3220-dont-finish-ios-consumable-transactions-until-we-see-them-in branch from 556d11e to 008002b Compare August 14, 2023 15:39

NachoSoto mentioned this pull request Aug 14, 2023

Integration Tests: added tests for non-renewing and non-consumable packages #3008

Merged

NachoSoto force-pushed the nacho/sdk-3220-dont-finish-ios-consumable-transactions-until-we-see-them-in branch from b744267 to d867701 Compare August 14, 2023 16:43

Base automatically changed from integration-test-consumable-non-renewing to main August 14, 2023 22:09

NachoSoto added a commit that referenced this pull request Aug 14, 2023

Expose new NonSubscriptionTransaction.storeTransactionIdentifier

fdd12d4

This will be used for #2841, but it can be useful for users as well.

NachoSoto force-pushed the nacho/sdk-3220-dont-finish-ios-consumable-transactions-until-we-see-them-in branch from d867701 to 49ed141 Compare August 14, 2023 22:33

NachoSoto added a commit that referenced this pull request Aug 14, 2023

Added internal `NonSubscriptionTransaction.storeTransactionIdentifi…

022cdea

…er` (#3009) This will be used for #2841.

NachoSoto added 2 commits August 14, 2023 16:15

TransactionPoster: don't finish transactions for non-subscriptions …

70d58d7

…if they're not processed

Added comment

3940482

NachoSoto force-pushed the nacho/sdk-3220-dont-finish-ios-consumable-transactions-until-we-see-them-in branch from 49ed141 to 3940482 Compare August 14, 2023 23:15

aboedo approved these changes Aug 15, 2023

View reviewed changes

NachoSoto merged commit 5d3263c into main Aug 15, 2023

NachoSoto deleted the nacho/sdk-3220-dont-finish-ios-consumable-transactions-until-we-see-them-in branch August 15, 2023 14:05

NachoSoto mentioned this pull request Aug 15, 2023

TransactionPoster: fix iOS 12 test #3018

Merged

This was referenced Aug 16, 2023

[AUTOMATIC] Release/4.25.5 #3024

Closed

[AUTOMATIC] Release/4.25.5 #3033

Merged

MarkVillacampa pushed a commit that referenced this pull request Sep 6, 2023

Added internal `NonSubscriptionTransaction.storeTransactionIdentifi…

534107a

…er` (#3009) This will be used for #2841.

MarkVillacampa pushed a commit that referenced this pull request Sep 6, 2023

TransactionPoster: don't finish transactions for non-subscriptions …

976659c

…if they're not processed (#2841)

NachoSoto mentioned this pull request Feb 6, 2024

NonSubscriptionTransaction: expose storeTransactionIdentifier #3639

Merged

Conversation

NachoSoto commented Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dependencies:

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 Aug 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NachoSoto Aug 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NachoSoto commented Jul 19, 2023

Uh oh!

Uh oh!

NachoSoto commented Aug 3, 2023

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

joshdholtz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

tonidero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NachoSoto commented Aug 14, 2023

Uh oh!

NachoSoto commented Aug 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

NachoSoto commented Jul 19, 2023 •

edited

Loading

NachoSoto Aug 11, 2023 •

edited

Loading

NachoSoto Aug 14, 2023 •

edited

Loading

codecov Bot commented Aug 3, 2023 •

edited

Loading