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

[V7] Update BTLocalPaymentRequest #1447

Merged
merged 18 commits into from
Nov 4, 2024
Merged

[V7] Update BTLocalPaymentRequest #1447

merged 18 commits into from
Nov 4, 2024

Conversation

richherrera
Copy link
Contributor

@richherrera richherrera commented Oct 23, 2024

Summary of changes

  • Update BTCard properties to have everything in the init vs using dot syntax

Docstrings:

Checklist

  • Added a changelog entry

Authors

@richherrera richherrera self-assigned this Oct 23, 2024
@richherrera richherrera requested a review from a team as a code owner October 23, 2024 21:36
@richherrera richherrera changed the title Update local payment [V7] Update BTLocalPaymentRequest Oct 23, 2024
/// - isShippingAddressRequired: Indicates whether or not the payment needs to be shipped. For digital goods, this should be `false`. Defaults to `false`.
/// - bic: Optional: Bank Identification Code of the customer (specific to iDEAL transactions).
public init(
paymentType: String? = nil,
Copy link
Contributor

Choose a reason for hiding this comment

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

is everything really optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for asking, i've updated it. paymentType, amount and currencyCode are required for all LPMs, others depend on the type.
As docs says: paymentTypeCountryCode is required for payment methods with multiple potential country codes, otherwise it will use the countryCode (reference)

@richherrera richherrera marked this pull request as draft October 24, 2024 15:54
let surname: String?
let phone: String?
var isShippingAddressRequired: Bool = false
let bic: String?

public weak var localPaymentFlowDelegate: BTLocalPaymentRequestDelegate?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the case of delegates, do we prefer using dot syntax or requiring them in the init? Apple uses dot syntax

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's follow apple's lead and use the dot syntax!

Copy link
Contributor

@agedd agedd left a comment

Choose a reason for hiding this comment

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

lgtm!

@richherrera richherrera marked this pull request as ready for review October 29, 2024 22:23
/// - phone: Optional: Phone number of the customer.
/// - isShippingAddressRequired: Indicates whether or not the payment needs to be shipped. For digital goods, this should be `false`. Defaults to `false`.
/// - bic: Optional: Bank Identification Code of the customer (specific to iDEAL transactions).
public init(
Copy link
Contributor

Choose a reason for hiding this comment

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

Take it or leave it - do we want to alphabetize these?

let surname: String?
let phone: String?
var isShippingAddressRequired: Bool = false
let bic: String?

public weak var localPaymentFlowDelegate: BTLocalPaymentRequestDelegate?
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's follow apple's lead and use the dot syntax!


/// Creates a LocalPaymentRequest
/// - Parameters:
/// - paymentType: The type of payment.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these listed anywhere? If so can we link to the doc for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated: 99b8064

@richherrera richherrera merged commit af7098f into v7 Nov 4, 2024
8 checks passed
@richherrera richherrera deleted the update-localPayment branch November 4, 2024 16:20
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.

5 participants