-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[WEB-863] Guard Segment Events Behind AppTrackingTransparency #1799
Conversation
6b2e410
to
fc53456
Compare
Codecov Report
@@ Coverage Diff @@
## main #1799 +/- ##
========================================
Coverage 88.92% 88.93%
========================================
Files 887 888 +1
Lines 80981 81086 +105
Branches 21258 21279 +21
========================================
+ Hits 72015 72114 +99
- Misses 8204 8207 +3
- Partials 762 765 +3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
based off of the Add CAPI events PR. |
fc53456
to
f1a39da
Compare
5ee34c9
to
1447d1a
Compare
827ce82
to
f9bfdea
Compare
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.
Almost there, a few things here.
- Should we send the IDFA with every call now that's required for every call?
- Save the advertisingIdentifier to AppEnvironment so we can use AppEnvironment, instead of recreating the struct everywhere (defeats the purpose of adding it to AppEnvironment)
@@ -0,0 +1,14 @@ | |||
import Library | |||
|
|||
struct MockAppTrackingTransparencyService: AppTrackingTransparencyType { |
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.
Nice, good work adding this.
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.
Just check spelling here and can we put this in the TestHelpers
folder.
Library/Tracking/KSRAnalytics.swift
Outdated
if featureConsentManagementDialogEnabled() { | ||
guard AppEnvironment.current.appTrackingTransparency.authorizationStatus() == .authorized | ||
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.
can be cleaner with
guard featureConsentManagementDialogEnabled(), AppEnvironment.current.appTrackingTransparency.authorizationStatus() == .authorized 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.
So given we're requiring IDFA here, would KS want to get IDFA on every call? If that's the case then we can add it to userProperties
which is sent up on every call.
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.
Do you think this as simple as adding
props["advertising_identifer"] = AppEnvironment.current.appTrackingTransparency.advertisingIdentifier
to the userProperties method in KSRAnalytics? Or would it require updating the user model with a new property and potentially add too much to this current PR?
@@ -398,7 +398,7 @@ public final class PledgePaymentMethodsViewModel: PledgePaymentMethodsViewModelT | |||
let (project, _) = projectSignal | |||
|
|||
guard featureFacebookConversionsAPIEnabled(), project.sendMetaCapiEvents == true, | |||
let externalId = AppTrackingTransparency.advertisingIdentifier() else { return } | |||
let externalId = AppTrackingTransparency().advertisingIdentifier() 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 we have it in the app environment, we can use AppEnvironment.current.appTrackingTransparency
instead of creating a new instance of the struct. The only place we'd need to create that struct is after user is prompted and the dialog returns their answer.
@@ -348,7 +348,7 @@ public final class ProjectPageViewModel: ProjectPageViewModelType, ProjectPageVi | |||
let (project, _) = projectAndRefTag | |||
|
|||
guard featureFacebookConversionsAPIEnabled(), project.sendMetaCapiEvents, | |||
let externalId = AppTrackingTransparency.advertisingIdentifier() else { return } | |||
let externalId = AppTrackingTransparency().advertisingIdentifier() 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.
Same as above.
@@ -236,7 +236,7 @@ public final class RewardsCollectionViewModel: RewardsCollectionViewModelType, | |||
let (project, _) = projectAndRefTag | |||
|
|||
guard featureFacebookConversionsAPIEnabled(), project.sendMetaCapiEvents, | |||
let externalId = AppTrackingTransparency.advertisingIdentifier() else { return } | |||
let externalId = AppTrackingTransparency().advertisingIdentifier() 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.
Same as above.
@@ -218,7 +218,7 @@ public final class ThanksViewModel: ThanksViewModelType, ThanksViewModelInputs, | |||
} | |||
|
|||
guard featureFacebookConversionsAPIEnabled(), project.sendMetaCapiEvents, | |||
let externalId = AppTrackingTransparency.advertisingIdentifier() else { return } | |||
let externalId = AppTrackingTransparency().advertisingIdentifier() 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.
Same as above.
a96c6bf
to
14c600a
Compare
14c600a
to
602a8a9
Compare
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.
Ok all good, great work!
One minor thing - if you could quickly add appTrackingTransparency
to replaceEnvironment
static func on AppEnvironment
that would make it consistent there as well.
Oh yea and get tests to pass ;) |
06dfa83
to
033936d
Compare
d8c7b3f
to
c6f9401
Compare
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.
Hey just asking for a few changes on the base TestCase
@@ -33,6 +33,9 @@ internal class TestCase: XCTestCase { | |||
var calendar = Calendar(identifier: .gregorian) | |||
calendar.timeZone = TimeZone(identifier: "GMT")! | |||
|
|||
self.optimizelyClient = MockOptimizelyClient() | |||
|> \.features .~ [OptimizelyFeature.consentManagementDialogEnabled.rawValue: true] | |||
|
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.
Can you remove this from the base TestCase
and put it back into the override setUp
for the individual test class? I was thinking about this method.
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.
Create a class level variable MockOptimizelyClient
in the override class setup
func in PledgeViewModelTests
for example, and then update it to false
for whatever flags you need to be setup, then update the individual cases that need to be true as you've already done.
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.
right. I was trying to avoid that because there are a few test files and it was easier to add it here. And then remove it once the feature is fully released. Removing it from one place would also be easier. What do you think about that?
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.
Yea that's one way to do it, I just get a bit worried because we might forgot to remove it later. We've left flags in the app for >6 months after release, so that will effect all our testcases until we remove the flag and after that amount of time we might forgot our intent for this change in the base case. Just muddies up the codebase a bit more than necessary.
In general try to scope the change to just the bare minimum if possible. Plus I think it's just as easy to remove the mockOptimizelyClient
in the PledgePageViewModelTests
as opposed to TestCase
.
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.
@msadoon Its a good point. We are making an effort to audit and clean up our flags as we think about moving to firebase as a manager though. There are a lot of test files (24) that have multiple tests (4-7) that are failing which feels like a lot of code changes for this.
I can make a ticket to remove as well so I don't forget to remove from TestCase. It also feels like there are so many places to make the update your suggesting that it would be just as easy to forget to remove this from some of them
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.
If it's not too much work, let's use the class level variables. I now we're trying to figure out Optimizely replacements and then yes we'll go ahead and rip out Optimizely everywhere for the replacement but that work isn't quite finalized yet.
I think the original creators intent of TestCase
is not to add customizations of AppEnvironment
.
We'll see that withEnvironment(
in XCTestCase+AppEnvironment
is intended to be customized for every test case.
Likely when we add the replacement for Optimizely
, we'll follow this same approach and add it to the withEnvironment(
function and customize it in every test case that needs it.
It shouldn't be too much boilerplate right? Because we're updating the class level mockOptimizelyClient
not in every test. The only tests that need to change are the the ones that require a true
flag. I understand its probably four different files, but it still feels like the tradeoff is worth it and sticks to existing conventions. Also when we have this new replacement for Optimizely, then those files are easy to change with the new dependency because we just go through AppEnvironment.
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.
Ok, looking into this now, it's fine, just do it the way you have in TestCase
, I'll approve this. The class level update doesn't really seem to work either.
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 to go!
📲 What
We should only send segment analytic events if the member has given consent via the AppTrackingTransparency dialog
🤔 Why
If the member doesn't want KS to track their data, then we shouldn't.
🛠 How
KSRAnalytics.track()
method.✅ Acceptance criteria