Fix #2476, #3312 and #3703: Move hint handler to domain layer (#3659)

* moved hint handler to domain layer * fixed app layer espresso tests * fix app layer robolectric tests * fixed domain layer tests * Added annotations to test exemptions * proto lint fix * fixed hint handler for training sessions * nit and removed test excemptions * added hint tests for config change * fixed test file exemptions * fixed failing test * made HintHandler injectable * fixed ktlint error * Added tests for hint handler * nits * fixed failing build * fixed failing build * fix build * fixed imports * nits and improved testing * updated exploration.proto * removed progress controller from kdoc exemptions * fix failing test * moved timer to domain * fixed build and nits * nit * added listener back to test exemptions * nit fixes and added more tests * lint fix * First round: make HintHandler independent. This also brings HintHandler into an interface + factory pattern. This isn't the final design since I think we can largely simplify the way hints work; that'll be my next pass. This breaks questions & HintHandlerTest; those will require further work later. * Simplify HelpIndex in proto. Move HelpIndex to PendingState to avoid the entire domain case of handling CompletedState. * Simplify hints & solutions. This removes the per-Hint/Solution tracking & completely leans on HelpIndex for proper hints & solution tracking both in the domain & UI layers. Fixes a bunch of other stuff, too, including QuestionAssessmentProgressController tests. * Clean up dead code paths & improve handler API. * fixed test modules and lint * renamed HintHandlerTest to HintHandlerImplTest * Add tests for HintHandler. This introduces some new explorations for making testing HintHandlerImpl easier. * Add remaining tests/exemptions for new files. * Lint fixes. * Post-merge fixes (including lint fixes). * Post-merge maven_install fix. * Revert "Merge branch 'develop' into move-hint-handler-to-domain" This reverts commit e2fea90, reversing changes made to 6659858. * Post-merge Gradle-discovered fixes. * Revert "Revert "Merge branch 'develop' into move-hint-handler-to-domain"" This reverts commit b1622c0. * Additional post-merge fixes. * Address first batch of review comments. (Clarified some proto fields & remove trailing comma). * Fix testing module tests & remove unnecessary changes. * Simplify & fix more reviewer comments. This simplifies some data pipelining in the UI, and improves HintHandler documentation in addition to fixing some more reviewer comments. * Improve documentation to address review comment. * Rename new module to prod module. This simplifies the changes & approvals needed in #3705. * Rename proto field (to address review comment). Also, fix broken reference error accidentally introduced in an earlier commit. * Kotlin lint fixes. Co-authored-by: yashraj-01 <yashrajprime@gmail.com> Co-authored-by: Ben Henning <henning.benmax@gmail.com>
oppia · Aug 19, 2021 · d2372ae · d2372ae
1 parent da652a4
commit d2372ae
Showing 181 changed files with 5,378 additions and 1,464 deletions.
diff --git a/app/src/main/java/org/oppia/android/app/application/ApplicationComponent.kt b/app/src/main/java/org/oppia/android/app/application/ApplicationComponent.kt
@@ -7,7 +7,6 @@ import dagger.Component
 import org.oppia.android.app.activity.ActivityComponent
 import org.oppia.android.app.devoptions.DeveloperOptionsModule
 import org.oppia.android.app.devoptions.DeveloperOptionsStarterModule
-import org.oppia.android.app.player.state.hintsandsolution.HintsAndSolutionConfigModule
 import org.oppia.android.app.shim.IntentFactoryShimModule
 import org.oppia.android.app.shim.ViewBindingShimModule
 import org.oppia.android.app.topic.PracticeTabModule
@@ -24,6 +23,8 @@ import org.oppia.android.domain.classify.rules.numericinput.NumericInputRuleModu
 import org.oppia.android.domain.classify.rules.ratioinput.RatioInputModule
 import org.oppia.android.domain.classify.rules.textinput.TextInputRuleModule
 import org.oppia.android.domain.exploration.lightweightcheckpointing.ExplorationStorageModule
+import org.oppia.android.domain.hintsandsolution.HintsAndSolutionConfigModule
+import org.oppia.android.domain.hintsandsolution.HintsAndSolutionProdModule
 import org.oppia.android.domain.onboarding.ExpirationMetaDataRetrieverModule
 import org.oppia.android.domain.oppialogger.ApplicationStartupListener
 import org.oppia.android.domain.oppialogger.LogStorageModule
@@ -76,10 +77,11 @@ import javax.inject.Singleton
     ExpirationMetaDataRetrieverModule::class, RatioInputModule::class,
     UncaughtExceptionLoggerModule::class, ApplicationStartupListenerModule::class,
     LogUploadWorkerModule::class, WorkManagerConfigurationModule::class,
-    HintsAndSolutionConfigModule::class, FirebaseLogUploaderModule::class,
-    NetworkModule::class, PracticeTabModule::class, PlatformParameterModule::class,
-    ExplorationStorageModule::class, DeveloperOptionsStarterModule::class,
-    DeveloperOptionsModule::class, NetworkConnectionUtilDebugModule::class,
+    HintsAndSolutionConfigModule::class, HintsAndSolutionProdModule::class,
+    FirebaseLogUploaderModule::class, NetworkModule::class, PracticeTabModule::class,
+    PlatformParameterModule::class, ExplorationStorageModule::class,
+    DeveloperOptionsStarterModule::class, DeveloperOptionsModule::class,
+    NetworkConnectionUtilDebugModule::class,
     // TODO(#59): Remove this module once we completely migrate to Bazel from Gradle as we can then
     //  directly exclude debug files from the build and thus won't be requiring this module.
     NetworkConnectionDebugUtilModule::class

diff --git a/app/src/main/java/org/oppia/android/app/hintsandsolution/HintsAndSolutionDialogFragment.kt b/app/src/main/java/org/oppia/android/app/hintsandsolution/HintsAndSolutionDialogFragment.kt
@@ -7,12 +7,14 @@ import android.view.View
 import android.view.ViewGroup
 import org.oppia.android.R
 import org.oppia.android.app.fragment.InjectableDialogFragment
+import org.oppia.android.app.model.HelpIndex
 import org.oppia.android.app.model.State
+import org.oppia.android.util.extensions.getProto
+import org.oppia.android.util.extensions.putProto
 import javax.inject.Inject
 
 private const val CURRENT_EXPANDED_LIST_INDEX_SAVED_KEY =
   "HintsAndSolutionDialogFragment.current_expanded_list_index"
-private const val STATE_SAVED_KEY = "HintsAndSolutionDialogFragment.state"
 private const val HINT_INDEX_SAVED_KEY = "HintsAndSolutionDialogFragment.hint_index"
 private const val IS_HINT_REVEALED_SAVED_KEY = "HintsAndSolutionDialogFragment.is_hint_revealed"
 private const val SOLUTION_INDEX_SAVED_KEY = "HintsAndSolutionDialogFragment.solution_index"
@@ -28,8 +30,6 @@ class HintsAndSolutionDialogFragment :
   @Inject
   lateinit var hintsAndSolutionDialogFragmentPresenter: HintsAndSolutionDialogFragmentPresenter
 
-  private lateinit var state: State
-
   private var currentExpandedHintListIndex: Int? = null
 
   private var index: Int? = null
@@ -40,30 +40,29 @@ class HintsAndSolutionDialogFragment :
   companion object {
 
     internal const val ID_ARGUMENT_KEY = "HintsAndSolutionDialogFragment.id"
-    internal const val NEW_AVAILABLE_HINT_INDEX_ARGUMENT_KEY =
-      "HintsAndSolutionDialogFragment.new_available_hint_index"
-    internal const val ALL_HINTS_EXHAUSTED_ARGUMENT_KEY =
-      "HintsAndSolutionDialogFragment.all_hints_exhausted"
+    internal const val STATE_KEY = "HintsAndSolutionDialogFragment.state"
+    internal const val HELP_INDEX_KEY = "HintsAndSolutionDialogFragment.help_index"
 
     /**
      * Creates a new instance of a DialogFragment to display hints and solution
+     *
      * @param id Used in ExplorationController/QuestionAssessmentProgressController to get current state data.
-     * @param newAvailableHintIndex Index of new available hint.
-     * @param allHintsExhausted Boolean set to true when all hints are exhausted.
+     * @param state the [State] being viewed by the learner
+     * @param helpIndex the [HelpIndex] corresponding to the current hints/solution configuration
      * @return [HintsAndSolutionDialogFragment]: DialogFragment
      */
     fun newInstance(
       id: String,
-      newAvailableHintIndex: Int,
-      allHintsExhausted: Boolean
+      state: State,
+      helpIndex: HelpIndex
     ): HintsAndSolutionDialogFragment {
-      val hintsAndSolutionFrag = HintsAndSolutionDialogFragment()
-      val args = Bundle()
-      args.putString(ID_ARGUMENT_KEY, id)
-      args.putInt(NEW_AVAILABLE_HINT_INDEX_ARGUMENT_KEY, newAvailableHintIndex)
-      args.putBoolean(ALL_HINTS_EXHAUSTED_ARGUMENT_KEY, allHintsExhausted)
-      hintsAndSolutionFrag.arguments = args
-      return hintsAndSolutionFrag
+      return HintsAndSolutionDialogFragment().apply {
+        arguments = Bundle().apply {
+          putString(ID_ARGUMENT_KEY, id)
+          putProto(STATE_KEY, state)
+          putProto(HELP_INDEX_KEY, helpIndex)
+        }
+      }
     }
   }
 
@@ -88,7 +87,6 @@ class HintsAndSolutionDialogFragment :
       if (currentExpandedHintListIndex == -1) {
         currentExpandedHintListIndex = null
       }
-      state = State.parseFrom(savedInstanceState.getByteArray(STATE_SAVED_KEY)!!)
       index = savedInstanceState.getInt(HINT_INDEX_SAVED_KEY, -1)
       if (index == -1) index = null
       isHintRevealed = savedInstanceState.getBoolean(IS_HINT_REVEALED_SAVED_KEY, false)
@@ -104,23 +102,17 @@ class HintsAndSolutionDialogFragment :
       checkNotNull(
         args.getString(ID_ARGUMENT_KEY)
       ) { "Expected id to be passed to HintsAndSolutionDialogFragment" }
-    val newAvailableHintIndex =
-      checkNotNull(
-        args.getInt(NEW_AVAILABLE_HINT_INDEX_ARGUMENT_KEY)
-      ) { "Expected hint index to be passed to HintsAndSolutionDialogFragment" }
-    val allHintsExhausted =
-      checkNotNull(
-        args.getBoolean(ALL_HINTS_EXHAUSTED_ARGUMENT_KEY)
-      ) { "Expected if hints exhausted to be passed to HintsAndSolutionDialogFragment" }
+
+    val state = args.getProto(STATE_KEY, State.getDefaultInstance())
+    val helpIndex = args.getProto(HELP_INDEX_KEY, HelpIndex.getDefaultInstance())
 
     return hintsAndSolutionDialogFragmentPresenter.handleCreateView(
       inflater,
       container,
       state,
+      helpIndex,
       id,
       currentExpandedHintListIndex,
-      newAvailableHintIndex,
-      allHintsExhausted,
       this as ExpandedHintListIndexListener,
       index,
       isHintRevealed,
@@ -151,7 +143,6 @@ class HintsAndSolutionDialogFragment :
     if (isSolutionRevealed != null) {
       outState.putBoolean(IS_SOLUTION_REVEALED_SAVED_KEY, isSolutionRevealed!!)
     }
-    outState.putByteArray(STATE_SAVED_KEY, state.toByteArray())
   }
 
   override fun onExpandListIconClicked(index: Int?) {
@@ -177,8 +168,4 @@ class HintsAndSolutionDialogFragment :
       isSolutionRevealed
     )
   }
-
-  fun loadState(state: State) {
-    this.state = state
-  }
 }
diff --git a/...in/java/org/oppia/android/app/hintsandsolution/HintsAndSolutionDialogFragmentPresenter.kt b/...in/java/org/oppia/android/app/hintsandsolution/HintsAndSolutionDialogFragmentPresenter.kt
@@ -6,6 +6,11 @@ import android.view.ViewGroup
 import androidx.fragment.app.Fragment
 import org.oppia.android.R
 import org.oppia.android.app.fragment.FragmentScope
+import org.oppia.android.app.model.HelpIndex
+import org.oppia.android.app.model.HelpIndex.IndexTypeCase.EVERYTHING_REVEALED
+import org.oppia.android.app.model.HelpIndex.IndexTypeCase.LATEST_REVEALED_HINT_INDEX
+import org.oppia.android.app.model.HelpIndex.IndexTypeCase.NEXT_AVAILABLE_HINT_INDEX
+import org.oppia.android.app.model.HelpIndex.IndexTypeCase.SHOW_SOLUTION
 import org.oppia.android.app.model.State
 import org.oppia.android.app.recyclerview.BindableAdapter
 import org.oppia.android.app.viewmodel.ViewModelProvider
@@ -16,6 +21,7 @@ import org.oppia.android.databinding.SolutionSummaryBinding
 import org.oppia.android.util.gcsresource.DefaultResourceBucketName
 import org.oppia.android.util.parser.html.ExplorationHtmlParserEntityType
 import org.oppia.android.util.parser.html.HtmlParser
+import java.lang.IllegalStateException
 import java.util.Locale
 import javax.inject.Inject
 
@@ -39,6 +45,7 @@ class HintsAndSolutionDialogFragmentPresenter @Inject constructor(
   private lateinit var expandedHintListIndexListener: ExpandedHintListIndexListener
   private lateinit var binding: HintsAndSolutionFragmentBinding
   private lateinit var state: State
+  private lateinit var helpIndex: HelpIndex
   private lateinit var itemList: List<HintsAndSolutionItemViewModel>
   private lateinit var bindingAdapter: BindableAdapter<HintsAndSolutionItemViewModel>
 
@@ -54,17 +61,15 @@ class HintsAndSolutionDialogFragmentPresenter @Inject constructor(
     inflater: LayoutInflater,
     container: ViewGroup?,
     state: State,
+    helpIndex: HelpIndex,
     id: String?,
     currentExpandedHintListIndex: Int?,
-    newAvailableHintIndex: Int,
-    allHintsExhausted: Boolean,
     expandedHintListIndexListener: ExpandedHintListIndexListener,
     index: Int?,
     isHintRevealed: Boolean?,
     solutionIndex: Int?,
     isSolutionRevealed: Boolean?
   ): View? {
-
     binding =
       HintsAndSolutionFragmentBinding.inflate(inflater, container, /* attachToRoot= */ false)
     this.currentExpandedHintListIndex = currentExpandedHintListIndex
@@ -86,25 +91,52 @@ class HintsAndSolutionDialogFragmentPresenter @Inject constructor(
     }
 
     this.state = state
-    // The newAvailableHintIndex received here is coming from state player but in this implementation
-    // hints/solutions are shown on every even index and on every odd index we show a divider.
-    // Therefore multiplying the original index by 2.
+    this.helpIndex = helpIndex
+    // The newAvailableHintIndex received here is coming from state player but in this
+    // implementation hints/solutions are shown on every even index and on every odd index we show a
+    // divider. The relative index therefore needs to be doubled to account for the divider.
+    val newAvailableHintIndex = computeNewAvailableHintIndex(helpIndex)
     viewModel.newAvailableHintIndex.set(
       newAvailableHintIndex * RECYCLERVIEW_INDEX_CORRECTION_MULTIPLIER
     )
-    viewModel.allHintsExhausted.set(allHintsExhausted)
+    viewModel.allHintsExhausted.set(computeWhetherAllHintsAreExhausted(helpIndex))
     viewModel.explorationId.set(id)
 
     loadHintsAndSolution(state)
 
     return binding.root
   }
 
+  private fun computeNewAvailableHintIndex(helpIndex: HelpIndex): Int {
+    return when (helpIndex.indexTypeCase) {
+      NEXT_AVAILABLE_HINT_INDEX -> helpIndex.nextAvailableHintIndex
+      LATEST_REVEALED_HINT_INDEX -> helpIndex.latestRevealedHintIndex
+      SHOW_SOLUTION, EVERYTHING_REVEALED -> {
+        // 1 is subtracted from the hint count because hints are indexed from 0.
+        state.interaction.hintCount - 1
+      }
+      else ->
+        throw IllegalStateException(
+          "Encountered invalid type for showing hints: ${helpIndex.indexTypeCase}"
+        )
+    }
+  }
+
+  private fun computeWhetherAllHintsAreExhausted(helpIndex: HelpIndex): Boolean {
+    return when (helpIndex.indexTypeCase) {
+      NEXT_AVAILABLE_HINT_INDEX, LATEST_REVEALED_HINT_INDEX -> false
+      SHOW_SOLUTION, EVERYTHING_REVEALED -> true
+      else ->
+        throw IllegalStateException(
+          "Encountered invalid type for showing hints: ${helpIndex.indexTypeCase}"
+        )
+    }
+  }
+
   private fun loadHintsAndSolution(state: State) {
     // Check if hints are available for this state.
-    if (state.interaction.hintList.size != 0) {
-      viewModel.setHintsList(state.interaction.hintList)
-      viewModel.setSolution(state.interaction.solution)
+    if (state.interaction.hintList.isNotEmpty()) {
+      viewModel.initialize(helpIndex, state.interaction.hintList, state.interaction.solution)
 
       itemList = viewModel.processHintList()
 
@@ -203,7 +235,6 @@ class HintsAndSolutionDialogFragmentPresenter @Inject constructor(
         hintsViewModel.isHintRevealed.set(true)
         expandedHintListIndexListener.onRevealHintClicked(position, /* isHintRevealed= */ true)
         (fragment.requireActivity() as? RevealHintListener)?.revealHint(
-          saveUserChoice = true,
           hintIndex = position / RECYCLERVIEW_INDEX_CORRECTION_MULTIPLIER
         )
         val previousIndex: Int? = currentExpandedHintListIndex

diff --git a/app/src/main/java/org/oppia/android/app/hintsandsolution/HintsViewModel.kt b/app/src/main/java/org/oppia/android/app/hintsandsolution/HintsViewModel.kt
@@ -3,8 +3,11 @@ package org.oppia.android.app.hintsandsolution
 import androidx.databinding.ObservableField
 import androidx.lifecycle.ViewModel
 import org.oppia.android.app.fragment.FragmentScope
+import org.oppia.android.app.model.HelpIndex
 import org.oppia.android.app.model.Hint
 import org.oppia.android.app.model.Solution
+import org.oppia.android.domain.hintsandsolution.isHintRevealed
+import org.oppia.android.domain.hintsandsolution.isSolutionRevealed
 import javax.inject.Inject
 
 /**
@@ -29,21 +32,21 @@ class HintsViewModel @Inject constructor() : HintsAndSolutionItemViewModel() {
 
   private lateinit var hintList: List<Hint>
   private lateinit var solution: Solution
+  private lateinit var helpIndex: HelpIndex
   val itemList: MutableList<HintsAndSolutionItemViewModel> = ArrayList()
 
-  fun setHintsList(hintList: List<Hint>) {
+  /** Initializes the view model to display hints and a solution. */
+  fun initialize(helpIndex: HelpIndex, hintList: List<Hint>, solution: Solution) {
+    this.helpIndex = helpIndex
     this.hintList = hintList
-  }
-
-  fun setSolution(solution: Solution) {
     this.solution = solution
   }
 
   fun processHintList(): List<HintsAndSolutionItemViewModel> {
     itemList.clear()
     for (index in hintList.indices) {
       if (itemList.isEmpty()) {
-        addHintToList(hintList[index])
+        addHintToList(index, hintList[index])
       } else if (itemList.size > 1) {
         val isLastHintRevealed =
           (itemList[itemList.size - RECYCLERVIEW_INDEX_CORRECTION_MULTIPLIER] as HintsViewModel)
@@ -53,7 +56,7 @@ class HintsViewModel @Inject constructor() : HintsAndSolutionItemViewModel() {
         if (isLastHintRevealed &&
           index <= availableHintIndex / RECYCLERVIEW_INDEX_CORRECTION_MULTIPLIER
         ) {
-          addHintToList(hintList[index])
+          addHintToList(index, hintList[index])
         } else {
           break
         }
@@ -76,11 +79,11 @@ class HintsViewModel @Inject constructor() : HintsAndSolutionItemViewModel() {
     return itemList
   }
 
-  private fun addHintToList(hint: Hint) {
+  private fun addHintToList(hintIndex: Int, hint: Hint) {
     val hintsViewModel = HintsViewModel()
     hintsViewModel.title.set(hint.hintContent.contentId)
     hintsViewModel.hintsAndSolutionSummary.set(hint.hintContent.html)
-    hintsViewModel.isHintRevealed.set(hint.hintIsRevealed)
+    hintsViewModel.isHintRevealed.set(helpIndex.isHintRevealed(hintIndex, hintList))
     itemList.add(hintsViewModel)
     addDividerItem()
   }
@@ -94,7 +97,7 @@ class HintsViewModel @Inject constructor() : HintsAndSolutionItemViewModel() {
     solutionViewModel.wholeNumber.set(solution.correctAnswer.wholeNumber)
     solutionViewModel.isNegative.set(solution.correctAnswer.isNegative)
     solutionViewModel.solutionSummary.set(solution.explanation.html)
-    solutionViewModel.isSolutionRevealed.set(solution.solutionIsRevealed)
+    solutionViewModel.isSolutionRevealed.set(helpIndex.isSolutionRevealed())
     itemList.add(solutionViewModel)
     addDividerItem()
   }

diff --git a/app/src/main/java/org/oppia/android/app/hintsandsolution/RevealHintListener.kt b/app/src/main/java/org/oppia/android/app/hintsandsolution/RevealHintListener.kt
@@ -1,10 +1,10 @@
 package org.oppia.android.app.hintsandsolution
 
-/** Interface to check the preference regarding alert for [HintsAndSolutionDialogFragment]. */
+/** Callback listener for when the user wishes to reveal a hint. */
 interface RevealHintListener {
   /**
-   * If saveUserChoice is true, show solution and save preference do not show dialog again.
-   * If saveUserChoice is false, show solution and do not save preference and show this dialog next time too.
+   * Called when the user indicates they want to reveal the hint corresponding to the specified
+   * index.
    */
-  fun revealHint(saveUserChoice: Boolean, hintIndex: Int)
+  fun revealHint(hintIndex: Int)
 }
diff --git a/app/src/main/java/org/oppia/android/app/player/exploration/ExplorationActivity.kt b/app/src/main/java/org/oppia/android/app/player/exploration/ExplorationActivity.kt
@@ -11,6 +11,7 @@ import org.oppia.android.app.hintsandsolution.HintsAndSolutionDialogFragment
 import org.oppia.android.app.hintsandsolution.HintsAndSolutionListener
 import org.oppia.android.app.hintsandsolution.RevealHintListener
 import org.oppia.android.app.hintsandsolution.RevealSolutionInterface
+import org.oppia.android.app.model.HelpIndex
 import org.oppia.android.app.model.ReadingTextSize
 import org.oppia.android.app.model.State
 import org.oppia.android.app.player.audio.AudioButtonListener
@@ -143,8 +144,8 @@ class ExplorationActivity :
     explorationActivityPresenter.onKeyboardAction(actionCode)
   }
 
-  override fun revealHint(saveUserChoice: Boolean, hintIndex: Int) {
-    explorationActivityPresenter.revealHint(saveUserChoice, hintIndex)
+  override fun revealHint(hintIndex: Int) {
+    explorationActivityPresenter.revealHint(hintIndex)
   }
 
   override fun revealSolution() = explorationActivityPresenter.revealSolution()
@@ -157,16 +158,14 @@ class ExplorationActivity :
 
   override fun routeToHintsAndSolution(
     explorationId: String,
-    newAvailableHintIndex: Int,
-    allHintsExhausted: Boolean
+    helpIndex: HelpIndex
   ) {
     if (getHintsAndSolution() == null) {
       val hintsAndSolutionDialogFragment = HintsAndSolutionDialogFragment.newInstance(
         explorationId,
-        newAvailableHintIndex,
-        allHintsExhausted
+        state,
+        helpIndex
       )
-      hintsAndSolutionDialogFragment.loadState(state)
       hintsAndSolutionDialogFragment.showNow(supportFragmentManager, TAG_HINTS_AND_SOLUTION_DIALOG)
     }
   }