Issue #753 - Downloads that open in a new tab leave user on an empty tab (#883)

* Adds sourceTabId property to TabEntity in order to track source tab to navigate back to.
Bumps DB version and adds migration sql
Adds viewModel method to close and return to source tab.
Adds download callbacks to return to source tab if empty tab is opened.

Specifically in this event when a download link opens a new blank tab and needs to close after download has been triggered.

* Removes need to pass entire TabEntity in favor of just passing sourceTabId

* Code clean up, scope fix, and mirrors TabDataRepo delete closer in deleteAndSelectSource

* Updates tests to expected addWithSource rather than just add for new tabs

* Addresses PR feedback primarily letting fragment lifecycle handle cleanup and referencing the liveSelectedTab from inside the repo rather than duplicating the value and passing as a param

* Addresses PR feedback - info related to the source tab will be pulled directly from the tabsDao rather than the LiveData object.

* Addresses PR feedback - Create and use transaction rather than separate db calls to ensure data integrity.

* Adds tests for the 2 new TabDataRepo methods (AddWithSource and DeleteCurrentTabAndSelectSource)

* updates db migrations and tests after merge

Author: JKane10

app/schemas/com.duckduckgo.app.global.db.AppDatabase/24.json

@@ -121,16 +121,18 @@ class BrowserViewModelTest {
 
     @Test
     fun whenNewTabRequestedThenTabAddedToRepository() = runBlocking<Unit> {
+        whenever(mockTabRepository.liveSelectedTab).doReturn(MutableLiveData())
         testee.onNewTabRequested()
-        verify(mockTabRepository).add()
+        verify(mockTabRepository).addWithSource()
     }
 
     @Test
     fun whenOpenInNewTabRequestedThenTabAddedToRepository() = runBlocking<Unit> {
         val url = "http://example.com"
         whenever(mockOmnibarEntryConverter.convertQueryToUrl(url)).thenReturn(url)
+        whenever(mockTabRepository.liveSelectedTab).doReturn(MutableLiveData())
         testee.onOpenInNewTabRequested(url)
-        verify(mockTabRepository).add(url)
+        verify(mockTabRepository).addWithSource(url)
     }
 
     @Test

app/src/androidTest/java/com/duckduckgo/app/browser/BrowserViewModelTest.kt

@@ -281,6 +281,11 @@ class AppDatabaseTest {
         return stage
     }
 
+    @Test
+    fun whenMigratingFromVersion23To24ThenValidationSucceeds() {
+        createDatabaseAndMigrate(23, 24, migrationsProvider.MIGRATION_23_TO_24)
+    }
+
     private fun createDatabase(version: Int) {
         testHelper.createDatabase(TEST_DB_NAME, version).close()
     }

app/src/androidTest/java/com/duckduckgo/app/global/db/AppDatabaseTest.kt

@@ -297,6 +297,39 @@ class TabDataRepositoryTest {
         db.close()
     }
 
+    @Test
+    fun whenAddWithSourceEnsureTabEntryContainsExpectedSourceId() = runBlocking<Unit> {
+        val db = createDatabase()
+        val dao = db.tabsDao()
+        val sourceTab = TabEntity(tabId = "sourceId", url = "http://www.example.com", position = 0)
+        dao.addAndSelectTab(sourceTab)
+
+        testee = TabDataRepository(dao, SiteFactory(mockPrivacyPractices, mockEntityLookup), mockWebViewPreviewPersister, useOurAppDetector)
+
+        val addedTabId = testee.addWithSource("http://www.example.com", skipHome = false, isDefaultTab = false)
+        val addedTab = testee.liveSelectedTab.blockingObserve()
+        assertEquals(addedTabId, addedTab?.tabId)
+        assertEquals(addedTab?.sourceTabId, sourceTab.tabId)
+    }
+
+    @Test
+    fun whenDeleteCurrentTabAndSelectSourceLiveSelectedTabReturnsToSourceTab() = runBlocking<Unit> {
+        val db = createDatabase()
+        val dao = db.tabsDao()
+        val sourceTab = TabEntity(tabId = "sourceId", url = "http://www.example.com", position = 0)
+        val tabToDelete = TabEntity(tabId = "tabToDeleteId", url = "http://www.example.com", position = 1, sourceTabId = "sourceId")
+        dao.addAndSelectTab(sourceTab)
+        dao.addAndSelectTab(tabToDelete)
+
+        testee = TabDataRepository(dao, SiteFactory(mockPrivacyPractices, mockEntityLookup), mockWebViewPreviewPersister, useOurAppDetector)
+
+        var currentSelectedTabId = testee.liveSelectedTab.blockingObserve()?.tabId
+        assertEquals(currentSelectedTabId, tabToDelete.tabId)
+        testee.deleteCurrentTabAndSelectSource()
+        currentSelectedTabId = testee.liveSelectedTab.blockingObserve()?.tabId
+        assertEquals(currentSelectedTabId, sourceTab.tabId)
+    }
+
     private fun createDatabase(): AppDatabase {
         return Room.inMemoryDatabaseBuilder(InstrumentationRegistry.getInstrumentation().targetContext, AppDatabase::class.java)
             .allowMainThreadQueries()

app/src/androidTest/java/com/duckduckgo/app/tabs/model/TabDataRepositoryTest.kt

@@ -1107,10 +1107,19 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope, DaxDialogLi
         }
     }
 
+    private fun closeAndReturnToSourceIfBlankTab() {
+        if (viewModel.url == null) {
+            launch {
+                viewModel.closeAndSelectSourceTab()
+            }
+        }
+    }
+
     private fun createDownloadListener(): FileDownloadListener {
         return object : FileDownloadListener {
             override fun downloadStarted() {
                 fileDownloadNotificationManager.showDownloadInProgressNotification()
+                closeAndReturnToSourceIfBlankTab()
             }
 
             override fun downloadFinished(file: File, mimeType: String?) {
@@ -1134,6 +1143,14 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope, DaxDialogLi
                 snackbar.show()
             }
 
+            override fun downloadCancelled() {
+                closeAndReturnToSourceIfBlankTab()
+            }
+
+            override fun downloadOpened() {
+                closeAndReturnToSourceIfBlankTab()
+            }
+
             private fun showDownloadManagerAppSettings() {
                 try {
                     val intent = Intent(Settings.ACTION_APPLICATION_DETAILS_SETTINGS)

app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt

@@ -505,6 +505,14 @@ class BrowserTabViewModel(
         viewModelScope.launch { removeCurrentTabFromRepository() }
     }
 
+    override fun closeAndSelectSourceTab() {
+        viewModelScope.launch { removeAndSelectTabFromRepository() }
+    }
+
+    private suspend fun removeAndSelectTabFromRepository() {
+        tabRepository.deleteCurrentTabAndSelectSource()
+    }
+
     fun onUserPressedForward() {
         navigationAwareLoginDetector.onEvent(NavigationEvent.UserAction.NavigateForward)
         if (!currentBrowserViewState().browserShowing) {

app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt

@@ -127,11 +127,15 @@ class BrowserViewModel(
     }
 
     suspend fun onNewTabRequested(isDefaultTab: Boolean = false): String {
-        return tabRepository.add(isDefaultTab = isDefaultTab)
+        return tabRepository.addWithSource(isDefaultTab = isDefaultTab)
     }
 
     suspend fun onOpenInNewTabRequested(query: String, skipHome: Boolean = false): String {
-        return tabRepository.add(queryUrlConverter.convertQueryToUrl(query), skipHome, isDefaultTab = false)
+        return tabRepository.addWithSource(
+                queryUrlConverter.convertQueryToUrl(query),
+                skipHome,
+                isDefaultTab = false
+        )
     }
 
     suspend fun onTabsUpdated(tabs: List<TabEntity>?) {

app/src/main/java/com/duckduckgo/app/browser/BrowserViewModel.kt

@@ -49,7 +49,7 @@ class DownloadConfirmationFragment : BottomSheetDialogFragment() {
     lateinit var downloadListener: FileDownloadListener
 
     private val pendingDownload: PendingFileDownload by lazy {
-        arguments!![PENDING_DOWNLOAD_BUNDLE_KEY] as PendingFileDownload
+        requireArguments()[PENDING_DOWNLOAD_BUNDLE_KEY] as PendingFileDownload
     }
 
     private var file: File? = null
@@ -87,6 +87,7 @@ class DownloadConfirmationFragment : BottomSheetDialogFragment() {
         }
         view.cancel.setOnClickListener {
             Timber.i("Cancelled download for url ${pendingDownload.url}")
+            downloadListener.downloadCancelled()
             dismiss()
         }
 
@@ -126,6 +127,7 @@ class DownloadConfirmationFragment : BottomSheetDialogFragment() {
                 Timber.e("No suitable activity found")
                 Toast.makeText(activity, R.string.downloadConfirmationUnableToOpenFileText, Toast.LENGTH_SHORT).show()
             }
+            downloadListener.downloadOpened()
         }
     }
 

app/src/main/java/com/duckduckgo/app/browser/DownloadConfirmationFragment.kt

@@ -47,6 +47,7 @@ interface WebViewClientListener {
     fun recoverFromRenderProcessGone()
     fun requiresAuthentication(request: BasicAuthenticationRequest)
     fun closeCurrentTab()
+    fun closeAndSelectSourceTab()
     fun upgradedToHttps()
     fun surrogateDetected(surrogate: SurrogateResponse)
 

app/src/main/java/com/duckduckgo/app/browser/WebViewClientListener.kt

@@ -52,6 +52,8 @@ class FileDownloader @Inject constructor(
         fun downloadStarted()
         fun downloadFinished(file: File, mimeType: String?)
         fun downloadFailed(message: String, downloadFailReason: DownloadFailReason)
+        fun downloadCancelled()
+        fun downloadOpened()
     }
 }