AdServices by joshdholtz · Pull Request #1727 · RevenueCat/purchases-ios

joshdholtz · 2022-06-22T04:22:50Z

Motivation

@beylmk did all of the hard work on this so she gets credit for this PR 😊

Description

New API

Purchases.shared.attribution.enableAdServicesAttributionTokenCollection()

⚠️ This was originally planned as Purchases.automaticAdServicesAttributionTokenCollection = true but changed when #1693 was introduced

💭 Any thoughts on this public API would be appreciated! Not sure if this feels too hidden or too long? 🤷‍♂️

Behavior

Will attempt to collect and post the AdServices token when enableAdServicesAttributionTokenCollection is called
Will attempt to send when app enters foreground
Will only send the token once

Co-authored-by: NachoSoto <NachoSoto@users.noreply.github.com> Co-authored-by: Josh Holtz <me@joshholtz.com>

# Conflicts: # RevenueCat.xcodeproj/project.pbxproj

# Conflicts: # Gemfile.lock # RevenueCat.xcodeproj/project.pbxproj # Sources/Logging/Strings/StoreKitStrings.swift # Sources/Misc/Deprecations.swift # Tests/UnitTests/Purchasing/PurchasesTests.swift

# Conflicts: # Tests/APITesters/SwiftAPITester/SwiftAPITester/PurchasesAPI.swift # Tests/UnitTests/Purchasing/Purchases/PurchasesTests.swift

Co-authored-by: NachoSoto <NachoSoto@users.noreply.github.com> Co-authored-by: Josh Holtz <me@joshholtz.com>

NachoSoto · 2022-06-22T15:32:55Z

 		A563F589271E1DAD00246E0C /* MockBeginRefundRequestHelper.swift in Sources */ = {isa = PBXBuildFile; fileRef = A563F585271E072B00246E0C /* MockBeginRefundRequestHelper.swift */; };
+		A56C2E012819C33500995421 /* BackendPostAdServicesTokenTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = A56C2E002819C33500995421 /* BackendPostAdServicesTokenTests.swift */; };
 		A56F9AB126990E9200AFC48F /* CustomerInfo.swift in Sources */ = {isa = PBXBuildFile; fileRef = A56F9AB026990E9200AFC48F /* CustomerInfo.swift */; };
+		A5B6CDD8280F3843007629D5 /* AdServices.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = A5B6CDD5280F3843007629D5 /* AdServices.framework */; platformFilters = (ios, maccatalyst, macos, ); settings = {ATTRIBUTES = (Weak, ); }; };


NachoSoto · 2022-06-22T15:36:02Z

        case .missing_advertiser_identifiers:
            return "Attribution error: identifierForAdvertisers is missing"

-        case .unknown_sk2_product_discount_type(let rawValue):


NachoSoto · 2022-06-22T15:45:16Z

        expect(invokedParameters.appUserID) == self.purchases?.appUserID
    }

-    func testAttributionDataSendsNetworkAppUserId() throws {


Should these have the equivalent tests now?

NachoSoto

The main issue is the old API still being available but "broken"

Fixes [CSDK-105]. This came up in #1534 (comment) and will simplify some of the code in #1727.

NachoSoto · 2022-06-27T19:04:34Z

+            let latestSent: [AttributionNetwork: String] =
+                latestAdvertisingIdsByRawNetworkSent.reduce(into: [:]) { adIdsByNetwork, adIdByRawNetworkString in
+                    if let networkRawValue = Int(adIdByRawNetworkString.key),
+                       let attributionNetwork = AttributionNetwork(rawValue: networkRawValue) {
+                    adIdsByNetwork[attributionNetwork] = adIdByRawNetworkString.value
+                } else {
+                    Logger.error(Strings.attribution.latest_attribution_sent_user_defaults_invalid)
+                }
+            }


#1734 is in main now so this can be simplified:

Suggested change

let latestSent: [AttributionNetwork: String] =

latestAdvertisingIdsByRawNetworkSent.reduce(into: [:]) { adIdsByNetwork, adIdByRawNetworkString in

if let networkRawValue = Int(adIdByRawNetworkString.key),

let attributionNetwork = AttributionNetwork(rawValue: networkRawValue) {

adIdsByNetwork[attributionNetwork] = adIdByRawNetworkString.value

} else {

Logger.error(Strings.attribution.latest_attribution_sent_user_defaults_invalid)

}

}

let latestSent: [AttributionNetwork: String] =

latestAdvertisingIdsByRawNetworkSent.compactMapKeys {

guard let networkRawValue = Int(adIdByRawNetworkString.key),

let attributionNetwork = AttributionNetwork(rawValue: networkRawValue) else {

Logger.error(Strings.attribution.latest_attribution_sent_user_defaults_invalid)

return nil

}

return attributionNetwork

}

NachoSoto · 2022-06-27T19:06:44Z

+            let latestAdIdsByRawNetworkStringSent =
+                latestAdvertisingIdsByNetworkSent.reduce(into: [:]) { adIdsByRawNetworkString, adIdByNetwork in
+                    adIdsByRawNetworkString[String(adIdByNetwork.key.rawValue)] = adIdByNetwork.value
+                }


Ditto:

Suggested change

let latestAdIdsByRawNetworkStringSent =

latestAdvertisingIdsByNetworkSent.reduce(into: [:]) { adIdsByRawNetworkString, adIdByNetwork in

adIdsByRawNetworkString[String(adIdByNetwork.key.rawValue)] = adIdByNetwork.value

}

let latestAdIdsByRawNetworkStringSent = latestAdvertisingIdsByNetworkSent.mapKeys { $0.key.rawValue }

NachoSoto · 2022-06-27T19:10:06Z

+    @available(iOS 14.3, macOS 11.1, macCatalyst 14.3, *)
+    @available(tvOS, unavailable)
+    @available(watchOS, unavailable)


I would put these in the extension to avoid repeating them in both methods.

NachoSoto · 2022-06-27T19:11:43Z

+        guard self.automaticAdServicesAttributionTokenCollection else {
+            return
+        }
+        attributionPoster.postAdServicesTokenIfNeeded()


Nit: this is more my opinion, but I think guard doesn't really add anything compared to a normal if:

Suggested change

guard self.automaticAdServicesAttributionTokenCollection else {

return

}

attributionPoster.postAdServicesTokenIfNeeded()

if self.automaticAdServicesAttributionTokenCollection {

self.attributionPoster.postAdServicesTokenIfNeeded()

}

Then we don't need that return.

NachoSoto · 2022-06-27T19:17:54Z

+
+#if canImport(AdServices)
+@available(iOS 14.3, macOS 11.1, macCatalyst 14.3, *)
+class AdServicesAttributionPosterTests: BaseAttributionPosterTests {


Ditto this needs an availability check in setUpWithError.

doing the availability check in setUpWithError leads to the "redundant availability check" error... and it seems like we've avoided making more specific checks in the AvailabilityChecks class?

I think without it these tests will fail on iOS 13 though (which won't be caught until this is merged in main.

NachoSoto

I just noticed this re-added PurchasesTests. If there are new tests there, you can put them in one of the subclasses or create another BasePurchasesTest subclass.

NachoSoto · 2022-06-27T19:20:27Z

+
+@testable import RevenueCat
+
+class PurchasesTests: BasePurchasesTests {


⚠️ this file was split starting from #1609, but looks like it got re-added here!

oops! removed, it shouldn't have had anything new...

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

NachoSoto · 2022-06-28T01:27:59Z

-        let newValueForNetwork = "\(identifierForAdvertisers ?? "(null)")_\(networkUserId ?? "(null)")"
-        guard latestSentToNetwork != newValueForNetwork else {
-            Logger.debug(Strings.attribution.skip_same_attributes)
+        guard let newDictToCache = getNewDictToCache(currentAppUserID: currentAppUserID,


Nit: explicit self per the style guide.

NachoSoto · 2022-06-28T01:34:41Z

Sorry I think I was reviewing the wrong diff 😕

NachoSoto

I think this is looking good!
The main thing left is the PurchasesTests file being re-added.

NachoSoto · 2022-06-28T01:36:26Z

+                 latestAdvertisingIdsByRawNetworkSent.compactMapKeys { network in
+                     guard let networkRawValue = Int(network),
+                        let attributionNetwork = AttributionNetwork(rawValue: networkRawValue) else {
+                             Logger.error(Strings.attribution.latest_attribution_sent_user_defaults_invalid)


I would include the network string in the error, so if we get a report with this message we can figure out why.

NachoSoto · 2022-06-28T01:36:45Z

+    func set(latestAdvertisingIdsByNetworkSent: [AttributionNetwork: String], appUserID: String) {
        self.userDefaults.write {
-            $0.setValue(latestNetworkAndAdvertisingIdsSent,
+            // convert AttributionNetwork to Integer as String


I think this comment doesn't add anything, that's implied in the types.

NachoSoto · 2022-06-28T01:40:30Z

I see what happened, I reviewed a bunch of (old) code that was simply re-added. You can ignore all those comments since that's existing code!

beylmk · 2022-06-28T01:45:53Z

I see what happened, I reviewed a bunch of (old) code that was simply re-added. You can ignore all those comments since that's existing code!

thanks for enduring all the confusion on this one 😬

beylmk · 2022-06-28T16:34:52Z

@NachoSoto i believe all updates are done/replied to... we won't actually merge this btw, we are going to use this branch to release a beta.

NachoSoto

Awesome!

NachoSoto · 2022-06-28T16:37:29Z

+        let latestTokenSent = latestNetworkIdAndAdvertisingIdentifierSent(network: .adServices)
+        guard latestTokenSent == nil else {
+            return
+        }


Since latestTokenSent isn't used in the method we don't need a variable for it (it's kind of confusing because it's always going to be nil. I'd change this to:

Suggested change

let latestTokenSent = latestNetworkIdAndAdvertisingIdentifierSent(network: .adServices)

guard latestTokenSent == nil else {

return

}

guard self.latestNetworkIdAndAdvertisingIdentifierSent(network: .adServices) == nil else {

return

}

NachoSoto · 2022-06-28T16:38:46Z

@@ -1,16 +0,0 @@
-{


I suspect this file is still needed to make iOS 12 tests pass.

ah yes, my bad. updating

wonder why the CI tests passed..

We only run iOS 12 in main

NachoSoto

I think this is ready except for the iOS 12/13 issue with availability checks. We can add a new method in AvailabilityChecks to deal with that.

NachoSoto · 2022-08-02T20:17:04Z

+        let latestTokenSent = latestNetworkIdAndAdvertisingIdentifierSent(network: .adServices)
+        guard latestTokenSent == nil else {
+            return
+        }


NachoSoto · 2022-08-02T20:19:11Z

+
+#if canImport(AdServices)
+@available(iOS 14.3, macOS 11.1, macCatalyst 14.3, *)
+class AdServicesAttributionPosterTests: BaseAttributionPosterTests {


I think without it these tests will fail on iOS 13 though (which won't be caught until this is merged in main.

NachoSoto · 2022-08-02T20:19:51Z

        expect(invokedParameters.appUserID) == self.purchases?.appUserID
    }

-    func testAttributionDataSendsNetworkAppUserId() throws {


This removes this unnecessary type in the global namespace. Also removed 2 `cases` introduced in #1727 that aren't actually used.

beylmk and others added 8 commits May 2, 2022 09:38

[CF-106] Fetch AdServices Token (#1519)

fd2fb2d

Co-authored-by: NachoSoto <NachoSoto@users.noreply.github.com> Co-authored-by: Josh Holtz <me@joshholtz.com>

Merge branch 'main' into ad-services-sdk

6879358

# Conflicts: # RevenueCat.xcodeproj/project.pbxproj

Merge branch 'main' into ad-services-sdk

d14960f

# Conflicts: # Gemfile.lock # RevenueCat.xcodeproj/project.pbxproj # Sources/Logging/Strings/StoreKitStrings.swift # Sources/Misc/Deprecations.swift # Tests/UnitTests/Purchasing/PurchasesTests.swift

Merge branch 'main' into ad-services-sdk

e1021cb

# Conflicts: # Tests/APITesters/SwiftAPITester/SwiftAPITester/PurchasesAPI.swift # Tests/UnitTests/Purchasing/Purchases/PurchasesTests.swift

[CF-553] Post AdServices token (#1534)

cbe9819

Co-authored-by: NachoSoto <NachoSoto@users.noreply.github.com> Co-authored-by: Josh Holtz <me@joshholtz.com>

Merged in main

cbc44cb

Update ios 14 snapshots tets

f5ed569

Reworked adservices to use new Purchases.shared.attribution API

c4b6440

joshdholtz requested a review from a team June 22, 2022 04:26

joshdholtz marked this pull request as ready for review June 22, 2022 04:28

NachoSoto reviewed Jun 22, 2022

View reviewed changes

NachoSoto suggested changes Jun 22, 2022

View reviewed changes

NachoSoto reviewed Jun 22, 2022

View reviewed changes

Comment thread Tests/APITesters/ObjCAPITester/ObjCAPITester/RCPurchasesAPI.m Outdated

joshdholtz added 2 commits June 22, 2022 20:36

Updated with some comments from PR

efa22df

Merged main

a3e6429

NachoSoto mentioned this pull request Jun 23, 2022

Added Dictionary.mapKeys and Dictionary.compactMapKeys #1734

Merged

NachoSoto added a commit that referenced this pull request Jun 23, 2022

Added Dictionary.mapKeys and Dictionary.compactMapKeys

8cc9ca0

Fixes [CSDK-105]. This came up in #1534 (comment) and will simplify some of the code in #1727.

Merge branch 'main' into ad-services-sdk

e18b65e

beylmk mentioned this pull request Jun 27, 2022

Add iAd code back #1739

Merged

NachoSoto added a commit that referenced this pull request Jun 27, 2022

Added Dictionary.mapKeys and Dictionary.compactMapKeys (#1734)

e2fb802

Fixes [CSDK-105]. This came up in #1534 (comment) and will simplify some of the code in #1727.

NachoSoto mentioned this pull request Jun 27, 2022

[DO NOT MERGE] iAd + AdServices #1740

Closed

NachoSoto reviewed Jun 27, 2022

View reviewed changes

Comment thread Sources/Misc/Deprecations.swift Outdated

NachoSoto reviewed Jun 27, 2022

View reviewed changes

NachoSoto suggested changes Jun 27, 2022

View reviewed changes

Add iAd code back (#1739)

45ace53

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

Maddie Beyl added 2 commits June 27, 2022 18:26

Merge branch 'main' into ad-services-sdk

64740f7

use new mapKeys dictionary extension for device cache

4f99f74

NachoSoto reviewed Jun 28, 2022

View reviewed changes

update for some pr comments

07efb51

beylmk changed the title ~~AdServices~~ [DO NOT MERGE] AdServices Jun 28, 2022

remove purchasestests

4b48fa4

NachoSoto approved these changes Jun 28, 2022

View reviewed changes

Maddie Beyl and others added 7 commits June 28, 2022 16:09

generate missing snapshots

30135d6

fix deprecation messages to clarify method now called on shared instance

a511a74

Fix for inserting nonstring into userdefaults

3cee7b5

Add tests for bug

760a4c1

fix lint

cdb8df6

Merge tag '4.7.0' into ad-services-sdk

9b3657f

Merge remote-tracking branch 'origin/main' into ad-services-sdk

7af4813

NachoSoto reviewed Aug 2, 2022

View reviewed changes

joshdholtz added 2 commits August 2, 2022 15:47

Added availablility checks for tests and removed unused varialbes

d33d501

Whoops... wrong class

04363d7

joshdholtz changed the title ~~[DO NOT MERGE] AdServices~~ AdServices Aug 4, 2022

joshdholtz merged commit 343ba7e into main Aug 8, 2022

joshdholtz deleted the ad-services-sdk branch August 8, 2022 12:42

This was referenced Aug 8, 2022

Release/4.10.0 #1799

Closed

Release/4.10.0 #1800

Merged

NachoSoto mentioned this pull request Aug 9, 2022

Moved AttributionFetcherError inside AttributionFetcher #1808

Merged

NachoSoto added a commit that referenced this pull request Aug 13, 2022

Moved AttributionFetcherError inside AttributionFetcher (#1808)

db96beb

This removes this unnecessary type in the global namespace. Also removed 2 `cases` introduced in #1727 that aren't actually used.


		@testable import RevenueCat

		class PurchasesTests: BasePurchasesTests {

Conversation

joshdholtz commented Jun 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Description

New API

Behavior

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

NachoSoto Jun 27, 2022 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

NachoSoto commented Jun 28, 2022

Uh oh!

beylmk commented Jun 28, 2022

Uh oh!

beylmk commented Jun 28, 2022

Uh oh!

NachoSoto left a comment

Choose a reason for hiding this comment

Uh oh!

joshdholtz commented Jun 22, 2022 •

edited

Loading

NachoSoto Jun 27, 2022 •

edited

Loading