Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: update isLimitAdTracking logic for Android 13 #233

Merged
merged 5 commits into from
Oct 14, 2022
Merged

feat: update isLimitAdTracking logic for Android 13 #233

merged 5 commits into from
Oct 14, 2022

Conversation

mchuangatmp
Copy link
Contributor

@mchuangatmp mchuangatmp commented Sep 1, 2022 •
edited
Loading

Instructions

  1. PR target branch should be against development
  2. PR title name should follow this format: https:/mParticle/mparticle-workflows/blob/stable/.github/workflows/pr-title-check.yml
  3. PR branch prefix should follow this format: https:/mParticle/mparticle-workflows/blob/stable/.github/workflows/pr-branch-check-name.yml

Summary

  • Removed deprecated AAID logic
  • Return null for GAID when LAT in enabled
  • Convert MParticleTest.java to kotlin

Testing Plan

  • Emulator returns 0s when isLimitAdTrackingEnabled = true
  • Emulator returns gaid when isLimitAdTrackingEnabled = false
  • Regression tests should pass (except crossplatforms since AGP hasn't been bumped up there)

Reference Issue

samdozor
samdozor requested changes Sep 1, 2022
View reviewed changes
android-core/src/main/java/com/mparticle/internal/ConfigManager.java
@@ -1213,7 +1198,7 @@ public void setIdentityApiContext(String context) {
public String getPreviousAdId() {
MPUtility.AdIdInfo adInfo = MPUtility.getAdIdInfo(mContext);
String currentAdId = null;
if (adInfo != null) {
if (adInfo != null && !adInfo.isLimitAdTrackingEnabled) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so reading this code - this is what could happen:

  1. user has limit ad tracking (LAT) false, and we persist the Ad ID (via setPreviousAdId below)
  2. they then disable limit ad tracking
  3. this method is called - and we skip over this block with your change - so it will return the previous Ad ID, which we should no longer be using

So that makes me nervous. Do we need these methods at all? Why are we storing the ad ID? If these methods are used to understand if the Ad ID changed, then I would think you'd want this method to return all zeroes or null when LAT is true, so that whatever caller knows that the Ad ID changed from something to null.

Are we triggering a modify call to identity based on these methods? If we are, i'd expect that modify call to remove the Ad ID from the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samdozor in AppStateManager.java updated the logic to allow it to send at least one modify call if currentGoogleAdId is null

android-core/src/main/java/com/mparticle/internal/ConfigManager.java Show resolved Hide resolved
android-core/src/main/java/com/mparticle/internal/DeviceAttributes.java Show resolved Hide resolved
@mchuangatmp mchuangatmp force-pushed the feat/SQDSDKS-4130-gaid branch 4 times, most recently from bc0cda0 to e6d8616 Compare October 13, 2022 21:57
samdozor
samdozor approved these changes Oct 14, 2022
View reviewed changes
Copy link
Member

@samdozor samdozor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good 👍

@sonarcloud
Copy link

sonarcloud bot commented Oct 14, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell C 5 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@mchuangatmp mchuangatmp merged commit f7b9284 into development Oct 14, 2022
@mchuangatmp mchuangatmp deleted the feat/SQDSDKS-4130-gaid branch October 14, 2022 16:58
mparticle-automation added a commit that referenced this pull request Oct 14, 2022
@mparticle-automation
chore: 5.46.0 (release)
716146c 
## [5.46.0](v5.45.2...v5.46.0) (2022-10-14)

### Features

* Bump up compile SDK to 33 and ktlint version ([#245](#245)) ([67dc378](67dc378))
* Port android core module java instrumented and unit tests to kotlin - phase1 ([#249](#249)) ([62e4254](62e4254))
* Port android core module java instrumented and unit tests to kotlin (p2) ([#250](#250)) ([23434d2](23434d2))
* Ported Android Kit Base tests to Kotlin ([#253](#253)) ([c21dd71](c21dd71))
* update isLimitAdTracking logic for Android 13 ([#233](#233)) ([f7b9284](f7b9284))

### Bug Fixes

* Convert MParticleTest.java file to kotlin and fix failing test ([#251](#251)) ([604c1b9](604c1b9))

### Documentation

* Update copy in readme for android 13 ([#255](#255)) ([c9a648d](c9a648d))

### Updates & Maintenance

* add in missing inherit in rebase job ([2197610](2197610))
* Update submodules ([7bdaf63](7bdaf63))
* Update to kotlin 1.7.20 ([#247](#247)) ([c9e1729](c9e1729))
@mparticle-automation
Copy link
Collaborator

🎉 This PR is included in version 5.46.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samdozor samdozor samdozor approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mchuangatmp @mparticle-automation @samdozor