From eec7ee19138f70d9e43694fd07731b15863f7b65 Mon Sep 17 00:00:00 2001 From: Michael Shafrir <45020849+mshafrir-stripe@users.noreply.github.com> Date: Wed, 5 Feb 2020 16:57:44 -0500 Subject: [PATCH] Fix off by 1 error in PaymentMethodSwipeCallback (#2141) When the Google Pay button is enabled in PaymentMethodsActivity, `PaymentMethodSwipeCallback#onSwiped()` would get the wrong element from the `paymentMethods` list. --- .../view/PaymentMethodSwipeCallback.kt | 2 +- .../android/view/PaymentMethodsAdapter.kt | 30 +++++++++++++++---- .../android/view/PaymentMethodsAdapterTest.kt | 30 ++++++++++++++----- 3 files changed, 47 insertions(+), 15 deletions(-) diff --git a/stripe/src/main/java/com/stripe/android/view/PaymentMethodSwipeCallback.kt b/stripe/src/main/java/com/stripe/android/view/PaymentMethodSwipeCallback.kt index e9dd9844f2f..be76b1b313d 100644 --- a/stripe/src/main/java/com/stripe/android/view/PaymentMethodSwipeCallback.kt +++ b/stripe/src/main/java/com/stripe/android/view/PaymentMethodSwipeCallback.kt @@ -42,7 +42,7 @@ internal class PaymentMethodSwipeCallback( } override fun onSwiped(viewHolder: RecyclerView.ViewHolder, direction: Int) { - val paymentMethod = adapter.paymentMethods[viewHolder.adapterPosition] + val paymentMethod = adapter.getPaymentMethodAtPosition(viewHolder.adapterPosition) listener.onSwiped(paymentMethod) } diff --git a/stripe/src/main/java/com/stripe/android/view/PaymentMethodsAdapter.kt b/stripe/src/main/java/com/stripe/android/view/PaymentMethodsAdapter.kt index 128a0746bc3..291ee12eced 100644 --- a/stripe/src/main/java/com/stripe/android/view/PaymentMethodsAdapter.kt +++ b/stripe/src/main/java/com/stripe/android/view/PaymentMethodsAdapter.kt @@ -192,6 +192,7 @@ internal class PaymentMethodsAdapter constructor( return ViewHolder.GooglePayViewHolder(itemView) } + @JvmSynthetic internal fun deletePaymentMethod(paymentMethod: PaymentMethod) { val indexToDelete = paymentMethods.indexOfFirst { it.id == paymentMethod.id } if (indexToDelete >= 0) { @@ -200,21 +201,38 @@ internal class PaymentMethodsAdapter constructor( } } + @JvmSynthetic internal fun resetPaymentMethod(paymentMethod: PaymentMethod) { - val indexToReset = paymentMethods.indexOfFirst { it.id == paymentMethod.id } - if (indexToReset >= 0) { - notifyItemChanged(indexToReset) + getPosition(paymentMethod)?.let { + notifyItemChanged(it) } } - private fun getPaymentMethodAtPosition(position: Int): PaymentMethod { - return paymentMethods[getPaymentMethodPosition(position)] + /** + * Given an adapter position, translate to a `paymentMethods` element + */ + @JvmSynthetic + internal fun getPaymentMethodAtPosition(position: Int): PaymentMethod { + return paymentMethods[getPaymentMethodIndex(position)] } - private fun getPaymentMethodPosition(position: Int): Int { + /** + * Given an adapter position, translate to a `paymentMethods` index + */ + private fun getPaymentMethodIndex(position: Int): Int { return position - googlePayCount } + /** + * Given a Payment Method, get its adapter position. For example, if the Google Pay button is + * being shown, the 2nd element in [paymentMethods] is actually the 3rd item in the adapter. + */ + internal fun getPosition(paymentMethod: PaymentMethod): Int? { + return paymentMethods.indexOf(paymentMethod).takeIf { it >= 0 }?.let { + it + googlePayCount + } + } + private fun getAddableTypesPosition(position: Int): Int { return position - paymentMethods.size - googlePayCount } diff --git a/stripe/src/test/java/com/stripe/android/view/PaymentMethodsAdapterTest.kt b/stripe/src/test/java/com/stripe/android/view/PaymentMethodsAdapterTest.kt index 4881a2fd2b4..1e6479de4c8 100644 --- a/stripe/src/test/java/com/stripe/android/view/PaymentMethodsAdapterTest.kt +++ b/stripe/src/test/java/com/stripe/android/view/PaymentMethodsAdapterTest.kt @@ -4,6 +4,7 @@ import android.content.Context import android.widget.FrameLayout import androidx.recyclerview.widget.RecyclerView import androidx.test.core.app.ApplicationProvider +import com.nhaarman.mockitokotlin2.mock import com.nhaarman.mockitokotlin2.verify import com.stripe.android.model.PaymentMethod import com.stripe.android.model.PaymentMethodFixtures @@ -13,9 +14,7 @@ import kotlin.test.assertEquals import kotlin.test.assertNotNull import kotlin.test.assertNull import org.junit.runner.RunWith -import org.mockito.Mock import org.mockito.Mockito.times -import org.mockito.MockitoAnnotations import org.robolectric.RobolectricTestRunner /** @@ -23,11 +22,8 @@ import org.robolectric.RobolectricTestRunner */ @RunWith(RobolectricTestRunner::class) class PaymentMethodsAdapterTest { - @Mock - private lateinit var adapterDataObserver: RecyclerView.AdapterDataObserver - - @Mock - private lateinit var listener: PaymentMethodsAdapter.Listener + private val adapterDataObserver: RecyclerView.AdapterDataObserver = mock() + private val listener: PaymentMethodsAdapter.Listener = mock() private val paymentMethodsAdapter: PaymentMethodsAdapter = PaymentMethodsAdapter(ARGS) @@ -37,7 +33,6 @@ class PaymentMethodsAdapterTest { @BeforeTest fun setup() { - MockitoAnnotations.initMocks(this) paymentMethodsAdapter.registerAdapterDataObserver(adapterDataObserver) } @@ -235,6 +230,25 @@ class PaymentMethodsAdapterTest { verify(listener).onGooglePayClick() } + @Test + fun getPosition_withValidPaymentMethod_returnsPosition() { + val adapter = PaymentMethodsAdapter(ARGS, shouldShowGooglePay = true) + adapter.setPaymentMethods(PaymentMethodFixtures.CARD_PAYMENT_METHODS) + + assertEquals( + 3, + adapter.getPosition(PaymentMethodFixtures.CARD_PAYMENT_METHODS.last()) + ) + } + + @Test + fun getPosition_withInvalidPaymentMethod_returnsNull() { + val adapter = PaymentMethodsAdapter(ARGS, shouldShowGooglePay = true) + adapter.setPaymentMethods(PaymentMethodFixtures.CARD_PAYMENT_METHODS) + + assertNull(adapter.getPosition(PaymentMethodFixtures.FPX_PAYMENT_METHOD)) + } + private companion object { private val ARGS = PaymentMethodsActivityStarter.Args.Builder()