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

Migrate attribution pixel to pixel kit #2564

Conversation

alessandroboron
Copy link
Contributor

@alessandroboron alessandroboron commented Apr 8, 2024

Task/Issue URL: https://app.asana.com/0/0/1207002879349167/f

Description:
Migrates the Installation Attribution Pixel to use PixelKit.

Screenshot 2024-04-08 at 4 07 47 PM

Steps to test this PR:

Download the Review App here
Make sure to clean up the app's data directory by running sh clean-app.sh review.
Run the App.
Search for pixel:m.mac.install in Kibana.

Extra:
Create the Origin.txt file manually and add it to the Bundle of the App (.app/Contents/Resources) to test that the pixel fires and has the origin field set.
This requires to re-sign the App again in order to run it.
Steps to re-sign the App here

In terminal:

codesign -d --entitlements :- DuckDuckGo.app > entitlements.plist
Copy result of security find-certificate -a -c "Developer ID Application" -Z | grep ^SHA-1 | cut -d " " -f3 | uniq
Code sign the App using the below:
--force
--sign <paste value generated from #2 here>
--options runtime
--entitlements entitlements.plist
--generate-entitlement-der “DuckDuckGo.app”
Once the App is re-signed you need to open it by right-click on the icon and select Open from the context menu otherwise you won’t be able to open it.


Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

Copy link
Contributor

github-actions bot commented Apr 8, 2024

🚫 The Asana task linked in the PR description is not added to macOS App Board project.

  1. Verify that the correct task is linked in the PR.
    • ⚠️ Please use the actual implementation task, rather than the Code Review subtask.
  2. Verify that the task is added to macOS App Board project.
  3. When ready, remove the bot: not in app board label to retrigger the check.

@github-actions github-actions bot added the bot: not in app board Added by automation for pull requests with tasks not added to macOS App Board Asana project label Apr 8, 2024
@alessandroboron alessandroboron changed the title Alessandro/migrate attribution pixel to pixel kit Migrate attribution pixel to pixel kit Apr 8, 2024
.installationAttribution,
.initial,
GeneralPixel.installationAttribution,
.standard,
Copy link
Contributor Author

@alessandroboron alessandroboron Apr 8, 2024

Choose a reason for hiding this comment

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

I had to change the frequency to standard. I couldn’t find a way to make the pixel fire again when testing.

This function

public func pixelLastFireDate(pixelName: String) -> Date? {
        var date = defaults.object(forKey: userDefaultsKeyName(forPixelName: pixelName)) as? Date
        if date == nil {
            date = defaults.object(forKey: legacyUserDefaultsKeyName(forPixelName: pixelName)) as? Date
        }
        return date
    }

returned a date even though I'd clean the App state with the clean-app.sh script.

Technically this is still safe because in StatisticsLoader we check the condition of statisticsStore.hasInstallStatistics. If this is false we fire the pixel otherwise we return. So it would fire only once per installation.

func load(completion: @escaping Completion = {}) {
        if statisticsStore.hasInstallStatistics {
            completion()
            return
        }

        requestInstallStatistics(completion: completion)
    }

where hasInstallStatistics is

var hasInstallStatistics: Bool {
       return atb != nil
}

var atb: String? {
       get {
           pixelDataStore.value(forKey: Keys.atb)
       }
       set {
           if let value = newValue {
               pixelDataStore.set(value, forKey: Keys.atb)
           } else {
               assertionFailure("Unexpected ATB removal")
               pixelDataStore.removeValue(forKey: Keys.atb)
           }
       }
   }

Copy link
Member

Choose a reason for hiding this comment

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

Understood thanks, PixelKit for now it lacks a function for cleaning up the UserDefaults used for that dates, I'm going to implement it before opening the PR

Copy link
Member

@federicocappelli federicocappelli left a comment

Choose a reason for hiding this comment

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

LGTM, I'll add a task for re-testing this after I have implemented a "reset" function for Pixelkit's data storage

@federicocappelli federicocappelli merged commit 70849bc into fcappelli/PixelKit Apr 8, 2024
21 of 28 checks passed
@federicocappelli federicocappelli deleted the alessandro/migrate-attribution-pixel-to-pixel-kit branch April 8, 2024 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: not in app board Added by automation for pull requests with tasks not added to macOS App Board Asana project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants