Improve back button handling in feedback (#4908)

Task/Issue URL: https://app.asana.com/0/488551667048375/1208045844925126/f ### Description This PR contains the following: - Fix for the back button bug in Browser Feedback - Use OnBackPressedCallback instead of overriding onBackPressed in Ppro unified feedback ### Steps to test this PR _Browser Feedback_ - [x] Install build - [x] Go to Settings > About > Share Feedback - [x] Smoke test back button and it behaves accordingly _PPro Unified Feedback_ Follow https://app.asana.com/0/0/1208071203780039/f
duckduckgo · Aug 16, 2024 · e408922 · e408922
1 parent e2ed0aa
commit e408922
Show file tree

Hide file tree

Showing 4 changed files with 108 additions and 79 deletions.
diff --git a/app/src/main/java/com/duckduckgo/app/feedback/ui/common/FeedbackActivity.kt b/app/src/main/java/com/duckduckgo/app/feedback/ui/common/FeedbackActivity.kt
@@ -21,6 +21,7 @@ import android.app.Activity.RESULT_OK
 import android.content.Context
 import android.content.Intent
 import android.os.Bundle
+import androidx.activity.OnBackPressedCallback
 import androidx.fragment.app.FragmentTransaction
 import androidx.fragment.app.transaction
 import com.duckduckgo.anvil.annotations.ContributeToActivityStarter
@@ -64,6 +65,14 @@ class FeedbackActivity :
         setContentView(binding.root)
         setupToolbar(toolbar)
         configureObservers()
+        onBackPressedDispatcher.addCallback(
+            this,
+            object : OnBackPressedCallback(true) {
+                override fun handleOnBackPressed() {
+                    viewModel.onBackPressed()
+                }
+            },
+        )
     }
 
     private fun configureObservers() {
@@ -153,11 +162,6 @@ class FeedbackActivity :
         }
     }
 
-    override fun onBackPressed() {
-        super.onBackPressed()
-        viewModel.onBackPressed()
-    }
-
     /**
      * Initial feedback page listeners
      */

diff --git a/.../src/main/java/com/duckduckgo/subscriptions/impl/feedback/SubscriptionFeedbackActivity.kt b/.../src/main/java/com/duckduckgo/subscriptions/impl/feedback/SubscriptionFeedbackActivity.kt
@@ -18,6 +18,7 @@ package com.duckduckgo.subscriptions.impl.feedback
 
 import android.os.Bundle
 import android.widget.Toast
+import androidx.activity.OnBackPressedCallback
 import androidx.annotation.StringRes
 import androidx.fragment.app.commit
 import androidx.lifecycle.Lifecycle
@@ -38,6 +39,7 @@ import com.duckduckgo.subscriptions.api.PrivacyProFeedbackScreens.PrivacyProFeed
 import com.duckduckgo.subscriptions.impl.R
 import com.duckduckgo.subscriptions.impl.databinding.ActivityFeedbackBinding
 import com.duckduckgo.subscriptions.impl.feedback.SubscriptionFeedbackViewModel.Command
+import com.duckduckgo.subscriptions.impl.feedback.SubscriptionFeedbackViewModel.Command.FeedbackCancelled
 import com.duckduckgo.subscriptions.impl.feedback.SubscriptionFeedbackViewModel.Command.FeedbackCompleted
 import com.duckduckgo.subscriptions.impl.feedback.SubscriptionFeedbackViewModel.Command.ShowHelpPages
 import com.duckduckgo.subscriptions.impl.feedback.SubscriptionFeedbackViewModel.FeedbackFragmentState
@@ -76,14 +78,14 @@ class SubscriptionFeedbackActivity :
         setupToolbar(toolbar)
         observeViewModel()
         handleInitialState()
-    }
-
-    override fun onBackPressed() {
-        if (viewModel.shouldGoBackInFeedbackFlow()) {
-            viewModel.handleBackInFlow()
-        } else {
-            super.onBackPressed()
-        }
+        onBackPressedDispatcher.addCallback(
+            this,
+            object : OnBackPressedCallback(true) {
+                override fun handleOnBackPressed() {
+                    viewModel.handleBackPress()
+                }
+            },
+        )
     }
 
     private fun handleInitialState() {
@@ -149,6 +151,7 @@ class SubscriptionFeedbackActivity :
 
     private fun handleCommands(command: Command) {
         when (command) {
+            is FeedbackCancelled -> finish()
             is FeedbackCompleted -> {
                 Toast.makeText(applicationContext, R.string.feedbackSubmitCompletedMessage, Toast.LENGTH_LONG).show()
                 finish()

diff --git a/...src/main/java/com/duckduckgo/subscriptions/impl/feedback/SubscriptionFeedbackViewModel.kt b/...src/main/java/com/duckduckgo/subscriptions/impl/feedback/SubscriptionFeedbackViewModel.kt
@@ -36,6 +36,7 @@ import com.duckduckgo.subscriptions.impl.feedback.SubscriptionFeedbackReportType
 import com.duckduckgo.subscriptions.impl.feedback.SubscriptionFeedbackReportType.REQUEST_FEATURE
 import com.duckduckgo.subscriptions.impl.feedback.SubscriptionFeedbackSubsSubCategory.ONE_TIME_PASSWORD
 import com.duckduckgo.subscriptions.impl.feedback.SubscriptionFeedbackSubsSubCategory.OTHER
+import com.duckduckgo.subscriptions.impl.feedback.SubscriptionFeedbackViewModel.Command.FeedbackCancelled
 import com.duckduckgo.subscriptions.impl.feedback.SubscriptionFeedbackViewModel.Command.FeedbackCompleted
 import com.duckduckgo.subscriptions.impl.feedback.SubscriptionFeedbackViewModel.Command.ShowHelpPages
 import com.duckduckgo.subscriptions.impl.feedback.SubscriptionFeedbackViewModel.FeedbackFragmentState.FeedbackAction
@@ -300,69 +301,77 @@ class SubscriptionFeedbackViewModel @Inject constructor(
         }
     }
 
-    fun shouldGoBackInFeedbackFlow(): Boolean = viewState.value.previousFragmentState != null
-
-    fun handleBackInFlow() {
+    fun handleBackPress() {
         viewModelScope.launch {
-            val newState = viewState.value.previousFragmentState
-            val currentFeedbackMetadata = viewState.value.feedbackMetadata
+            if (shouldGoBackInFeedbackFlow()) {
+                handleBackInFlow()
+            } else {
+                command.send(FeedbackCancelled)
+            }
+        }
+    }
 
-            if (newState != null) {
-                when (newState) {
-                    is FeedbackGeneral -> ViewState(
-                        feedbackMetadata = currentFeedbackMetadata.copy(
-                            reportType = null,
-                        ),
-                        currentFragmentState = newState,
-                        previousFragmentState = null,
-                        isForward = false,
-                    )
+    private fun shouldGoBackInFeedbackFlow(): Boolean = viewState.value.previousFragmentState != null
 
-                    is FeedbackAction -> ViewState(
-                        feedbackMetadata = currentFeedbackMetadata.copy(
-                            reportType = null,
-                        ),
-                        currentFragmentState = newState,
-                        previousFragmentState = if (currentFeedbackMetadata.source == DDG_SETTINGS) FeedbackGeneral else null,
-                        isForward = false,
-                    )
+    private suspend fun handleBackInFlow() {
+        val newState = viewState.value.previousFragmentState
+        val currentFeedbackMetadata = viewState.value.feedbackMetadata
 
-                    is FeedbackCategory -> ViewState(
+        if (newState != null) {
+            when (newState) {
+                is FeedbackGeneral -> ViewState(
+                    feedbackMetadata = currentFeedbackMetadata.copy(
+                        reportType = null,
+                    ),
+                    currentFragmentState = newState,
+                    previousFragmentState = null,
+                    isForward = false,
+                )
+
+                is FeedbackAction -> ViewState(
+                    feedbackMetadata = currentFeedbackMetadata.copy(
+                        reportType = null,
+                    ),
+                    currentFragmentState = newState,
+                    previousFragmentState = if (currentFeedbackMetadata.source == DDG_SETTINGS) FeedbackGeneral else null,
+                    isForward = false,
+                )
+
+                is FeedbackCategory -> ViewState(
+                    feedbackMetadata = currentFeedbackMetadata.copy(
+                        category = null,
+                    ),
+                    currentFragmentState = newState,
+                    previousFragmentState = FeedbackAction,
+                    isForward = false,
+                )
+
+                is FeedbackSubCategory -> {
+                    val autoAssignedCategory = when (currentFeedbackMetadata.source) {
+                        SUBSCRIPTION_SETTINGS, VPN_MANAGEMENT, VPN_EXCLUDED_APPS -> true
+                        else -> false
+                    }
+                    val previousState =
+                        if (currentFeedbackMetadata.reportType == REPORT_PROBLEM && autoAssignedCategory) {
+                            FeedbackAction
+                        } else {
+                            FeedbackCategory(
+                                currentFeedbackMetadata.reportType?.asTitle() ?: -1,
+                            )
+                        }
+                    ViewState(
                         feedbackMetadata = currentFeedbackMetadata.copy(
-                            category = null,
+                            subCategory = null,
                         ),
                         currentFragmentState = newState,
-                        previousFragmentState = FeedbackAction,
+                        previousFragmentState = previousState,
                         isForward = false,
                     )
-
-                    is FeedbackSubCategory -> {
-                        val autoAssignedCategory = when (currentFeedbackMetadata.source) {
-                            SUBSCRIPTION_SETTINGS, VPN_MANAGEMENT, VPN_EXCLUDED_APPS -> true
-                            else -> false
-                        }
-                        val previousState =
-                            if (currentFeedbackMetadata.reportType == REPORT_PROBLEM && autoAssignedCategory) {
-                                FeedbackAction
-                            } else {
-                                FeedbackCategory(
-                                    currentFeedbackMetadata.reportType?.asTitle() ?: -1,
-                                )
-                            }
-                        ViewState(
-                            feedbackMetadata = currentFeedbackMetadata.copy(
-                                subCategory = null,
-                            ),
-                            currentFragmentState = newState,
-                            previousFragmentState = previousState,
-                            isForward = false,
-                        )
-                    }
-
-                    is FeedbackSubmit -> null // Not possible to go back to this page via back press
-                }?.also {
-                    viewState.emit(it)
                 }
+
+                is FeedbackSubmit -> null // Not possible to go back to this page via back press
+            }?.also {
+                viewState.emit(it)
             }
         }
     }
@@ -467,6 +476,7 @@ class SubscriptionFeedbackViewModel @Inject constructor(
 
     sealed class Command {
         data object FeedbackCompleted : Command()
+        data object FeedbackCancelled : Command()
         data class ShowHelpPages(val url: String) : Command()
     }
 

diff --git a/...test/java/com/duckduckgo/subscriptions/impl/feedback/SubscriptionFeedbackViewModelTest.kt b/...test/java/com/duckduckgo/subscriptions/impl/feedback/SubscriptionFeedbackViewModelTest.kt
@@ -14,6 +14,7 @@ import com.duckduckgo.subscriptions.impl.feedback.SubscriptionFeedbackCategory.V
 import com.duckduckgo.subscriptions.impl.feedback.SubscriptionFeedbackReportType.GENERAL_FEEDBACK
 import com.duckduckgo.subscriptions.impl.feedback.SubscriptionFeedbackReportType.REPORT_PROBLEM
 import com.duckduckgo.subscriptions.impl.feedback.SubscriptionFeedbackReportType.REQUEST_FEATURE
+import com.duckduckgo.subscriptions.impl.feedback.SubscriptionFeedbackViewModel.Command.FeedbackCancelled
 import com.duckduckgo.subscriptions.impl.feedback.SubscriptionFeedbackViewModel.Command.ShowHelpPages
 import com.duckduckgo.subscriptions.impl.feedback.SubscriptionFeedbackViewModel.FeedbackFragmentState.FeedbackAction
 import com.duckduckgo.subscriptions.impl.feedback.SubscriptionFeedbackViewModel.FeedbackFragmentState.FeedbackCategory
@@ -557,7 +558,7 @@ class SubscriptionFeedbackViewModelTest {
             viewModel.allowUserToChooseFeedbackType() // Show General
             viewModel.onProFeedbackSelected() // Show Action
 
-            viewModel.handleBackInFlow()
+            viewModel.handleBackPress()
 
             expectMostRecentItem().assertViewStateMoveBack(
                 expectedPreviousFragmentState = null,
@@ -575,7 +576,7 @@ class SubscriptionFeedbackViewModelTest {
             viewModel.allowUserToChooseReportType(source = SUBSCRIPTION_SETTINGS) // Show action
             viewModel.onReportTypeSelected(REQUEST_FEATURE) // Show submit
 
-            viewModel.handleBackInFlow()
+            viewModel.handleBackPress()
 
             expectMostRecentItem().assertViewStateMoveBack(
                 expectedPreviousFragmentState = null,
@@ -593,7 +594,7 @@ class SubscriptionFeedbackViewModelTest {
             viewModel.allowUserToChooseReportType(source = SUBSCRIPTION_SETTINGS) // Show action
             viewModel.onReportTypeSelected(GENERAL_FEEDBACK) // Show submit
 
-            viewModel.handleBackInFlow()
+            viewModel.handleBackPress()
 
             expectMostRecentItem().assertViewStateMoveBack(
                 expectedPreviousFragmentState = null,
@@ -611,7 +612,7 @@ class SubscriptionFeedbackViewModelTest {
             viewModel.allowUserToChooseReportType(source = SUBSCRIPTION_SETTINGS) // Show action
             viewModel.onReportTypeSelected(REPORT_PROBLEM) // Show subcategory
 
-            viewModel.handleBackInFlow()
+            viewModel.handleBackPress()
 
             expectMostRecentItem().assertViewStateMoveBack(
                 expectedPreviousFragmentState = null,
@@ -630,7 +631,7 @@ class SubscriptionFeedbackViewModelTest {
             viewModel.allowUserToChooseReportType(source = VPN_MANAGEMENT) // Show action
             viewModel.onReportTypeSelected(REPORT_PROBLEM) // Show subcategory
 
-            viewModel.handleBackInFlow()
+            viewModel.handleBackPress()
 
             expectMostRecentItem().assertViewStateMoveBack(
                 expectedPreviousFragmentState = null,
@@ -650,7 +651,7 @@ class SubscriptionFeedbackViewModelTest {
             viewModel.onProFeedbackSelected() // show action
             viewModel.onReportTypeSelected(REPORT_PROBLEM) // Show category
 
-            viewModel.handleBackInFlow()
+            viewModel.handleBackPress()
 
             expectMostRecentItem().assertViewStateMoveBack(
                 expectedPreviousFragmentState = FeedbackGeneral,
@@ -670,7 +671,7 @@ class SubscriptionFeedbackViewModelTest {
             viewModel.onReportTypeSelected(REPORT_PROBLEM) // Show category
             viewModel.onCategorySelected(VPN) // Show subcategory
 
-            viewModel.handleBackInFlow()
+            viewModel.handleBackPress()
 
             expectMostRecentItem().assertViewStateMoveBack(
                 expectedPreviousFragmentState = FeedbackAction,
@@ -691,7 +692,7 @@ class SubscriptionFeedbackViewModelTest {
             viewModel.onReportTypeSelected(REPORT_PROBLEM) // Show category
             viewModel.onCategorySelected(SUBS_AND_PAYMENTS) // Show subcategory
 
-            viewModel.handleBackInFlow()
+            viewModel.handleBackPress()
 
             expectMostRecentItem().assertViewStateMoveBack(
                 expectedPreviousFragmentState = FeedbackAction,
@@ -712,7 +713,7 @@ class SubscriptionFeedbackViewModelTest {
             viewModel.onReportTypeSelected(REPORT_PROBLEM) // Show category
             viewModel.onCategorySelected(PIR) // Show subcategory
 
-            viewModel.handleBackInFlow()
+            viewModel.handleBackPress()
 
             expectMostRecentItem().assertViewStateMoveBack(
                 expectedPreviousFragmentState = FeedbackAction,
@@ -733,7 +734,7 @@ class SubscriptionFeedbackViewModelTest {
             viewModel.onReportTypeSelected(REPORT_PROBLEM) // Show category
             viewModel.onCategorySelected(ITR) // Show subcategory
 
-            viewModel.handleBackInFlow()
+            viewModel.handleBackPress()
 
             expectMostRecentItem().assertViewStateMoveBack(
                 expectedPreviousFragmentState = FeedbackAction,
@@ -755,7 +756,7 @@ class SubscriptionFeedbackViewModelTest {
             viewModel.onCategorySelected(SUBS_AND_PAYMENTS) // Show subcategory
             viewModel.onSubcategorySelected(SubscriptionFeedbackSubsSubCategory.ONE_TIME_PASSWORD) // Show submit
 
-            viewModel.handleBackInFlow()
+            viewModel.handleBackPress()
 
             expectMostRecentItem().assertViewStateMoveBack(
                 expectedPreviousFragmentState = FeedbackCategory(R.string.feedbackActionReportIssue),
@@ -776,7 +777,7 @@ class SubscriptionFeedbackViewModelTest {
             viewModel.onReportTypeSelected(REPORT_PROBLEM) // Show subcategory
             viewModel.onSubcategorySelected(BROWSER_CRASH_FREEZE) // Show submit
 
-            viewModel.handleBackInFlow()
+            viewModel.handleBackPress()
 
             expectMostRecentItem().assertViewStateMoveBack(
                 expectedPreviousFragmentState = FeedbackAction,
@@ -797,7 +798,7 @@ class SubscriptionFeedbackViewModelTest {
             viewModel.onReportTypeSelected(REPORT_PROBLEM) // Show subcategory
             viewModel.onSubcategorySelected(SubscriptionFeedbackSubsSubCategory.ONE_TIME_PASSWORD) // Show submit
 
-            viewModel.handleBackInFlow()
+            viewModel.handleBackPress()
 
             expectMostRecentItem().assertViewStateMoveBack(
                 expectedPreviousFragmentState = FeedbackAction,
@@ -811,6 +812,17 @@ class SubscriptionFeedbackViewModelTest {
         }
     }
 
+    @Test
+    fun whenUserIsInFirstPageWhenBackIsPressedThenEmitCancelledCommand() = runTest {
+        viewModel.allowUserToChooseReportType(source = SUBSCRIPTION_SETTINGS) // Show action
+
+        viewModel.handleBackPress()
+
+        viewModel.commands().test {
+            assertEquals(FeedbackCancelled, expectMostRecentItem())
+        }
+    }
+
     @Test
     fun whenFeatureRequestSubmittedTheSendFeatureRequestPixel() = runTest {
         viewModel.allowUserToChooseFeedbackType() // Show general