Gotta consider how this works with #2841.

maybe we should get both into an integration branch before shipping and test them together?

we should add a test to verify the behavior if the same transaction gets posted over and over (like if it's not finished)

basically to ensure that we don't grow the cache forever

It's stored as a Set, but yeah I can add that.

Done:

func testHandlePurchasedTransactionMultipleTimesPostsItOnce() throws {
    let transactionData = PurchasedTransactionData(
        appUserID: "user",
        source: .init(isRestore: false, initiationSource: .queue)
    )
    let count = 10

    self.backend.stubbedPostReceiptResult = .success(self.createCustomerInfo(nonSubscriptionProductID: nil))

    for _ in 0..<count {
        _ = try self.handleTransaction(transactionData)
    }

    expect(self.backend.invokedPostReceiptDataCount) == 1
    expect(self.mockTransaction.finishInvokedCount) == count

    expect(self.cache.postedTransactions) == [self.mockTransaction.transactionIdentifier]
}

🤔
This is a weird case to think about. Not sure fetching customer info is the best path here?

I can only imagine arriving here if:

• we've posted the transaction before
• we call invalidate customer info cache
• we get notified again for an unfinished transaction

Or similar thinking but someone calls restoreCompletedTransactions.

Feels like it'd be safer to just post in this case? Would that be a huge refactor? WDYT?

That complicates the code unnecessarily. I'd have to give TransactionPoster knowledge of whether there is a cached customer info or not.

There's a possibility that this ends up with nil customer info and nil error, leading to a crash: https://app.circleci.com/pipelines/github/RevenueCat/purchases-ios/13951/workflows/3b380d52-bc83-4449-aa91-cf6b55235ec8/jobs/103818

Thread 0 Crashed:
0   libswiftCore.dylib            	       0x1021b1e81 _assertionFailure(_:_:file:line:flags:) + 353
1   RevenueCat                    	       0x1310b2dc7 Result.init(_:_:file:line:) + 1047 (Result+Extensions.swift:24)
2   RevenueCat                    	       0x130f35cf9 closure #1 in closure #1 in Purchases.purchaseAsync(product:promotionalOffer:) + 361 (Purchases+async.swift:97)
3   RevenueCat                    	       0x1310dcd9f closure #2 in PurchasesOrchestrator.purchase(sk1Product:payment:package:wrapper:completion:) + 847 (PurchasesOrchestrator.swift:395)
4   RevenueCat                    	       0x1310ec8dc partial apply for closure #2 in PurchasesOrchestrator.purchase(sk1Product:payment:package:wrapper:completion:) + 44
5   RevenueCat                    	       0x1310e4ce9 thunk for @escaping @callee_guaranteed @Sendable (@guaranteed StoreTransaction?, @guaranteed CustomerInfo?, @guaranteed NSError?, @unowned Bool) -> () + 25
6   RevenueCat                    	       0x1310e8806 thunk for @escaping @callee_guaranteed @Sendable (@in_guaranteed StoreTransaction?, @in_guaranteed CustomerInfo?, @in_guaranteed NSError?, @in_guaranteed Bool) -> (@out ()) + 134
7   RevenueCat                    	       0x1310e4ce9 thunk for @escaping @callee_guaranteed @Sendable (@guaranteed StoreTransaction?, @guaranteed CustomerInfo?, @guaranteed NSError?, @unowned Bool) -> () + 25
8   RevenueCat                    	       0x1310e8806 thunk for @escaping @callee_guaranteed @Sendable (@in_guaranteed StoreTransaction?, @in_guaranteed CustomerInfo?, @in_guaranteed NSError?, @in_guaranteed Bool) -> (@out ()) + 134
9   RevenueCat                    	       0x1310eb387 closure #1 in closure #1 in PurchasesOrchestrator.handlePurchasedTransaction(_:storefront:restored:) + 759 (PurchasesOrchestrator.swift:1092)
10  RevenueCat                    	       0x130edc340 closure #1 in static OperationDispatcher.dispatchOnMainActor(_:) + 80 (OperationDispatcher.swift:60)
11  RevenueCat                    	       0x130edc5d1 partial apply for closure #1 in static OperationDispatcher.dispatchOnMainActor(_:) + 1
12  RevenueCat                    	       0x130edc3e1 thunk for @escaping @callee_guaranteed @Sendable @async () -> () + 1
13  RevenueCat                    	       0x130edc6e1 partial apply for thunk for @escaping @callee_guaranteed @Sendable @async () -> () + 1
14  RevenueCat                    	       0x130e6bbe1 thunk for @escaping @callee_guaranteed @Sendable @async () -> (@out A) + 1
15  RevenueCat                    	       0x130e6bd11 partial apply for thunk for @escaping @callee_guaranteed @Sendable @async () -> (@out A) + 1

Need to write a unit test for this and fix it.

Fixed and tested.

A subtle change here, suggested by @aboedo: we no longer call finishTransactionIfNeeded from the main actor, we only call the completion (see complete) above block after we're done with everything.

Updated this test to have some previously posted transactions too.

This is extracting these 2 CustomerInfos to avoid duplicate code.

This test no longer had the same semantics because it used the same transaction, which was a shortcut. I made this reflect a more normal flow, and added another one below that uses the same transaction, for which the second one we don't post

testHandlePurchasedTransactionWithMissingReceipt is basically testing a not-finishable error. In this case we don't save the posted transaction.

For finishable errors we do.

This isn't a Result anymore, so can't use beSuccess(). But checking .value is equivalent.

Calling it 10 times to ensure that we only post it once (and finish 10 times).

Added these to all tests that are relevant.

I actually broke this (thankfully that broke other tests), so now I'm covering it with a test.

Improved the basic "can purchase package" test with these 2 assertions.

Hmm this seems unused now after the latest changes?

Thanks, removed!

The comment is clear but I'm concerned the name of the function doesn't indicate that this might initiate a fetch of the customer info... Maybe we could rename to toResultOrFetchCustomerInfo? Or something along those lines?

Swift (and Obj-C) are different from other languages, the name of this method isn't just toResult, it's toResult(completion:customerInfoFetcher:), so the "fetch customer info" part is already part of the name.
It would have to toResultOrFetchCustomerInfo(completion:_:) which is less clear IMO.

Kinda unrelated to this PR but I was wondering, do we clear the cache values at any point? In android we clear them when we have a token but not a corresponding active purchase anymore (so the purchase has expired). We perform this check on every app foreground.

That's a fair question. There's no direct equivalent on iOS, in that a purchase never goes away from the receipt (other than in Sandbox).

We could give them a TTL based on expiration date, with the assumption that if a purchase has been posted and finished, if say, 90 days go by, you're probably not gonna find it in the queue again and should be safe to remove.

It's not perfectly true in that calling restoreCompletedTransactions could technically bring it back, but should work for the vast majority of cases. Thoughts?

Done in #3023 and rebased this.

⚠️ Another change here. We were handling transactions in the main thread without this, which now made us read/write cache in the main thread.
Thanks to the new assertions we're avoiding this now.