Re-fetch cached offerings and products after Storefront changes (3/4) by Juanpe · Pull Request #1743 · RevenueCat/purchases-ios

Juanpe · 2022-06-28T16:47:54Z

Follow up to #1583. Subtask of #1382

Description

This PR aims to invalidate and re-fetch if appropriate the stuff in the cache when the Storefront changes. I've used both OfferingsManager and ProductsManager to manage the logic of invalidate and re-fetch the data.

Changes:

Add invalidateAndReFetchCachedOfferingsIfAppropiate method to OfferingsManager class.
Rename clearCachedProducts method to invalidateAndReFetchCachedProductsIfAppropiate in ProductsManager class.
Add OfferingsManager as a dependency of PurchasesOrchestrator.
Clear cache methods returns the identifiers of the elements that have been removed or an empty list if there was no stuff in the cache.
Add tests to cover some scenarios.

…uct identifiers

… OfferingsManager as dependency

Juanpe · 2022-06-28T16:49:22Z

    }

-    func clearCachedProducts() {
+    func invalidateAndReFetchCachedProductsIfAppropiate() {


I'm not sure here if we should log the error when the re-request fails. WDYT? 🤔

Juanpe · 2022-06-28T16:53:21Z

        return productIDsFromBackend.subtracting(productIDsFromStore)
    }

+    func invalidateAndReFetchCachedOfferingsIfAppropiate(appUserID: String) {


I didn't add any test case to cover this logic. The problem is the OfferingsManager tests file belongs to the UnitTests framework I'm not sure if we should move it to StoreKitTests framework in order to be able to change the Storefront. I also thought about creating another test file called OfferingsManagerSKTests that contains the test cases related to StoreKit. WDYT? 🤔

I also thought about creating another test file called OfferingsManagerSKTests that contains the test cases related to StoreKit. WDYT?

Yeah that makes sense, we do that for other classes 👍🏻 Maybe OfferingsManagerStoreKitTests

NachoSoto

This looks great! Just a few suggestions.

NachoSoto · 2022-06-28T16:50:45Z

+        let cachedProductIdentifiers = self.cachedProductsByIdentifier.keys
+        if !cachedProductIdentifiers.isEmpty {
+            self.cachedProductsByIdentifier.removeAll(keepingCapacity: false)
+        }
+        return Set(cachedProductIdentifiers)


I was like "this is a race condition!". Then realized this is an actor 😄

NachoSoto · 2022-06-28T16:51:24Z

-                await productsFetcherSK2.clearCache()
+                let removedProductIdentifiers = await productsFetcherSK2.clearCache()
+                if !removedProductIdentifiers.isEmpty {
+                    _ = try? await self.productsFetcherSK2.products(identifiers: removedProductIdentifiers)


I think this makes sense 👍🏻
Maybe we can add a debug log here? :)

Do you mean only when the request fails or just to inform that the products have been re-fetched? 🤔

I think both

Co-authored-by: NachoSoto <NachoSoto@users.noreply.github.com>

Juanpe · 2022-06-28T18:24:05Z

This looks great! Just a few suggestions.

Thanks for the feedback 👌🏼 I made the suggested changes. LMK if you find anything else that could be improved 👍🏼

NachoSoto

Cool I think this is ready.

NachoSoto · 2022-06-29T02:40:23Z

        // changes to invalidate the cache. The changes are now managed by
        // `StoreKit2StorefrontListenerDelegate`.
-        sk1Fetcher.clearCache()
+        sk1Fetcher.clearCache { _ in }


You should be able to revert this now with the optional completion

NachoSoto · 2022-06-29T02:40:30Z

        // changes to invalidate the cache. The changes are now managed by
        // `StoreKit2StorefrontListenerDelegate`.
-        await sk2Fetcher.clearCache()
+        _ = await sk2Fetcher.clearCache()


NachoSoto · 2022-06-29T02:40:36Z

        expect(cachedProducts).notTo(beEmpty())

-        await productsFetcherSK2.clearCache()
+        _ = await productsFetcherSK2.clearCache()


NachoSoto · 2022-06-29T02:40:48Z

        // changes to invalidate the cache. The changes are now managed by
        // `StoreKit2StorefrontListenerDelegate`.
-        sk1Fetcher.clearCache()
+        sk1Fetcher.clearCache { _ in }


NachoSoto · 2022-06-29T02:40:53Z

        // changes to invalidate the cache. The changes are now managed by
        // `StoreKit2StorefrontListenerDelegate`.
-        await sk2Fetcher.clearCache()
+        _ = await sk2Fetcher.clearCache()


NachoSoto · 2022-06-29T02:41:02Z

        mockProducts.forEach(self.productsFetcherSK1.cacheProduct)

-        productsFetcherSK1.clearCache()
+        productsFetcherSK1.clearCache { _ in }


NachoSoto · 2022-07-04T14:40:07Z

+            let cachedProductIdentifiers = self.cachedProductsByIdentifier.keys
+            if !cachedProductIdentifiers.isEmpty {
+                Logger.debug(Strings.offering.product_cache_invalid_for_storefront_change)
+                self.cachedProductsByIdentifier.removeAll()


Maybe add keepingCapacity: false here too?

Just to be sure, is there a possibility that there may not be the same number of products in different Storefronts?

Oh no, it should be the same. I was just suggesting that since you had keepingCapacity: false in the SK2 version. Might make sense to empty cache storage in both cases?

yep, well, keepingCapacity: false was already in SK2 and false is the default value, but I think it might be a good idea to explicitly state that we are completely clearing the storage 👍🏼

Done ✅ (0f59e57)

I was just asking to be consistent hehe 👍🏻

…_change

…thub.com/Juanpe/purchases-ios into refetch_products_after_Storefront_change

NachoSoto · 2022-07-04T19:10:55Z

There's a couple lint issues in MockOfferingsManager. Maybe add switlint:ignore identifier_name?

…ter_Storefront_change

Juanpe · 2022-07-05T12:33:16Z

There's a couple lint issues in MockOfferingsManager. Maybe add switlint:ignore identifier_name?

Oups, you're right. I've disabled this rule for these properties. (9988fb5)

BTW, my local copy of the project has 35 warnings related to the linter. Is it ok? I checked and I think I have the config files up to date 🤔

NachoSoto

Weird. What version of SwiftLint do you have? I wonder if there a new rule that got added?

NachoSoto · 2022-07-05T16:42:27Z

+    var invokedInvalidateAndReFetchCachedOfferingsIfAppropiate = false
+    var invokedInvalidateAndReFetchCachedOfferingsIfAppropiateCount = 0
+
+    // swiftlint:disable identifier_name


This is just my technique, if you're disabling a rule next to the code that breaks it I'd either re-enable it afterwards or do disable:next. Otherwise I would move this to the top of the file.

Makes sense. I used this technique because this code is at the end of this file and as far as I know this disable only affects the code below. However, I agree with you that we need to re-enable this rule after code because we may add more code in the future that would be affected by this disable. Let me change it 👍🏼

Juanpe · 2022-07-05T20:25:13Z

I made the change. Finally, I disable the identifier_name for the entire file. It's a mock and we did the same for other classes, like MockTrialOrIntroPriceEligibilityChecker or MockSubscriberAttributesManager.

BTW, in order to make all the necessary changes...could you please review the comment related to keep the capacity after cleaning the cache?

Thanks for the review and the feedback! 🤙

Juanpe added 4 commits June 28, 2022 17:20

Update clearCache signature, this method now returns the removed prod…

77330e5

…uct identifiers

Update logic to clear cache and re-fetch the cached products

b4a202b

Fix failing tests and add other to cover this change

04eab6f

Create invalidateAndReFetchCachedOfferingsIfAppropiate method and add…

06894c4

… OfferingsManager as dependency

Juanpe mentioned this pull request Jun 28, 2022

Respond to Storefront updates #1382

Closed

Juanpe commented Jun 28, 2022

View reviewed changes

NachoSoto reviewed Jun 28, 2022

View reviewed changes

Juanpe and others added 6 commits June 28, 2022 19:15

Update Sources/Purchasing/OfferingsManager.swift

a4d423f

Co-authored-by: NachoSoto <NachoSoto@users.noreply.github.com>

Update Sources/Purchasing/StoreKit1/ProductsFetcherSK1.swift

bd6f97b

Co-authored-by: NachoSoto <NachoSoto@users.noreply.github.com>

Update Sources/Purchasing/StoreKit2/ProductsFetcherSK2.swift

eb51f2b

Co-authored-by: NachoSoto <NachoSoto@users.noreply.github.com>

Update Sources/Purchasing/ProductsManager.swift

f8c9592

Co-authored-by: NachoSoto <NachoSoto@users.noreply.github.com>

Add test case to cover logic Storefront changes in OfferingsManager

a7e0a1f

Add log messages and move methods to a private extension

7dd7afa

Add log when the Storefront change and there are products in the cache

5a50b00

NachoSoto approved these changes Jul 4, 2022

View reviewed changes

Juanpe added 4 commits July 4, 2022 18:34

Merge branch 'RevenueCat:main' into refetch_products_after_Storefront…

a78e3ac

…_change

Merge branch 'main' into refetch_products_after_Storefront_change

6a4c069

Merge branch 'refetch_products_after_Storefront_change' of https://gi…

952196d

…thub.com/Juanpe/purchases-ios into refetch_products_after_Storefront_change

Update tests with optional completions

f0b6a3c

Juanpe added 2 commits July 5, 2022 13:56

Merge remote-tracking branch 'Official/main' into refetch_products_af…

fbf41e7

…ter_Storefront_change

Disable a swiftlint rule in the MockOfferingsManager class

9988fb5

NachoSoto approved these changes Jul 5, 2022

View reviewed changes

Disable identifier_name rule for the entire file MockOfferingsManager

b58bc31

Explicitly state that we are completely clearing the storage

0f59e57

NachoSoto merged commit 9f498b8 into RevenueCat:main Jul 6, 2022

taquitos mentioned this pull request Jul 12, 2022

Release/4.8.0 #1758

Merged

Uh oh!

Conversation

Juanpe commented Jun 28, 2022

Description

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

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Juanpe commented Jun 28, 2022

Uh oh!

NachoSoto 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!

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 Jul 4, 2022

Uh oh!

Juanpe commented Jul 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NachoSoto 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!

Juanpe commented Jul 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Juanpe commented Jul 5, 2022 •

edited

Loading