Adds conversion of attribution to subscriber attributes #609
Conversation
| } | ||
|
|
||
| func testAdjustAttributionConversionWorksWithNullIDFA() { | ||
| let adjustData = adjustData(withIDFA: nil) |
There was a problem hiding this comment.
These tests are failing because I am inserting nils in the dictionary, which I am actually not sure it represents the real use of our addAttributionData function. I've looked at our swift interface and the data we accept in Purchases.addAttributionData is of type [AnyHashable: Any], but in these tests I assume developers can pass nil as values in the data. I don't think there's a way developers can do that right? @aboedo tagging you since you reviewed the Android counterpart and might know the answer.
There was a problem hiding this comment.
tricky one.
in objc, you can't set nil in a dict, but you can do NSNull.
in swift, you can't pass in something that's nil directly, as it won't technically be Any if it's nil.
So the following code:
Purchases.addAttributionData(["foo": nil], from: .adjust)produces
'nil' is not compatible with expected dictionary value type 'Any'
buuuut
the following code:
let myDict: [String: Any?] = ["lalala": nil]
Purchases.addAttributionData(myDict, from: .adjust)compiles just fine, because Any? counts as Any.
As you have probably guessed, using Any sucks. We should always strive to constrain the interface further than that to prevent this kind of issue.
I tried this out just to see what would happen with the old implementation, and it seems to get translated to NSNull.
There was a problem hiding this comment.
and the NSNull eventually gets sent to the backend as the string "<null>" I believe. So in order to preserve this behavior we just need to make sure that we're still compatible with something like
Purchases.addAttributionData(["foo": NSNull()], from: .adjust)
vegaro
force-pushed
the
attribution-data-conversion
branch
3 times, most recently
from
3405cc1
to
c4837cb
Compare
vegaro
force-pushed
the
attribution-data-conversion
branch
from
c4837cb
to
7c8ad4f
Compare
PurchasesCoreSwift/SubscriberAttributes/AttributionDataMigrator.swift
Outdated
Show resolved
Hide resolved
| if let value = fixedData[AttributionKey.networkID.rawValue] { | ||
| convertedAttribution[SpecialSubscriberAttributes.appsFlyerID] = value | ||
| } | ||
| if let value = fixedData[AttributionKey.AppsFlyer.id.rawValue] { | ||
| convertedAttribution[SpecialSubscriberAttributes.appsFlyerID] = value | ||
| } |
There was a problem hiding this comment.
I'm not familiar with how the mapping should go for these, so I'm assuming that they're correct, and that these are mostly coming from reverse-engineering the values, right? there's no documentation we can point to?
There was a problem hiding this comment.
I looked into khepri's code. I could point to that, but being this open source I am not sure it would make sense.
There was a problem hiding this comment.
I added a comment to the new extracted function
PurchasesCoreSwift/SubscriberAttributes/AttributionNetwork.swift
Outdated
Show resolved
Hide resolved
| // swiftlint:disable identifier_name | ||
| @objc(RCSpecialSubscriberAttributes) public class SpecialSubscriberAttributes: NSObject { | ||
| @objc public static let email = "$email" |
There was a problem hiding this comment.
I'm guessing that this is done this way so we can share it with obj-c, and we'll probably update it during the swift migration, right?
| @@ -0,0 +1,162 @@ | |||
| // | |||
| // RCAttributionFetcher.m | |||
PurchasesCoreSwift/SubscriberAttributes/AttributionDataMigrator.swift
Outdated
Show resolved
Hide resolved
| private func function(keyPresence: KeyPresence, data: inout [String: Any], key: String, defaultValue: String) { | ||
| switch keyPresence { | ||
| case .defaultValue: | ||
| data[key] = defaultValue | ||
| case .nsNull: | ||
| data[key] = NSNull() | ||
| case .notPresent: | ||
| break | ||
| } |
There was a problem hiding this comment.
perhaps we can improve the naming for this func? updateMappingInData?
Also, super nitpicky but I'd usually have the inout param as the first item, so it's clear that that's the item being modified.
It'd also allow for naming like: updateMapping(inData: data, keyPresence: .nsNull, key: "idfa", defaultValue: "idfa")
There was a problem hiding this comment.
oh yes I forgot to update hahaha function lol 😆 Good catch!
This was a tedious one because I had to basically mimic what the backend is doing to convert attribution data to subscriber attributes.
I removed the backend call, because it shouldn't be used anymore, and added conversion functions for each of the attribution networks.
Best way of reviewing this is by looking into the backend code specific for each network in the
attribution_fieldsfolder.