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.

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

Add install pixel #3656

Merged
merged 6 commits into from
Dec 4, 2024
Merged

Add install pixel #3656

merged 6 commits into from
Dec 4, 2024

Conversation

samsymons
Copy link
Contributor

@samsymons samsymons commented Dec 1, 2024

Task/Issue URL: https://app.asana.com/0/1199333091098016/1208873240363373/f
Tech Design URL:
CC:

Description:

This PR adds an m.install.ios.* pixel that is sent when calling the exti URL.

Steps to test this PR:

  1. Reset your simulator and run the app, look for m.install in the console, with the parameters reinstall and locale, which match the current state of your device - you should expect to see reinstall=false if you reset your simulator, then you can delete the app and reinstall it to get reinstall=true
  2. Now kill and relaunch the app and check that you do not see m.install again

Definition of Done (Internal Only):

Device Testing:

  • iPhone SE (1st Gen)
  • iPhone 8
  • iPhone X
  • iPhone 14 Pro
  • iPad

OS Testing:

  • iOS 15
  • iOS 16
  • iOS 17

Theme Testing:

  • Light theme
  • Dark theme

Internal references:

Software Engineering Expectations
Technical Design Template

@samsymons samsymons requested a review from Bunn December 1, 2024 22:42
Comment on lines +284 to +285
Thread.sleep(forTimeInterval: .seconds(0.1))
testExpectation.fulfill()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This small delay isn't ideal; it's in place since the refreshAppRetentionAtb callback is waiting for the exti call, not the pixel call, so there is a chance that this could complete before the pixel call has had time to do its thing.

It would be better to wait for the pixel firing mock to have the right value, with a ~1 second timeout. I'll look at updating this.

@samsymons samsymons changed the title Add install pixel. Add install pixel Dec 3, 2024
@samsymons samsymons changed the base branch from main to release/7.148.0 December 3, 2024 00:48
Copy link
Contributor

@Bunn Bunn left a comment

Choose a reason for hiding this comment

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

@samsymons I'm afraid there's an issue here, or I'm missing something when testing this.
No matter what I do, reset simulator, use a brand new simulator, I always see the pixel
Pixel fired m.install ["locale": "en-US", "reinstall": "true"] being fired.

After some investigation, I found out that the installCompletedWithATB is called before the fireInstallPixel which causes the ReturnUserMeasurement to return true

* release/7.148.0:
  Release 7.148.0-1 (#3661)
  Make a simple state machine to identify incorrect transitions  (#3660)
  support local storage fireproofing (#3612)
  Make NTP Grid layout adaptive for various screen sizes (#3657)
  Release 7.148.0-0 (#3659)
  Bump BSK, add Free Trials Feature Flag (#3655)
Copy link
Contributor

@Bunn Bunn left a comment

Choose a reason for hiding this comment

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

Working as expected now, I was able to trigger both pixels, only once per install

Pixel fired m.install ["locale": "en-US", "reinstall": "true"]
Pixel fired m.install ["locale": "en-US", "reinstall": "false"]

@samsymons samsymons merged commit a58b9c9 into release/7.148.0 Dec 4, 2024
15 checks passed
@samsymons samsymons deleted the sam/install-pixel branch December 4, 2024 19:48
samsymons added a commit that referenced this pull request Dec 5, 2024
* main:
  Release 7.148.0-2 (#3676)
  Merge hotfix changes from 7.147.1 (#3674)
  Add `install` pixel (#3656)
  AI Chat browsing menu (#3635)
  Release 7.147.1-0 (#3672)
  Use actor isolation for `SubscriptionFeatureMappingCache` (#3670)
  cherry picked - already approved in #3666
  fix suggestion scrolling (#3654)
  Update autoconsent to v12.1.0 (#3650)
  Malware protection: bump BSK (#3617)
  Release 7.148.0-1 (#3661)
  Make a simple state machine to identify incorrect transitions  (#3660)
  support local storage fireproofing (#3612)
  Make NTP Grid layout adaptive for various screen sizes (#3657)
  Release 7.148.0-0 (#3659)
  Bump BSK, add Free Trials Feature Flag (#3655)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants