Forces refreshing the receipt when calling restore #287

vegaro · 2020-07-17T22:29:18Z

If the receipt does not contain any subscription purchases, calling /verifyReceipt will not update an empty receipt with any purchased subscriptions.

We should trigger an SKReceiptRefresh if the device receipt does not contain any subscription purchases.

Things missing

I have tested this but we really need to make sure it will not trigger login popups constantly. Maybe worth releasing a beta?
Unit tests
Test with Mac

jeiting · 2020-07-20T15:33:17Z

Purchases/Public/RCPurchases.m

- (void)receiptData:(void (^ _Nonnull)(NSData * _Nonnull data))completion
-{
+- (void)receiptData:(void (^ _Nonnull)(NSData * _Nonnull data))completion {
+    [self receiptDataWithForceRefresh:false completion:completion];


false -> NO

jeiting · 2020-07-20T15:34:04Z

Purchases/Public/RCPurchases.m

+    [self receiptDataWithForceRefresh:false completion:completion];
+}
+
+- (void)receiptDataWithForceRefresh:(BOOL)forceRefresh completion:(void (^ _Nonnull)(NSData * _Nonnull data))completion  {


Chance for cleanup, this should probably have a typedef:

(void (^ _Nonnull)(NSData * _Nonnull data))

jeiting · 2020-07-20T15:34:48Z

Purchases/Public/RCPurchases.m

+    [self receiptDataWithForceRefresh:false completion:completion];
+}
+
+- (void)receiptDataWithForceRefresh:(BOOL)forceRefresh completion:(void (^ _Nonnull)(NSData * _Nonnull data))completion  {
    NSData *receiptData = [self.receiptFetcher receiptData];
    if (receiptData == nil) {


I wonder if we should a zero-bytes check here too. I've seen that before, where there is a file, but it's empty.

Possible there is a check in the fetcher.

We have the check in a couple of places in the RCPurchases.m:
https://github.com/RevenueCat/purchases-ios/blob/cesardelavega/ch68/force-refresh-receipt/Purchases/Public/RCPurchases.m#L629
https://github.com/RevenueCat/purchases-ios/blob/cesardelavega/ch68/force-refresh-receipt/Purchases/Public/RCPurchases.m#L1084

I will add it a check when we fetch so we also log a message in that case and also here so that we also refresh if the receipt has 0 bytes.

aboedo · 2020-07-21T14:59:57Z

Purchases/Public/RCPurchases.h

+/**
+ * Completion block for calls that send back receipt data
+ */
+typedef void (^RCReceiveReceiptDataBlock)(NSData *);


does this need to be public? if not, you can declare it in the interface within the .m

aboedo · 2020-07-21T15:07:59Z

Purchases/Public/RCPurchases.m

+- (void)receiptData:(RCReceiveReceiptDataBlock)completion {
+    [self receiptDataWithForceRefresh:NO completion:completion];
+}
+
+- (void)receiptDataWithForceRefresh:(BOOL)forceRefresh completion:(RCReceiveReceiptDataBlock)completion  {


[not for this PR] we really should move all of this logic out of RCPurchases.

aboedo · 2020-07-21T15:09:23Z

Purchases/Public/RCPurchases.m

    NSData *receiptData = [self.receiptFetcher receiptData];
-    if (receiptData == nil) {
+    if (receiptData == nil || receiptData.length == 0) {


I can't really think of a scenario where we would want to treat an empty receipt differently than a missing one. It might be a nice simplification to have the fetcher return nil if the data is empty. or even better, some form of an error.
Again, not for this PR.

yeah, I thought the same, right now the fetcher makes the data empty when the receipt is nil. And then there's some error handling in RCPurchases.m (like in the restore function) that checks if the data is empty instead of nil and then sends an error. I thought about moving the error to the fetcher, but it was going to make this PR bigger. I totally agree. I will add it to my list of things to refactor

I might tackle this as a part of local receipt validation, since I may have to do some new receipt fetching logic.

aboedo · 2020-07-21T15:14:01Z

Purchases/Public/RCPurchases.m

        RCDebugLog(@"Receipt empty, fetching");
        [self refreshReceipt:completion];
+    } else if (forceRefresh) {
+        RCDebugLog(@"Forced receipt refresh");
+        [self refreshReceipt:completion];
    } else {


if we're going to refresh the receipt anyway, this should be the first step, that way we don't make unnecessary calls to read the receipt from disk.

as in:

if (forceRefresh) { [self refreshReceipt:completion]; return; } // current logic here ... NSData *receiptData = [self.receiptFetcher receiptData]; if (receiptData == nil || receiptData.length == 0) { ...

aboedo

looks great! I'll mark approved once the tests are in.

Purchases/Public/RCPurchases.m

aboedo · 2020-07-23T18:50:45Z

marking approved for now, but feel free to ask for another review when tests are in

aboedo

looks great, left a question about a comment

aboedo · 2020-07-27T19:11:08Z

PurchasesTests/Purchasing/PurchasesTests.swift

-        expect(self.requestFetcher.refreshReceiptCalled).to(beFalse())
+        expect(self.requestFetcher.refreshReceiptCalled).to(beTrue())


aboedo · 2020-07-27T19:13:57Z

PurchasesTests/Purchasing/PurchasesTests.swift

+        // Second one issues an error
+        self.purchases?.purchaseProduct(product) { (tx, info, error, userCancelled) in }


what does the comment mean?
also, we should make sure that this doesn't no-op if purchases is nil, right?
lastly, no need for explicit self, or even to name the variables in the block, so this is equivalent to

guard let purchases = purchases else { fatalError() } purchases.purchaseProduct(product) { }

however, if the transaction should be made after the product is purchased, then maybe the best way to do this would be to move the creation of the transaction and the storeKitWrapperDelegate call to inside the completion of purchaseProduct.

The thing is that the purchaseProduct call will add the completion block to the array of completion blocks and we need to call self.storeKitWrapper.delegate?.storeKitWrapper(self.storeKitWrapper, updatedTransaction: transaction) in order to trigger the completion block of purchaseProduct.

In this case we don't care about the completion block of purchaseProduct, but we need to call it because it adds the payment to the "queue".

The weird comment is a copy paste issue

vegaro · 2020-07-28T03:45:24Z

It looks like macOS will consider the login details stale after 30 minutes and trigger the login popup. This means that if someone is calling restore every app start, there will be an annoying popup every app start. I think we are fine since we state that people shouldn't be calling restore on app start in the docs, but I wanted to make sure everyone is in the loop @aboedo @jeiting @rkotzy

EDIT: Actually, I just tested on my iPad with iOS 13 and same result, the login details cache only lasts for like 15-20 mins on my tests!

forces refreshing the receipt when calling restore

f1957c7

vegaro marked this pull request as draft July 17, 2020 22:31

jeiting reviewed Jul 20, 2020

View reviewed changes

vegaro added 3 commits July 20, 2020 16:48

sends NO instead of false

a146fff

typedef for completion block

76a7a0b

add zero bytes check

d2b5985

jeiting approved these changes Jul 21, 2020

View reviewed changes

aboedo reviewed Jul 21, 2020

View reviewed changes

vegaro added 2 commits July 21, 2020 14:18

removes RCReceiveReceiptDataBlock from the header file

5925c89

moves forceRefresh to first thing in function

856645f

vegaro requested a review from aboedo July 21, 2020 21:19

aboedo reviewed Jul 21, 2020

View reviewed changes

Purchases/Public/RCPurchases.m Show resolved Hide resolved

added log back

384d9af

aboedo approved these changes Jul 23, 2020

View reviewed changes

Fixed tests

e6cb0e3

vegaro marked this pull request as ready for review July 24, 2020 18:38

vegaro changed the title ~~[HOLD] Forces refreshing the receipt when calling restore~~ Forces refreshing the receipt when calling restore Jul 24, 2020

vegaro requested a review from aboedo July 24, 2020 21:29

aboedo approved these changes Jul 27, 2020

View reviewed changes

vegaro added 2 commits July 27, 2020 19:13

removes unnecessary comment

96cb4e1

makes suggested changes

e3a4246

fixes compilation

c0fa9ee

vegaro merged commit 4c89d35 into develop Jul 31, 2020

vegaro deleted the cesardelavega/ch68/force-refresh-receipt branch July 31, 2020 01:18

vegaro mentioned this pull request Aug 4, 2020

Version 3.6.0-beta #307

Merged

aboedo mentioned this pull request Aug 27, 2020

release: 3.6.0-beta-2 #322

Merged

aboedo mentioned this pull request Sep 10, 2020

Release/3.6.0 #335

Merged

aboedo mentioned this pull request Jul 13, 2021

Release: 3.12.0 #648

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Forces refreshing the receipt when calling restore #287

Forces refreshing the receipt when calling restore #287

vegaro commented Jul 17, 2020 •

edited

Loading

jeiting Jul 20, 2020

jeiting Jul 20, 2020

jeiting Jul 20, 2020

jeiting Jul 20, 2020

vegaro Jul 21, 2020

aboedo Jul 21, 2020

aboedo Jul 21, 2020

aboedo Jul 21, 2020

vegaro Jul 21, 2020

aboedo Jul 21, 2020

aboedo Jul 21, 2020

aboedo left a comment

aboedo commented Jul 23, 2020

aboedo left a comment

aboedo Jul 27, 2020

aboedo Jul 27, 2020

aboedo Jul 27, 2020

vegaro Jul 28, 2020

vegaro commented Jul 28, 2020 •

edited

Loading

		expect(self.requestFetcher.refreshReceiptCalled).to(beFalse())
		expect(self.requestFetcher.refreshReceiptCalled).to(beTrue())

		// Second one issues an error
		self.purchases?.purchaseProduct(product) { (tx, info, error, userCancelled) in }

Forces refreshing the receipt when calling restore #287

Forces refreshing the receipt when calling restore #287

Conversation

vegaro commented Jul 17, 2020 • edited Loading

Things missing

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 left a comment

Choose a reason for hiding this comment

aboedo commented Jul 23, 2020

aboedo left a 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

vegaro commented Jul 28, 2020 • edited Loading

vegaro commented Jul 17, 2020 •

edited

Loading

vegaro commented Jul 28, 2020 •

edited

Loading