Fix NPS Survey Gating

app/src/main/java/org/oppia/android/app/player/exploration/ExplorationActivityPresenter.kt

@@ -298,11 +298,7 @@ class ExplorationActivityPresenter @Inject constructor(
         }
         is AsyncResult.Success -> {
           oppiaLogger.d("ExplorationActivity", "Successfully stopped exploration")
-          if (isCompletion) {
-            maybeShowSurveyDialog(profileId, topicId)
-          } else {
-            backPressActivitySelector()
-          }
+          maybeShowSurveyDialog(profileId, topicId)
         }
       }
     }
@@ -560,11 +556,12 @@ class ExplorationActivityPresenter @Inject constructor(
                   .add(dialogFragment, TAG_SURVEY_WELCOME_DIALOG)
                   .addToBackStack(null)
                   .commit()
+
+                // Changes to underlying DataProviders will update the gating result.
+                liveData.removeObserver(this)
               } else {
                 backPressActivitySelector()
               }
-              // Changes to underlying DataProviders will update the gating result.
-              liveData.removeObserver(this)
             }
           }
         }

app/src/main/java/org/oppia/android/app/player/state/StateFragmentPresenter.kt

@@ -464,7 +464,7 @@ class StateFragmentPresenter @Inject constructor(
 
     // Only check gating result when the previous operation has completed because gating depends on
     // result of saving the time spent in the exploration, at the end of the exploration.
-    markStoryCompletedLivedata.observe(activity, { maybeShowSurveyDialog(profileId, topicId) })
+    markStoryCompletedLivedata.observe(fragment, { maybeShowSurveyDialog(profileId, topicId) })
   }
 
   private fun showHintsAndSolutions(helpIndex: HelpIndex, isCurrentStatePendingState: Boolean) {
@@ -569,12 +569,13 @@ class StateFragmentPresenter @Inject constructor(
                 transaction
                   .add(dialogFragment, TAG_SURVEY_WELCOME_DIALOG)
                   .commitNow()
+
+                // Changes to underlying DataProviders will update the gating result.
+                // liveData.removeObserver(this)
               } else {
                 (activity as StopStatePlayingSessionWithSavedProgressListener)
                   .deleteCurrentProgressAndStopSession(isCompletion = true)
               }
-              // Changes to underlying DataProviders will update the gating result.
-              liveData.removeObserver(this)
             }
           }
         }

app/src/main/java/org/oppia/android/app/player/state/testing/StateFragmentTestActivityPresenter.kt

@@ -3,21 +3,16 @@ package org.oppia.android.app.player.state.testing
 import android.widget.Button
 import androidx.appcompat.app.AppCompatActivity
 import androidx.databinding.DataBindingUtil
-import androidx.lifecycle.Observer
 import org.oppia.android.R
 import org.oppia.android.app.activity.ActivityScope
 import org.oppia.android.app.model.ProfileId
 import org.oppia.android.app.player.exploration.HintsAndSolutionExplorationManagerFragment
 import org.oppia.android.app.player.exploration.TAG_HINTS_AND_SOLUTION_EXPLORATION_MANAGER
 import org.oppia.android.app.player.state.StateFragment
-import org.oppia.android.app.player.stopplaying.StopStatePlayingSessionWithSavedProgressListener
-import org.oppia.android.app.survey.SurveyWelcomeDialogFragment
-import org.oppia.android.app.survey.TAG_SURVEY_WELCOME_DIALOG
 import org.oppia.android.app.viewmodel.ViewModelProvider
 import org.oppia.android.databinding.StateFragmentTestActivityBinding
 import org.oppia.android.domain.exploration.ExplorationDataController
 import org.oppia.android.domain.oppialogger.OppiaLogger
-import org.oppia.android.domain.survey.SurveyGatingController
 import org.oppia.android.domain.topic.TEST_EXPLORATION_ID_2
 import org.oppia.android.domain.topic.TEST_STORY_ID_0
 import org.oppia.android.domain.topic.TEST_TOPIC_ID_0
@@ -32,9 +27,8 @@ private const val TEST_ACTIVITY_TAG = "TestActivity"
 class StateFragmentTestActivityPresenter @Inject constructor(
   private val activity: AppCompatActivity,
   private val explorationDataController: ExplorationDataController,
-  private val surveyGatingController: SurveyGatingController,
   private val oppiaLogger: OppiaLogger,
-  private val viewModelProvider: ViewModelProvider<StateFragmentTestViewModel>,
+  private val viewModelProvider: ViewModelProvider<StateFragmentTestViewModel>
 ) {
 
   private var profileId: Int = 1
@@ -103,7 +97,7 @@ class StateFragmentTestActivityPresenter @Inject constructor(
     }
     startPlayingProvider.toLiveData().observe(
       activity,
-      Observer<AsyncResult<Any?>> { result ->
+      { result ->
         when (result) {
           is AsyncResult.Pending -> oppiaLogger.d(TEST_ACTIVITY_TAG, "Loading exploration")
           is AsyncResult.Failure ->
@@ -117,46 +111,6 @@ class StateFragmentTestActivityPresenter @Inject constructor(
     )
   }
 
-  private fun maybeShowSurveyDialog(profileId: ProfileId, topicId: String) {
-    val liveData = surveyGatingController.maybeShowSurvey(profileId, topicId).toLiveData()
-    liveData.observe(
-      activity,
-      object : Observer<AsyncResult<Boolean>> {
-        override fun onChanged(gatingResult: AsyncResult<Boolean>?) {
-          when (gatingResult) {
-            is AsyncResult.Pending -> {
-              oppiaLogger.d("StateFragmentTest", "A gating decision is pending")
-            }
-            is AsyncResult.Failure -> {
-              oppiaLogger.e(
-                "StateFragmentTest",
-                "Failed to retrieve gating decision",
-                gatingResult.error
-              )
-              (activity as StopStatePlayingSessionWithSavedProgressListener)
-                .deleteCurrentProgressAndStopSession(isCompletion = true)
-            }
-            is AsyncResult.Success -> {
-              if (gatingResult.value) {
-                val surveyPopup = SurveyWelcomeDialogFragment.newInstance(
-                  profileId,
-                  topicId,
-                  explorationId,
-                  listOf()
-                )
-                activity.supportFragmentManager.beginTransaction()
-                  .add(R.id.state_fragment_placeholder, surveyPopup, TAG_SURVEY_WELCOME_DIALOG)
-                  .commitNow()
-              }
-              // Changes to underlying DataProviders will update the gating result.
-              liveData.removeObserver(this)
-            }
-          }
-        }
-      }
-    )
-  }
-
   /**
    * Initializes fragments that depend on ephemeral state (which isn't valid to do until the play
    * session is fully started).
@@ -187,9 +141,6 @@ class StateFragmentTestActivityPresenter @Inject constructor(
   private fun finishExploration(isCompletion: Boolean) {
     explorationDataController.stopPlayingExploration(isCompletion)
 
-    val userProfileId = ProfileId.newBuilder().setInternalId(profileId).build()
-    maybeShowSurveyDialog(userProfileId, topicId)
-
     getStateFragment()?.let { fragment ->
       activity.supportFragmentManager.beginTransaction().remove(fragment).commitNow()
     }

app/src/sharedTest/java/org/oppia/android/app/player/exploration/ExplorationActivityTest.kt

@@ -112,6 +112,7 @@ import org.oppia.android.domain.oppialogger.analytics.CpuPerformanceSnapshotterM
 import org.oppia.android.domain.oppialogger.logscheduler.MetricLogSchedulerModule
 import org.oppia.android.domain.oppialogger.loguploader.LogReportWorkerModule
 import org.oppia.android.domain.platformparameter.PlatformParameterSingletonModule
+import org.oppia.android.domain.profile.ProfileManagementController
 import org.oppia.android.domain.question.QuestionModule
 import org.oppia.android.domain.spotlight.SpotlightStateController
 import org.oppia.android.domain.topic.FRACTIONS_EXPLORATION_ID_0
@@ -222,20 +223,25 @@ class ExplorationActivityTest {
   @Inject
   lateinit var fakeAccessibilityService: FakeAccessibilityService
 
+  @Inject
+  lateinit var profileManagementController: ProfileManagementController
+
   private val internalProfileId: Int = 0
 
+  private val afternoonUtcTimestampMillis = 1556101812000
+
   @Before
   fun setUp() {
     Intents.init()
     setUpTestApplicationComponent()
     testCoroutineDispatchers.registerIdlingResource()
     profileTestHelper.initializeProfiles()
     fakeOppiaClock.setFakeTimeMode(FakeOppiaClock.FakeTimeMode.MODE_UPTIME_MILLIS)
+    testCoroutineDispatchers.unregisterIdlingResource()
   }
 
   @After
   fun tearDown() {
-    testCoroutineDispatchers.unregisterIdlingResource()
     Intents.release()
   }
 

app/src/sharedTest/java/org/oppia/android/app/player/state/StateFragmentTest.kt

@@ -204,21 +204,35 @@ import javax.inject.Singleton
 // SameParameterValue: tests should have specific context included/excluded for readability.
 @Suppress("FunctionName", "SameParameterValue")
 class StateFragmentTest {
-  @get:Rule val initializeDefaultLocaleRule = InitializeDefaultLocaleRule()
-  @get:Rule val oppiaTestRule = OppiaTestRule()
-
-  @Inject lateinit var profileTestHelper: ProfileTestHelper
-  @Inject lateinit var context: Context
-  @Inject lateinit var testCoroutineDispatchers: TestCoroutineDispatchers
-  @Inject lateinit var editTextInputAction: EditTextInputAction
-  @field:[Inject BackgroundDispatcher] lateinit var backgroundDispatcher: CoroutineDispatcher
-  @Inject lateinit var explorationCheckpointTestHelper: ExplorationCheckpointTestHelper
-  @Inject lateinit var translationController: TranslationController
-  @Inject lateinit var monitorFactory: DataProviderTestMonitor.Factory
-  @Inject lateinit var testGlideImageLoader: TestGlideImageLoader
-  @Inject lateinit var profileManagementController: ProfileManagementController
-  @Inject lateinit var fakeAnalyticsEventLogger: FakeAnalyticsEventLogger
-  @Inject lateinit var oppiaClock: FakeOppiaClock
+  @get:Rule
+  val initializeDefaultLocaleRule = InitializeDefaultLocaleRule()
+  @get:Rule
+  val oppiaTestRule = OppiaTestRule()
+
+  @Inject
+  lateinit var profileTestHelper: ProfileTestHelper
+  @Inject
+  lateinit var context: Context
+  @Inject
+  lateinit var testCoroutineDispatchers: TestCoroutineDispatchers
+  @Inject
+  lateinit var editTextInputAction: EditTextInputAction
+  @field:[Inject BackgroundDispatcher]
+  lateinit var backgroundDispatcher: CoroutineDispatcher
+  @Inject
+  lateinit var explorationCheckpointTestHelper: ExplorationCheckpointTestHelper
+  @Inject
+  lateinit var translationController: TranslationController
+  @Inject
+  lateinit var monitorFactory: DataProviderTestMonitor.Factory
+  @Inject
+  lateinit var testGlideImageLoader: TestGlideImageLoader
+  @Inject
+  lateinit var profileManagementController: ProfileManagementController
+  @Inject
+  lateinit var fakeAnalyticsEventLogger: FakeAnalyticsEventLogger
+  @Inject
+  lateinit var oppiaClock: FakeOppiaClock
 
   private val profileId = ProfileId.newBuilder().apply { internalId = 1 }.build()
 
@@ -4373,6 +4387,40 @@ class StateFragmentTest {
     }
   }
 
+  @Test
+  @RunOn(TestPlatform.ESPRESSO) // TODO(#1612): Enable for Robolectric.
+  fun testFinishChapter_allGatingConditionsMet_surveyDismissed_popupDoesNotShowAgain() {
+    setUpTestWithSurveyFeatureOn()
+    oppiaClock.setFakeTimeMode(FakeOppiaClock.FakeTimeMode.MODE_FIXED_FAKE_TIME)
+    oppiaClock.setCurrentTimeMs(EVENING_UTC_TIMESTAMP_MILLIS)
+
+    launchForExploration(TEST_EXPLORATION_ID_2, shouldSavePartialProgress = true).use {
+      startPlayingExploration()
+
+      playThroughPrototypeExploration()
+
+      oppiaClock.setCurrentTimeMs(EVENING_UTC_TIMESTAMP_MILLIS + SESSION_LENGTH_LONG)
+
+      clickReturnToTopicButton()
+
+      onView(withId(R.id.maybe_later_button))
+        .perform(click())
+      testCoroutineDispatchers.runCurrent()
+
+      // Check that the fragment is removed.
+      // When the survey popup is shown, the lastShownDateProvider is updated with current time,
+      // consequently updating the combined gating data provider. Recomputation of the gating result
+      // should not re-trigger the survey.
+      onView(withId(R.id.play_test_exploration_button)).check(
+        matches(
+          withEffectiveVisibility(
+            VISIBLE
+          )
+        )
+      )
+    }
+  }
+
   @Test
   @RunOn(TestPlatform.ESPRESSO) // TODO(#1612): Enable for Robolectric.
   fun testFinishChapter_surveyFeatureOff_allGatingConditionsMet_noSurveyPopup() {
@@ -4410,6 +4458,43 @@ class StateFragmentTest {
     }
   }
 
+  @Test
+  @RunOn(TestPlatform.ESPRESSO) // TODO(#1612): Enable for Robolectric.
+  fun testFinishChapter_updateGatingProvider_surveyGatingCriteriaMetEarlier_doesntUpdateUI() {
+    setUpTestWithSurveyFeatureOn()
+    oppiaClock.setFakeTimeMode(FakeOppiaClock.FakeTimeMode.MODE_FIXED_FAKE_TIME)
+    oppiaClock.setCurrentTimeMs(AFTERNOON_UTC_TIMESTAMP_MILLIS)
+
+    launchForExploration(TEST_EXPLORATION_ID_2, shouldSavePartialProgress = true).use {
+      startPlayingExploration()
+
+      playThroughPrototypeExploration()
+
+      oppiaClock.setCurrentTimeMs(AFTERNOON_UTC_TIMESTAMP_MILLIS + SESSION_LENGTH_LONG)
+
+      clickReturnToTopicButton()
+
+      onView(withText(R.string.survey_onboarding_title_text))
+        .inRoot(isDialog())
+        .check(matches(isDisplayed()))
+      onView(withText(R.string.survey_onboarding_message_text))
+        .inRoot(isDialog())
+        .check(matches(isDisplayed()))
+
+      // Update the SurveyLastShownTimestamp to trigger an update in the data provider and notify
+      // subscribers of an update.
+      profileManagementController.updateSurveyLastShownTimestamp(profileId)
+      testCoroutineDispatchers.runCurrent()
+
+      onView(withText(R.string.survey_onboarding_title_text))
+        .inRoot(isDialog())
+        .check(matches(isDisplayed()))
+      onView(withText(R.string.survey_onboarding_message_text))
+        .inRoot(isDialog())
+        .check(matches(isDisplayed()))
+    }
+  }
+
   private fun addShadowMediaPlayerException(dataSource: Any, exception: Exception) {
     val classLoader = StateFragmentTest::class.java.classLoader!!
     val shadowMediaPlayerClass = classLoader.loadClass("org.robolectric.shadows.ShadowMediaPlayer")