[CF-106] Fetch AdServices Token by beylmk · Pull Request #1519 · RevenueCat/purchases-ios

beylmk · 2022-04-20T20:16:54Z

AdServices token fetching and removal of old iAD code. Posting to the backend will be the next step.

deprecates old automaticAppleSearchAdsAttributionCollection
adds new automaticAdServicesAttributionTokenCollection
fetches attribution token

There is a follow-up ticket to investigate linking, and we will probably switch to weak-linking then.

…ction, deprecated automaticAppleSearchAdsAttributionCollection

NachoSoto · 2022-04-20T20:19:48Z

 		A563F586271E072B00246E0C /* MockBeginRefundRequestHelper.swift in Sources */ = {isa = PBXBuildFile; fileRef = A563F585271E072B00246E0C /* MockBeginRefundRequestHelper.swift */; };
 		A563F589271E1DAD00246E0C /* MockBeginRefundRequestHelper.swift in Sources */ = {isa = PBXBuildFile; fileRef = A563F585271E072B00246E0C /* MockBeginRefundRequestHelper.swift */; };
 		A56F9AB126990E9200AFC48F /* CustomerInfo.swift in Sources */ = {isa = PBXBuildFile; fileRef = A56F9AB026990E9200AFC48F /* CustomerInfo.swift */; };
+		A5B6CDD8280F3843007629D5 /* AdServices.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = A5B6CDD5280F3843007629D5 /* AdServices.framework */; };


Should this be weakly linked?

In case I don't get to review this again, just wanted to point out that this is probably my most critical comment. I believe this has to be weakly linked so we don't make all users of the SDK implicitly link AdServices.

+1 on this, this part is a very delicate change that we should research and test out thoroughly.
It might even be a red flag for privacy-minded folks who're looking into using our SDK. Perhaps we can do the extra work of calling this through the same "reflection" we did for iAd?

are you both okay with me doing the linking research as the follow-up ticket i created? would definitely be before merging into main. this just feels like enough work to warrant a separate PR

absolutely! It's only important to get the linking right before merging to main, but it can totally be done as a separate thing

okay then that's the plan!

NachoSoto · 2022-04-20T20:20:03Z


+    @available(iOS 14.3, *)
+    func adServicesToken(completion: @escaping (String?, Error?) -> Void) {
+        // TODO check for library?


I think canImport would help here too :)

NachoSoto · 2022-04-20T20:20:19Z

            return "This StoreProduct represents an SK1 product, the type of product cannot be determined, " +
            "the value will be undefined. Use `StoreProduct.productCategory` instead."
+
+        case .unknown_sk2_product_discount_type(let rawValue):


NachoSoto · 2022-04-20T20:20:39Z

+     * Enable automatic collection of AdServices attribution token. Defaults to `false`.
     */
-    @objc public static var automaticAppleSearchAdsAttributionCollection: Bool = false
+    @available(iOS 14.3, *)


I think this will need the tvOS/macCatalyst (macOS too?) equivalents too

yep good call. xcode never suggests the full checks for us, do they...

Nope, only for the currently selected target unfortunately.

NachoSoto · 2022-04-20T20:20:57Z

+     * Enable automatic collection of Apple Search Ads attribution. Defaults to `false`.
+     */
+    @available(*, deprecated, message: "use Purchases.automaticAdServicesAttributionTokenCollection for iOS 14.3+ instead")
+    @objc static var automaticAppleSearchAdsAttributionCollection: Bool = false


Maybe move this to Deprecations.swift?

is there a difference between methods in Deprecations.swift and those in the // MARK: Deprecated section of Purchases.swift?

Oh I didn't realize we still had some there. The idea was to make the main file (which is already huge) smaller by only having the current API.

makes sense to me! i can move the rest while i'm at it...

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

…o ad-services

This reverts commit 47ecfff.

beylmk · 2022-04-27T23:16:12Z

-        expect(MockAdClientProxy.requestAttributionDetailsCallCount) == 0
-    }
-
-    func testPostAppleSearchAdsAttributionIfNeededSkipsIfAlreadySent() throws {


i'll add back related tests in the post PR

beylmk · 2022-04-28T15:56:54Z

need to figure out why build-tv-watch-and-macos is failing, but otherwise this is ready for re-review

NachoSoto

Looks good! Just a few minor comments.
Let me know if I can help figuring out the watchOS issue, might be because AdServices isn't available, so maybe weak linking might fix that?

NachoSoto · 2022-04-28T16:01:35Z

-        afficheClientProxy.requestAttributionDetails(completion)
 #else
-        completion(nil, AttributionFetcherError.identifierForAdvertiserUnavailableForPlatform)
+        Logger.warn(Strings.attribution.adservices_not_supported)


NachoSoto · 2022-04-28T16:02:09Z

-            return
+    // should match OS availability in https://developer.apple.com/documentation/ad_services
+    @available(iOS 14.3, macOS 11.1, macCatalyst 14.3, *)
+    func adServicesToken() -> String? {


I think this would be better as a property:

Suggested change

func adServicesToken() -> String? {

var adServicesToken: String? {

NachoSoto · 2022-04-28T16:02:40Z

+        if let attributionToken = try? AAAttribution.attributionToken() {
+            return attributionToken
+        } else {
+            Logger.warn(Strings.attribution.adservices_token_fetch_failed)


Maybe do a do/catch instead of try? to add the underlying error to this?
Also I wonder if it should be Logger.appleError

NachoSoto · 2022-04-28T16:05:06Z

-        postAppleSearchAddsAttributionCollectionIfNeeded()
+
+        // should match OS availability in https://developer.apple.com/documentation/ad_services
+        if #available(iOS 14.3, macOS 11.1, macCatalyst 14.3, *) {            postAdServicesTokenIfNeeded()


Formatting here is weird, did you mean to put it in a new line?

Suggested change

if #available(iOS 14.3, macOS 11.1, macCatalyst 14.3, *) { postAdServicesTokenIfNeeded()

if #available(iOS 14.3, macOS 11.1, macCatalyst 14.3, *) {

postAdServicesTokenIfNeeded()

oh woops yes. was on a tiny screen this day and didn't realize, good catch

NachoSoto · 2022-04-28T16:05:29Z

            return .promotional
        default:
-            Logger.warn(Strings.attribution.unknown_sk2_product_discount_type(rawValue: sk2Discount.type.rawValue))
+            Logger.warn(Strings.storeKit.unknown_sk2_product_discount_type(rawValue: sk2Discount.type.rawValue))


NachoSoto · 2022-04-28T16:06:15Z


 class MockAttributionFetcher: AttributionFetcher {

+    var adServicesTokenCollectionCalled = false


Nit: maybe move this right on top of adServicesToken? I think that's what other mocks do

NachoSoto · 2022-04-28T16:06:26Z

@@ -1,16 +0,0 @@
-{
-  "headers" : {


Thanks for removing these too!

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

beylmk · 2022-04-28T19:24:04Z

Does this have to be optional?

that was me trying to weak link the framework. i'm not seeing very clear guidance on how to do that in an initial google search...

beylmk · 2022-04-28T22:15:17Z

Looks good! Just a few minor comments. Let me know if I can help figuring out the watchOS issue, might be because AdServices isn't available, so maybe weak linking might fix that?

if you have time to look that'd be great 🙏

NachoSoto · 2022-04-29T20:28:01Z

 	objects = {

 /* Begin PBXBuildFile section */
+		2C396F5E281C64B700669657 /* AdServices.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 2C396F5D281C64B700669657 /* AdServices.framework */; };


⚠️ I think we shouldn't do this. The fact that APITesters needs it is a sign that users will need to add it as well, which isn't good. I'm guessing the actual problem was this: #1554.

agreed. promise i won't forget this before merging to main.

This fixes the watchOS and tvOS compilation (yay CI test coverage catching this!) Now the framework is only linked for the rest of the platforms.

beylmk · 2022-04-29T22:08:23Z

@NachoSoto thanks for your help!! i think this finally passed everything if you have time to review :)

NachoSoto · 2022-05-02T16:39:33Z

-            self.post(attributionData: attributionDetails, fromNetwork: .appleSearchAds, networkUserId: nil)
-        }
+        Logger.debug("Logging attribution token for now to avoid lint warning: \(attributionToken)")
+        // post


Maybe leave this as a TODO? it's okay if lint fails since we're not merging this in main yet. That way we ensure it doesn't slip through the cracks.

NachoSoto · 2022-05-02T16:42:23Z

+     * Should match OS availability in https://developer.apple.com/documentation/ad_services
     */
-    @objc public static var automaticAppleSearchAdsAttributionCollection: Bool = false
+    @available(iOS 14.3, macOS 11.1, macCatalyst 14.3, *)


This is being implicitly marked as available for any version of tvOS and watchOS. You need to add this:

@available(tvOS, unavailable) @available(watchOS, unavailable)

NachoSoto · 2022-05-02T16:42:29Z

-    private func postAppleSearchAddsAttributionCollectionIfNeeded() {
-        guard Self.automaticAppleSearchAdsAttributionCollection else {
+    // should match OS availability in https://developer.apple.com/documentation/ad_services
+    @available(iOS 14.3, macOS 11.1, macCatalyst 14.3, *)


oops missed this, will get into the post PR

* [CF-106] Fetch AdServices Token (#1519) Co-authored-by: NachoSoto <NachoSoto@users.noreply.github.com> Co-authored-by: Josh Holtz <me@joshholtz.com> * [CF-553] Post AdServices token (#1534) Co-authored-by: NachoSoto <NachoSoto@users.noreply.github.com> Co-authored-by: Josh Holtz <me@joshholtz.com> * Update ios 14 snapshots tets * Reworked adservices to use new Purchases.shared.attribution API * Updated with some comments from PR * Add iAd code back (#1739) Co-authored-by: NachoSoto <NachoSoto@users.noreply.github.com> * use new mapKeys dictionary extension for device cache * update for some pr comments * remove purchasestests * generate missing snapshots * fix deprecation messages to clarify method now called on shared instance * Fix for inserting nonstring into userdefaults * Add tests for bug * fix lint * Added availablility checks for tests and removed unused varialbes * Whoops... wrong class Co-authored-by: beylmk <maddie@revenuecat.com> Co-authored-by: NachoSoto <NachoSoto@users.noreply.github.com>

Maddie Beyl added 9 commits April 20, 2022 10:15

Add new configuration option automaticAdServicesAttributionTokenColle…

f3131f9

…ction, deprecated automaticAppleSearchAdsAttributionCollection

Add postAdServicesTokenIfNeeded, call in same places as old iAD calls

d322c7c

Fetch and post ad services token

dbcdc4c

Fix incorrect available check

c557ba5

Pull discount string from attribution strings to StoreKitStrings

882a936

Merge branch 'main' into ad-services

21aa633

fetching unit tests

cc93cdb

formatting/renaming

9706097

Remove code for fetching

c98031a

NachoSoto reviewed Apr 20, 2022

View reviewed changes

beylmk and others added 3 commits April 20, 2022 13:34

Use canImport

03357e7

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

Update availability checks, move to Deprecations.swift

bcd58a2

Merge branch 'ad-services' of github.com:RevenueCat/purchases-ios int…

5c957b7

…o ad-services

beylmk force-pushed the ad-services branch from 5bd7362 to aac476f Compare April 21, 2022 19:16

Remove some todos, fix formatting

4c6332c

beylmk force-pushed the ad-services branch 2 times, most recently from 2abd531 to 160efb4 Compare April 22, 2022 10:54

fix lint

4a952c4

beylmk force-pushed the ad-services branch from 160efb4 to 4a952c4 Compare April 22, 2022 14:01

aboedo reviewed Apr 25, 2022

View reviewed changes

Comment thread Sources/Purchasing/Purchases.swift

fix mockattributionfetcher

3811408

beylmk force-pushed the ad-services branch from fd857c1 to 3811408 Compare April 25, 2022 15:51

Maddie Beyl added 5 commits April 25, 2022 14:14

API testers

e02c0d2

Merge branch 'ad-services-sdk' into ad-services

81f2628

move rest of deprecations to deprecations.swift

47ecfff

Add comment about OS availability

d51c37f

Revert "move rest of deprecations to deprecations.swift"

f2014f8

This reverts commit 47ecfff.

beylmk force-pushed the ad-services branch from 899d574 to f2014f8 Compare April 25, 2022 21:43

Maddie Beyl added 2 commits April 25, 2022 18:11

Add explicit integer values for attribution networks

8923389

Fix deprecation warning ...

e5dea3a

beylmk mentioned this pull request Apr 27, 2022

Update deprecations #1544

Closed

Maddie Beyl added 2 commits April 27, 2022 12:45

remove iad-associated unit tests

67b3e7a

Merge branch 'ad-services-sdk' into ad-services

4d800ac

beylmk force-pushed the ad-services branch from 5268820 to 4d800ac Compare April 27, 2022 16:57

Maddie Beyl added 3 commits April 27, 2022 14:19

fix more unit tests

2b21373

Fix bad merges

61c28a3

remove PostAttributionDataOperation and AfficheClientProxy

26b02f7

beylmk commented Apr 27, 2022

View reviewed changes

beylmk requested a review from a team April 28, 2022 15:56

NachoSoto approved these changes Apr 28, 2022

View reviewed changes

Maddie Beyl and others added 5 commits April 28, 2022 09:48

Make adServicesToken a var

a11742a

use do catch, more details in error log

6a7697b

formatting

60db73b

Update Sources/Purchasing/Purchases.swift

1df28a0

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

linking optional

3e3d383

Maddie Beyl and others added 4 commits April 28, 2022 12:24

directly return

cc98cbe

CustomerInfo: moved deprecated property to Deprecations (#1549)

839bc8d

make error not optional

2c16386

Merge branch 'main' into ad-services

42bbabc

Add AdServices.framework to APITesters

3de0061

NachoSoto reviewed Apr 29, 2022

View reviewed changes

Don't link AdServices.framework on watchOS & tvOS (#1554)

d9f757b

This fixes the watchOS and tvOS compilation (yay CI test coverage catching this!) Now the framework is only linked for the rest of the platforms.

beylmk merged commit fd2fb2d into ad-services-sdk May 2, 2022

beylmk deleted the ad-services branch May 2, 2022 16:38

NachoSoto reviewed May 2, 2022

View reviewed changes

	func adServicesToken() -> String? {
	var adServicesToken: String? {

	if #available(iOS 14.3, macOS 11.1, macCatalyst 14.3, *) { postAdServicesTokenIfNeeded()
	if #available(iOS 14.3, macOS 11.1, macCatalyst 14.3, *) {
	postAdServicesTokenIfNeeded()


		class MockAttributionFetcher: AttributionFetcher {

		var adServicesTokenCollectionCalled = false

Conversation

beylmk commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beylmk commented Apr 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!

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!

beylmk commented Apr 28, 2022

Uh oh!

beylmk commented Apr 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beylmk commented Apr 29, 2022

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!

Reviewers

Assignees

beylmk commented Apr 20, 2022 •

edited

Loading