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

DO NOT MERGE Add metadata to purchase #4293

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Conversation

vlopezferrando
Copy link

@vlopezferrando vlopezferrando commented Sep 23, 2024

Motivation

The backend now supports (with the right feature flag) the addition of metadata when doing a purchase. This metadata is included in all generated events and CustomerInfo.

Description

In this PR, we:

  • Add an optional parameter metadata with default value nil to all purchase methods.
  • Parse the new metadata field in the CustomerInfo.

Tests

  • Added calls to purchases methods in PurchasesAPI.swift.
  • Added an integration test testPurchaseMetadataIsInResponse() that does a purchase with metadata, then gets the customerInfo and checks it includes the metadata.

@vlopezferrando vlopezferrando added the pr:feat A new feature label Sep 23, 2024
Copy link
Contributor

@fire-at-will fire-at-will left a comment

Choose a reason for hiding this comment

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

Looks like it's off to a good start! Let me know if you'd like to pair tomorrow on CI/writing tests/anything else and I'll be glad to hop on a call to help out 😄

purchasesOrchestrator.purchase(product: product, package: nil, transactionMetadata: transactionMetadata, completion: completion)
}

public func purchase(package: Package, transactionMetadata: [String : String]?, completion: @escaping PurchaseCompletedBlock) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of making brand new purchase functions to make a purchase with metadata, I'd consider modifying the existing purchase functions and making the transactionMetadata parameter default to nil so that it isn't a breaking change.

There are quite a few purchase functions today across both Purchases.swift and Purchases+async.swift. Ideally folks would be able to upload transaction metadata no matter which function call they're already using without having to migrate to a new one.

@vlopezferrando vlopezferrando marked this pull request as draft October 1, 2024 09:38
@vlopezferrando vlopezferrando changed the title Attatch metadata to purchase Add metadata to purchase Oct 1, 2024
@vlopezferrando vlopezferrando marked this pull request as ready for review October 3, 2024 20:40
@vlopezferrando
Copy link
Author

@MarkVillacampa @fire-at-will Could you take a look at this PR? It is ready for review.

@@ -112,6 +113,7 @@ final class TransactionPoster: TransactionPosterType {
receipt: encodedReceipt,
product: product,
appTransaction: appTransaction,
metadata: data.metadata,
Copy link
Contributor

@greenietea greenietea Oct 10, 2024

Choose a reason for hiding this comment

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

Feel free to ignore since I know nothing about SDK. Why have a separate metadata parameter when data is already passed along in purchasedTransactionData?

Copy link
Author

Choose a reason for hiding this comment

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

I think you are right... in fact, I believe we are not even using that parameter and should remove it. I'll check if that's true.

Copy link
Author

Choose a reason for hiding this comment

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

Confirmed and removed unnecessary parameters. Great catch!!

@vlopezferrando vlopezferrando changed the title Add metadata to purchase DO NOT MERGE Add metadata to purchase Oct 10, 2024
Copy link
Member

@JayShortway JayShortway left a comment

Choose a reason for hiding this comment

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

Nice job! Going in the right direction. Just a single comment on binary incompatibility.

Comment on lines -959 to +961
func purchase(product: StoreProduct) async throws -> PurchaseResultData {
return try await purchaseAsync(product: product)
func purchase(product: StoreProduct, metadata: [String: String]? = nil) async throws -> PurchaseResultData {
return try await purchaseAsync(product: product, metadata: metadata)
Copy link
Member

Choose a reason for hiding this comment

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

While adding a parameter with a default value is source compatible, it is not binary compatible. See Library Evolution in Swift:

Examples of non-resilient changes

[...]

  • Adding a parameter to a function’s parameter list (even if a default value is provided)

This can cause crashes when other libraries depend on purchases-ios and encounter an incompatible version at runtime.

Following SemVer strictly, this would require a major release.

Copy link
Member

Choose a reason for hiding this comment

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

Reverting to your previous approach of adding new functions would be both source and binary compatible, but I definitely agree with @fire-at-will that that would result in too many purchase functions.

Copy link
Member

Choose a reason for hiding this comment

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

Ah since default arguments are not even source compatible from an ObjC perspective (see Andy's comment), adding new functions seems to be the way to go here.

purchasesOrchestrator.purchase(product: product, package: nil, completion: completion)
@objc(purchaseProduct:metadata:withCompletion:)
func purchase(product: StoreProduct,
metadata: [String: String]? = nil,
Copy link
Member

Choose a reason for hiding this comment

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

Just curious whether a deliberate decision has been made between defaulting to nil and empty?

Copy link
Member

Choose a reason for hiding this comment

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

FWIW I think we might need to make it a net new method rather than adding an optional param to the existing one to retain Objective-C compatibility:

Objective-C doesn’t have a concept of optional parameters, but our ObjC API is generated from the Swift API (through the @objc modifier there) so as this PR stands, it's a breaking change in ObjC.

For an example of us doing these multiple methods as a workaround, you can look here: https://github.com/RevenueCat/purchases-ios/blob/main/Sources/Misc/Obsoletions.swift#L618
https://github.com/RevenueCat/purchases-ios/blob/main/Sources/Misc/Obsoletions.swift#L632

Copy link
Member

Choose a reason for hiding this comment

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

This is good to know, thanks! Adding new functions also keeps it binary compatible from a Swift perspective.

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.

this is gonna be great! Left one thing that we'll have to change in the API before we ship

purchasesOrchestrator.purchase(product: product, package: nil, completion: completion)
@objc(purchaseProduct:metadata:withCompletion:)
func purchase(product: StoreProduct,
metadata: [String: String]? = nil,
Copy link
Member

Choose a reason for hiding this comment

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

FWIW I think we might need to make it a net new method rather than adding an optional param to the existing one to retain Objective-C compatibility:

Objective-C doesn’t have a concept of optional parameters, but our ObjC API is generated from the Swift API (through the @objc modifier there) so as this PR stands, it's a breaking change in ObjC.

For an example of us doing these multiple methods as a workaround, you can look here: https://github.com/RevenueCat/purchases-ios/blob/main/Sources/Misc/Obsoletions.swift#L618
https://github.com/RevenueCat/purchases-ios/blob/main/Sources/Misc/Obsoletions.swift#L632

@vlopezferrando
Copy link
Author

Thanks, @JayShortway and @aboedo. We'll refactor the PR to use new methods instead of replacing the old ones. @MarkVillacampa will pick it up from here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants