-
Notifications
You must be signed in to change notification settings - Fork 316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AdServices #1727
AdServices #1727
Conversation
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>
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, ); }; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
@@ -115,8 +119,21 @@ extension AttributionStrings: CustomStringConvertible { | |||
case .missing_advertiser_identifiers: | |||
return "Attribution error: identifierForAdvertisers is missing" | |||
|
|||
case .unknown_sk2_product_discount_type(let rawValue): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call
@@ -73,74 +56,4 @@ class PurchasesAttributionDataTests: BasePurchasesTests { | |||
expect(invokedParameters.appUserID) == self.purchases?.appUserID | |||
} | |||
|
|||
func testAttributionDataSendsNetworkAppUserId() throws { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these have the equivalent tests now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump?
Tests/UnitTests/SubscriberAttributes/SubscriberAttributesManagerTests.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Fixes [CSDK-105]. This came up in #1534 (comment) and will simplify some of the code in #1727.
Sources/Caching/DeviceCache.swift
Outdated
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) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1734 is in main
now so this can be simplified:
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 | |
} |
Sources/Caching/DeviceCache.swift
Outdated
let latestAdIdsByRawNetworkStringSent = | ||
latestAdvertisingIdsByNetworkSent.reduce(into: [:]) { adIdsByRawNetworkString, adIdByNetwork in | ||
adIdsByRawNetworkString[String(adIdByNetwork.key.rawValue)] = adIdByNetwork.value | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto:
let latestAdIdsByRawNetworkStringSent = | |
latestAdvertisingIdsByNetworkSent.reduce(into: [:]) { adIdsByRawNetworkString, adIdByNetwork in | |
adIdsByRawNetworkString[String(adIdByNetwork.key.rawValue)] = adIdByNetwork.value | |
} | |
let latestAdIdsByRawNetworkStringSent = latestAdvertisingIdsByNetworkSent.mapKeys { $0.key.rawValue } |
@available(iOS 14.3, macOS 11.1, macCatalyst 14.3, *) | ||
@available(tvOS, unavailable) | ||
@available(watchOS, unavailable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put these in the extension
to avoid repeating them in both methods.
guard self.automaticAdServicesAttributionTokenCollection else { | ||
return | ||
} | ||
attributionPoster.postAdServicesTokenIfNeeded() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this is more my opinion, but I think guard
doesn't really add anything compared to a normal if
:
guard self.automaticAdServicesAttributionTokenCollection else { | |
return | |
} | |
attributionPoster.postAdServicesTokenIfNeeded() | |
if self.automaticAdServicesAttributionTokenCollection { | |
self.attributionPoster.postAdServicesTokenIfNeeded() | |
} |
Then we don't need that return
.
|
||
#if canImport(AdServices) | ||
@available(iOS 14.3, macOS 11.1, macCatalyst 14.3, *) | ||
class AdServicesAttributionPosterTests: BaseAttributionPosterTests { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto this needs an availability check in setUpWithError
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think without it these tests will fail on iOS 13 though (which won't be caught until this is merged in main
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
@testable import RevenueCat | ||
|
||
class PurchasesTests: BasePurchasesTests { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops! removed, it shouldn't have had anything new...
Co-authored-by: NachoSoto <NachoSoto@users.noreply.github.com>
let newValueForNetwork = "\(identifierForAdvertisers ?? "(null)")_\(networkUserId ?? "(null)")" | ||
guard latestSentToNetwork != newValueForNetwork else { | ||
Logger.debug(Strings.attribution.skip_same_attributes) | ||
guard let newDictToCache = getNewDictToCache(currentAppUserID: currentAppUserID, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: explicit self
per the style guide.
Sorry I think I was reviewing the wrong diff 😕 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is looking good!
The main thing left is the PurchasesTests
file being re-added.
Sources/Caching/DeviceCache.swift
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would include the network
string in the error, so if we get a report with this message we can figure out why.
Sources/Caching/DeviceCache.swift
Outdated
self.userDefaults.write { | ||
$0.setValue(latestNetworkAndAdvertisingIdsSent, | ||
// convert AttributionNetwork to Integer as String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment doesn't add anything, that's implied in the types.
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 😬 |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
let latestTokenSent = latestNetworkIdAndAdvertisingIdentifierSent(network: .adServices) | ||
guard latestTokenSent == nil else { | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
let latestTokenSent = latestNetworkIdAndAdvertisingIdentifierSent(network: .adServices) | |
guard latestTokenSent == nil else { | |
return | |
} | |
guard self.latestNetworkIdAndAdvertisingIdentifierSent(network: .adServices) == nil else { | |
return | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump
@@ -1,16 +0,0 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this file is still needed to make iOS 12 tests pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yes, my bad. updating
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wonder why the CI tests passed..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only run iOS 12 in main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
let latestTokenSent = latestNetworkIdAndAdvertisingIdentifierSent(network: .adServices) | ||
guard latestTokenSent == nil else { | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump
|
||
#if canImport(AdServices) | ||
@available(iOS 14.3, macOS 11.1, macCatalyst 14.3, *) | ||
class AdServicesAttributionPosterTests: BaseAttributionPosterTests { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think without it these tests will fail on iOS 13 though (which won't be caught until this is merged in main
.
@@ -73,74 +56,4 @@ class PurchasesAttributionDataTests: BasePurchasesTests { | |||
expect(invokedParameters.appUserID) == self.purchases?.appUserID | |||
} | |||
|
|||
func testAttributionDataSendsNetworkAppUserId() throws { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump?
This removes this unnecessary type in the global namespace. Also removed 2 `cases` introduced in #1727 that aren't actually used.
Motivation
@beylmk did all of the hard work on this so she gets credit for this PR 😊
Description
New API
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
enableAdServicesAttributionTokenCollection
is called