Fix unable to start web checkout in subsequent transactions #251

warmkesselj · 2024-04-15T18:57:39Z

Summary of changes

This PR fixes an issue with allowing follow up transactions to occur.
- Provide a removeObservers method to clear out the observers on completion of a transaction.

Checklist

Added a changelog entry

Authors

@warmkesselj
@sshropshire
@saperi22

[email protected] [email protected]

Demo/src/main/java/com/paypal/android/uishared/components/ActionButtonColumn.kt

Demo/src/main/java/com/paypal/android/uishared/components/ErrorView.kt

jaxdesmarais

It looks like there are some Detekt errors that we should resolve. Can we also add some tests around the removeObservers methods?

CardPayments/src/main/java/com/paypal/android/cardpayments/CardClient.kt

PayPalWebPayments/src/main/java/com/paypal/android/paypalwebpayments/PayPalWebCheckoutClient.kt

…thub.com/paypal/paypal-android into fix_unableToStartWebCheckoutSubsequently

CardPayments/src/main/java/com/paypal/android/cardpayments/CardClient.kt

jaxdesmarais · 2024-04-24T15:47:42Z

Demo/src/main/java/com/paypal/android/uishared/components/ActionButtonColumn.kt

@@ -117,7 +118,9 @@ fun StatefulActionButtonPreview() {
 modifier = Modifier
 .fillMaxWidth()
 .padding(UIConstants.paddingMedium)
- )
+ ) { state ->
+ Text(text = "Sample Text", modifier = Modifier.padding(64.dp))


Curious what this is needed for?

@jaxdesmarais looks like this is part of compose preview (annotated by @Preview) https://github.com/paypal/paypal-android/pull/251/files/cb5d5b7a7818424851e4a2f68406e5e9e233538b#diff-dbdf8e5b958c81bbcff72ecd0e61151a09aae144ce2f9726e2b1616511de536bR107

It is a tool that allows users to view the UI in the IDE while developing.

This was just for previewing purposes to see the behavior when inflating the action column.

Ah gotcha, didn't see it was in a preview and was confused by the "Sample Text". 😄

PayPalWebPayments/src/main/java/com/paypal/android/paypalwebpayments/PayPalWebCheckoutClient.kt

CHANGELOG.md

Co-authored-by: Jax DesMarais-Leder <[email protected]>

sarahkoop · 2024-04-24T18:07:27Z

PayPalWebPayments/src/main/java/com/paypal/android/paypalwebpayments/PayPalWebCheckoutClient.kt

@@ -13,7 +14,7 @@ import com.paypal.android.corepayments.analytics.AnalyticsService
 */
 class PayPalWebCheckoutClient internal constructor(
 // NEXT MAJOR VERSION: remove hardcoded activity reference
- private val activity: FragmentActivity,
+ activity: FragmentActivity,


Does this need to be public now?

It is no longer a class variable - it's a linter optimization.
The scope of activity is now limited to the constructor and a class level variable, so the activity object is no longer needed after the constructor is done constructing. It is essentially a local object and needs no private modifier.

Appears it can remain a private property. Pushing changes now.

…thub.com/paypal/paypal-android into fix_unableToStartWebCheckoutSubsequently

sarahkoop · 2024-04-24T18:25:59Z

PayPalWebPayments/src/main/java/com/paypal/android/paypalwebpayments/PayPalWebCheckoutClient.kt

+ * Call this method at the end of the web checkout flow to clear out all observers and listeners
+ */
+ fun removeObservers() {
+ activityReference.get()?.let { it.lifecycle.removeObserver(observer) }


is lifecycle always non null here?

It is non-null all the time.

@NonNull @Override public Lifecycle getLifecycle() { return mLifecycleRegistry; }

My guess would be that it's always not null, but I don't know that much about Android. Perhaps I can add some guards around it

sarahkoop · 2024-04-24T18:26:35Z

Looks like the latest google play compliance changes on main aren't updated here - can we update this branch to the latest main?

…fix_unableToStartWebCheckoutSubsequently

sarahkoop · 2024-04-24T18:42:24Z

PayPalWebPayments/src/main/java/com/paypal/android/paypalwebpayments/PayPalWebCheckoutClient.kt

@@ -138,7 +138,10 @@ class PayPalWebCheckoutClient internal constructor(
 * Call this method at the end of the web checkout flow to clear out all observers and listeners
 */
 fun removeObservers() {
- activityReference.get()?.let { it.lifecycle.removeObserver(observer) }
+ activity.lifecycle?.let { lifecycle ->


Sorry I think we want to revert this change - it should be on the activityReference right? And I think Sai is correct it's always non-null, so disregard my comment

+1 need to revert the change.

sarahkoop · 2024-04-24T18:49:36Z

+1 to Jax's comment - Is it possible to unit test removeObservers?

jaxdesmarais · 2024-04-24T20:52:01Z

Demo/src/main/java/com/paypal/android/uishared/components/ActionButtonColumn.kt

@@ -117,7 +118,9 @@ fun StatefulActionButtonPreview() {
 modifier = Modifier
 .fillMaxWidth()
 .padding(UIConstants.paddingMedium)
- )
+ ) { state ->
+ Text(text = "Sample Text", modifier = Modifier.padding(64.dp))


Ah gotcha, didn't see it was in a preview and was confused by the "Sample Text". 😄

sarahkoop · 2024-04-24T21:32:12Z

PayPalWebPayments/src/main/java/com/paypal/android/paypalwebpayments/PayPalWebCheckoutClient.kt

+ activityReference.get()?.let { activity ->
+ payPalWebLauncher.launchPayPalWebCheckout(activity, request)?.let { launchError ->
+ notifyWebCheckoutFailure(launchError, request.orderId)
+ }


I think we should return an error if activity we can't get the activity reference - PayPal won't be launched but merchant wont get any notification (same for vault)

PayPalWebPayments/src/main/java/com/paypal/android/paypalwebpayments/PayPalWebCheckoutClient.kt

detekt/detekt-config.yml

warmkesselj added 3 commits April 9, 2024 09:49

Clear observers on paypalClient for WebCheckout flow

793cc9a

[email protected] [email protected]

Clear observers and listeners on cardClient

a29ee8b

[email protected] [email protected]

Use weak references to activities

16516b7

warmkesselj requested a review from a team as a code owner April 15, 2024 18:57

warmkesselj mentioned this pull request Apr 15, 2024

use “paypal-web-payments” way,the first time it can call back to our app, the second time it can't #247

Open

jaxdesmarais reviewed Apr 15, 2024

View reviewed changes

warmkesselj added 3 commits April 18, 2024 08:04

Respond to pull request.

4f07e4a

Remove extra line

e8f75db

Remove extra line

75c0082

jaxdesmarais reviewed Apr 23, 2024

View reviewed changes

CardPayments/src/main/java/com/paypal/android/cardpayments/CardClient.kt Outdated Show resolved Hide resolved

PayPalWebPayments/src/main/java/com/paypal/android/paypalwebpayments/PayPalWebCheckoutClient.kt Outdated Show resolved Hide resolved

warmkesselj and others added 6 commits April 23, 2024 08:28

Update based on detekt warnings

26facd7

Update based on comments

1ef41e0

Clear out observers after PayPal and Card Vault flow terminates

53a24e1

Update CHANGELOG.md

01eb6b5

Update var to full name based on comment

d8cbcc2

Merge branch 'fix_unableToStartWebCheckoutSubsequently' of https://gi…

cb5d5b7

…thub.com/paypal/paypal-android into fix_unableToStartWebCheckoutSubsequently

jaxdesmarais reviewed Apr 24, 2024

View reviewed changes

saperi22 and others added 4 commits April 24, 2024 10:50

Update kdoc

5c40462

Co-authored-by: Jax DesMarais-Leder <[email protected]>

Update kdoc

0126907

Co-authored-by: Jax DesMarais-Leder <[email protected]>

Update CHANGELOG.md

42ed095

Co-authored-by: Jax DesMarais-Leder <[email protected]>

Update CHANGELOG.md

b6d8d66

sarahkoop reviewed Apr 24, 2024

View reviewed changes

warmkesselj added 2 commits April 24, 2024 08:18

Change back to private

679d939

Merge branch 'fix_unableToStartWebCheckoutSubsequently' of https://gi…

831d42c

…thub.com/paypal/paypal-android into fix_unableToStartWebCheckoutSubsequently

sarahkoop reviewed Apr 24, 2024

View reviewed changes

warmkesselj added 2 commits April 24, 2024 08:27

Merge branch 'main' of https://github.com/paypal/paypal-android into …

ff7cc22

…fix_unableToStartWebCheckoutSubsequently

Ensure lifecycle is not null when removing observers

39d8c44

sarahkoop reviewed Apr 24, 2024

View reviewed changes

Undo changes

22b429e

warmkesselj added 4 commits April 24, 2024 08:59

Test unit test

08c0d22

Test unit test

0921bb2

Update unit test

1123f69

Add unit test to CardClient

c9e7425

jaxdesmarais approved these changes Apr 24, 2024

View reviewed changes

sarahkoop reviewed Apr 24, 2024

View reviewed changes

warmkesselj added 3 commits April 24, 2024 11:43

Check for activity being null and returning an error

3fac4d5

Remove comment

c8a7ed2

Update vault method to include notifyVaultFailure(error)

49f0be5

sarahkoop approved these changes Apr 24, 2024

View reviewed changes

tdchow reviewed Apr 25, 2024

View reviewed changes

PayPalWebPayments/src/main/java/com/paypal/android/paypalwebpayments/PayPalWebCheckoutClient.kt Show resolved Hide resolved

detekt/detekt-config.yml Show resolved Hide resolved

warmkesselj added 2 commits April 25, 2024 09:35

Remove the run command

2330a22

Remove unused import

8ceea87

warmkesselj merged commit ece573b into main Apr 25, 2024
7 checks passed

warmkesselj deleted the fix_unableToStartWebCheckoutSubsequently branch April 25, 2024 21:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unable to start web checkout in subsequent transactions #251

Fix unable to start web checkout in subsequent transactions #251

warmkesselj commented Apr 15, 2024 •

edited by saperi22

jaxdesmarais left a comment

jaxdesmarais Apr 24, 2024

saperi22 Apr 24, 2024 •

edited

warmkesselj Apr 24, 2024

jaxdesmarais Apr 24, 2024

sarahkoop Apr 24, 2024

saperi22 Apr 24, 2024 •

edited

warmkesselj Apr 24, 2024

sarahkoop Apr 24, 2024

saperi22 Apr 24, 2024

warmkesselj Apr 24, 2024

sarahkoop commented Apr 24, 2024

sarahkoop Apr 24, 2024

saperi22 Apr 24, 2024

sarahkoop commented Apr 24, 2024

jaxdesmarais Apr 24, 2024

sarahkoop Apr 24, 2024

Fix unable to start web checkout in subsequent transactions #251

Fix unable to start web checkout in subsequent transactions #251

Conversation

warmkesselj commented Apr 15, 2024 • edited by saperi22

Summary of changes

Checklist

Authors

jaxdesmarais left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saperi22 Apr 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saperi22 Apr 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahkoop commented Apr 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahkoop commented Apr 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warmkesselj commented Apr 15, 2024 •

edited by saperi22

saperi22 Apr 24, 2024 •

edited

saperi22 Apr 24, 2024 •

edited