Ensure no existing parent is present before adding to container (#128)

kiftio · web-flow · commit 41f5a5869f8f · 2024-10-02T15:15:19.000+01:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,9 @@
 # Changelog
 
+## 3.1.1 October 2, 2024
+
+- Ensure that cached WebView instances don't have existing parents before trying to add them to their container.
+
 ## 3.1.0 September 6, 2024
 
 - Implement and expose `onShowFileChooser()`, to support clients with checkouts that need to show a native file chooser. Example in the MobileBuyIntegration demo app.
diff --git a/README.md b/README.md
@@ -23,7 +23,7 @@ your project:
 #### Gradle
 
 ```groovy
-implementation "com.shopify:checkout-sheet-kit:3.1.0"
+implementation "com.shopify:checkout-sheet-kit:3.1.1"
 ```
 
 #### Maven
@@ -33,7 +33,7 @@ implementation "com.shopify:checkout-sheet-kit:3.1.0"
 <dependency>
    <groupId>com.shopify</groupId>
    <artifactId>checkout-sheet-kit</artifactId>
-   <version>3.1.0</version>
+   <version>3.1.1</version>
 </dependency>
 ```
 
diff --git a/lib/build.gradle b/lib/build.gradle
@@ -14,7 +14,7 @@ def resolveEnvVarValue(name, defaultValue) {
     return rawValue ? rawValue : defaultValue
 }
 
-def versionName = resolveEnvVarValue("CHECKOUT_SHEET_KIT_VERSION", "3.1.0")
+def versionName = resolveEnvVarValue("CHECKOUT_SHEET_KIT_VERSION", "3.1.1")
 
 ext {
     app_compat_version = '1.6.1'
diff --git a/lib/src/main/java/com/shopify/checkoutsheetkit/BaseWebView.kt b/lib/src/main/java/com/shopify/checkoutsheetkit/BaseWebView.kt
@@ -30,6 +30,7 @@ import android.os.Build
 import android.util.AttributeSet
 import android.view.KeyEvent
 import android.view.View
+import android.view.ViewGroup
 import android.view.ViewGroup.LayoutParams
 import android.view.ViewGroup.LayoutParams.MATCH_PARENT
 import android.webkit.PermissionRequest
@@ -214,3 +215,15 @@ internal abstract class BaseWebView(context: Context, attributeSet: AttributeSet
         private val CLIENT_ERROR = 400..499
     }
 }
+
+/**
+ * Removes the WebView from its parent if a parent exists
+ */
+internal fun BaseWebView.removeFromParent() {
+    val parent = this.parent
+    if (parent is ViewGroup) {
+        // Ensure view is not destroyed when removing from parent
+        CheckoutWebViewContainer.retainCacheEntry = RetainCacheEntry.YES
+        parent.removeView(this)
+    }
+}
diff --git a/lib/src/main/java/com/shopify/checkoutsheetkit/CheckoutDialog.kt b/lib/src/main/java/com/shopify/checkoutsheetkit/CheckoutDialog.kt
@@ -89,7 +89,7 @@ internal class CheckoutDialog(
 
         addWebViewToContainer(colorScheme, checkoutWebView)
         setOnCancelListener {
-            CheckoutWebViewContainer.retainCache = ShopifyCheckoutSheetKit.configuration.preloading.enabled
+            CheckoutWebViewContainer.retainCacheEntry = RetainCacheEntry.IF_NOT_STALE
             checkoutEventProcessor.onCheckoutCanceled()
         }
 
@@ -119,12 +119,13 @@ internal class CheckoutDialog(
 
     private fun addWebViewToContainer(
         colorScheme: ColorScheme,
-        checkoutWebView: WebView
+        checkoutWebView: BaseWebView,
     ) {
         findViewById<RelativeLayout>(R.id.checkoutSdkContainer).apply {
             setBackgroundColor(colorScheme.webViewBackgroundColor())
             val layoutParams = RelativeLayout.LayoutParams(WRAP_CONTENT, MATCH_PARENT)
             layoutParams.addRule(RelativeLayout.BELOW, R.id.progressBar)
+            checkoutWebView.removeFromParent()
             addView(checkoutWebView, layoutParams)
         }
     }
diff --git a/lib/src/main/java/com/shopify/checkoutsheetkit/CheckoutWebViewContainer.kt b/lib/src/main/java/com/shopify/checkoutsheetkit/CheckoutWebViewContainer.kt
@@ -38,18 +38,31 @@ internal class CheckoutWebViewContainer @JvmOverloads constructor(
     // We should only clear the cache and destroy the WebView after it's been removed from it's parent
     override fun onViewRemoved(child: View?) {
         super.onViewRemoved(child)
-        if (child is CheckoutWebView && (!retainCache || CheckoutWebView.cacheEntry?.isStale == true)) {
-            CheckoutWebView.clearCache()
+        if (child is CheckoutWebView) {
+           if (retainCacheEntry == RetainCacheEntry.IF_NOT_STALE && CheckoutWebView.cacheEntry?.isStale == true) {
+               CheckoutWebView.clearCache()
+           }
         }
 
         if (child is FallbackWebView) {
             child.destroy()
         }
 
-        retainCache = false
+        retainCacheEntry = RetainCacheEntry.IF_NOT_STALE
     }
 
     companion object {
-        internal var retainCache = false
+        internal var retainCacheEntry = RetainCacheEntry.IF_NOT_STALE
     }
 }
+
+internal enum class RetainCacheEntry {
+    /**
+     * Retain a WebView in the cache if it is not stale
+     */
+    IF_NOT_STALE,
+    /**
+     * Always retain the WebView in the cache, regardless of whether it is stale or not
+     */
+    YES,
+}
diff --git a/lib/src/test/java/com/shopify/checkoutsheetkit/CheckoutWebViewContainerTest.kt b/lib/src/test/java/com/shopify/checkoutsheetkit/CheckoutWebViewContainerTest.kt
@@ -26,39 +26,55 @@ import android.os.Looper
 import androidx.activity.ComponentActivity
 import org.assertj.core.api.Assertions.assertThat
 import org.junit.After
+import org.junit.Before
 import org.junit.Test
 import org.junit.runner.RunWith
+import org.mockito.kotlin.mock
+import org.mockito.kotlin.whenever
 import org.robolectric.Robolectric
 import org.robolectric.RobolectricTestRunner
 import org.robolectric.Shadows.shadowOf
+import kotlin.time.Duration.Companion.minutes
 
 @RunWith(RobolectricTestRunner::class)
 class CheckoutWebViewContainerTest {
 
+    private val mockCacheClock = mock<CheckoutWebView.CheckoutWebViewCacheClock>()
+
+    @Before
+    fun setUp() {
+        CheckoutWebView.cacheClock = mockCacheClock
+    }
+
     @After
     fun tearDown() {
         CheckoutWebView.clearCache()
+        shadowOf(Looper.getMainLooper()).runToEndOfTasks()
     }
 
     @Test
-    fun `should destroy FallbackWebView when it is removed`() {
+    fun `should destroy FallbackWebView when it is removed in all cases`() {
         Robolectric.buildActivity(ComponentActivity::class.java).use { activityController ->
-            val activity = activityController.get()
+            withPreloadingEnabled {
+                val activity = activityController.get()
 
-            val container = CheckoutWebViewContainer(activity)
-            val fallbackView = FallbackWebView(activity)
-            val shadow = shadowOf(fallbackView)
+                val container = CheckoutWebViewContainer(activity)
+                val fallbackView = FallbackWebView(activity)
+                val shadow = shadowOf(fallbackView)
 
-            container.addView(fallbackView)
-            assertThat(shadow.wasDestroyCalled()).isFalse()
+                container.addView(fallbackView)
+                assertThat(shadow.wasDestroyCalled()).isFalse()
 
-            container.removeView(fallbackView)
-            assertThat(shadow.wasDestroyCalled()).isTrue()
+                container.removeView(fallbackView)
+
+                assertThat(shadow.wasDestroyCalled()).isTrue()
+            }
         }
     }
 
+    // cache entries are essentially immediately stale if preloading is disabled
     @Test
-    fun `should destroy CheckoutWebView when it is removed if preloading disabled`() {
+    fun `should destroy CheckoutWebView when retainCacheEntry is IF_NOT_STALE and preloading is disabled`() {
         Robolectric.buildActivity(ComponentActivity::class.java).use { activityController ->
             val activity = activityController.get()
 
@@ -69,6 +85,7 @@ class CheckoutWebViewContainerTest {
             container.addView(checkoutWebView)
             assertThat(shadow.wasDestroyCalled()).isFalse()
 
+            CheckoutWebViewContainer.retainCacheEntry = RetainCacheEntry.IF_NOT_STALE
             container.removeView(checkoutWebView)
             shadowOf(Looper.getMainLooper()).runToEndOfTasks()
 
@@ -77,9 +94,11 @@ class CheckoutWebViewContainerTest {
     }
 
     @Test
-    fun `should destroy CheckoutWebView when it is removed if preloading and retain is false`() {
+    fun `should destroy CheckoutWebView when retainCacheEntry is IF_NOT_STALE and entry is stale`() {
         Robolectric.buildActivity(ComponentActivity::class.java).use { activityController ->
             withPreloadingEnabled {
+                whenever(mockCacheClock.currentTimeMillis()).thenReturn(System.currentTimeMillis())
+
                 val activity = activityController.get()
 
                 val container = CheckoutWebViewContainer(activity)
@@ -89,6 +108,9 @@ class CheckoutWebViewContainerTest {
                 container.addView(checkoutWebView)
                 assertThat(shadow.wasDestroyCalled()).isFalse()
 
+                CheckoutWebViewContainer.retainCacheEntry = RetainCacheEntry.IF_NOT_STALE
+                makeCacheEntryStale()
+
                 container.removeView(checkoutWebView)
                 shadowOf(Looper.getMainLooper()).runToEndOfTasks()
 
@@ -98,9 +120,33 @@ class CheckoutWebViewContainerTest {
     }
 
     @Test
-    fun `should not destroy CheckoutWebView when it is removed if preloading and retain is true`() {
+    fun `should not destroy non-stale CheckoutWebView when retainCacheEntry == IF_NOT_STALE and entry is not stale`() {
+        Robolectric.buildActivity(ComponentActivity::class.java).use { activityController ->
+            withPreloadingEnabled {
+                val activity = activityController.get()
+
+                val container = CheckoutWebViewContainer(activity)
+                val checkoutWebView = CheckoutWebView.cacheableCheckoutView("https://shopify.dev", activity, true)
+                val shadow = shadowOf(checkoutWebView)
+
+                container.addView(checkoutWebView)
+                assertThat(shadow.wasDestroyCalled()).isFalse()
+
+                CheckoutWebViewContainer.retainCacheEntry = RetainCacheEntry.IF_NOT_STALE
+                container.removeView(checkoutWebView)
+                shadowOf(Looper.getMainLooper()).runToEndOfTasks()
+
+                assertThat(shadow.wasDestroyCalled()).isFalse()
+            }
+        }
+    }
+
+    @Test
+    fun `should not destroy CheckoutWebView when retainCacheEntry == YES and entry is stale`() {
         Robolectric.buildActivity(ComponentActivity::class.java).use { activityController ->
             withPreloadingEnabled {
+                whenever(mockCacheClock.currentTimeMillis()).thenReturn(System.currentTimeMillis())
+
                 val activity = activityController.get()
 
                 val container = CheckoutWebViewContainer(activity)
@@ -110,13 +156,44 @@ class CheckoutWebViewContainerTest {
                 container.addView(checkoutWebView)
                 assertThat(shadow.wasDestroyCalled()).isFalse()
 
-                CheckoutWebViewContainer.retainCache = true
+                CheckoutWebViewContainer.retainCacheEntry = RetainCacheEntry.YES
+                makeCacheEntryStale()
+
                 container.removeView(checkoutWebView)
                 shadowOf(Looper.getMainLooper()).runToEndOfTasks()
+                assertThat(shadow.wasDestroyCalled()).isFalse()
+                assertThat(CheckoutWebViewContainer.retainCacheEntry).isEqualTo(RetainCacheEntry.IF_NOT_STALE)
+            }
+        }
+    }
+
+    @Test
+    fun `should not destroy CheckoutWebView when retainCacheEntry == YES and entry is not stale`() {
+        Robolectric.buildActivity(ComponentActivity::class.java).use { activityController ->
+            withPreloadingEnabled {
+                whenever(mockCacheClock.currentTimeMillis()).thenReturn(System.currentTimeMillis())
+
+                val activity = activityController.get()
+
+                val container = CheckoutWebViewContainer(activity)
+                val checkoutWebView = CheckoutWebView.cacheableCheckoutView("https://shopify.dev", activity, true)
+                val shadow = shadowOf(checkoutWebView)
 
+                container.addView(checkoutWebView)
                 assertThat(shadow.wasDestroyCalled()).isFalse()
-                assertThat(CheckoutWebViewContainer.retainCache).isFalse()
+
+                CheckoutWebViewContainer.retainCacheEntry = RetainCacheEntry.YES
+
+                container.removeView(checkoutWebView)
+                shadowOf(Looper.getMainLooper()).runToEndOfTasks()
+                assertThat(shadow.wasDestroyCalled()).isFalse()
+                assertThat(CheckoutWebViewContainer.retainCacheEntry).isEqualTo(RetainCacheEntry.IF_NOT_STALE)
             }
         }
     }
+
+    private fun makeCacheEntryStale() {
+        val initialTime = mockCacheClock.currentTimeMillis()
+        whenever(mockCacheClock.currentTimeMillis()).thenReturn(initialTime.plus(60 * 10 * 1000))
+    }
 }
diff --git a/lib/src/test/java/com/shopify/checkoutsheetkit/CheckoutWebViewTest.kt b/lib/src/test/java/com/shopify/checkoutsheetkit/CheckoutWebViewTest.kt
@@ -280,6 +280,42 @@ class CheckoutWebViewTest {
         }
     }
 
+    @Test
+    fun `removeFromParent() should remove parent if a parent exists but not destroy WebView`() {
+        withPreloadingEnabled {
+            Robolectric.buildActivity(ComponentActivity::class.java).use { activityController ->
+                val ctx = activityController.get()
+                val webView = CheckoutWebView.cacheableCheckoutView("https://shopify.dev", ctx, true)
+                val container = CheckoutWebViewContainer(ctx)
+                container.addView(webView)
+                assertThat(webView.parent).isNotNull()
+
+                webView.removeFromParent()
+                shadowOf(Looper.getMainLooper()).runToEndOfTasks()
+
+                val shadow = shadowOf(webView)
+                assertThat(shadow.wasDestroyCalled()).isFalse()
+                assertThat(webView.parent).isNull()
+            }
+        }
+    }
+
+    @Test
+    fun `removeFromParent() should do nothing if no parent exists`() {
+        withPreloadingEnabled {
+            Robolectric.buildActivity(ComponentActivity::class.java).use { activityController ->
+                val ctx = activityController.get()
+                val webView = CheckoutWebView.cacheableCheckoutView("https://shopify.dev", ctx, true)
+                webView.removeFromParent()
+                shadowOf(Looper.getMainLooper()).runToEndOfTasks()
+
+                val shadow = shadowOf(webView)
+                assertThat(shadow.wasDestroyCalled()).isFalse()
+                assertThat(webView.parent).isNull()
+            }
+        }
+    }
+
     companion object {
         private const val URL = "https://a.checkout.testurl"
     }

Original file line number	Diff line number	Diff line change
`@@ -14,7 +14,7 @@ def resolveEnvVarValue(name, defaultValue) {`
`14`	`14`	`return rawValue ? rawValue : defaultValue`
`15`	`15`	`}`
`16`	`16`
`17`		`-def versionName = resolveEnvVarValue("CHECKOUT_SHEET_KIT_VERSION", "3.1.0")`
	`17`	`+def versionName = resolveEnvVarValue("CHECKOUT_SHEET_KIT_VERSION", "3.1.1")`
`18`	`18`
`19`	`19`	`ext {`
`20`	`20`	`app_compat_version = '1.6.1'`