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 Marketplace Postback handling #3357

Merged
merged 14 commits into from
Sep 13, 2024
Merged

Add Marketplace Postback handling #3357

merged 14 commits into from
Sep 13, 2024

Conversation

Bunn
Copy link
Contributor

@Bunn Bunn commented Sep 12, 2024

Task/Issue URL: https://app.asana.com/0/72649045549333/1208126219488944/f
Tech Design URL: https://app.asana.com/0/72649045549333/1208274586553401/f

Description:
Add Marketplace Postback handling

Steps to test this PR:

  1. Add a breakpoint here
  2. Either comment out the conditional check here or use this config (https://jsonblob.com/api/1283828391185604608)
  3. Run the app as a new user (never installed before), validate that we call postBack value 0 abd coarse value low
  4. Do the same as before with a returning user, validate that we call postBack value 1 abd coarse value high
  5. Validate that we have https://duckduckgo.com set as Advertising attribution report endpoint URL in the plist

1 - erase all content and settings
2 - Run app
3 - Validate that the postback is not sent
4 - Set the https://jsonblob.com/api/1283828391185604608 config or remove the feature flag check
5 - Run the app again
6 - Validate that the postback is sent as a new user

1 - Remove the app
2 - Run the app again
3 - Set the https://jsonblob.com/api/1283828391185604608 config or remove the feature flag check (If you had removed the flag on code before, skip steps 4, 5 and 6)
4 - Validate that the postback is not sent
5 - Set the https://jsonblob.com/api/1283828391185604608 config or remove the feature flag check
6 - Run the app again
7 - Validate that the postback is sent as a returning user

@Bunn Bunn requested a review from samsymons September 12, 2024 17:02
@available(iOS 16.1, *)
private func updateSKANPostback(_ postback: MarketplaceAdPostback, lockPostback: Bool) async {
do {
try await SKAdNetwork.updatePostbackConversionValue(postback.fineValue,
Copy link
Contributor

Choose a reason for hiding this comment

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

At risk of seeming like we're going backwards, what do you think about using the completion handler based API for SKAN? The reason I mention it is that we're seeing rare crashes with this call after moving to the async API. You can see an example here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we're seeing this error I believe it makes sense, then, if we still see a problem with these calls, i.e: nothing really changes, we can always revert it back to async. I'll do the change

samsymons

This comment was marked as outdated.

Copy link

github-actions bot commented Sep 13, 2024

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against d60cc4f

@samsymons samsymons self-requested a review September 13, 2024 18:55
Comment on lines +68 to +75
SKAdNetwork.updatePostbackConversionValue(postback.fineValue,
coarseValue: postback.SKAdCoarseValue) { error in
if let error = error {
Logger.general.error("Attribution: SKAN 4 postback failed \(String(describing: error), privacy: .public)")
} else {
Logger.general.debug("Attribution: SKAN 4 postback succeeded")
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this call doesn't work on the simulator, we could wrap it in a target OS check just to reduce any error logging to the console when debugging. (This was a problem with the previous implementation as well)

Copy link
Contributor

@samsymons samsymons left a comment

Choose a reason for hiding this comment

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

LGTM, tested the new/returning check at app launch, and that the postback call is made correctly after the remote config has been refreshed. Left one non-blocking comment about the attribution APIs not working in the simulator.

@Bunn Bunn merged commit 2834118 into main Sep 13, 2024
13 checks passed
@Bunn Bunn deleted the bunn/od/skan branch September 13, 2024 19:55
samsymons added a commit that referenced this pull request Sep 13, 2024
# By Alessandro Boron (7) and others
# Via Bartek Waresiak (1) and others
* main: (31 commits)
  Add Marketplace Postback handling (#3357)
  SKAD4 crash fix (#3361)
  Enroll all internal users in experiment && Update BSK (#3359)
  update for macOS: visited links (#3353)
  Ensure toast closures are called on the main thread (#3347)
  Update survey builder OS version (#3348)
  BSK - Add feature flag for SKAN API (#3356)
  Remove checking for negative attribution case  (#3355)
  C.S.S Bump (Via BSK) (#3346)
  Update Onboarding gradients (#3350)
  Alessandro/onboarding copy and private search options (#3349)
  Onboarding Intro - Add choose address bar position (#3340)
  Fix PrivacyDashboard appearance on entering foreground (#3345)
  Improve Data Store ID managing (#3335)
  Alessandro/onboarding choose app icon (#3330)
  Fix Localizable strings (#3341)
  Move lazy var access to the MainActor (#3333)
  Release 7.137.0-2 (#3344)
  Fix privacy icon glitch (#3343)
  BSK bump for macOS password import promotion flow (#3332)
  ...

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
samsymons added a commit that referenced this pull request Sep 16, 2024
* main:
  Remove VPN feature flag checks (#3334)
  Add Marketplace Postback handling (#3357)
  SKAD4 crash fix (#3361)
  Enroll all internal users in experiment && Update BSK (#3359)
  update for macOS: visited links (#3353)
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