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

Shopper insights rp2 feature #1256

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

Conversation

warmkesselj
Copy link
Contributor

Summary of changes

Merge ShopperInsights Feature RP2 to main. Updated from main -> rp1 -> rp2.

Checklist

  • Added a changelog entry
  • Relevant test coverage
  • Tested and confirmed payment flows affected by this change are functioning as expected

Authors

@jwarmkessel
@warmkesselj

sarahkoop and others added 12 commits December 3, 2024 14:25
…hts (#1228)

* Add methods to ShopperInsightsClient

* update unit tests

* Add CHANGELOG

* Fix typo
* Add session ID param to client

* Add unit tests

* Add CHANGELOG

* fix lint

* Address PR feedback
* Add shopper insights session ID to paypalRequest

* Update factory for paypal request

* remove extra setting of email address

* Remove optional null and check for null in PayPalRequestFactory

* Use String type and fix check for null to prevent run time crashes

* Remove DEMO UI updates

* Update comment for shopperSessionId

* Add experimentalAPI for shopper session Id

* Remove unused shopperInsightsSessionIdNullSwitch

* Check for null and empty string

* Update to use putOpt method for shopperId

* Update changelog

* Check for null and empty strings for request properties.

* Update unit tests

* Update test

* Update unit tests

* Update changelog
* setSessionId from paypalClient

* Move analytics instance out of constructor

* Update PayPalClient tests to take in a mock analyticsParamRepo

* Add unit tests
* Add new shopper session ID analytics param

* Fix unit tests

* Fix missing analytics

* fix lint

* Fix lint and add unit tests

* update doc string

* fix lint
* Add new classes and enums

* Update new objects, introduce new method

* Use a data class instead

* Use the built in functionality to convert the treatment to a string

* Update content, update analyticsEventParam to take in a buttonType

* Use refactored sendPresentedEvent fun

* Remove comments

* Update sendPresentedEvent signature

* Remove unused methods

* Update comments button order

* Update comments for button type

* Update comments

* Update signature

* Resolve conflicts

* Update PresentmentDetails signature

* Update analyticsEvent and add button selected string name

* Add comment for shopperSessionId

* Update AnalyticsClient

* Update unit test

* Update presentment signature

* Add changelog

* Remove shopperSessionId from signature

* Remove comment

* Remove extra line

* Fix lint issues

* Newline

* Update unit test to test venmo and control type button

* Lint issues

* Update ButtonOrder

* Update keys

* PR Comments

* Analytics update

* Add accessors to enum func to get internal string value

* Use upper case notation for enums with a string representation for the page type

* Updates

* Fix unit test
* fun sendSelectedEvent

* Remove PresentmentDetails from selected analytics

* Update comment
* remove paymentMethodsDisplayed
…-feature

# Conflicts:
#	CHANGELOG.md
#	PayPal/src/test/java/com/braintreepayments/api/paypal/PayPalClientUnitTest.java
@warmkesselj warmkesselj requested a review from a team as a code owner January 15, 2025 00:18
CHANGELOG.md Outdated
Comment on lines 4 to 6

* PayPal
* Fix bug to ensure that `PayPalVaultRequest.userAuthenticationEmail` is not sent as an empty string
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 not delete this

CHANGELOG.md Outdated
Comment on lines 5 to 8
* ShopperInsights (BETA)
* Add `isPayPalAppInstalled` and `isVenmoAppInstalled` methods
* Add `shopperSessionId` parameter to `ShopperInsightsClient`
* BraintreePayPal
* Add `shopperSessionId` to `PayPalCheckoutRequest` and `PayPalVaultRequest`
* Fix bug to ensure that `PayPalVaultRequest.userAuthenticationEmail` is not sent as an empty string
* Replace `sendPayPalPresentedEvent()` and `sendVenmoPresentedEvent()` with `sendPresentedEvent()`
* Replace `sendPayPalSelectedEvent()` and `sendVenmoSelectedEvent()` with `sendSelectedEvent()`
* ThreeDSecure
* Return error if no `dfReferenceId` is returned in the 3D Secure flow
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit pick - I think the indentation is correct on the original 3DS entry and the new entries are indented too far

Comment on lines 7 to 8
* @property buttonOrder optional Represents this buttons order in context of other buttons.
* @property pageType optional Represents the page or view the button is rendered on.
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
* @property buttonOrder optional Represents this buttons order in context of other buttons.
* @property pageType optional Represents the page or view the button is rendered on.
* @property buttonOrder Represents this buttons order in context of other buttons.
* @property pageType Represents the page or view the button is rendered on.

These are not optional based on the code - should they be?

@sarahkoop
Copy link
Contributor

Can we do PR step "Tested and confirmed payment flows affected by this change are functioning as expected" to make sure everything is working as expected before merging?

paymentMethodsDisplayed = paymentMethodsDisplayed
)
)
fun isPayPalAppInstalled(context: Context): Boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

Quick ? - should these methods be marked beta too?

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.

4 participants