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.

Sign up for GitHub

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

Jump to bottom

[Important] Adding imageUrl, upcType, upcCode to PayPalLineItem #1165

Merged
merged 24 commits into from
Jan 18, 2024
Merged

[Important] Adding imageUrl, upcType, upcCode to PayPalLineItem #1165

merged 24 commits into from
Jan 18, 2024

Conversation

vankads
Copy link
Contributor

@vankads vankads commented Dec 26, 2023 •
edited
Loading

Thank you for your contribution to Braintree.

Before submitting this PR, note we cannot accept language translation PRs. We support the same languages that are supported by PayPal, and have a dedicated localization team to provide the translations. If there is an error in a specific translation, you may open an issue and we will escalate it to the localization team.

Summary of changes

  • Added the ability for merchants to pass imageUrl, upcCode, upcType in PayPalLineItem.

Checklist

  • Added a changelog entry

Authors

List GitHub usernames for everyone who contributed to this pull request.
@vankads

@vankads vankads changed the title WIP: Adding imageUrl, upc, upcCode to PayPalLineItem Adding imageUrl, upc, upcCode to PayPalLineItem Jan 4, 2024
@vankads vankads marked this pull request as ready for review January 4, 2024 00:12
@vankads vankads requested a review from a team as a code owner January 4, 2024 00:12
vankads
vankads commented Jan 4, 2024
View reviewed changes
Sources/BraintreePayPal/BTPayPalLineItem.swift Outdated
@@ -27,16 +52,25 @@ import Foundation
public let kind: BTPayPalLineItemKind

/// Optional: Per-unit tax price of the item. Can include up to 2 decimal places. This value can't be negative or zero.
public let unitTaxAmount: String? = nil
public var unitTaxAmount: String? = nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is required to set values to these properties once the object is created.

@vankads vankads changed the title Adding imageUrl, upc, upcCode to PayPalLineItem Adding imageUrl, upcType, upcCode to PayPalLineItem Jan 4, 2024
ragavi98
ragavi98 approved these changes Jan 4, 2024
View reviewed changes
Copy link

@ragavi98 ragavi98 left a comment

Choose a reason for hiding this comment

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

🍍

@vankads vankads changed the title Adding imageUrl, upcType, upcCode to PayPalLineItem [Important] Adding imageUrl, upcType, upcCode to PayPalLineItem Jan 5, 2024
jaxdesmarais
jaxdesmarais requested changes Jan 8, 2024
View reviewed changes
Demo/Application/Features/PayPalWebCheckoutViewController.swift Outdated Show resolved Hide resolved
Sources/BraintreePayPal/BTPayPalLineItem.swift Outdated Show resolved Hide resolved
Sources/BraintreePayPal/BTPayPalLineItem.swift Outdated Show resolved Hide resolved
Sources/BraintreePayPal/BTPayPalLineItem.swift Outdated Show resolved Hide resolved
Sources/BraintreePayPal/BTPayPalLineItem.swift Outdated Show resolved Hide resolved
Sources/BraintreePayPal/BTPayPalLineItem.swift Outdated Show resolved Hide resolved
@vankads vankads requested a review from jaxdesmarais January 10, 2024 00:18
jaxdesmarais
jaxdesmarais reviewed Jan 10, 2024
View reviewed changes
Sources/BraintreePayPal/BTPayPalLineItem.swift Outdated Show resolved Hide resolved
Sources/BraintreePayPal/BTPayPalLineItem.swift Outdated
@@ -9,6 +9,54 @@ import Foundation
case credit
}


/// Use this option to specify the Upc type of the lien item.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Use this option to specify the Upc type of the lien item.
/// Use this option to specify the UPC type of the line item.

Additionally are there more descriptive UPC code definitions somewhere that we can link to? I'm not sure what all these cases mean and assume merchants may be confused as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://en.wikipedia.org/wiki/Universal_Product_Code is the best we found about these definitions. Not sure if we can link wiki page.
PayPal developer documentation doesn't have much details https://developer.paypal.com/api/limited-release/orders/v2/#definition-universal_product_code

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, these docs are fine then since they seem to match paypal's public docs. We can hopefully assume merchants that use these know what they mean. 🙈

CHANGELOG.md Outdated Show resolved Hide resolved
Demo/Application/Features/PayPalWebCheckoutViewController.swift Outdated Show resolved Hide resolved
Sources/BraintreePayPal/BTPayPalLineItem.swift Outdated Show resolved Hide resolved
@vankads vankads requested a review from jaxdesmarais January 11, 2024 22:04
@vankads vankads requested a review from jaxdesmarais January 12, 2024 17:52
jaxdesmarais
jaxdesmarais approved these changes Jan 12, 2024
View reviewed changes
Copy link
Contributor

@jaxdesmarais jaxdesmarais left a comment

Choose a reason for hiding this comment

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

Couple of very tiny cleanup comments but should be straightforward to apply.

CHANGELOG.md Outdated Show resolved Hide resolved
Sources/BraintreePayPal/BTPayPalLineItem.swift Show resolved Hide resolved
vankads and others added 2 commits January 12, 2024 11:04
@vankads @jaxdesmarais
Update CHANGELOG.md
436aea1 
Co-authored-by: Jax DesMarais-Leder <[email protected]>
@vankads @jaxdesmarais
Update Sources/BraintreePayPal/BTPayPalLineItem.swift
9366878 
Co-authored-by: Jax DesMarais-Leder <[email protected]>
warmkesselj
warmkesselj approved these changes Jan 17, 2024
View reviewed changes
Copy link
Contributor

@warmkesselj warmkesselj 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

@jaxdesmarais
Copy link
Contributor

Not sure why the integration tests are flaky now but UI tests have been flaking out for a while and are unrelated to this PR. I pulled down this branch and ran the integration tests and they are all passing outside of CI locally. Going to move forward with merging in this work as the Integration tests failing here are all passing. We will add this to our CI test investigation work.

@jaxdesmarais jaxdesmarais merged commit 307cbdf into braintree:main Jan 18, 2024
5 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@warmkesselj warmkesselj warmkesselj approved these changes

@ragavi98 ragavi98 ragavi98 approved these changes

@jaxdesmarais jaxdesmarais jaxdesmarais approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vankads @jaxdesmarais @ragavi98 @warmkesselj