diff --git a/domain/src/main/java/org/oppia/domain/classify/AnswerClassificationController.kt b/domain/src/main/java/org/oppia/domain/classify/AnswerClassificationController.kt index e6dbfad25bd..8389fbdb154 100644 --- a/domain/src/main/java/org/oppia/domain/classify/AnswerClassificationController.kt +++ b/domain/src/main/java/org/oppia/domain/classify/AnswerClassificationController.kt @@ -3,24 +3,59 @@ package org.oppia.domain.classify import org.oppia.app.model.Interaction import org.oppia.app.model.InteractionObject import org.oppia.app.model.Outcome +import org.oppia.app.model.State import javax.inject.Inject // TODO(#59): Restrict the visibility of this class to only other controllers. /** * Controller responsible for classifying user answers to a specific outcome based on Oppia's interaction rule engine. * This controller is not meant to be interacted with directly by the UI. Instead, UIs wanting to submit answers should - * do so via various progress controllers, like [StoryProgressController]. + * do so via various progress controllers, like [org.oppia.domain.topic.StoryProgressController]. * * This controller should only be interacted with via background threads. */ class AnswerClassificationController @Inject constructor() { + // TODO(#114): Add support for classifying answers based on an actual exploration. Also, classify() should take an + // Interaction, not a State. + /** * Classifies the specified answer in the context of the specified [Interaction] and returns the [Outcome] that best * matches the learner's answer. */ - internal fun classify(interaction: Interaction, answer: InteractionObject): Outcome { - // Assume only the default outcome is returned currently since this stubbed implementation is not actually used by - // downstream stubbed progress controllers. - return interaction.defaultOutcome + internal fun classify(currentState: State, answer: InteractionObject): Outcome { + return when (currentState.name) { + "Welcome!" -> simulateMultipleChoiceForWelcomeState(currentState, answer) + "What language" -> simulateTextInputForWhatLanguageState(currentState, answer) + "Numeric input" -> simulateNumericInputForNumericInputState(currentState, answer) + "Things you can do" -> currentState.interaction.defaultOutcome + else -> throw Exception("Cannot submit answer to unexpected state: ${currentState.name}.") + } + } + + private fun simulateMultipleChoiceForWelcomeState(currentState: State, answer: InteractionObject): Outcome { + return when { + answer.objectTypeCase != InteractionObject.ObjectTypeCase.NON_NEGATIVE_INT -> + throw Exception("Expected int answer, not $answer.") + answer.nonNegativeInt == 0 -> currentState.interaction.answerGroupsList.first().outcome + else -> currentState.interaction.defaultOutcome + } + } + + private fun simulateTextInputForWhatLanguageState(currentState: State, answer: InteractionObject): Outcome { + return when { + answer.objectTypeCase != InteractionObject.ObjectTypeCase.NORMALIZED_STRING -> + throw Exception("Expected string answer, not $answer.") + answer.normalizedString.toLowerCase() == "finnish" -> currentState.interaction.getAnswerGroups(7).outcome + else -> currentState.interaction.defaultOutcome + } + } + + private fun simulateNumericInputForNumericInputState(currentState: State, answer: InteractionObject): Outcome { + return when { + answer.objectTypeCase != InteractionObject.ObjectTypeCase.NON_NEGATIVE_INT -> + throw Exception("Expected int answer, not $answer.") + answer.nonNegativeInt == 121 -> currentState.interaction.answerGroupsList.first().outcome + else -> currentState.interaction.defaultOutcome + } } } diff --git a/domain/src/main/java/org/oppia/domain/exploration/ExplorationDataController.kt b/domain/src/main/java/org/oppia/domain/exploration/ExplorationDataController.kt index a867bc052ce..d0a53ef356d 100644 --- a/domain/src/main/java/org/oppia/domain/exploration/ExplorationDataController.kt +++ b/domain/src/main/java/org/oppia/domain/exploration/ExplorationDataController.kt @@ -7,6 +7,8 @@ import org.oppia.util.data.DataProviders import javax.inject.Inject private const val EXPLORATION_DATA_PROVIDER_ID = "ExplorationDataProvider" +private const val START_PLAYING_EXPLORATION_RESULT_DATA_PROVIDER_ID = "StartPlayingExplorationResultDataProvider" +private const val STOP_PLAYING_EXPLORATION_RESULT_DATA_PROVIDER_ID = "StopPlayingExplorationResultDataProvider" /** * Controller for loading explorations by ID, or beginning to play an exploration. @@ -34,17 +36,26 @@ class ExplorationDataController @Inject constructor( * * This must be called only if no active exploration is being played. The previous exploration must have first been * stopped using [stopPlayingExploration] otherwise an exception will be thrown. + * + * @return a one-time [LiveData] to observe whether initiating the play request succeeded. The exploration may still + * fail to load, but this provides early-failure detection. */ - fun startPlayingExploration(explorationId: String) { - explorationProgressController.beginExploration(explorationId) + fun startPlayingExploration(explorationId: String): LiveData> { + val operation = explorationProgressController.beginExplorationAsync(explorationId) + val dataProvider = dataProviders.createDeferredDataProviderAsync( + START_PLAYING_EXPLORATION_RESULT_DATA_PROVIDER_ID, operation) + return dataProviders.convertToLiveData(dataProvider) } /** * Finishes the most recent exploration started by [startPlayingExploration]. This method should only be called if an * active exploration is being played, otherwise an exception will be thrown. */ - fun stopPlayingExploration() { - explorationProgressController.finishExploration() + fun stopPlayingExploration(): LiveData> { + val operation = explorationProgressController.finishExplorationAsync() + val dataProvider = dataProviders.createDeferredDataProviderAsync( + STOP_PLAYING_EXPLORATION_RESULT_DATA_PROVIDER_ID, operation) + return dataProviders.convertToLiveData(dataProvider) } @Suppress("RedundantSuspendModifier") // DataProviders expects this function to be a suspend function. diff --git a/domain/src/main/java/org/oppia/domain/exploration/ExplorationProgressController.kt b/domain/src/main/java/org/oppia/domain/exploration/ExplorationProgressController.kt index 4e461f0e938..73a86207cff 100644 --- a/domain/src/main/java/org/oppia/domain/exploration/ExplorationProgressController.kt +++ b/domain/src/main/java/org/oppia/domain/exploration/ExplorationProgressController.kt @@ -1,7 +1,7 @@ package org.oppia.domain.exploration import androidx.lifecycle.LiveData -import androidx.lifecycle.MutableLiveData +import kotlinx.coroutines.Deferred import org.oppia.app.model.AnswerAndResponse import org.oppia.app.model.AnswerOutcome import org.oppia.app.model.CompletedState @@ -12,9 +12,11 @@ import org.oppia.app.model.Outcome import org.oppia.app.model.PendingState import org.oppia.app.model.State import org.oppia.app.model.SubtitledHtml +import org.oppia.domain.classify.AnswerClassificationController import org.oppia.util.data.AsyncDataSubscriptionManager import org.oppia.util.data.AsyncResult import org.oppia.util.data.DataProviders +import org.oppia.util.data.InMemoryBlockingCache import javax.inject.Inject import javax.inject.Singleton @@ -22,56 +24,60 @@ import javax.inject.Singleton private const val TERMINAL_INTERACTION_ID = "EndExploration" private const val CURRENT_STATE_DATA_PROVIDER_ID = "CurrentStateDataProvider" +private const val SUBMIT_ANSWER_RESULT_DATA_PROVIDER_ID = "SubmitAnswerResultDataProvider" +private const val MOVE_TO_NEXT_STATE_RESULT_DATA_PROVIDER_ID = "MoveToNextStateResultDataProvider" +private const val MOVE_TO_PREVIOUS_STATE_RESULT_DATA_PROVIDER_ID = "MoveToPreviousStateResultDataProvider" /** * Controller that tracks and reports the learner's ephemeral/non-persisted progress through an exploration. Note that * this controller only supports one active exploration at a time. * * The current exploration session is started via the exploration data controller. + * + * This class uses an [InMemoryBlockingCache] for performing synchronization. This means that although access to state + * is thread-safe, ordering is still arbitrary. Calling code should take care to ensure that uses of this class off the + * main thread do not specifically depend on ordering since observing state changes cannot be done reliably without a + * UI-bound [LiveData] observer. Note that the UI-bound observers are reliable since the in-memory cache guarantees that + * the ordering of operations to the controller have to be observed in equal order on the UI thread. */ @Singleton class ExplorationProgressController @Inject constructor( + inMemoryBlockingCacheFactory: InMemoryBlockingCache.Factory, private val dataProviders: DataProviders, private val asyncDataSubscriptionManager: AsyncDataSubscriptionManager, - private val explorationRetriever: ExplorationRetriever + private val explorationRetriever: ExplorationRetriever, + private val answerClassificationController: AnswerClassificationController ) { // TODO(#180): Add support for hints. // TODO(#179): Add support for parameters. // TODO(#181): Add support for solutions. // TODO(#182): Add support for refresher explorations. - // TODO(BenHenning): Make the controller's internal data structures thread-safe as necessary. - private lateinit var currentExplorationId: String - private lateinit var currentExploration: Exploration - private var playStage = PlayStage.NOT_PLAYING private val currentStateDataProvider = dataProviders.createInMemoryDataProviderAsync(CURRENT_STATE_DATA_PROVIDER_ID, this::retrieveCurrentStateAsync) - private val stateGraph: StateGraph by lazy { - StateGraph( - currentExploration.statesMap - ) - } - private val stateDeck: StateDeck by lazy { - StateDeck( - stateGraph.getState(currentExploration.initStateName) - ) - } + private val explorationProgress = inMemoryBlockingCacheFactory.create(ExplorationProgress()) /** Resets this controller to begin playing the specified [Exploration]. */ - internal fun beginExploration(explorationId: String) { - check(playStage == PlayStage.NOT_PLAYING) { - "Expected to finish previous exploration before starting a new one." - } + internal fun beginExplorationAsync(explorationId: String): Deferred<*> { + return explorationProgress.updateInPlaceIfPresentAsync { progress -> + check(progress.playStage == PlayStage.NOT_PLAYING) { + "Expected to finish previous exploration before starting a new one." + } - currentExplorationId = explorationId - advancePlayStageTo(PlayStage.START_LOAD_EXPLORATION) + progress.currentExplorationId = explorationId + progress.advancePlayStageTo(PlayStage.LOADING_EXPLORATION) + asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) + } } - internal fun finishExploration() { - check(playStage != PlayStage.NOT_PLAYING) { - "Cannot finish playing an exploration that hasn't yet been started" + /** Indicates that the current exploration being played is now completed. */ + internal fun finishExplorationAsync(): Deferred<*> { + return explorationProgress.updateInPlaceIfPresentAsync { progress -> + check(progress.playStage != PlayStage.NOT_PLAYING) { + "Cannot finish playing an exploration that hasn't yet been started" + } + progress.advancePlayStageTo(PlayStage.NOT_PLAYING) } - advancePlayStageTo(PlayStage.NOT_PLAYING) } /** @@ -93,39 +99,47 @@ class ExplorationProgressController @Inject constructor( * state to the next pending state using [moveToNextState]. * * This method cannot be called until an exploration has started and [getCurrentState] returns a non-pending result - * or an exception will be thrown. Calling code must also take care not to allow users to submit an answer while a - * previous answer is pending. That scenario will also result in an exception being thrown. + * or the result will fail. Calling code must also take care not to allow users to submit an answer while a previous + * answer is pending. That scenario will also result in a failed answer submission. * - * It's possible for the returned [LiveData] to finish after the [LiveData] from [getCurrentState] is updated. + * No assumptions should be made about the completion order of the returned [LiveData] vs. the [LiveData] from + * [getCurrentState]. Also note that the returned [LiveData] will only have a single value and not be reused after + * that point. */ fun submitAnswer(answer: InteractionObject): LiveData> { - check(playStage != PlayStage.NOT_PLAYING) { "Cannot submit an answer if an exploration is not being played." } - check(playStage != PlayStage.SUBMITTING_ANSWER) { "Cannot submit an answer while another answer is pending." } - check(playStage != PlayStage.START_LOAD_EXPLORATION && playStage != PlayStage.FINISH_LOAD_EXPLORATION) { - "Cannot submit an answer while the exploration is being loaded." - } + val operation = explorationProgress.updateInPlaceIfPresentAsync { progress -> + check(progress.playStage != PlayStage.NOT_PLAYING) { + "Cannot submit an answer if an exploration is not being played." + } + check(progress.playStage != PlayStage.SUBMITTING_ANSWER) { + "Cannot submit an answer while another answer is pending." + } + check(progress.playStage != PlayStage.LOADING_EXPLORATION) { + "Cannot submit an answer while the exploration is being loaded." + } - // Notify observers that the submitted answer is currently pending. - advancePlayStageTo(PlayStage.SUBMITTING_ANSWER) - asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) - - // TODO(#115): Perform answer classification on a background thread. - val topPendingState = stateDeck.getPendingTopState() - val outcome = simulateSubmitAnswer(topPendingState, answer) - val answerOutcome = stateGraph.computeAnswerOutcomeForResult(topPendingState, outcome) - stateDeck.submitAnswer(answer, answerOutcome.feedback) - if (answerOutcome.correctAnswer) { - check(answerOutcome.destinationCase == AnswerOutcome.DestinationCase.STATE_NAME) { - "Expecting a correct answer to only route to a new state, not to: ${answerOutcome.destinationCase}." + // Notify observers that the submitted answer is currently pending. + progress.advancePlayStageTo(PlayStage.SUBMITTING_ANSWER) + asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) + + val topPendingState = progress.stateDeck.getPendingTopState() + val outcome = answerClassificationController.classify(topPendingState, answer) + val answerOutcome = progress.stateGraph.computeAnswerOutcomeForResult(topPendingState, outcome) + progress.stateDeck.submitAnswer(answer, answerOutcome.feedback) + // Follow the answer's outcome to another part of the graph if it's different. + if (answerOutcome.destinationCase == AnswerOutcome.DestinationCase.STATE_NAME) { + progress.stateDeck.pushState(progress.stateGraph.getState(answerOutcome.stateName)) } - stateDeck.pushState(stateGraph.getState(answerOutcome.stateName)) - } - // Return to viewing the state. - advancePlayStageTo(PlayStage.VIEWING_STATE) - asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) + // Return to viewing the state. + progress.advancePlayStageTo(PlayStage.VIEWING_STATE) + asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) + + answerOutcome // Propagate the classification result. + } - return MutableLiveData(AsyncResult.success(answerOutcome)) + val dataProvider = dataProviders.createDeferredDataProviderAsync(SUBMIT_ANSWER_RESULT_DATA_PROVIDER_ID, operation) + return dataProviders.convertToLiveData(dataProvider) } /** @@ -136,44 +150,68 @@ class ExplorationProgressController @Inject constructor( * This method cannot be called until an exploration has started and [getCurrentState] returns a non-pending result or * an exception will be thrown. */ - fun moveToPreviousState() { - check(playStage != PlayStage.NOT_PLAYING) { - "Cannot navigate to a previous state if an exploration is not being played." - } - check(playStage != PlayStage.START_LOAD_EXPLORATION && playStage != PlayStage.FINISH_LOAD_EXPLORATION) { - "Cannot navigate to a previous state if an exploration is being loaded." - } - check(playStage != PlayStage.SUBMITTING_ANSWER) { - "Cannot navigate to a previous state if an answer submission is pending." + /** + * Navigates to the previous state in the graph. If the learner is currently on the initial state, this method will + * throw an exception. Calling code is responsible for ensuring this method is only called when it's possible to + * navigate backward. + * + * @return a one-time [LiveData] indicating whether the movement to the previous state was successful, or a failure if + * state navigation was attempted at an invalid time in the state graph (e.g. if currently vieiwng the initial + * state of the exploration). It's recommended that calling code only listen to this result for failures, and + * instead rely on [getCurrentState] for observing a successful transition to another state. + */ + fun moveToPreviousState(): LiveData> { + val operation: Deferred<*> = explorationProgress.updateInPlaceIfPresentAsync { progress -> + check(progress.playStage != PlayStage.NOT_PLAYING) { + "Cannot navigate to a previous state if an exploration is not being played." + } + check(progress.playStage != PlayStage.LOADING_EXPLORATION) { + "Cannot navigate to a previous state if an exploration is being loaded." + } + check(progress.playStage != PlayStage.SUBMITTING_ANSWER) { + "Cannot navigate to a previous state if an answer submission is pending." + } + progress.stateDeck.navigateToPreviousState() + asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) } - stateDeck.navigateToPreviousState() - asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) + + val dataProvider = dataProviders.createDeferredDataProviderAsync( + MOVE_TO_NEXT_STATE_RESULT_DATA_PROVIDER_ID, operation) + return dataProviders.convertToLiveData(dataProvider) } /** * Navigates to the next state in the graph. This method is only valid if the current [EphemeralState] reported by - * [getCurrentState] is a completed state. If the current state is pending or terminal, this function will throw an - * exception since it's not possible for the learner to actually navigate forward. Calling code is responsible for - * ensuring this method is only called when it's possible to navigate forward. + * [getCurrentState] is a completed state. Calling code is responsible for ensuring this method is only called when + * it's possible to navigate forward. * * Note that if the current state is a pending state, the user needs to submit a correct answer that routes to a later - * state via [submitAnswer] in order for the current state to change to a completed state. + * state via [submitAnswer] in order for the current state to change to a completed state before forward navigation + * can occur. * - * This method cannot be called until an exploration has been started and [getCurrentState] returns a non-pending - * result or an exception will be thrown. + * @return a one-time [LiveData] indicating whether the movement to the next state was successful, or a failure if + * state navigation was attempted at an invalid time in the state graph (e.g. if the current state is pending or + * terminal). It's recommended that calling code only listen to this result for failures, and instead rely on + * [getCurrentState] for observing a successful transition to another state. */ - fun moveToNextState() { - check(playStage != PlayStage.NOT_PLAYING) { - "Cannot navigate to a next state if an exploration is not being played." - } - check(playStage != PlayStage.START_LOAD_EXPLORATION && playStage != PlayStage.FINISH_LOAD_EXPLORATION) { - "Cannot navigate to a next state if an exploration is being loaded." - } - check(playStage != PlayStage.SUBMITTING_ANSWER) { - "Cannot navigate to a next state if an answer submission is pending." + fun moveToNextState(): LiveData> { + val operation: Deferred<*> = explorationProgress.updateInPlaceIfPresentAsync { progress -> + check(progress.playStage != PlayStage.NOT_PLAYING) { + "Cannot navigate to a next state if an exploration is not being played." + } + check(progress.playStage != PlayStage.LOADING_EXPLORATION) { + "Cannot navigate to a next state if an exploration is being loaded." + } + check(progress.playStage != PlayStage.SUBMITTING_ANSWER) { + "Cannot navigate to a next state if an answer submission is pending." + } + progress.stateDeck.navigateToNextState() + asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) } - stateDeck.navigateToNextState() - asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) + + val dataProvider = dataProviders.createDeferredDataProviderAsync( + MOVE_TO_PREVIOUS_STATE_RESULT_DATA_PROVIDER_ID, operation) + return dataProviders.convertToLiveData(dataProvider) } /** @@ -203,112 +241,40 @@ class ExplorationProgressController @Inject constructor( } private suspend fun retrieveCurrentStateAsync(): AsyncResult { - return when (playStage) { - PlayStage.NOT_PLAYING -> AsyncResult.pending() - PlayStage.START_LOAD_EXPLORATION -> { - beginLoadExploration() - AsyncResult.pending() - } - PlayStage.FINISH_LOAD_EXPLORATION -> { - try { - finishLoadExploration() - AsyncResult.success(stateDeck.getCurrentEphemeralState()) - } catch (e: Exception) { - AsyncResult.failed(e) - } - } - PlayStage.VIEWING_STATE -> AsyncResult.success(stateDeck.getCurrentEphemeralState()) - PlayStage.SUBMITTING_ANSWER -> AsyncResult.pending() - } - } - - private fun beginLoadExploration() { - // Advance the state if an exploration is still requested to be loaded, and notifies downstream observers a change - // is coming. This is done to notify the UI that a load is pending, then use the same data provider to actually - // perform the load rather than posting to a background thread. Note also that the async notify is intentional here - // since the load should be asynchronous to provide time for the UI to respond to the state update. - if (playStage == PlayStage.START_LOAD_EXPLORATION) { - advancePlayStageTo(PlayStage.FINISH_LOAD_EXPLORATION) - asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) + return try { + retrieveCurrentStateWithinCacheAsync().await() + } catch (e: Exception) { + AsyncResult.failed(e) } } - private suspend fun finishLoadExploration() { - // Only load the exploration if the stage hasn't changed. - if (playStage == PlayStage.FINISH_LOAD_EXPLORATION) { - // The exploration must be initialized first since other lazy fields depend on it being inited. - val exploration = explorationRetriever.loadExploration(currentExplorationId) - currentExploration = exploration - stateGraph.resetStateGraph(exploration.statesMap) - stateDeck.resetDeck(stateGraph.getState(exploration.initStateName)) - - // Advance the stage, but do not notify observers since the current state can be reported immediately to the UI. - advancePlayStageTo(PlayStage.VIEWING_STATE) + private suspend fun retrieveCurrentStateWithinCacheAsync(): Deferred> { + return explorationProgress.updateInPlaceIfPresentAsync { progress -> + when (progress.playStage) { + PlayStage.NOT_PLAYING -> AsyncResult.pending() + PlayStage.LOADING_EXPLORATION -> { + try { + finishLoadExploration(progress) + AsyncResult.success(progress.stateDeck.getCurrentEphemeralState()) + } catch (e: Exception) { + AsyncResult.failed(e) + } + } + PlayStage.VIEWING_STATE -> AsyncResult.success(progress.stateDeck.getCurrentEphemeralState()) + PlayStage.SUBMITTING_ANSWER -> AsyncResult.pending() + } } } - private fun simulateSubmitAnswer(currentState: State, answer: InteractionObject): Outcome { - return when (currentState.name) { - TEST_INIT_STATE_NAME -> currentState.interaction.answerGroupsList.first().outcome - TEST_MIDDLE_STATE_NAME -> simulateTextInputAnswerCheck(currentState, answer) - else -> throw Exception("Cannot submit answer to unexpected state: ${currentState.name}.") - } - } + private suspend fun finishLoadExploration(progress: ExplorationProgress) { + // The exploration must be initialized first since other lazy fields depend on it being inited. + val exploration = explorationRetriever.loadExploration(progress.currentExplorationId) + progress.currentExploration = exploration + progress.stateGraph.resetStateGraph(exploration.statesMap) + progress.stateDeck.resetDeck(progress.stateGraph.getState(exploration.initStateName)) - private fun simulateTextInputAnswerCheck(currentState: State, answer: InteractionObject): Outcome { - val expectedValue = currentState.interaction.answerGroupsList.first().ruleSpecsList.first().inputsMap.getValue("x") - return when { - answer.objectTypeCase != InteractionObject.ObjectTypeCase.NORMALIZED_STRING -> - throw Exception("Expected string answer, not ${answer}.") - answer.normalizedString.toLowerCase() == expectedValue.normalizedString.toLowerCase() -> - currentState.interaction.answerGroupsList.first().outcome - else -> currentState.interaction.defaultOutcome - } - } - - /** - * Advances the current play stage to the specified stage, verifying that the transition is correct. - * - * Calling code should prevent this method from failing by checking state ahead of calling this method and providing - * more useful errors to UI calling code since errors thrown by this method will be more obscure. This method aims to - * ensure the internal state of the controller remains correct. This method is not meant to be covered in unit tests - * since none of the failures here should ever be exposed to controller callers. - */ - private fun advancePlayStageTo(nextPlayStage: PlayStage) { - when (nextPlayStage) { - PlayStage.NOT_PLAYING -> { - // All transitions to NOT_PLAYING are valid except itself. Stopping playing can happen at any time. - check(playStage != PlayStage.NOT_PLAYING) { "Cannot transition to NOT_PLAYING from NOT_PLAYING" } - playStage = nextPlayStage - } - PlayStage.START_LOAD_EXPLORATION -> { - // An exploration can only be requested to be loaded from the initial NOT_PLAYING stage. - check(playStage == PlayStage.NOT_PLAYING) { "Cannot transition to LOADING_EXPLORATION from $playStage" } - playStage = nextPlayStage - } - PlayStage.FINISH_LOAD_EXPLORATION -> { - // An exploration can only be finished loading after being requested to be started. - check(playStage == PlayStage.START_LOAD_EXPLORATION) { - "Cannot transition to FINISH_LOAD_EXPLORATION from $playStage" - } - playStage = nextPlayStage - } - PlayStage.VIEWING_STATE -> { - // A state can be viewed after loading an exploration, after viewing another state, or after submitting an - // answer. It cannot be viewed without a loaded exploration. - check(playStage == PlayStage.FINISH_LOAD_EXPLORATION - || playStage == PlayStage.VIEWING_STATE - || playStage == PlayStage.SUBMITTING_ANSWER) { - "Cannot transition to VIEWING_STATE from $playStage" - } - playStage = nextPlayStage - } - PlayStage.SUBMITTING_ANSWER -> { - // An answer can only be submitted after viewing a stage. - check(playStage == PlayStage.VIEWING_STATE) { "Cannot transition to SUBMITTING_ANSWER from $playStage" } - playStage = nextPlayStage - } - } + // Advance the stage, but do not notify observers since the current state can be reported immediately to the UI. + progress.advancePlayStageTo(PlayStage.VIEWING_STATE) } /** Different stages in which the progress controller can exist. */ @@ -317,14 +283,7 @@ class ExplorationProgressController @Inject constructor( NOT_PLAYING, /** An exploration is being prepared to be played. */ - START_LOAD_EXPLORATION, - - /** - * An exploration is being loaded. Note that this state is to allow the UI to quickly get a pending result when a - * load begins, but not have to wait for the load to complete. This also mitigates needing to introduce another - * thread hop when performing the load (e.g. by posting the operation on a background dispatcher). - */ - FINISH_LOAD_EXPLORATION, + LOADING_EXPLORATION, /** The controller is currently viewing a State. */ VIEWING_STATE, @@ -333,6 +292,64 @@ class ExplorationProgressController @Inject constructor( SUBMITTING_ANSWER } + /** + * Private class that encapsulates the mutable state of the progress controller. This class is thread-safe. This class + * can exist across multiple exploration instances, but calling code is responsible for ensuring it is properly reset. + */ + private class ExplorationProgress { + internal lateinit var currentExplorationId: String + internal lateinit var currentExploration: Exploration + internal var playStage = PlayStage.NOT_PLAYING + internal val stateGraph: StateGraph by lazy { + StateGraph( + currentExploration.statesMap + ) + } + internal val stateDeck: StateDeck by lazy { + StateDeck( + stateGraph.getState(currentExploration.initStateName) + ) + } + + /** + * Advances the current play stage to the specified stage, verifying that the transition is correct. + * + * Calling code should prevent this method from failing by checking state ahead of calling this method and providing + * more useful errors to UI calling code since errors thrown by this method will be more obscure. This method aims to + * ensure the internal state of the controller remains correct. This method is not meant to be covered in unit tests + * since none of the failures here should ever be exposed to controller callers. + */ + internal fun advancePlayStageTo(nextPlayStage: PlayStage) { + when (nextPlayStage) { + PlayStage.NOT_PLAYING -> { + // All transitions to NOT_PLAYING are valid except itself. Stopping playing can happen at any time. + check(playStage != PlayStage.NOT_PLAYING) { "Cannot transition to NOT_PLAYING from NOT_PLAYING" } + playStage = nextPlayStage + } + PlayStage.LOADING_EXPLORATION -> { + // An exploration can only be requested to be loaded from the initial NOT_PLAYING stage. + check(playStage == PlayStage.NOT_PLAYING) { "Cannot transition to LOADING_EXPLORATION from $playStage" } + playStage = nextPlayStage + } + PlayStage.VIEWING_STATE -> { + // A state can be viewed after loading an exploration, after viewing another state, or after submitting an + // answer. It cannot be viewed without a loaded exploration. + check(playStage == PlayStage.LOADING_EXPLORATION + || playStage == PlayStage.VIEWING_STATE + || playStage == PlayStage.SUBMITTING_ANSWER) { + "Cannot transition to VIEWING_STATE from $playStage" + } + playStage = nextPlayStage + } + PlayStage.SUBMITTING_ANSWER -> { + // An answer can only be submitted after viewing a stage. + check(playStage == PlayStage.VIEWING_STATE) { "Cannot transition to SUBMITTING_ANSWER from $playStage" } + playStage = nextPlayStage + } + } + } + } + /** * Graph that provides lookup access for [State]s and functionality for processing the outcome of a submitted learner * answer. @@ -352,7 +369,7 @@ class ExplorationProgressController @Inject constructor( internal fun computeAnswerOutcomeForResult(currentState: State, outcome: Outcome): AnswerOutcome { val answerOutcomeBuilder = AnswerOutcome.newBuilder() .setFeedback(outcome.feedback) - .setCorrectAnswer(outcome.labelledAsCorrect) + .setLabelledAsCorrectAnswer(outcome.labelledAsCorrect) when { outcome.refresherExplorationId.isNotEmpty() -> answerOutcomeBuilder.refresherExplorationId = outcome.refresherExplorationId diff --git a/domain/src/main/java/org/oppia/domain/exploration/ExplorationRetriever.kt b/domain/src/main/java/org/oppia/domain/exploration/ExplorationRetriever.kt index 8a199df089b..65f41980fed 100644 --- a/domain/src/main/java/org/oppia/domain/exploration/ExplorationRetriever.kt +++ b/domain/src/main/java/org/oppia/domain/exploration/ExplorationRetriever.kt @@ -18,7 +18,8 @@ import javax.inject.Inject const val TEST_EXPLORATION_ID_5 = "test_exp_id_5" const val TEST_EXPLORATION_ID_6 = "test_exp_id_6" -// TODO(#59): Make this class inaccessible outside of the domain package. UI code should not depend on it. +// TODO(#59): Make this class inaccessible outside of the domain package except for tests. UI code should not be allowed +// to depend on this utility. /** Internal class for actually retrieving an exploration object for uses in domain controllers. */ class ExplorationRetriever @Inject constructor(private val context: Context) { @@ -50,7 +51,7 @@ class ExplorationRetriever @Inject constructor(private val context: Context) { // Returns a JSON Object if it exists, else returns null private fun getJsonObject(parentObject: JSONObject, key: String): JSONObject? { - return parentObject.getJSONObject(key) + return parentObject.optJSONObject(key) } // Loads the JSON string from an asset and converts it to a JSONObject @@ -68,14 +69,15 @@ class ExplorationRetriever @Inject constructor(private val context: Context) { val statesIterator = statesKeys.iterator() while (statesIterator.hasNext()) { val key = statesIterator.next() - statesMap[key] = createStateFromJson(statesJsonObject.getJSONObject(key)) + statesMap[key] = createStateFromJson(key, statesJsonObject.getJSONObject(key)) } return statesMap } // Creates a single state object from JSON - private fun createStateFromJson(stateJson: JSONObject?): State { + private fun createStateFromJson(stateName: String, stateJson: JSONObject?): State { return State.newBuilder() + .setName(stateName) .setContent( SubtitledHtml.newBuilder().setHtml( stateJson?.getJSONObject("content")?.getString("html") @@ -209,7 +211,7 @@ class ExplorationRetriever @Inject constructor(private val context: Context) { "NumericInput" -> InteractionObject.newBuilder() .setReal(inputJson.getDouble(keyName)) .build() - else -> InteractionObject.getDefaultInstance() + else -> throw IllegalStateException("Encountered unexpected interaction ID: $interactionId") } } diff --git a/domain/src/test/java/org/oppia/domain/classify/AnswerClassificationControllerTest.kt b/domain/src/test/java/org/oppia/domain/classify/AnswerClassificationControllerTest.kt index 3f41de9c952..adbe60f58ef 100644 --- a/domain/src/test/java/org/oppia/domain/classify/AnswerClassificationControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/classify/AnswerClassificationControllerTest.kt @@ -16,6 +16,7 @@ import org.oppia.app.model.AnswerGroup import org.oppia.app.model.Interaction import org.oppia.app.model.InteractionObject import org.oppia.app.model.Outcome +import org.oppia.app.model.State import org.oppia.app.model.SubtitledHtml import org.robolectric.annotation.Config import javax.inject.Inject @@ -25,7 +26,9 @@ import javax.inject.Singleton @RunWith(AndroidJUnit4::class) @Config(manifest = Config.NONE) class AnswerClassificationControllerTest { - private val ARBITRARY_SAMPLE_ANSWER = InteractionObject.newBuilder().setNormalizedString("Some value").build() + private val TEST_STRING_ANSWER = InteractionObject.newBuilder().setNormalizedString("Some value").build() + private val TEST_INT_2_ANSWER = InteractionObject.newBuilder().setNonNegativeInt(2).build() + private val TEST_INT_121_ANSWER = InteractionObject.newBuilder().setNonNegativeInt(121).build() private val OUTCOME_0 = Outcome.newBuilder() .setDestStateName("First state") @@ -54,19 +57,21 @@ class AnswerClassificationControllerTest { .setDefaultOutcome(OUTCOME_0) .build() - val outcome = answerClassificationController.classify(interaction, ARBITRARY_SAMPLE_ANSWER) + val state = createTestState("Things you can do", interaction) + val outcome = answerClassificationController.classify(state, TEST_STRING_ANSWER) assertThat(outcome).isEqualTo(OUTCOME_0) } @Test - fun testClassify_testInteraction_withMultipleDefaultOutcomes_returnsDefaultOutcome() { + fun testClassify_testInteraction_withMultipleOutcomes_wrongAnswer_returnsDefaultOutcome() { val interaction = Interaction.newBuilder() .setDefaultOutcome(OUTCOME_1) .addAnswerGroups(AnswerGroup.newBuilder().setOutcome(OUTCOME_2)) .build() - val outcome = answerClassificationController.classify(interaction, ARBITRARY_SAMPLE_ANSWER) + val state = createTestState("Welcome!", interaction) + val outcome = answerClassificationController.classify(state, TEST_INT_2_ANSWER) assertThat(outcome).isEqualTo(OUTCOME_1) } @@ -80,13 +85,22 @@ class AnswerClassificationControllerTest { val interaction2 = Interaction.newBuilder() .setDefaultOutcome(OUTCOME_2) .build() - answerClassificationController.classify(interaction1, ARBITRARY_SAMPLE_ANSWER) + val state1 = createTestState("Numeric input", interaction1) + answerClassificationController.classify(state1, TEST_INT_121_ANSWER) - val outcome = answerClassificationController.classify(interaction2, ARBITRARY_SAMPLE_ANSWER) + val state2 = createTestState("Things you can do", interaction2) + val outcome = answerClassificationController.classify(state2, TEST_STRING_ANSWER) assertThat(outcome).isEqualTo(OUTCOME_2) } + private fun createTestState(stateName: String, interaction: Interaction): State { + return State.newBuilder() + .setName(stateName) + .setInteraction(interaction) + .build() + } + private fun setUpTestApplicationComponent() { DaggerAnswerClassificationControllerTest_TestApplicationComponent.builder() .setApplication(ApplicationProvider.getApplicationContext()) diff --git a/domain/src/test/java/org/oppia/domain/exploration/ExplorationDataControllerTest.kt b/domain/src/test/java/org/oppia/domain/exploration/ExplorationDataControllerTest.kt index 754e2fbe6c0..da49561b8cd 100644 --- a/domain/src/test/java/org/oppia/domain/exploration/ExplorationDataControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/exploration/ExplorationDataControllerTest.kt @@ -46,9 +46,6 @@ import javax.inject.Qualifier import javax.inject.Singleton import kotlin.coroutines.EmptyCoroutineContext -const val TEST_EXPLORATION_ID_0 = "test_exp_id_0" -const val TEST_EXPLORATION_ID_1 = "test_exp_id_1" - /** Tests for [ExplorationDataController]. */ @RunWith(AndroidJUnit4::class) @Config(manifest = Config.NONE) @@ -108,9 +105,9 @@ class ExplorationDataControllerTest { @Test @ExperimentalCoroutinesApi fun testController_providesInitialLiveDataForTheWelcomeExploration() = runBlockingTest(coroutineContext) { - val explorationLiveData = explorationDataController.getExplorationById(TEST_EXPLORATION_ID_0) + val explorationLiveData = explorationDataController.getExplorationById(TEST_EXPLORATION_ID_5) advanceUntilIdle() - explorationLiveData!!.observeForever(mockExplorationObserver) + explorationLiveData.observeForever(mockExplorationObserver) val expectedExplorationStateSet = listOf( "END", "Estimate 100", "Numeric input", "Things you can do", "Welcome!", "What language" @@ -129,9 +126,9 @@ class ExplorationDataControllerTest { @Test @ExperimentalCoroutinesApi fun testController_providesInitialLiveDataForTheAboutOppiaExploration() = runBlockingTest(coroutineContext) { - val explorationLiveData = explorationDataController.getExplorationById(TEST_EXPLORATION_ID_1) + val explorationLiveData = explorationDataController.getExplorationById(TEST_EXPLORATION_ID_6) advanceUntilIdle() - explorationLiveData!!.observeForever(mockExplorationObserver) + explorationLiveData.observeForever(mockExplorationObserver) val expectedExplorationStateSet = listOf( "About this website", "Contact", "Contribute", "Credits", "END", "End Card", "Example1", "Example3", "First State", "Site License", "So what can I tell you" @@ -152,7 +149,7 @@ class ExplorationDataControllerTest { fun testController_returnsNullForNonExistentExploration() = runBlockingTest(coroutineContext) { val explorationLiveData = explorationDataController.getExplorationById("NON_EXISTENT_TEST") advanceUntilIdle() - explorationLiveData!!.observeForever(mockExplorationObserver) + explorationLiveData.observeForever(mockExplorationObserver) verify(mockExplorationObserver, atLeastOnce()).onChanged(explorationResultCaptor.capture()) assertThat(explorationResultCaptor.value.isFailure()).isTrue() } diff --git a/domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressControllerTest.kt b/domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressControllerTest.kt index f443d551597..99b2183d9ff 100644 --- a/domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressControllerTest.kt @@ -29,8 +29,10 @@ import org.mockito.junit.MockitoJUnit import org.mockito.junit.MockitoRule import org.oppia.app.model.EphemeralState import org.oppia.app.model.EphemeralState.StateTypeCase.PENDING_STATE +import org.oppia.app.model.Exploration import org.oppia.util.data.AsyncResult import org.oppia.util.threading.BackgroundDispatcher +import org.oppia.util.threading.BlockingDispatcher import org.robolectric.annotation.Config import java.lang.IllegalStateException import javax.inject.Inject @@ -56,6 +58,10 @@ class ExplorationProgressControllerTest { @Inject lateinit var explorationProgressController: ExplorationProgressController + @Inject + lateinit var explorationRetriever: ExplorationRetriever + + @ExperimentalCoroutinesApi @Inject @field:TestDispatcher lateinit var testDispatcher: TestCoroutineDispatcher @@ -63,9 +69,16 @@ class ExplorationProgressControllerTest { @Mock lateinit var mockCurrentStateLiveDataObserver: Observer> + @Mock + lateinit var mockAsyncResultLiveDataObserver: Observer> + @Captor lateinit var currentStateResultCaptor: ArgumentCaptor> + @Captor + lateinit var asyncResultCaptor: ArgumentCaptor> + + @ExperimentalCoroutinesApi private val coroutineContext by lazy { EmptyCoroutineContext + testDispatcher } @@ -92,6 +105,18 @@ class ExplorationProgressControllerTest { assertThat(currentStateResultCaptor.value.isPending()).isTrue() } + @Test + @ExperimentalCoroutinesApi + fun testPlayExploration_invalid_returnsSuccess() = runBlockingTest(coroutineContext) { + val resultLiveData = explorationDataController.startPlayingExploration("invalid_exp_id") + resultLiveData.observeForever(mockAsyncResultLiveDataObserver) + advanceUntilIdle() + + // An invalid exploration is not known until it's fully loaded, and that's observed via getCurrentState. + verify(mockAsyncResultLiveDataObserver).onChanged(asyncResultCaptor.capture()) + assertThat(asyncResultCaptor.value.isSuccess()).isTrue() + } + @Test @ExperimentalCoroutinesApi fun testGetCurrentState_playInvalidExploration_returnsFailure() = runBlockingTest(coroutineContext) { @@ -108,6 +133,17 @@ class ExplorationProgressControllerTest { .contains("Invalid exploration ID: invalid_exp_id") } + @Test + @ExperimentalCoroutinesApi + fun testPlayExploration_valid_returnsSuccess() = runBlockingTest(coroutineContext) { + val resultLiveData = explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) + resultLiveData.observeForever(mockAsyncResultLiveDataObserver) + advanceUntilIdle() + + verify(mockAsyncResultLiveDataObserver).onChanged(asyncResultCaptor.capture()) + assertThat(asyncResultCaptor.value.isSuccess()).isTrue() + } + @Test @ExperimentalCoroutinesApi fun testGetCurrentState_playExploration_returnsPendingResultFromLoadingExploration() = runBlockingTest( @@ -131,6 +167,7 @@ class ExplorationProgressControllerTest { fun testGetCurrentState_playExploration_loaded_returnsInitialStatePending() = runBlockingTest( coroutineContext ) { + val exploration = getTestExploration5() explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) val currentStateLiveData = explorationProgressController.getCurrentState() @@ -141,7 +178,7 @@ class ExplorationProgressControllerTest { assertThat(currentStateResultCaptor.value.isSuccess()).isTrue() assertThat(currentStateResultCaptor.value.getOrThrow().stateTypeCase).isEqualTo(PENDING_STATE) assertThat(currentStateResultCaptor.value.getOrThrow().hasPreviousState).isFalse() - assertThat(currentStateResultCaptor.value.getOrThrow().state.name).isEqualTo(TEST_INIT_STATE_NAME) + assertThat(currentStateResultCaptor.value.getOrThrow().state.name).isEqualTo(exploration.initStateName) } @Test @@ -149,6 +186,7 @@ class ExplorationProgressControllerTest { fun testGetCurrentState_playInvalidExploration_thenPlayValidExp_returnsInitialPendingState() = runBlockingTest( coroutineContext ) { + val exploration = getTestExploration5() // Start with playing an invalid exploration. explorationDataController.startPlayingExploration("invalid_exp_id") explorationDataController.stopPlayingExploration() @@ -164,61 +202,86 @@ class ExplorationProgressControllerTest { assertThat(currentStateResultCaptor.value.isSuccess()).isTrue() assertThat(currentStateResultCaptor.value.getOrThrow().stateTypeCase).isEqualTo(PENDING_STATE) assertThat(currentStateResultCaptor.value.getOrThrow().hasPreviousState).isFalse() - assertThat(currentStateResultCaptor.value.getOrThrow().state.name).isEqualTo(TEST_INIT_STATE_NAME) + assertThat(currentStateResultCaptor.value.getOrThrow().state.name).isEqualTo(exploration.initStateName) } @Test @ExperimentalCoroutinesApi - fun testFinishExploration_beforePlaying_fails() = runBlockingTest(coroutineContext) { - val exception = assertThrows(IllegalStateException::class) { explorationDataController.stopPlayingExploration() } + fun testFinishExploration_beforePlaying_failWithError() = runBlockingTest(coroutineContext) { + val resultLiveData = explorationDataController.stopPlayingExploration() + resultLiveData.observeForever(mockAsyncResultLiveDataObserver) + advanceUntilIdle() - assertThat(exception).hasMessageThat().contains("Cannot finish playing an exploration that hasn't yet been started") + verify(mockAsyncResultLiveDataObserver).onChanged(asyncResultCaptor.capture()) + assertThat(asyncResultCaptor.value.isFailure()).isTrue() + assertThat(asyncResultCaptor.value.getErrorOrNull()) + .hasMessageThat() + .contains("Cannot finish playing an exploration that hasn't yet been started") } @Test @ExperimentalCoroutinesApi - fun testPlayExploration_withoutFinishingPrevious_fails() = runBlockingTest(coroutineContext) { + fun testPlayExploration_withoutFinishingPrevious_failsWithError() = runBlockingTest(coroutineContext) { explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) // Try playing another exploration without finishing the previous one. - val exception = assertThrows(IllegalStateException::class) { - explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) - } + val resultLiveData = explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) + resultLiveData.observeForever(mockAsyncResultLiveDataObserver) + advanceUntilIdle() - assertThat(exception) + verify(mockAsyncResultLiveDataObserver).onChanged(asyncResultCaptor.capture()) + assertThat(asyncResultCaptor.value.isFailure()).isTrue() + assertThat(asyncResultCaptor.value.getErrorOrNull()) .hasMessageThat() .contains("Expected to finish previous exploration before starting a new one.") } // testGetCurrentState_playSecondExploration_afterFinishingPrevious_loaded_returnsInitialState - // testSubmitAnswer_forContinueButton_returnsAnswerIsCorrect + // testSubmitAnswer_forMultipleChoice_correctAnswer_succeeds + // testSubmitAnswer_forMultipleChoice_wrongAnswer_succeeds + // testSubmitAnswer_forMultipleChoice_correctAnswer_returnsOutcomeWithTransition + // testSubmitAnswer_forMultipleChoice_wrongAnswer_returnsDefaultOutcome // testGetCurrentState_whileSubmittingAnswer_becomesPending + // testGetCurrentState_afterSubmittingWrongAnswer_updatesPendingState // testGetCurrentState_afterSubmittingCorrectAnswer_becomesCompletedState - // testSubmitAnswer_forTextInput_wrongAnswer_returnsAnswerIsWrong - // testSubmitAnswer_forTextInput_correctAnswer_returnsAnswerIsCorrect - // testGetCurrentState_afterPreviousState_submitWrongAnswer_updatePendingState - // testGetCurrentState_afterPreviousState_submitRightAnswer_pendingStateBecomesCompleted - // testGetCurrentState_thirdState_isTerminalState - // testSubmitAnswer_beforePlaying_fails - // testSubmitAnswer_whileLoading_fails - // testSubmitAnswer_whileSubmittingAnotherAnswer_fails - // testMoveToPrevious_beforePlaying_fails - // testMoveToPrevious_whileLoadingExploration_fails - // testMoveToPrevious_whileSubmittingAnswer_fails - // testMoveToPrevious_onInitialState_fails + // testSubmitAnswer_forTextInput_correctAnswer_returnsOutcomeWithTransition + // testSubmitAnswer_forTextInput_wrongAnswer_returnsDefaultOutcome + // testGetCurrentState_secondState_submitWrongAnswer_updatePendingState + // testGetCurrentState_secondState_submitRightAnswer_pendingStateBecomesCompleted + // testSubmitAnswer_forNumericInput_correctAnswer_returnsOutcomeWithTransition + // testSubmitAnswer_forNumericInput_wrongAnswer_returnsDefaultOutcome + // testSubmitAnswer_forContinue_returnsOutcomeWithTransition + // testGetCurrentState_fifthState_isTerminalState + // testSubmitAnswer_beforePlaying_failsWithError + // testSubmitAnswer_whileLoading_failsWithError + // testSubmitAnswer_whileSubmittingAnotherAnswer_failsWithError + // testMoveToPrevious_beforePlaying_failsWithError + // testMoveToPrevious_whileLoadingExploration_failsWithError + // testMoveToPrevious_whileSubmittingAnswer_failsWithError + // testMoveToPrevious_onInitialState_failsWithError + // testMoveToPrevious_forStateWithCompletedPreviousState_succeeds // testGetCurrentState_afterMoveToPrevious_onSecondState_updatesToCompletedFirstState // testGetCurrentState_afterMoveToPrevious_onThirdState_updatesToCompletedSecondState - // testMoveToNext_beforePlaying_fails - // testMoveToNext_whileLoadingExploration_fails - // testMoveToNext_whileSubmittingAnswer_fails - // testMoveToNext_onFinalState_fails - // testMoveToNext_forPendingInitialState_fails + // testMoveToNext_beforePlaying_failsWithError + // testMoveToNext_whileLoadingExploration_failsWithError + // testMoveToNext_whileSubmittingAnswer_failsWithError + // testMoveToNext_onFinalState_failsWithError + // testMoveToNext_forPendingInitialState_failsWithError + // testMoveToNext_forCompletedState_succeeds // testGetCurrentState_afterMoveToNext_onCompletedFirstState_updatesToPendingSecondState // testGetCurrentState_afterMoveToNext_onCompletedSecondState_updatesToTerminalThirdState // testGetCurrentState_afterMovePreviousAndNext_returnsCurrentState // testGetCurrentState_afterMoveNextAndPrevious_returnsCurrentState // testGetCurrentState_afterMoveToPrevious_onSecondState_newObserver_receivesCompletedFirstState + private suspend fun getTestExploration5(): Exploration { + return explorationRetriever.loadExploration(TEST_EXPLORATION_ID_5) + } + + private suspend fun getTestExploration6(): Exploration { + return explorationRetriever.loadExploration(TEST_EXPLORATION_ID_6) + } + private fun setUpTestApplicationComponent() { DaggerExplorationProgressControllerTest_TestApplicationComponent.builder() .setApplication(ApplicationProvider.getApplicationContext()) @@ -261,12 +324,21 @@ class ExplorationProgressControllerTest { return TestCoroutineDispatcher() } + @ExperimentalCoroutinesApi @Singleton @Provides @BackgroundDispatcher fun provideBackgroundDispatcher(@TestDispatcher testDispatcher: TestCoroutineDispatcher): CoroutineDispatcher { return testDispatcher } + + @ExperimentalCoroutinesApi + @Singleton + @Provides + @BlockingDispatcher + fun provideBlockingDispatcher(@TestDispatcher testDispatcher: TestCoroutineDispatcher): CoroutineDispatcher { + return testDispatcher + } } // TODO(#89): Move this to a common test application component. diff --git a/model/src/main/proto/exploration.proto b/model/src/main/proto/exploration.proto index 9183787cf26..338994bbd94 100644 --- a/model/src/main/proto/exploration.proto +++ b/model/src/main/proto/exploration.proto @@ -192,8 +192,9 @@ message AnswerOutcome { // Oppia's feedback to the learner's most recent answer. SubtitledHtml feedback = 1; - // Whether the answer the learner submitted is the correct answer. - bool correct_answer = 2; + // Whether the answer the learner submitted is the correct answer. Note that this is only an indication of the + // correctness, and not all correct answers are guaranteed to be labelled as correct. + bool labelled_as_correct_answer = 2; // One of several destinations the learner should be routed to as a result of submitting this answer. oneof destination { diff --git a/utility/src/main/java/org/oppia/util/data/DataProviders.kt b/utility/src/main/java/org/oppia/util/data/DataProviders.kt index 94fe26441f9..73811b0c61f 100644 --- a/utility/src/main/java/org/oppia/util/data/DataProviders.kt +++ b/utility/src/main/java/org/oppia/util/data/DataProviders.kt @@ -6,6 +6,7 @@ import androidx.lifecycle.MediatorLiveData import androidx.lifecycle.MutableLiveData import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Deferred import kotlinx.coroutines.Job import kotlinx.coroutines.launch import org.oppia.util.threading.BackgroundDispatcher @@ -115,6 +116,20 @@ class DataProviders @Inject constructor( } } + /** + * Returns an in-memory [DataProvider] which wraps the specified [Deferred] and provides either its successful value + * or its resulting failure. + */ + fun createDeferredDataProviderAsync(id: Any, deferred: Deferred): DataProvider { + return createInMemoryDataProviderAsync(id) { + try { + AsyncResult.success(deferred.await()) + } catch (e: Exception ) { + AsyncResult.failed(e) + } + } + } + /** * Converts a [DataProvider] to [LiveData]. This will use a background executor to handle processing of the coroutine, * but [LiveData] guarantees that final delivery of the result will happen on the main thread. @@ -205,7 +220,7 @@ class DataProviders @Inject constructor( private fun dequeuePendingCoroutineLiveData() { coroutineLiveDataLock.withLock { pendingCoroutineLiveData?.let { - removeSource(it) + removeSource(it) // This can trigger onInactive() situations for long-standing operations, leading to them being cancelled. pendingCoroutineLiveData = null } } @@ -234,7 +249,7 @@ class DataProviders @Inject constructor( override fun onInactive() { super.onInactive() - runningJob?.cancel() + runningJob?.cancel() // This can cancel downstream operations that may want to complete side effects. runningJob = null } } diff --git a/utility/src/main/java/org/oppia/util/data/InMemoryBlockingCache.kt b/utility/src/main/java/org/oppia/util/data/InMemoryBlockingCache.kt index f84fb92092a..dedce02c969 100644 --- a/utility/src/main/java/org/oppia/util/data/InMemoryBlockingCache.kt +++ b/utility/src/main/java/org/oppia/util/data/InMemoryBlockingCache.kt @@ -12,6 +12,9 @@ import javax.inject.Singleton * An in-memory cache that provides blocking CRUD operations such that each operation is guaranteed to operate exactly * after any prior started operations began, and before any future operations. This class is thread-safe. Note that it's * safe to execute long-running operations in lambdas passed into the methods of this class. + * + * This cache is primarily intended to be used with immutable payloads, but mutable payloads can be used if calling code + * takes caution to restrict all read/write access to those mutable values to operations invoked by this class. */ class InMemoryBlockingCache private constructor(blockingDispatcher: CoroutineDispatcher, initialValue: T?) { private val blockingScope = CoroutineScope(blockingDispatcher) @@ -92,6 +95,17 @@ class InMemoryBlockingCache private constructor(blockingDispatcher: Cor } } + /** + * Returns a [Deferred] in the same way and for the same conditions as [updateIfPresentAsync] except the provided + * function is expected to update the cache in-place and return a custom value to propagate to the result of the + * [Deferred] object. + */ + fun updateInPlaceIfPresentAsync(update: suspend (T) -> O): Deferred { + return blockingScope.async { + update(checkNotNull(value) { "Expected to update the cache only after it's been created" }) + } + } + /** * Returns a [Deferred] that executes when this cache has been fully cleared, or if it's already been cleared. */