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

[SK2] send unsynced attributes when syncing purchases #4245

Merged
merged 5 commits into from
Sep 9, 2024

Conversation

MarkVillacampa
Copy link
Member

@MarkVillacampa MarkVillacampa commented Sep 5, 2024

Checklist

  • If applicable, unit tests
  • If applicable, create follow-up issues for purchases-android and hybrids

Motivation

When calling syncPurchases with StoreKit 2, we aren't including any unsynced subscriber attributes.

Description

This PR modifies the StoreKit 2 syncPurchases network request to include unsynced subscriber attributes, the storefront, and a nil presentedOfferingContext.

@MarkVillacampa MarkVillacampa added bug pr:fix A bug fix and removed bug labels Sep 5, 2024
@@ -1142,6 +1142,9 @@ private extension PurchasesOrchestrator {
self.backend.post(receipt: .empty,
productData: nil,
transactionData: .init(appUserID: currentAppUserID,
presentedOfferingContext: nil,
unsyncedAttributes: unsyncedAttributes,
storefront: await Storefront.currentStorefront,
Copy link
Member Author

Choose a reason for hiding this comment

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

we dont really need the storefront here I think since there are no purchases

Copy link
Member

Choose a reason for hiding this comment

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

nitpick: maybe it's worth splitting this into its own line, like we do in the else code path, that explicitly does createProductRequestData

Copy link
Contributor

Choose a reason for hiding this comment

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

@aboedo Done! We're now creating transactionData on it's own line 😎

@@ -1142,6 +1142,9 @@ private extension PurchasesOrchestrator {
self.backend.post(receipt: .empty,
productData: nil,
transactionData: .init(appUserID: currentAppUserID,
presentedOfferingContext: nil,
unsyncedAttributes: unsyncedAttributes,
storefront: await Storefront.currentStorefront,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have any concerns about the additional await here adding extra time to this function? I feel like I recall awaiting on something SK2-related like this when doing network requests added latency in certain scenarios, but I can't recall exactly what it was off the top of my head right now.

Copy link
Member

Choose a reason for hiding this comment

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

we're already awaiting sk2 twice in this codepath already though, right? first verified transaction + app transaction jws?

@fire-at-will fire-at-will requested a review from a team September 5, 2024 14:48
Copy link
Member

@aboedo aboedo 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, but I wouldn't oppose skipping the storefront if it doesn't add value either

@@ -1142,6 +1142,9 @@ private extension PurchasesOrchestrator {
self.backend.post(receipt: .empty,
productData: nil,
transactionData: .init(appUserID: currentAppUserID,
presentedOfferingContext: nil,
unsyncedAttributes: unsyncedAttributes,
storefront: await Storefront.currentStorefront,
Copy link
Member

Choose a reason for hiding this comment

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

we're already awaiting sk2 twice in this codepath already though, right? first verified transaction + app transaction jws?

@@ -1142,6 +1142,9 @@ private extension PurchasesOrchestrator {
self.backend.post(receipt: .empty,
productData: nil,
transactionData: .init(appUserID: currentAppUserID,
presentedOfferingContext: nil,
unsyncedAttributes: unsyncedAttributes,
storefront: await Storefront.currentStorefront,
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: maybe it's worth splitting this into its own line, like we do in the else code path, that explicitly does createProductRequestData

@fire-at-will
Copy link
Contributor

@MarkVillacampa There are two other TransactionData properties that we're not including in syncPurchases() that we are including in the POST /receipt call in the normal SK2 purchase flow: aadAttributionToken and presentedPaywall (source).

Should we be including these as well? I can see a scenario where the user presses the syncPurchases button from a paywall and then presentedPaywall wouldn't have to be nil.

@MarkVillacampa MarkVillacampa merged commit 6115d8c into main Sep 9, 2024
5 checks passed
@MarkVillacampa MarkVillacampa deleted the fix-unsynced-atributes branch September 9, 2024 13:58
This was referenced Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:fix A bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants