From e19af2bd003e27d32713b44b0ff86d77274c30a1 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Tue, 24 Sep 2019 23:47:03 -0700 Subject: [PATCH 001/146] Introduce first pass interface for ExplorationProgressController. --- .../ExplorationProgressController.kt | 79 +++++++++++++++++++ model/src/main/proto/exploration.proto | 76 ++++++++++++++++++ 2 files changed, 155 insertions(+) create mode 100644 domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressController.kt diff --git a/domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressController.kt b/domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressController.kt new file mode 100644 index 00000000000..035025c8003 --- /dev/null +++ b/domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressController.kt @@ -0,0 +1,79 @@ +package org.oppia.domain.exploration + +import androidx.lifecycle.LiveData +import androidx.lifecycle.MutableLiveData +import org.oppia.app.model.AnswerOutcome +import org.oppia.app.model.EphemeralState +import org.oppia.app.model.InteractionObject +import org.oppia.util.data.AsyncResult + +/** + * 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. + */ +class ExplorationProgressController { + // 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. + + /** + * Submits an answer to the current state and returns how the UI should respond to this answer. The returned + * [LiveData] will only have at most two results posted: a pending result, and then a completed success/failure + * result. Failures in this case represent a failure of the app (possibly due to networking conditions). The app + * should report this error in a consumable way to the user so that they may take action on it. No additional values + * will be reported to the [LiveData]. Each call to this method returns a new, distinct, [LiveData] object that must + * be observed. + * + * If the app undergoes a configuration change, calling code should rely on the [LiveData] from [getCurrentState] to + * know whether a current answer is pending. That [LiveData] will have its state changed to pending during answer + * submission and until answer resolution. + * + * Submitting an answer may result in the learner staying in the current state, moving to a new state in the + * exploration, being shown a concept card, or being navigated to another exploration altogether. Note that once a + * correct answer is processed, the current state reported to [getCurrentState] will change from a pending state to a + * completed state since the learner completed that card. The learner can then proceed from the current completed + * state to the next pending state using [moveToNextState]. + */ + fun submitAnswer(answer: InteractionObject): LiveData> { + return MutableLiveData(AsyncResult.pending()) + } + + /** + * Navigates to the previous state in the stack. If the learner is currently on the initial state, this method will + * throw an exception. Calling code is responsible to make sure that this method is not called when it's not possible + * to navigate to a previous card. + */ + fun moveToPreviousState() {} + + /** + * 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. + * + * 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. + */ + fun moveToNextState() {} + + /** + * Returns a [LiveData] monitoring the current [EphemeralState] the learner is currently viewing. If this state + * corresponds to a a terminal state, then the learner has completed the exploration. Note that [moveToPreviousState] + * and [moveToNextState] will automatically update observers of this live data when the next state is navigated to. + * + * Note that the returned [LiveData] is always the same object no matter when this method is called, except + * potentially when a new exploration is started. + * + * This [LiveData] may initially be pending while the exploration object is loaded. It may also switch from a + * completed to a pending result during transient operations like submitting an answer via [submitAnswer]. Calling + * code should be made resilient to this by caching the current state object to display since it may disappear + * temporarily during answer submission. Calling code should persist this state object across configuration changes if + * needed since it cannot rely on this [LiveData] for immediate state reconstitution after configuration changes. + */ + fun getCurrentState(): LiveData> { + return MutableLiveData(AsyncResult.pending()) + } +} diff --git a/model/src/main/proto/exploration.proto b/model/src/main/proto/exploration.proto index 317b9b7a35f..895cb4d1748 100644 --- a/model/src/main/proto/exploration.proto +++ b/model/src/main/proto/exploration.proto @@ -155,3 +155,79 @@ message RuleSpec { map inputs = 1; string rule_type = 2; } + +/* The following structures are specific to ephemeral exploration sessions and should never be persisted on disk. */ + +// Corresponds to an exploration state that exists ephemerally in the UI and will disappear once the user finishes the +// exploration or navigates away from it. This model contains additional information to help the UI display an exact +// representation of their interaction with the lesson. +message EphemeralState { + // The actual state to display to the user. + State state = 1; + + // Whether there is a state prior to this one that the learner has already completed, or if this is the initial state + // of the exploration. + bool has_previous_state = 2; + + // Different types this state can take depending on whether the learner needs to finish an existing card, has + // navigated to a previous card, or has reached the end of the exploration. + oneof state_type { + // A pending state that requires a correct answer to continue. + PendingState pending_state = 3; + + // A previous state completed by the learner. + CompletedState completed_state = 4; + + // This value is always true in the case where the state type is terminal. This type may change in the future to a + // message structure if additional data needs to be passed along to the terminal state card. + bool terminal_state = 5; + } +} + +// Corresponds to an exploration state that hasn't yet had a correct answer filled in. +message PendingState { + // A list of previous wrong answers that led back to this state, and Oppia's responses. These responses are in the + // order the learner submitted them. + repeated AnswerAndResponse wrong_answer = 1; +} + +// Corresponds to an exploration state that the learner has previous completed. +message CompletedState { + // The list of answers and responses that were used to finish this state. These answers are in the order that the + // learner submitted them, so the last answer is guaranteed to be the correct answer. + repeated AnswerAndResponse answer = 1; +} + +message AnswerAndResponse { + // A previous answer the learner submitted. + InteractionObject user_answer = 1; + + // Oppia's response to the answer the learner submitted. + SubtitledHtml feedback = 2; +} + +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; + + // One of several destinations the learner should be routed to as a result of submitting this answer. + oneof destination { + // Indicates that the learner should not progress past the current state. + bool same_state = 3; + + // Indicates that the learner should move to the next state. This contains the name of that state, but this isn't + // meant to be used by the UI directly beyond for logging purposes. + string state_name = 4; + + // Indicates that the learner should be shown a concept card corresponding to the specified skill ID to refresh the + // concept before proceeding. + string missing_prerequisite_skill_id = 5; + + // Indicates that the learner needs to play the specified exploration ID to refresh missing topics and then return + // to restart the current exploration. + string refresher_exploration_id = 6; + } +} From 43766b0a60410ac25b5ffed11e7f58a9633129d9 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 25 Sep 2019 23:41:05 -0700 Subject: [PATCH 002/146] Fill in the stubbed logic for ExplorationProgressController. Still no tests to verify correctness. Also, added a method to facilitate notifying of DataProvider changes on the UI thread. --- .../ExplorationProgressController.kt | 436 ++++++++++++++++++ .../ExplorationProgressController.kt | 79 ---- model/src/main/proto/exploration.proto | 16 +- .../util/data/AsyncDataSubscriptionManager.kt | 20 +- 4 files changed, 461 insertions(+), 90 deletions(-) create mode 100644 domain/src/main/java/org/oppia/domain/exploration/ExplorationProgressController.kt delete mode 100644 domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressController.kt diff --git a/domain/src/main/java/org/oppia/domain/exploration/ExplorationProgressController.kt b/domain/src/main/java/org/oppia/domain/exploration/ExplorationProgressController.kt new file mode 100644 index 00000000000..d099ab634b0 --- /dev/null +++ b/domain/src/main/java/org/oppia/domain/exploration/ExplorationProgressController.kt @@ -0,0 +1,436 @@ +package org.oppia.domain.exploration + +import androidx.lifecycle.LiveData +import androidx.lifecycle.MutableLiveData +import org.oppia.app.model.AnswerAndResponse +import org.oppia.app.model.AnswerGroup +import org.oppia.app.model.AnswerOutcome +import org.oppia.app.model.CompletedState +import org.oppia.app.model.EphemeralState +import org.oppia.app.model.Exploration +import org.oppia.app.model.Interaction +import org.oppia.app.model.InteractionObject +import org.oppia.app.model.Outcome +import org.oppia.app.model.PendingState +import org.oppia.app.model.RuleSpec +import org.oppia.app.model.State +import org.oppia.app.model.SubtitledHtml +import org.oppia.util.data.AsyncDataSubscriptionManager +import org.oppia.util.data.AsyncResult +import org.oppia.util.data.DataProviders +import javax.inject.Inject + +// TODO(#186): Use an interaction repository to retrieve whether a specific ID corresponds to a terminal interaction. +private const val TERMINAL_INTERACTION_ID = "EndExploration" + +private const val CURRENT_STATE_DATA_PROVIDER_ID = "CurrentStateDataProvider" + +const val TEST_INIT_STATE_NAME = "First State" +const val TEST_MIDDLE_STATE_NAME = "Second State" +const val TEST_END_STATE_NAME = "Last State" + +/** + * 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. + */ +class ExplorationProgressController @Inject constructor( + private val dataProviders: DataProviders, + private val asyncDataSubscriptionManager: AsyncDataSubscriptionManager +) { + // 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 currentExploration: Exploration + private var playingExploration = false + private var isSubmittingAnswer = false + 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) + ) + } + + init { + simulateBeginExploration() + } + + /** Resets this controller to begin playing the specified [Exploration]. */ + internal fun beginExploration(exploration: Exploration) { + stateGraph.resetStateGraph(exploration.statesMap) + stateDeck.resetDeck(stateGraph.getState(exploration.initStateName)) + currentExploration = exploration + playingExploration = true + isSubmittingAnswer = false + } + + internal fun finishExploration() { + playingExploration = false + isSubmittingAnswer = false + } + + /** + * Submits an answer to the current state and returns how the UI should respond to this answer. The returned + * [LiveData] will only have at most two results posted: a pending result, and then a completed success/failure + * result. Failures in this case represent a failure of the app (possibly due to networking conditions). The app + * should report this error in a consumable way to the user so that they may take action on it. No additional values + * will be reported to the [LiveData]. Each call to this method returns a new, distinct, [LiveData] object that must + * be observed. Note also that the returned [LiveData] is not guaranteed to begin with a pending state. + * + * If the app undergoes a configuration change, calling code should rely on the [LiveData] from [getCurrentState] to + * know whether a current answer is pending. That [LiveData] will have its state changed to pending during answer + * submission and until answer resolution. + * + * Submitting an answer may result in the learner staying in the current state, moving to a new state in the + * exploration, being shown a concept card, or being navigated to another exploration altogether. Note that once a + * correct answer is processed, the current state reported to [getCurrentState] will change from a pending state to a + * completed state since the learner completed that card. The learner can then proceed from the current completed + * state to the next pending state using [moveToNextState]. + * + * This method cannot be called until an exploration has started 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. + * + * It's possible for the returned [LiveData] to finish after the [LiveData] from [getCurrentState] is updated. + */ + fun submitAnswer(answer: InteractionObject): LiveData> { + check(playingExploration) { "Cannot submit an answer if an exploration is not being played." } + check(!isSubmittingAnswer) { "Cannot submit an answer while another answer is pending." } + + // Notify observers that the submitted answer is currently pending. + isSubmittingAnswer = true + 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}." + } + stateDeck.pushState(stateGraph.getState(answerOutcome.stateName)) + } + + isSubmittingAnswer = false + asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) + + return MutableLiveData(AsyncResult.success(answerOutcome)) + } + + /** + * Navigates to the previous state in the stack. If the learner is currently on the initial state, this method will + * throw an exception. Calling code is responsible to make sure that this method is not called when it's not possible + * to navigate to a previous card. + * + * This method cannot be called until an exploration has started or an exception will be thrown. + */ + fun moveToPreviousState() { + check(playingExploration) { "Cannot navigate to a previous state if an exploration is not being played." } + stateDeck.navigateToPreviousState() + asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) + } + + /** + * 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. + * + * 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. + * + * This method cannot be called until an exploration has started or an exception will be thrown. + */ + fun moveToNextState() { + check(playingExploration) { "Cannot navigate to a next state if an exploration is not being played." } + stateDeck.navigateToNextState() + asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) + } + + /** + * Returns a [LiveData] monitoring the current [EphemeralState] the learner is currently viewing. If this state + * corresponds to a a terminal state, then the learner has completed the exploration. Note that [moveToPreviousState] + * and [moveToNextState] will automatically update observers of this live data when the next state is navigated to. + * + * Note that the returned [LiveData] is always the same object no matter when this method is called, except + * potentially when a new exploration is started. + * + * This [LiveData] may initially be pending while the exploration object is loaded. It may also switch from a + * completed to a pending result during transient operations like submitting an answer via [submitAnswer]. Calling + * code should be made resilient to this by caching the current state object to display since it may disappear + * temporarily during answer submission. Calling code should persist this state object across configuration changes if + * needed since it cannot rely on this [LiveData] for immediate state reconstitution after configuration changes. + * + * The underlying state returned by this function can only be changed by calls to [moveToNextState] and + * [moveToPreviousState], or the exploration data controller if another exploration is loaded. UI code can be + * confident only calls from the UI layer will trigger state changes here to ensure atomicity between receiving and + * making state changes. + * + * This method is safe to be called before an exploration has started. If there is no ongoing exploration, it should + * return a pending state. + */ + fun getCurrentState(): LiveData> { + return dataProviders.convertToLiveData(currentStateDataProvider) + } + + @Suppress("RedundantSuspendModifier") // DataProviders expects this function to be a suspend function. + private suspend fun retrieveCurrentStateAsync(): AsyncResult { + if (!playingExploration || isSubmittingAnswer) { + return AsyncResult.pending() + } + return AsyncResult.success(stateDeck.getCurrentEphemeralState()) + } + + private fun simulateBeginExploration() { + beginExploration( + Exploration.newBuilder() + .putStates(TEST_INIT_STATE_NAME, createInitState()) + .putStates(TEST_MIDDLE_STATE_NAME, createStateWithTwoOutcomes()) + .putStates(TEST_END_STATE_NAME, createTerminalState()) + .setInitStateName(TEST_INIT_STATE_NAME) + .setObjective("To provide a stub for the UI to reasonably interact with ExplorationProgressController.") + .setTitle("Stub Exploration") + .setLanguageCode("en") + .build() + ) + } + + 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 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 + } + } + + private fun createInitState(): State { + return State.newBuilder() + .setName(TEST_INIT_STATE_NAME) + .setContent(SubtitledHtml.newBuilder().setContentId("state_0_content").setHtml("First State")) + .setInteraction( + Interaction.newBuilder() + .setId("Continue") + .addAnswerGroups( + AnswerGroup.newBuilder() + .setOutcome( + Outcome.newBuilder() + .setDestStateName(TEST_MIDDLE_STATE_NAME) + .setFeedback(SubtitledHtml.newBuilder().setContentId("state_0_feedback").setHtml("Let's continue.")) + ) + ) + ) + .build() + } + + private fun createStateWithTwoOutcomes(): State { + return State.newBuilder() + .setName(TEST_MIDDLE_STATE_NAME) + .setContent(SubtitledHtml.newBuilder().setContentId("state_1_content").setHtml("What language is 'Oppia' from?")) + .setInteraction( + Interaction.newBuilder() + .setId("TextInput") + .addAnswerGroups( + AnswerGroup.newBuilder() + .addRuleSpecs( + RuleSpec.newBuilder() + .putInputs("x", InteractionObject.newBuilder().setNormalizedString("Finnish").build()) + .setRuleType("CaseSensitiveEquals") + ) + .setOutcome( + Outcome.newBuilder() + .setDestStateName(TEST_END_STATE_NAME) + .setFeedback(SubtitledHtml.newBuilder().setContentId("state_1_pos_feedback").setHtml("Correct!")) + ) + ) + .setDefaultOutcome( + Outcome.newBuilder() + .setDestStateName(TEST_MIDDLE_STATE_NAME) + .setFeedback(SubtitledHtml.newBuilder().setContentId("state_1_neg_feedback").setHtml("Not quite right.")) + ) + ) + .build() + } + + private fun createTerminalState(): State { + return State.newBuilder() + .setName(TEST_END_STATE_NAME) + .setContent(SubtitledHtml.newBuilder().setContentId("state_2_content").setHtml("Thanks for playing")) + .setInteraction(Interaction.newBuilder().setId("EndExploration")) + .build() + } + + /** + * Graph that provides lookup access for [State]s and functionality for processing the outcome of a submitted learner + * answer. + */ + private class StateGraph internal constructor(private var stateGraph: Map) { + /** Resets this graph to the new graph represented by the specified [Map]. */ + internal fun resetStateGraph(stateGraph: Map) { + this.stateGraph = stateGraph + } + + /** Returns the [State] corresponding to the specified name. */ + internal fun getState(stateName: String): State { + return stateGraph.getValue(stateName) + } + + /** Returns an [AnswerOutcome] based on the current state and resulting [Outcome] from the learner's answer. */ + internal fun computeAnswerOutcomeForResult(currentState: State, outcome: Outcome): AnswerOutcome { + val answerOutcomeBuilder = AnswerOutcome.newBuilder() + .setFeedback(outcome.feedback) + .setCorrectAnswer(outcome.labelledAsCorrect) + when { + outcome.refresherExplorationId.isNotEmpty() -> + answerOutcomeBuilder.refresherExplorationId = outcome.refresherExplorationId + outcome.missingPrerequisiteSkillId.isNotEmpty() -> + answerOutcomeBuilder.missingPrerequisiteSkillId = outcome.missingPrerequisiteSkillId + outcome.destStateName == currentState.name -> answerOutcomeBuilder.setSameState(true) + else -> answerOutcomeBuilder.stateName = outcome.destStateName + } + return answerOutcomeBuilder.build() + } + } + + private class StateDeck internal constructor(initialState: State) { + private var pendingTopState: State = initialState + private val previousStates: MutableList = ArrayList() + private val currentDialogInteractions: MutableList = ArrayList() + private var stateIndex: Int = 0 + + /** Resets this deck to a new, specified initial [State]. */ + internal fun resetDeck(initialState: State) { + pendingTopState = initialState + previousStates.clear() + currentDialogInteractions.clear() + stateIndex = 0 + } + + /** Navigates to the previous State in the deck, or fails if this isn't possible. */ + internal fun navigateToPreviousState() { + check(!isCurrentStateInitial()) { "Cannot navigate to previous State; at initial state." } + stateIndex-- + } + + /** Navigates to the next State in the deck, or fails if this isn't possible. */ + internal fun navigateToNextState() { + check(!isCurrentStateTopOfDeck()) { "Cannot navigate to next State; at most recent State." } + stateIndex++ + } + + /** + * Returns the [State] corresponding to the latest card in the deck, regardless of whichever State the learner is + * currently viewing. + */ + internal fun getPendingTopState(): State { + return pendingTopState + } + + /** Returns the current [EphemeralState] the learner is viewing. */ + internal fun getCurrentEphemeralState(): EphemeralState { + return when { + stateIndex == previousStates.size -> getCurrentPendingState() + isCurrentStateTerminal() -> getCurrentTerminalState() + else -> getPreviousState() + } + } + + /** + * Pushes a new State onto the deck. This cannot happen if the learner isn't at the most recent State, if the + * current State is not terminal, or if the learner hasn't submitted an answer to the most recent State. This + * operation implies that the most recently submitted answer was the correct answer to the previously current State. + */ + internal fun pushState(state: State) { + check(isCurrentStateTopOfDeck()) { "Cannot push a new State unless the learner is at the most recent State." } + check(!isCurrentStateTerminal()) { "Cannot push another State after reaching a terminal State." } + check(currentDialogInteractions.size != 0) { "Cannot push another State without an answer." } + check(state.name != pendingTopState.name) { "Cannot route from the same State to itself as a new card." } + previousStates += EphemeralState.newBuilder() + .setState(pendingTopState) + .setHasPreviousState(!isCurrentStateInitial()) + .setCompletedState(CompletedState.newBuilder().addAllAnswer(currentDialogInteractions)) + .build() + currentDialogInteractions.clear() + pendingTopState = state + stateIndex++ + } + + /** + * Submits an answer & feedback dialog the learner experience in the current State. This fails if the user is not at + * the most recent State in the deck, or if the most recent State is terminal (since no answer can be submitted to a + * terminal interaction). + */ + internal fun submitAnswer(userAnswer: InteractionObject, feedback: SubtitledHtml) { + check(isCurrentStateTopOfDeck()) { "Cannot submit an answer except to the most recent State." } + check(!isCurrentStateTerminal()) { "Cannot submit an answer to a terminal State." } + currentDialogInteractions += AnswerAndResponse.newBuilder() + .setUserAnswer(userAnswer) + .setFeedback(feedback) + .build() + } + + private fun getCurrentPendingState(): EphemeralState { + return EphemeralState.newBuilder() + .setState(pendingTopState) + .setHasPreviousState(!isCurrentStateInitial()) + .setPendingState(PendingState.newBuilder().addAllWrongAnswer(currentDialogInteractions)) + .build() + } + + private fun getCurrentTerminalState(): EphemeralState { + return EphemeralState.newBuilder() + .setState(pendingTopState) + .setHasPreviousState(!isCurrentStateInitial()) + .setTerminalState(true) + .build() + } + + private fun getPreviousState(): EphemeralState { + return previousStates[stateIndex] + } + + /** Returns whether the current scrolled State is the first State of the exploration. */ + private fun isCurrentStateInitial(): Boolean { + return stateIndex == 0 + } + + /** Returns whether the current scrolled State is the most recent State played by the learner. */ + private fun isCurrentStateTopOfDeck(): Boolean { + return stateIndex == previousStates.size + } + + /** Returns whether the current State is terminal. */ + private fun isCurrentStateTerminal(): Boolean { + // Cards not on top of the deck cannot be terminal/the terminal card must be the last card in the deck, if it's + // present. + return isCurrentStateTopOfDeck() && isTopOfDeckTerminal() + } + + /** Returns whether the most recent card on the deck is terminal. */ + private fun isTopOfDeckTerminal(): Boolean { + return pendingTopState.interaction.id == TERMINAL_INTERACTION_ID + } + } +} diff --git a/domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressController.kt b/domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressController.kt deleted file mode 100644 index 035025c8003..00000000000 --- a/domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressController.kt +++ /dev/null @@ -1,79 +0,0 @@ -package org.oppia.domain.exploration - -import androidx.lifecycle.LiveData -import androidx.lifecycle.MutableLiveData -import org.oppia.app.model.AnswerOutcome -import org.oppia.app.model.EphemeralState -import org.oppia.app.model.InteractionObject -import org.oppia.util.data.AsyncResult - -/** - * 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. - */ -class ExplorationProgressController { - // 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. - - /** - * Submits an answer to the current state and returns how the UI should respond to this answer. The returned - * [LiveData] will only have at most two results posted: a pending result, and then a completed success/failure - * result. Failures in this case represent a failure of the app (possibly due to networking conditions). The app - * should report this error in a consumable way to the user so that they may take action on it. No additional values - * will be reported to the [LiveData]. Each call to this method returns a new, distinct, [LiveData] object that must - * be observed. - * - * If the app undergoes a configuration change, calling code should rely on the [LiveData] from [getCurrentState] to - * know whether a current answer is pending. That [LiveData] will have its state changed to pending during answer - * submission and until answer resolution. - * - * Submitting an answer may result in the learner staying in the current state, moving to a new state in the - * exploration, being shown a concept card, or being navigated to another exploration altogether. Note that once a - * correct answer is processed, the current state reported to [getCurrentState] will change from a pending state to a - * completed state since the learner completed that card. The learner can then proceed from the current completed - * state to the next pending state using [moveToNextState]. - */ - fun submitAnswer(answer: InteractionObject): LiveData> { - return MutableLiveData(AsyncResult.pending()) - } - - /** - * Navigates to the previous state in the stack. If the learner is currently on the initial state, this method will - * throw an exception. Calling code is responsible to make sure that this method is not called when it's not possible - * to navigate to a previous card. - */ - fun moveToPreviousState() {} - - /** - * 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. - * - * 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. - */ - fun moveToNextState() {} - - /** - * Returns a [LiveData] monitoring the current [EphemeralState] the learner is currently viewing. If this state - * corresponds to a a terminal state, then the learner has completed the exploration. Note that [moveToPreviousState] - * and [moveToNextState] will automatically update observers of this live data when the next state is navigated to. - * - * Note that the returned [LiveData] is always the same object no matter when this method is called, except - * potentially when a new exploration is started. - * - * This [LiveData] may initially be pending while the exploration object is loaded. It may also switch from a - * completed to a pending result during transient operations like submitting an answer via [submitAnswer]. Calling - * code should be made resilient to this by caching the current state object to display since it may disappear - * temporarily during answer submission. Calling code should persist this state object across configuration changes if - * needed since it cannot rely on this [LiveData] for immediate state reconstitution after configuration changes. - */ - fun getCurrentState(): LiveData> { - return MutableLiveData(AsyncResult.pending()) - } -} diff --git a/model/src/main/proto/exploration.proto b/model/src/main/proto/exploration.proto index 895cb4d1748..22e290ba0b5 100644 --- a/model/src/main/proto/exploration.proto +++ b/model/src/main/proto/exploration.proto @@ -44,18 +44,20 @@ enum ObjectType { // Structure for a single state // Maps from: data/src/main/java/org/oppia/data/backends/gae/model/GaeState.kt message State { + // The name of the State. + string name = 1; // Mapping from content_id to a VoiceoverMapping - map recorded_voiceovers = 1; - SubtitledHtml content = 2; + map recorded_voiceovers = 2; + SubtitledHtml content = 3; // Mapping from content_id to a TranslationMapping - map written_translations = 3; - repeated ParamChange param_changes = 4; - string classifier_model_id = 5; - Interaction interaction = 6; + map written_translations = 4; + repeated ParamChange param_changes = 5; + string classifier_model_id = 6; + Interaction interaction = 7; // Boolean indicating whether the creator wants to ask for answer details // from the learner about why they picked a particular answer while // playing the exploration. - bool solicit_answer_details = 7; + bool solicit_answer_details = 8; } // Structure for customization args for ParamChange objects. diff --git a/utility/src/main/java/org/oppia/util/data/AsyncDataSubscriptionManager.kt b/utility/src/main/java/org/oppia/util/data/AsyncDataSubscriptionManager.kt index 67adb7bc92e..64d176d2361 100644 --- a/utility/src/main/java/org/oppia/util/data/AsyncDataSubscriptionManager.kt +++ b/utility/src/main/java/org/oppia/util/data/AsyncDataSubscriptionManager.kt @@ -1,8 +1,10 @@ package org.oppia.util.data +import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.async +import kotlinx.coroutines.launch +import org.oppia.util.threading.BackgroundDispatcher import org.oppia.util.threading.ConcurrentQueueMap import org.oppia.util.threading.dequeue import org.oppia.util.threading.enqueue @@ -17,9 +19,12 @@ internal typealias ObserveAsyncChange = suspend () -> Unit * changes to custom [DataProvider]s. */ @Singleton -class AsyncDataSubscriptionManager @Inject constructor() { +class AsyncDataSubscriptionManager @Inject constructor( + @BackgroundDispatcher private val backgroundDispatcher: CoroutineDispatcher +) { private val subscriptionMap = ConcurrentQueueMap() private val associatedIds = ConcurrentQueueMap() + private val backgroundCoroutineScope = CoroutineScope(backgroundDispatcher) /** Subscribes the specified callback function to the specified [DataProvider] ID. */ internal fun subscribe(id: Any, observeChange: ObserveAsyncChange) { @@ -51,18 +56,25 @@ class AsyncDataSubscriptionManager @Inject constructor() { * Notifies all subscribers of the specified [DataProvider] id that the provider has been changed and should be * re-queried for its latest state. */ - @Suppress("DeferredResultUnused") // Exceptions on the main thread will cause app crashes. No action needed. suspend fun notifyChange(id: Any) { // Ensure observed changes are called specifically on the main thread since that's what NotifiableAsyncLiveData // expects. // TODO(#90): Update NotifiableAsyncLiveData so that observeChange() can occur on background threads to avoid any // load on the UI thread until the final data value is ready for delivery. val scope = CoroutineScope(Dispatchers.Main) - scope.async { + scope.launch { subscriptionMap.getQueue(id).forEach { observeChange -> observeChange() } } // Also notify all children observing this parent. associatedIds.getQueue(id).forEach { childId -> notifyChange(childId) } } + + /** + * Same as [notifyChange] except this may be called on the main thread since it will notify changes on a background + * thread. + */ + fun notifyChangeAsync(id: Any) { + backgroundCoroutineScope.launch { notifyChange(id) } + } } From c8eaa6637645dd0fd4892850ee8eacd48a63cc3f Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Fri, 27 Sep 2019 00:27:17 -0700 Subject: [PATCH 003/146] Fix lateinit issue in ExplorationProgressController due to wrongly ordered initialization. --- .../oppia/domain/exploration/ExplorationProgressController.kt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 d099ab634b0..07e2319d10b 100644 --- a/domain/src/main/java/org/oppia/domain/exploration/ExplorationProgressController.kt +++ b/domain/src/main/java/org/oppia/domain/exploration/ExplorationProgressController.kt @@ -67,9 +67,10 @@ class ExplorationProgressController @Inject constructor( /** Resets this controller to begin playing the specified [Exploration]. */ internal fun beginExploration(exploration: Exploration) { + // The exploration must be initialized first since other lazy fields depend on it being inited. + currentExploration = exploration stateGraph.resetStateGraph(exploration.statesMap) stateDeck.resetDeck(stateGraph.getState(exploration.initStateName)) - currentExploration = exploration playingExploration = true isSubmittingAnswer = false } From a0eb3ce1929f8bf9d616dcf273f805c89cf21882 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Sun, 29 Sep 2019 14:05:31 -0700 Subject: [PATCH 004/146] Fix a variaty of issues in the exp progress controller, properly hook it up to the data controller, and start adding tests. --- domain/build.gradle | 1 + .../exploration/ExplorationDataController.kt | 33 ++ .../ExplorationProgressController.kt | 248 +++++++++------ .../exploration/ExplorationRetriever.kt | 95 ++++++ .../ExplorationProgressControllerTest.kt | 286 ++++++++++++++++++ model/src/main/proto/exploration.proto | 19 +- 6 files changed, 577 insertions(+), 105 deletions(-) create mode 100644 domain/src/main/java/org/oppia/domain/exploration/ExplorationDataController.kt create mode 100644 domain/src/main/java/org/oppia/domain/exploration/ExplorationRetriever.kt create mode 100644 domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressControllerTest.kt diff --git a/domain/build.gradle b/domain/build.gradle index f8615018a63..afaac92e097 100644 --- a/domain/build.gradle +++ b/domain/build.gradle @@ -52,6 +52,7 @@ dependencies { 'com.google.dagger:dagger:2.24', 'com.google.truth:truth:0.43', 'junit:junit:4.12', + "org.jetbrains.kotlin:kotlin-reflect:$kotlin_version", 'org.jetbrains.kotlinx:kotlinx-coroutines-android:1.2.2', 'org.jetbrains.kotlinx:kotlinx-coroutines-test:1.2.2', 'org.mockito:mockito-core:2.19.0', diff --git a/domain/src/main/java/org/oppia/domain/exploration/ExplorationDataController.kt b/domain/src/main/java/org/oppia/domain/exploration/ExplorationDataController.kt new file mode 100644 index 00000000000..3a030bcadd7 --- /dev/null +++ b/domain/src/main/java/org/oppia/domain/exploration/ExplorationDataController.kt @@ -0,0 +1,33 @@ +package org.oppia.domain.exploration + +import javax.inject.Inject + +/** + * Controller for loading explorations by ID, or beginning to play an exploration. + * + * At most one exploration may be played at a given time, and its state will be managed by + * [ExplorationProgressController]. + */ +class ExplorationDataController @Inject constructor( + private val explorationProgressController: ExplorationProgressController +) { + /** + * Begins playing an exploration of the specified ID. This method is not expected to fail. + * [ExplorationProgressController] should be used to manage the play state, and monitor the load success/failure of + * the exploration. + * + * 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. + */ + fun startPlayingExploration(explorationId: String) { + explorationProgressController.beginExploration(explorationId) + } + + /** + * 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() + } +} 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 07e2319d10b..b5a9937a11b 100644 --- a/domain/src/main/java/org/oppia/domain/exploration/ExplorationProgressController.kt +++ b/domain/src/main/java/org/oppia/domain/exploration/ExplorationProgressController.kt @@ -3,22 +3,20 @@ package org.oppia.domain.exploration import androidx.lifecycle.LiveData import androidx.lifecycle.MutableLiveData import org.oppia.app.model.AnswerAndResponse -import org.oppia.app.model.AnswerGroup import org.oppia.app.model.AnswerOutcome import org.oppia.app.model.CompletedState import org.oppia.app.model.EphemeralState import org.oppia.app.model.Exploration -import org.oppia.app.model.Interaction import org.oppia.app.model.InteractionObject import org.oppia.app.model.Outcome import org.oppia.app.model.PendingState -import org.oppia.app.model.RuleSpec import org.oppia.app.model.State import org.oppia.app.model.SubtitledHtml import org.oppia.util.data.AsyncDataSubscriptionManager import org.oppia.util.data.AsyncResult import org.oppia.util.data.DataProviders import javax.inject.Inject +import javax.inject.Singleton // TODO(#186): Use an interaction repository to retrieve whether a specific ID corresponds to a terminal interaction. private const val TERMINAL_INTERACTION_ID = "EndExploration" @@ -35,9 +33,11 @@ const val TEST_END_STATE_NAME = "Last State" * * The current exploration session is started via the exploration data controller. */ +@Singleton class ExplorationProgressController @Inject constructor( private val dataProviders: DataProviders, - private val asyncDataSubscriptionManager: AsyncDataSubscriptionManager + private val asyncDataSubscriptionManager: AsyncDataSubscriptionManager, + private val explorationRetriever: ExplorationRetriever ) { // TODO(#180): Add support for hints. // TODO(#179): Add support for parameters. @@ -45,9 +45,9 @@ class ExplorationProgressController @Inject constructor( // 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 playingExploration = false - private var isSubmittingAnswer = false + private var playStage = PlayStage.NOT_PLAYING private val currentStateDataProvider = dataProviders.createInMemoryDataProviderAsync(CURRENT_STATE_DATA_PROVIDER_ID, this::retrieveCurrentStateAsync) private val stateGraph: StateGraph by lazy { @@ -61,23 +61,21 @@ class ExplorationProgressController @Inject constructor( ) } - init { - simulateBeginExploration() - } - /** Resets this controller to begin playing the specified [Exploration]. */ - internal fun beginExploration(exploration: Exploration) { - // The exploration must be initialized first since other lazy fields depend on it being inited. - currentExploration = exploration - stateGraph.resetStateGraph(exploration.statesMap) - stateDeck.resetDeck(stateGraph.getState(exploration.initStateName)) - playingExploration = true - isSubmittingAnswer = false + internal fun beginExploration(explorationId: String) { + check(playStage == PlayStage.NOT_PLAYING) { + "Expected to finish previous exploration before starting a new one." + } + + currentExplorationId = explorationId + advancePlayStageTo(PlayStage.START_LOAD_EXPLORATION) } internal fun finishExploration() { - playingExploration = false - isSubmittingAnswer = false + check(playStage != PlayStage.NOT_PLAYING) { + "Cannot finish playing an exploration that hasn't yet been started" + } + advancePlayStageTo(PlayStage.NOT_PLAYING) } /** @@ -98,18 +96,21 @@ class ExplorationProgressController @Inject constructor( * completed state since the learner completed that card. The learner can then proceed from the current completed * state to the next pending state using [moveToNextState]. * - * This method cannot be called until an exploration has started 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. + * 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. * * It's possible for the returned [LiveData] to finish after the [LiveData] from [getCurrentState] is updated. */ fun submitAnswer(answer: InteractionObject): LiveData> { - check(playingExploration) { "Cannot submit an answer if an exploration is not being played." } - check(!isSubmittingAnswer) { "Cannot submit an answer while another answer is pending." } + 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." + } // Notify observers that the submitted answer is currently pending. - isSubmittingAnswer = true + advancePlayStageTo(PlayStage.SUBMITTING_ANSWER) asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) // TODO(#115): Perform answer classification on a background thread. @@ -124,7 +125,8 @@ class ExplorationProgressController @Inject constructor( stateDeck.pushState(stateGraph.getState(answerOutcome.stateName)) } - isSubmittingAnswer = false + // Return to viewing the state. + advancePlayStageTo(PlayStage.VIEWING_STATE) asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) return MutableLiveData(AsyncResult.success(answerOutcome)) @@ -135,10 +137,19 @@ class ExplorationProgressController @Inject constructor( * throw an exception. Calling code is responsible to make sure that this method is not called when it's not possible * to navigate to a previous card. * - * This method cannot be called until an exploration has started or an exception will be thrown. + * 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(playingExploration) { "Cannot navigate to a previous state if an exploration is not being played." } + 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." + } stateDeck.navigateToPreviousState() asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) } @@ -152,10 +163,19 @@ class ExplorationProgressController @Inject constructor( * 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. * - * This method cannot be called until an exploration has started or an exception will be thrown. + * This method cannot be called until an exploration has been started and [getCurrentState] returns a non-pending + * result or an exception will be thrown. */ fun moveToNextState() { - check(playingExploration) { "Cannot navigate to a next state if an exploration is not being played." } + 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." + } stateDeck.navigateToNextState() asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) } @@ -186,26 +206,49 @@ class ExplorationProgressController @Inject constructor( return dataProviders.convertToLiveData(currentStateDataProvider) } - @Suppress("RedundantSuspendModifier") // DataProviders expects this function to be a suspend function. private suspend fun retrieveCurrentStateAsync(): AsyncResult { - if (!playingExploration || isSubmittingAnswer) { - return AsyncResult.pending() + 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() } - return AsyncResult.success(stateDeck.getCurrentEphemeralState()) } - private fun simulateBeginExploration() { - beginExploration( - Exploration.newBuilder() - .putStates(TEST_INIT_STATE_NAME, createInitState()) - .putStates(TEST_MIDDLE_STATE_NAME, createStateWithTwoOutcomes()) - .putStates(TEST_END_STATE_NAME, createTerminalState()) - .setInitStateName(TEST_INIT_STATE_NAME) - .setObjective("To provide a stub for the UI to reasonably interact with ExplorationProgressController.") - .setTitle("Stub Exploration") - .setLanguageCode("en") - .build() - ) + 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) + } + } + + 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 fun simulateSubmitAnswer(currentState: State, answer: InteractionObject): Outcome { @@ -227,60 +270,71 @@ class ExplorationProgressController @Inject constructor( } } - private fun createInitState(): State { - return State.newBuilder() - .setName(TEST_INIT_STATE_NAME) - .setContent(SubtitledHtml.newBuilder().setContentId("state_0_content").setHtml("First State")) - .setInteraction( - Interaction.newBuilder() - .setId("Continue") - .addAnswerGroups( - AnswerGroup.newBuilder() - .setOutcome( - Outcome.newBuilder() - .setDestStateName(TEST_MIDDLE_STATE_NAME) - .setFeedback(SubtitledHtml.newBuilder().setContentId("state_0_feedback").setHtml("Let's continue.")) - ) - ) - ) - .build() + /** + * 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 + } + } } - private fun createStateWithTwoOutcomes(): State { - return State.newBuilder() - .setName(TEST_MIDDLE_STATE_NAME) - .setContent(SubtitledHtml.newBuilder().setContentId("state_1_content").setHtml("What language is 'Oppia' from?")) - .setInteraction( - Interaction.newBuilder() - .setId("TextInput") - .addAnswerGroups( - AnswerGroup.newBuilder() - .addRuleSpecs( - RuleSpec.newBuilder() - .putInputs("x", InteractionObject.newBuilder().setNormalizedString("Finnish").build()) - .setRuleType("CaseSensitiveEquals") - ) - .setOutcome( - Outcome.newBuilder() - .setDestStateName(TEST_END_STATE_NAME) - .setFeedback(SubtitledHtml.newBuilder().setContentId("state_1_pos_feedback").setHtml("Correct!")) - ) - ) - .setDefaultOutcome( - Outcome.newBuilder() - .setDestStateName(TEST_MIDDLE_STATE_NAME) - .setFeedback(SubtitledHtml.newBuilder().setContentId("state_1_neg_feedback").setHtml("Not quite right.")) - ) - ) - .build() - } + /** Different stages in which the progress controller can exist. */ + private enum class PlayStage { + /** No exploration is currently being played. */ + 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, + + /** The controller is currently viewing a State. */ + VIEWING_STATE, - private fun createTerminalState(): State { - return State.newBuilder() - .setName(TEST_END_STATE_NAME) - .setContent(SubtitledHtml.newBuilder().setContentId("state_2_content").setHtml("Thanks for playing")) - .setInteraction(Interaction.newBuilder().setId("EndExploration")) - .build() + /** The controller is in the process of submitting an answer. */ + SUBMITTING_ANSWER } /** diff --git a/domain/src/main/java/org/oppia/domain/exploration/ExplorationRetriever.kt b/domain/src/main/java/org/oppia/domain/exploration/ExplorationRetriever.kt new file mode 100644 index 00000000000..a1e2e4b847d --- /dev/null +++ b/domain/src/main/java/org/oppia/domain/exploration/ExplorationRetriever.kt @@ -0,0 +1,95 @@ +package org.oppia.domain.exploration + +import org.oppia.app.model.AnswerGroup +import org.oppia.app.model.Exploration +import org.oppia.app.model.Interaction +import org.oppia.app.model.InteractionObject +import org.oppia.app.model.Outcome +import org.oppia.app.model.RuleSpec +import org.oppia.app.model.State +import org.oppia.app.model.SubtitledHtml +import javax.inject.Inject + +const val TEST_EXPLORATION_ID_5 = "test_exp_id_5" + +// TODO(#59): Make this class inaccessible outside of the domain package. UI code should not depend on it. + +/** Internal class for actually retrieving an exploration object for uses in domain controllers. */ +class ExplorationRetriever @Inject constructor() { + /** Loads and returns an exploration for the specified exploration ID, or fails. */ + @Suppress("RedundantSuspendModifier") // Force callers to call this on a background thread. + internal suspend fun loadExploration(explorationId: String): Exploration { + // TODO(#193): Load this exploration from a file, instead. + check(explorationId == TEST_EXPLORATION_ID_5) { "Invalid exploration ID: $explorationId" } + return createTestExploration0() + } + + private fun createTestExploration0(): Exploration { + return Exploration.newBuilder() + .setId(TEST_EXPLORATION_ID_5) + .putStates(TEST_INIT_STATE_NAME, createInitState()) + .putStates(TEST_MIDDLE_STATE_NAME, createStateWithTwoOutcomes()) + .putStates(TEST_END_STATE_NAME, createTerminalState()) + .setInitStateName(TEST_INIT_STATE_NAME) + .setObjective("To provide a stub for the UI to reasonably interact with ExplorationProgressController.") + .setTitle("Stub Exploration") + .setLanguageCode("en") + .build() + } + + private fun createInitState(): State { + return State.newBuilder() + .setName(TEST_INIT_STATE_NAME) + .setContent(SubtitledHtml.newBuilder().setContentId("state_0_content").setHtml("First State")) + .setInteraction( + Interaction.newBuilder() + .setId("Continue") + .addAnswerGroups( + AnswerGroup.newBuilder() + .setOutcome( + Outcome.newBuilder() + .setDestStateName(TEST_MIDDLE_STATE_NAME) + .setFeedback(SubtitledHtml.newBuilder().setContentId("state_0_feedback").setHtml("Let's continue.")) + ) + ) + ) + .build() + } + + private fun createStateWithTwoOutcomes(): State { + return State.newBuilder() + .setName(TEST_MIDDLE_STATE_NAME) + .setContent(SubtitledHtml.newBuilder().setContentId("state_1_content").setHtml("What language is 'Oppia' from?")) + .setInteraction( + Interaction.newBuilder() + .setId("TextInput") + .addAnswerGroups( + AnswerGroup.newBuilder() + .addRuleSpecs( + RuleSpec.newBuilder() + .putInputs("x", InteractionObject.newBuilder().setNormalizedString("Finnish").build()) + .setRuleType("CaseSensitiveEquals") + ) + .setOutcome( + Outcome.newBuilder() + .setDestStateName(TEST_END_STATE_NAME) + .setFeedback(SubtitledHtml.newBuilder().setContentId("state_1_pos_feedback").setHtml("Correct!")) + ) + ) + .setDefaultOutcome( + Outcome.newBuilder() + .setDestStateName(TEST_MIDDLE_STATE_NAME) + .setFeedback(SubtitledHtml.newBuilder().setContentId("state_1_neg_feedback").setHtml("Not quite right.")) + ) + ) + .build() + } + + private fun createTerminalState(): State { + return State.newBuilder() + .setName(TEST_END_STATE_NAME) + .setContent(SubtitledHtml.newBuilder().setContentId("state_2_content").setHtml("Thanks for playing")) + .setInteraction(Interaction.newBuilder().setId("EndExploration")) + .build() + } +} diff --git a/domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressControllerTest.kt b/domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressControllerTest.kt new file mode 100644 index 00000000000..f443d551597 --- /dev/null +++ b/domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressControllerTest.kt @@ -0,0 +1,286 @@ +package org.oppia.domain.exploration + +import android.app.Application +import android.content.Context +import androidx.lifecycle.Observer +import androidx.test.core.app.ApplicationProvider +import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.google.common.truth.Truth.assertThat +import dagger.BindsInstance +import dagger.Component +import dagger.Module +import dagger.Provides +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.TestCoroutineDispatcher +import kotlinx.coroutines.test.runBlockingTest +import org.junit.Assert.fail +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.ArgumentCaptor +import org.mockito.Captor +import org.mockito.Mock +import org.mockito.Mockito.atLeast +import org.mockito.Mockito.atLeastOnce +import org.mockito.Mockito.verify +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.util.data.AsyncResult +import org.oppia.util.threading.BackgroundDispatcher +import org.robolectric.annotation.Config +import java.lang.IllegalStateException +import javax.inject.Inject +import javax.inject.Qualifier +import javax.inject.Singleton +import kotlin.coroutines.EmptyCoroutineContext +import kotlin.reflect.KClass +import kotlin.reflect.full.cast + +/** Tests for [ExplorationProgressController]. */ +@RunWith(AndroidJUnit4::class) +@Config(manifest = Config.NONE) +class ExplorationProgressControllerTest { + // TODO(#114): Add much more thorough tests for the integration pathway. + + @Rule + @JvmField + val mockitoRule: MockitoRule = MockitoJUnit.rule() + + @Inject + lateinit var explorationDataController: ExplorationDataController + + @Inject + lateinit var explorationProgressController: ExplorationProgressController + + @Inject + @field:TestDispatcher + lateinit var testDispatcher: TestCoroutineDispatcher + + @Mock + lateinit var mockCurrentStateLiveDataObserver: Observer> + + @Captor + lateinit var currentStateResultCaptor: ArgumentCaptor> + + private val coroutineContext by lazy { + EmptyCoroutineContext + testDispatcher + } + + @Before + @ExperimentalCoroutinesApi + fun setUp() { + setUpTestApplicationComponent() + + // Require coroutines to be flushed to avoid synchronous execution that could interfere with testing ordered async + // logic that behaves differently in prod. + testDispatcher.pauseDispatcher() + } + + @Test + @ExperimentalCoroutinesApi + fun testGetCurrentState_noExploration_isPending() = runBlockingTest(coroutineContext) { + val currentStateLiveData = explorationProgressController.getCurrentState() + + currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) + advanceUntilIdle() + + verify(mockCurrentStateLiveDataObserver, atLeastOnce()).onChanged(currentStateResultCaptor.capture()) + assertThat(currentStateResultCaptor.value.isPending()).isTrue() + } + + @Test + @ExperimentalCoroutinesApi + fun testGetCurrentState_playInvalidExploration_returnsFailure() = runBlockingTest(coroutineContext) { + val currentStateLiveData = explorationProgressController.getCurrentState() + currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) + + explorationDataController.startPlayingExploration("invalid_exp_id") + advanceUntilIdle() + + verify(mockCurrentStateLiveDataObserver, atLeastOnce()).onChanged(currentStateResultCaptor.capture()) + assertThat(currentStateResultCaptor.value.isFailure()).isTrue() + assertThat(currentStateResultCaptor.value.getErrorOrNull()) + .hasMessageThat() + .contains("Invalid exploration ID: invalid_exp_id") + } + + @Test + @ExperimentalCoroutinesApi + fun testGetCurrentState_playExploration_returnsPendingResultFromLoadingExploration() = runBlockingTest( + coroutineContext + ) { + explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) + + val currentStateLiveData = explorationProgressController.getCurrentState() + currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) + advanceUntilIdle() + + // The second-to-latest result stays pending since the exploration was loading (the actual result is the fully + // loaded exploration). + verify(mockCurrentStateLiveDataObserver, atLeast(2)).onChanged(currentStateResultCaptor.capture()) + val results = currentStateResultCaptor.allValues + assertThat(results[results.size - 2].isPending()).isTrue() + } + + @Test + @ExperimentalCoroutinesApi + fun testGetCurrentState_playExploration_loaded_returnsInitialStatePending() = runBlockingTest( + coroutineContext + ) { + explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) + + val currentStateLiveData = explorationProgressController.getCurrentState() + currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) + advanceUntilIdle() + + verify(mockCurrentStateLiveDataObserver, atLeastOnce()).onChanged(currentStateResultCaptor.capture()) + 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) + } + + @Test + @ExperimentalCoroutinesApi + fun testGetCurrentState_playInvalidExploration_thenPlayValidExp_returnsInitialPendingState() = runBlockingTest( + coroutineContext + ) { + // Start with playing an invalid exploration. + explorationDataController.startPlayingExploration("invalid_exp_id") + explorationDataController.stopPlayingExploration() + + // Then a valid one. + explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) + val currentStateLiveData = explorationProgressController.getCurrentState() + currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) + advanceUntilIdle() + + // The latest result should correspond to the valid ID, and the progress controller should gracefully recover. + verify(mockCurrentStateLiveDataObserver, atLeastOnce()).onChanged(currentStateResultCaptor.capture()) + 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) + } + + @Test + @ExperimentalCoroutinesApi + fun testFinishExploration_beforePlaying_fails() = runBlockingTest(coroutineContext) { + val exception = assertThrows(IllegalStateException::class) { explorationDataController.stopPlayingExploration() } + + assertThat(exception).hasMessageThat().contains("Cannot finish playing an exploration that hasn't yet been started") + } + + @Test + @ExperimentalCoroutinesApi + fun testPlayExploration_withoutFinishingPrevious_fails() = 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) + } + + assertThat(exception) + .hasMessageThat() + .contains("Expected to finish previous exploration before starting a new one.") + } + + // testGetCurrentState_playSecondExploration_afterFinishingPrevious_loaded_returnsInitialState + // testSubmitAnswer_forContinueButton_returnsAnswerIsCorrect + // testGetCurrentState_whileSubmittingAnswer_becomesPending + // 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 + // testGetCurrentState_afterMoveToPrevious_onSecondState_updatesToCompletedFirstState + // testGetCurrentState_afterMoveToPrevious_onThirdState_updatesToCompletedSecondState + // testMoveToNext_beforePlaying_fails + // testMoveToNext_whileLoadingExploration_fails + // testMoveToNext_whileSubmittingAnswer_fails + // testMoveToNext_onFinalState_fails + // testMoveToNext_forPendingInitialState_fails + // testGetCurrentState_afterMoveToNext_onCompletedFirstState_updatesToPendingSecondState + // testGetCurrentState_afterMoveToNext_onCompletedSecondState_updatesToTerminalThirdState + // testGetCurrentState_afterMovePreviousAndNext_returnsCurrentState + // testGetCurrentState_afterMoveNextAndPrevious_returnsCurrentState + // testGetCurrentState_afterMoveToPrevious_onSecondState_newObserver_receivesCompletedFirstState + + private fun setUpTestApplicationComponent() { + DaggerExplorationProgressControllerTest_TestApplicationComponent.builder() + .setApplication(ApplicationProvider.getApplicationContext()) + .build() + .inject(this) + } + + // TODO(#89): Move to a common test library. + /** A replacement to JUnit5's assertThrows(). */ + private fun assertThrows(type: KClass, operation: () -> Unit): T { + try { + operation() + fail("Expected to encounter exception of $type") + throw IllegalStateException("JUnit failed to interrupt execution flow.") + } catch (t: Throwable) { + if (type.isInstance(t)) { + return type.cast(t) + } + // Unexpected exception; throw it. + throw t + } + } + + @Qualifier annotation class TestDispatcher + + // TODO(#89): Move this to a common test application component. + @Module + class TestModule { + @Provides + @Singleton + fun provideContext(application: Application): Context { + return application + } + + @ExperimentalCoroutinesApi + @Singleton + @Provides + @TestDispatcher + fun provideTestDispatcher(): TestCoroutineDispatcher { + return TestCoroutineDispatcher() + } + + @Singleton + @Provides + @BackgroundDispatcher + fun provideBackgroundDispatcher(@TestDispatcher testDispatcher: TestCoroutineDispatcher): CoroutineDispatcher { + return testDispatcher + } + } + + // TODO(#89): Move this to a common test application component. + @Singleton + @Component(modules = [TestModule::class]) + interface TestApplicationComponent { + @Component.Builder + interface Builder { + @BindsInstance + fun setApplication(application: Application): Builder + + fun build(): TestApplicationComponent + } + + fun inject(explorationProgressControllerTest: ExplorationProgressControllerTest) + } +} diff --git a/model/src/main/proto/exploration.proto b/model/src/main/proto/exploration.proto index 22e290ba0b5..43beb9be733 100644 --- a/model/src/main/proto/exploration.proto +++ b/model/src/main/proto/exploration.proto @@ -11,15 +11,18 @@ option java_multiple_files = true; // Structure for a single exploration. // Maps from: data/src/main/java/org/oppia/data/backends/gae/model/GaeExploration.kt message Exploration { + // The ID of the exploration. + string id = 1; + // Mapping from a state name to a state object - map states = 1; - repeated ParamChange param_changes = 2; - repeated ParamSpec param_specs = 3; - string init_state_name = 4; - string objective = 5; - bool correctness_feedback_enabled = 6; - string title = 7; - string language_code = 8; + map states = 2; + repeated ParamChange param_changes = 3; + repeated ParamSpec param_specs = 4; + string init_state_name = 5; + string objective = 6; + bool correctness_feedback_enabled = 7; + string title = 8; + string language_code = 9; } // Structure for a param change. From 41141b6ebe730bf348a989e3cc9b010d45bd204f Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Mon, 30 Sep 2019 21:59:38 -0700 Subject: [PATCH 005/146] Created a separate ExplorationRetriever, hooked up AnswerClassificationController, and attempted to make ExplorationProgressController thread-safe. The thread-safety led to significant interface changes in the progress controller, and led to discovering some issues with the mediator live data approach to interop coroutines and LiveData. This locking mechanism will need to change since the optimal solution requires resolving #90. --- .../AnswerClassificationController.kt | 45 +- .../exploration/ExplorationDataController.kt | 19 +- .../ExplorationProgressController.kt | 393 +++++++++--------- .../exploration/ExplorationRetriever.kt | 12 +- .../AnswerClassificationControllerTest.kt | 26 +- .../ExplorationDataControllerTest.kt | 13 +- .../ExplorationProgressControllerTest.kt | 128 ++++-- model/src/main/proto/exploration.proto | 5 +- .../java/org/oppia/util/data/DataProviders.kt | 19 +- .../oppia/util/data/InMemoryBlockingCache.kt | 14 + 10 files changed, 426 insertions(+), 248 deletions(-) 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. */ From 5f326fc16f54448394022f34cfee37135c474ee3 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Wed, 2 Oct 2019 22:43:49 -0700 Subject: [PATCH 006/146] Change the locking mechanism for ExplorationProgressController to work with the current MediatorLiveData implementation (see #90 for more context). Fix existing progress controller tests and add a few more. All current progress controller tests are passing. --- .../exploration/ExplorationDataController.kt | 23 +- .../ExplorationProgressController.kt | 201 ++++++++++-------- .../ExplorationProgressControllerTest.kt | 128 ++++++++++- .../java/org/oppia/util/data/DataProviders.kt | 14 -- 4 files changed, 245 insertions(+), 121 deletions(-) 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 d0a53ef356d..142ed016b8b 100644 --- a/domain/src/main/java/org/oppia/domain/exploration/ExplorationDataController.kt +++ b/domain/src/main/java/org/oppia/domain/exploration/ExplorationDataController.kt @@ -1,14 +1,13 @@ package org.oppia.domain.exploration import androidx.lifecycle.LiveData +import androidx.lifecycle.MutableLiveData import org.oppia.app.model.Exploration import org.oppia.util.data.AsyncResult 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. @@ -41,10 +40,12 @@ class ExplorationDataController @Inject constructor( * fail to load, but this provides early-failure detection. */ 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) + return try { + explorationProgressController.beginExplorationAsync(explorationId) + MutableLiveData(AsyncResult.success(null)) + } catch (e: Exception) { + MutableLiveData(AsyncResult.failed(e)) + } } /** @@ -52,10 +53,12 @@ class ExplorationDataController @Inject constructor( * active exploration is being played, otherwise an exception will be thrown. */ fun stopPlayingExploration(): LiveData> { - val operation = explorationProgressController.finishExplorationAsync() - val dataProvider = dataProviders.createDeferredDataProviderAsync( - STOP_PLAYING_EXPLORATION_RESULT_DATA_PROVIDER_ID, operation) - return dataProviders.convertToLiveData(dataProvider) + return try { + explorationProgressController.finishExplorationAsync() + MutableLiveData(AsyncResult.success(null)) + } catch (e: Exception) { + MutableLiveData(AsyncResult.failed(e)) + } } @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 73a86207cff..92c35fea302 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 kotlinx.coroutines.Deferred +import androidx.lifecycle.MutableLiveData import org.oppia.app.model.AnswerAndResponse import org.oppia.app.model.AnswerOutcome import org.oppia.app.model.CompletedState @@ -16,17 +16,15 @@ 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 java.util.concurrent.locks.ReentrantLock import javax.inject.Inject import javax.inject.Singleton +import kotlin.concurrent.withLock // TODO(#186): Use an interaction repository to retrieve whether a specific ID corresponds to a terminal interaction. 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 @@ -34,15 +32,11 @@ private const val MOVE_TO_PREVIOUS_STATE_RESULT_DATA_PROVIDER_ID = "MoveToPrevio * * 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. + * This class is thread-safe, but the order of applied operations is arbitrary. Calling code should take care to ensure + * that uses of this class do not specifically depend on ordering. */ @Singleton class ExplorationProgressController @Inject constructor( - inMemoryBlockingCacheFactory: InMemoryBlockingCache.Factory, private val dataProviders: DataProviders, private val asyncDataSubscriptionManager: AsyncDataSubscriptionManager, private val explorationRetriever: ExplorationRetriever, @@ -52,31 +46,35 @@ class ExplorationProgressController @Inject constructor( // TODO(#179): Add support for parameters. // TODO(#181): Add support for solutions. // TODO(#182): Add support for refresher explorations. + // TODO(#90): Update the internal locking of this controller to use something like an in-memory blocking cache to + // simplify state locking. However, doing this correctly requires a fix in MediatorLiveData to avoid unexpected + // cancellations in chained cross-scope coroutines. private val currentStateDataProvider = dataProviders.createInMemoryDataProviderAsync(CURRENT_STATE_DATA_PROVIDER_ID, this::retrieveCurrentStateAsync) - private val explorationProgress = inMemoryBlockingCacheFactory.create(ExplorationProgress()) + private val explorationProgress = ExplorationProgress() + private val explorationProgressLock = ReentrantLock() /** Resets this controller to begin playing the specified [Exploration]. */ - internal fun beginExplorationAsync(explorationId: String): Deferred<*> { - return explorationProgress.updateInPlaceIfPresentAsync { progress -> - check(progress.playStage == PlayStage.NOT_PLAYING) { + internal fun beginExplorationAsync(explorationId: String) { + explorationProgressLock.withLock { + check(explorationProgress.playStage == PlayStage.NOT_PLAYING) { "Expected to finish previous exploration before starting a new one." } - progress.currentExplorationId = explorationId - progress.advancePlayStageTo(PlayStage.LOADING_EXPLORATION) + explorationProgress.currentExplorationId = explorationId + explorationProgress.advancePlayStageTo(PlayStage.LOADING_EXPLORATION) asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) } } /** Indicates that the current exploration being played is now completed. */ - internal fun finishExplorationAsync(): Deferred<*> { - return explorationProgress.updateInPlaceIfPresentAsync { progress -> - check(progress.playStage != PlayStage.NOT_PLAYING) { + internal fun finishExplorationAsync() { + explorationProgressLock.withLock { + check(explorationProgress.playStage != PlayStage.NOT_PLAYING) { "Cannot finish playing an exploration that hasn't yet been started" } - progress.advancePlayStageTo(PlayStage.NOT_PLAYING) + explorationProgress.advancePlayStageTo(PlayStage.NOT_PLAYING) } } @@ -107,39 +105,40 @@ class ExplorationProgressController @Inject constructor( * that point. */ fun submitAnswer(answer: InteractionObject): LiveData> { - 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. - progress.advancePlayStageTo(PlayStage.SUBMITTING_ANSWER) - asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) + try { + explorationProgressLock.withLock { + check(explorationProgress.playStage != PlayStage.NOT_PLAYING) { + "Cannot submit an answer if an exploration is not being played." + } + check(explorationProgress.playStage != PlayStage.SUBMITTING_ANSWER) { + "Cannot submit an answer while another answer is pending." + } + check(explorationProgress.playStage != PlayStage.LOADING_EXPLORATION) { + "Cannot submit an answer while the exploration is being loaded." + } - 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)) - } + // Notify observers that the submitted answer is currently pending. + explorationProgress.advancePlayStageTo(PlayStage.SUBMITTING_ANSWER) + asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) + + val topPendingState = explorationProgress.stateDeck.getPendingTopState() + val outcome = answerClassificationController.classify(topPendingState, answer) + val answerOutcome = explorationProgress.stateGraph.computeAnswerOutcomeForResult(topPendingState, outcome) + explorationProgress.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) { + explorationProgress.stateDeck.pushState(explorationProgress.stateGraph.getState(answerOutcome.stateName)) + } - // Return to viewing the state. - progress.advancePlayStageTo(PlayStage.VIEWING_STATE) - asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) + // Return to viewing the state. + explorationProgress.advancePlayStageTo(PlayStage.VIEWING_STATE) + asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) - answerOutcome // Propagate the classification result. + return MutableLiveData(AsyncResult.success(answerOutcome)) + } + } catch (e: Exception) { + return MutableLiveData(AsyncResult.failed(e)) } - - val dataProvider = dataProviders.createDeferredDataProviderAsync(SUBMIT_ANSWER_RESULT_DATA_PROVIDER_ID, operation) - return dataProviders.convertToLiveData(dataProvider) } /** @@ -161,23 +160,24 @@ class ExplorationProgressController @Inject constructor( * 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." + try { + explorationProgressLock.withLock { + check(explorationProgress.playStage != PlayStage.NOT_PLAYING) { + "Cannot navigate to a previous state if an exploration is not being played." + } + check(explorationProgress.playStage != PlayStage.LOADING_EXPLORATION) { + "Cannot navigate to a previous state if an exploration is being loaded." + } + check(explorationProgress.playStage != PlayStage.SUBMITTING_ANSWER) { + "Cannot navigate to a previous state if an answer submission is pending." + } + explorationProgress.stateDeck.navigateToPreviousState() + asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) } - progress.stateDeck.navigateToPreviousState() - asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) + return MutableLiveData(AsyncResult.success(null)) + } catch (e: Exception) { + return MutableLiveData(AsyncResult.failed(e)) } - - val dataProvider = dataProviders.createDeferredDataProviderAsync( - MOVE_TO_NEXT_STATE_RESULT_DATA_PROVIDER_ID, operation) - return dataProviders.convertToLiveData(dataProvider) } /** @@ -195,23 +195,24 @@ class ExplorationProgressController @Inject constructor( * [getCurrentState] for observing a successful transition to another state. */ 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." + try { + explorationProgressLock.withLock { + check(explorationProgress.playStage != PlayStage.NOT_PLAYING) { + "Cannot navigate to a next state if an exploration is not being played." + } + check(explorationProgress.playStage != PlayStage.LOADING_EXPLORATION) { + "Cannot navigate to a next state if an exploration is being loaded." + } + check(explorationProgress.playStage != PlayStage.SUBMITTING_ANSWER) { + "Cannot navigate to a next state if an answer submission is pending." + } + explorationProgress.stateDeck.navigateToNextState() + asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) } - progress.stateDeck.navigateToNextState() - asyncDataSubscriptionManager.notifyChangeAsync(CURRENT_STATE_DATA_PROVIDER_ID) + return MutableLiveData(AsyncResult.success(null)) + } catch (e: Exception) { + return MutableLiveData(AsyncResult.failed(e)) } - - val dataProvider = dataProviders.createDeferredDataProviderAsync( - MOVE_TO_PREVIOUS_STATE_RESULT_DATA_PROVIDER_ID, operation) - return dataProviders.convertToLiveData(dataProvider) } /** @@ -242,33 +243,55 @@ class ExplorationProgressController @Inject constructor( private suspend fun retrieveCurrentStateAsync(): AsyncResult { return try { - retrieveCurrentStateWithinCacheAsync().await() + retrieveCurrentStateWithinCacheAsync() } catch (e: Exception) { AsyncResult.failed(e) } } - private suspend fun retrieveCurrentStateWithinCacheAsync(): Deferred> { - return explorationProgress.updateInPlaceIfPresentAsync { progress -> - when (progress.playStage) { + private suspend fun retrieveCurrentStateWithinCacheAsync(): AsyncResult { + var explorationId: String? = null + lateinit var currentStage: PlayStage + explorationProgressLock.withLock { + currentStage = explorationProgress.playStage + if (currentStage == PlayStage.LOADING_EXPLORATION) { + explorationId = explorationProgress.currentExplorationId + } + } + + val exploration: Exploration? = + if (explorationId != null) explorationRetriever.loadExploration(explorationId!!) else null + + explorationProgressLock.withLock { + // It's possible for the exploration ID or stage to change between critical sections. However, this is the only + // way to ensure the exploration is loaded since suspended functions cannot be called within a mutex. + check(exploration == null || explorationProgress.currentExplorationId == explorationId) { + "Encountered race condition when retrieving exploration. ID changed from $explorationId" + + " to ${explorationProgress.currentExplorationId}" + } + check(explorationProgress.playStage == currentStage) { + "Encountered race condition when retrieving exploration. ID changed from $explorationId" + + " to ${explorationProgress.currentExplorationId}" + } + return when (explorationProgress.playStage) { PlayStage.NOT_PLAYING -> AsyncResult.pending() PlayStage.LOADING_EXPLORATION -> { try { - finishLoadExploration(progress) - AsyncResult.success(progress.stateDeck.getCurrentEphemeralState()) + // The exploration must be available for this stage since it was loaded above. + finishLoadExploration(exploration!!, explorationProgress) + AsyncResult.success(explorationProgress.stateDeck.getCurrentEphemeralState()) } catch (e: Exception) { AsyncResult.failed(e) } } - PlayStage.VIEWING_STATE -> AsyncResult.success(progress.stateDeck.getCurrentEphemeralState()) + PlayStage.VIEWING_STATE -> AsyncResult.success(explorationProgress.stateDeck.getCurrentEphemeralState()) PlayStage.SUBMITTING_ANSWER -> AsyncResult.pending() } } } - private suspend fun finishLoadExploration(progress: ExplorationProgress) { + private fun finishLoadExploration(exploration: Exploration, 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)) 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 99b2183d9ff..207355a7c40 100644 --- a/domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressControllerTest.kt @@ -27,9 +27,11 @@ import org.mockito.Mockito.atLeastOnce import org.mockito.Mockito.verify import org.mockito.junit.MockitoJUnit import org.mockito.junit.MockitoRule +import org.oppia.app.model.AnswerOutcome import org.oppia.app.model.EphemeralState import org.oppia.app.model.EphemeralState.StateTypeCase.PENDING_STATE import org.oppia.app.model.Exploration +import org.oppia.app.model.InteractionObject import org.oppia.util.data.AsyncResult import org.oppia.util.threading.BackgroundDispatcher import org.oppia.util.threading.BlockingDispatcher @@ -72,12 +74,18 @@ class ExplorationProgressControllerTest { @Mock lateinit var mockAsyncResultLiveDataObserver: Observer> + @Mock + lateinit var mockAsyncAnswerOutcomeObserver: Observer> + @Captor lateinit var currentStateResultCaptor: ArgumentCaptor> @Captor lateinit var asyncResultCaptor: ArgumentCaptor> + @Captor + lateinit var asyncAnswerOutcomeCaptor: ArgumentCaptor> + @ExperimentalCoroutinesApi private val coroutineContext by lazy { EmptyCoroutineContext + testDispatcher @@ -149,14 +157,15 @@ class ExplorationProgressControllerTest { fun testGetCurrentState_playExploration_returnsPendingResultFromLoadingExploration() = runBlockingTest( coroutineContext ) { - explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) - val currentStateLiveData = explorationProgressController.getCurrentState() currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) advanceUntilIdle() + explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) + advanceUntilIdle() + // The second-to-latest result stays pending since the exploration was loading (the actual result is the fully - // loaded exploration). + // loaded exploration). This is only true if the observer begins before starting to load the exploration. verify(mockCurrentStateLiveDataObserver, atLeast(2)).onChanged(currentStateResultCaptor.capture()) val results = currentStateResultCaptor.allValues assertThat(results[results.size - 2].isPending()).isTrue() @@ -236,11 +245,114 @@ class ExplorationProgressControllerTest { .contains("Expected to finish previous exploration before starting a new one.") } - // testGetCurrentState_playSecondExploration_afterFinishingPrevious_loaded_returnsInitialState - // testSubmitAnswer_forMultipleChoice_correctAnswer_succeeds - // testSubmitAnswer_forMultipleChoice_wrongAnswer_succeeds - // testSubmitAnswer_forMultipleChoice_correctAnswer_returnsOutcomeWithTransition - // testSubmitAnswer_forMultipleChoice_wrongAnswer_returnsDefaultOutcome + @Test + @ExperimentalCoroutinesApi + fun testGetCurrentState_playSecondExploration_afterFinishingPrevious_loaded_returnsInitialState() = runBlockingTest( + coroutineContext + ) { + val currentStateLiveData = explorationProgressController.getCurrentState() + currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) + // Start with playing a valid exploration, then stop. + explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) + explorationDataController.stopPlayingExploration() + advanceUntilIdle() + + // Then another valid one. + explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_6) + advanceUntilIdle() + + // The latest result should correspond to the valid ID, and the progress controller should gracefully recover. + verify(mockCurrentStateLiveDataObserver, atLeastOnce()).onChanged(currentStateResultCaptor.capture()) + 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(getTestExploration6().initStateName) + } + + @Test + @ExperimentalCoroutinesApi + fun testSubmitAnswer_forMultipleChoice_correctAnswer_succeeds() = runBlockingTest( + coroutineContext + ) { + val currentStateLiveData = explorationProgressController.getCurrentState() + currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) + explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) + advanceUntilIdle() + + // The current interaction is multiple choice. + val result = explorationProgressController.submitAnswer(InteractionObject.newBuilder().setNonNegativeInt(0).build()) + result.observeForever(mockAsyncAnswerOutcomeObserver) + advanceUntilIdle() + + // Verify that the answer submission was successful. + verify(mockAsyncAnswerOutcomeObserver, atLeastOnce()).onChanged(asyncAnswerOutcomeCaptor.capture()) + assertThat(asyncAnswerOutcomeCaptor.value.isSuccess()).isTrue() + } + + @Test + @ExperimentalCoroutinesApi + fun testSubmitAnswer_forMultipleChoice_correctAnswer_returnsOutcomeWithTransition() = runBlockingTest( + coroutineContext + ) { + val currentStateLiveData = explorationProgressController.getCurrentState() + currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) + explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) + advanceUntilIdle() + + // The current interaction is multiple choice. + val result = explorationProgressController.submitAnswer(InteractionObject.newBuilder().setNonNegativeInt(0).build()) + result.observeForever(mockAsyncAnswerOutcomeObserver) + advanceUntilIdle() + + // Verify that the answer submission was successful. + verify(mockAsyncAnswerOutcomeObserver, atLeastOnce()).onChanged(asyncAnswerOutcomeCaptor.capture()) + val answerOutcome = asyncAnswerOutcomeCaptor.value.getOrThrow() + assertThat(answerOutcome.destinationCase).isEqualTo(AnswerOutcome.DestinationCase.STATE_NAME) + assertThat(answerOutcome.feedback.html).isEqualTo("Yes!") + } + + @Test + @ExperimentalCoroutinesApi + fun testSubmitAnswer_forMultipleChoice_wrongAnswer_succeeds() = runBlockingTest( + coroutineContext + ) { + val currentStateLiveData = explorationProgressController.getCurrentState() + currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) + explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) + advanceUntilIdle() + + // The current interaction is multiple choice. + val result = explorationProgressController.submitAnswer(InteractionObject.newBuilder().setNonNegativeInt(0).build()) + result.observeForever(mockAsyncAnswerOutcomeObserver) + advanceUntilIdle() + + // Verify that the answer submission was successful. + verify(mockAsyncAnswerOutcomeObserver, atLeastOnce()).onChanged(asyncAnswerOutcomeCaptor.capture()) + assertThat(asyncAnswerOutcomeCaptor.value.isSuccess()).isTrue() + } + + @Test + @ExperimentalCoroutinesApi + fun testSubmitAnswer_forMultipleChoice_wrongAnswer_providesDefaultFeedbackAndNewStateTransition() = runBlockingTest( + coroutineContext + ) { + val currentStateLiveData = explorationProgressController.getCurrentState() + currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) + explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) + advanceUntilIdle() + + // The current interaction is multiple choice. + val result = explorationProgressController.submitAnswer(InteractionObject.newBuilder().setNonNegativeInt(1).build()) + result.observeForever(mockAsyncAnswerOutcomeObserver) + advanceUntilIdle() + + // Verify that the answer submission was successful. + verify(mockAsyncAnswerOutcomeObserver, atLeastOnce()).onChanged(asyncAnswerOutcomeCaptor.capture()) + val answerOutcome = asyncAnswerOutcomeCaptor.value.getOrThrow() + assertThat(answerOutcome.destinationCase).isEqualTo(AnswerOutcome.DestinationCase.STATE_NAME) + assertThat(answerOutcome.feedback.html).contains("Hm, it certainly looks like it") + } + // testGetCurrentState_whileSubmittingAnswer_becomesPending // testGetCurrentState_afterSubmittingWrongAnswer_updatesPendingState // testGetCurrentState_afterSubmittingCorrectAnswer_becomesCompletedState 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 73811b0c61f..82cc4f13aed 100644 --- a/utility/src/main/java/org/oppia/util/data/DataProviders.kt +++ b/utility/src/main/java/org/oppia/util/data/DataProviders.kt @@ -116,20 +116,6 @@ 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. From b5b74858b9a9f9cc47928f5ffd4488ad4182fc81 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Fri, 4 Oct 2019 00:52:45 -0700 Subject: [PATCH 007/146] Finish tests for ExplorationProgressController and add test classification support for the second test exploration (about_oppia). --- .../AnswerClassificationController.kt | 40 +- .../ExplorationProgressController.kt | 36 +- .../AnswerClassificationControllerTest.kt | 6 +- .../ExplorationProgressControllerTest.kt | 879 ++++++++++++++++-- 4 files changed, 847 insertions(+), 114 deletions(-) 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 8389fbdb154..d6a8b65b8f5 100644 --- a/domain/src/main/java/org/oppia/domain/classify/AnswerClassificationController.kt +++ b/domain/src/main/java/org/oppia/domain/classify/AnswerClassificationController.kt @@ -24,37 +24,55 @@ class AnswerClassificationController @Inject constructor() { */ 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) + // Exp 5 + "Welcome!" -> simulateMultipleChoiceForWelcomeStateExp5(currentState, answer) + "What language" -> simulateTextInputForWhatLanguageStateExp5(currentState, answer) + "Numeric input" -> simulateNumericInputForNumericInputStateExp5(currentState, answer) "Things you can do" -> currentState.interaction.defaultOutcome + // Exp 6 + "First State" -> currentState.interaction.defaultOutcome + "So what can I tell you" -> simulateMultipleChoiceForWelcomeStateExp6(currentState, answer) + "Example1" -> currentState.interaction.defaultOutcome // TextInput with ignored answer. + "Example3" -> currentState.interaction.defaultOutcome + "End Card" -> currentState.interaction.defaultOutcome else -> throw Exception("Cannot submit answer to unexpected state: ${currentState.name}.") } } - private fun simulateMultipleChoiceForWelcomeState(currentState: State, answer: InteractionObject): Outcome { + private fun simulateMultipleChoiceForWelcomeStateExp5(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 + throw Exception("Expected non-negative int answer, not $answer.") + answer.nonNegativeInt == 0 -> currentState.interaction.answerGroupsList[0].outcome + answer.nonNegativeInt == 2 -> currentState.interaction.answerGroupsList[1].outcome else -> currentState.interaction.defaultOutcome } } - private fun simulateTextInputForWhatLanguageState(currentState: State, answer: InteractionObject): Outcome { + private fun simulateTextInputForWhatLanguageStateExp5(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 + answer.normalizedString.toLowerCase() == "finnish" -> currentState.interaction.getAnswerGroups(6).outcome else -> currentState.interaction.defaultOutcome } } - private fun simulateNumericInputForNumericInputState(currentState: State, answer: InteractionObject): Outcome { + private fun simulateNumericInputForNumericInputStateExp5(currentState: State, answer: InteractionObject): Outcome { + return when { + answer.objectTypeCase != InteractionObject.ObjectTypeCase.SIGNED_INT -> + throw Exception("Expected signed int answer, not $answer.") + answer.signedInt == 121 -> currentState.interaction.answerGroupsList.first().outcome + else -> currentState.interaction.defaultOutcome + } + } + + private fun simulateMultipleChoiceForWelcomeStateExp6(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 + throw Exception("Expected non-negative int answer, not $answer.") + answer.nonNegativeInt == 3 -> currentState.interaction.answerGroupsList[1].outcome + answer.nonNegativeInt == 0 -> currentState.interaction.answerGroupsList[2].outcome else -> currentState.interaction.defaultOutcome } } 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 92c35fea302..f439d5463cd 100644 --- a/domain/src/main/java/org/oppia/domain/exploration/ExplorationProgressController.kt +++ b/domain/src/main/java/org/oppia/domain/exploration/ExplorationProgressController.kt @@ -48,7 +48,10 @@ class ExplorationProgressController @Inject constructor( // TODO(#182): Add support for refresher explorations. // TODO(#90): Update the internal locking of this controller to use something like an in-memory blocking cache to // simplify state locking. However, doing this correctly requires a fix in MediatorLiveData to avoid unexpected - // cancellations in chained cross-scope coroutines. + // cancellations in chained cross-scope coroutines. Note that this is also essential to ensure post-load operations + // can be queued before load completes to avoid cases in tests where the exploration load operation needs to be fully + // finished before performing a post-load operation. The current state of the controller is leaking this + // implementation detail to tests. private val currentStateDataProvider = dataProviders.createInMemoryDataProviderAsync(CURRENT_STATE_DATA_PROVIDER_ID, this::retrieveCurrentStateAsync) @@ -90,7 +93,7 @@ class ExplorationProgressController @Inject constructor( * know whether a current answer is pending. That [LiveData] will have its state changed to pending during answer * submission and until answer resolution. * - * Submitting an answer may result in the learner staying in the current state, moving to a new state in the + * Submitting an answer should result in the learner staying in the current state, moving to a new state in the * exploration, being shown a concept card, or being navigated to another exploration altogether. Note that once a * correct answer is processed, the current state reported to [getCurrentState] will change from a pending state to a * completed state since the learner completed that card. The learner can then proceed from the current completed @@ -110,12 +113,12 @@ class ExplorationProgressController @Inject constructor( check(explorationProgress.playStage != PlayStage.NOT_PLAYING) { "Cannot submit an answer if an exploration is not being played." } - check(explorationProgress.playStage != PlayStage.SUBMITTING_ANSWER) { - "Cannot submit an answer while another answer is pending." - } check(explorationProgress.playStage != PlayStage.LOADING_EXPLORATION) { "Cannot submit an answer while the exploration is being loaded." } + check(explorationProgress.playStage != PlayStage.SUBMITTING_ANSWER) { + "Cannot submit an answer while another answer is pending." + } // Notify observers that the submitted answer is currently pending. explorationProgress.advancePlayStageTo(PlayStage.SUBMITTING_ANSWER) @@ -421,13 +424,13 @@ class ExplorationProgressController @Inject constructor( /** Navigates to the previous State in the deck, or fails if this isn't possible. */ internal fun navigateToPreviousState() { - check(!isCurrentStateInitial()) { "Cannot navigate to previous State; at initial state." } + check(!isCurrentStateInitial()) { "Cannot navigate to previous state; at initial state." } stateIndex-- } /** Navigates to the next State in the deck, or fails if this isn't possible. */ internal fun navigateToNextState() { - check(!isCurrentStateTopOfDeck()) { "Cannot navigate to next State; at most recent State." } + check(!isCurrentStateTopOfDeck()) { "Cannot navigate to next state; at most recent state." } stateIndex++ } @@ -441,9 +444,12 @@ class ExplorationProgressController @Inject constructor( /** Returns the current [EphemeralState] the learner is viewing. */ internal fun getCurrentEphemeralState(): EphemeralState { + // Note that the terminal state is evaluated first since it can only return true if the current state is the top + // of the deck, and that state is the terminal one. Otherwise the terminal check would never be triggered since + // the second case assumes the top of the deck must be pending. return when { - stateIndex == previousStates.size -> getCurrentPendingState() isCurrentStateTerminal() -> getCurrentTerminalState() + stateIndex == previousStates.size -> getCurrentPendingState() else -> getPreviousState() } } @@ -452,12 +458,13 @@ class ExplorationProgressController @Inject constructor( * Pushes a new State onto the deck. This cannot happen if the learner isn't at the most recent State, if the * current State is not terminal, or if the learner hasn't submitted an answer to the most recent State. This * operation implies that the most recently submitted answer was the correct answer to the previously current State. + * This does NOT change the user's position in the deck, it just marks the current state as completed. */ internal fun pushState(state: State) { - check(isCurrentStateTopOfDeck()) { "Cannot push a new State unless the learner is at the most recent State." } - check(!isCurrentStateTerminal()) { "Cannot push another State after reaching a terminal State." } - check(currentDialogInteractions.size != 0) { "Cannot push another State without an answer." } - check(state.name != pendingTopState.name) { "Cannot route from the same State to itself as a new card." } + check(isCurrentStateTopOfDeck()) { "Cannot push a new state unless the learner is at the most recent state." } + check(!isCurrentStateTerminal()) { "Cannot push another state after reaching a terminal state." } + check(currentDialogInteractions.size != 0) { "Cannot push another state without an answer." } + check(state.name != pendingTopState.name) { "Cannot route from the same state to itself as a new card." } previousStates += EphemeralState.newBuilder() .setState(pendingTopState) .setHasPreviousState(!isCurrentStateInitial()) @@ -465,7 +472,6 @@ class ExplorationProgressController @Inject constructor( .build() currentDialogInteractions.clear() pendingTopState = state - stateIndex++ } /** @@ -474,8 +480,8 @@ class ExplorationProgressController @Inject constructor( * terminal interaction). */ internal fun submitAnswer(userAnswer: InteractionObject, feedback: SubtitledHtml) { - check(isCurrentStateTopOfDeck()) { "Cannot submit an answer except to the most recent State." } - check(!isCurrentStateTerminal()) { "Cannot submit an answer to a terminal State." } + check(isCurrentStateTopOfDeck()) { "Cannot submit an answer except to the most recent state." } + check(!isCurrentStateTerminal()) { "Cannot submit an answer to a terminal state." } currentDialogInteractions += AnswerAndResponse.newBuilder() .setUserAnswer(userAnswer) .setFeedback(feedback) 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 adbe60f58ef..690388c65a7 100644 --- a/domain/src/test/java/org/oppia/domain/classify/AnswerClassificationControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/classify/AnswerClassificationControllerTest.kt @@ -27,8 +27,8 @@ import javax.inject.Singleton @Config(manifest = Config.NONE) class AnswerClassificationControllerTest { 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 TEST_INT_2_ANSWER = InteractionObject.newBuilder().setNonNegativeInt(1).build() + private val TEST_SIGNED_INT_121_ANSWER = InteractionObject.newBuilder().setSignedInt(121).build() private val OUTCOME_0 = Outcome.newBuilder() .setDestStateName("First state") @@ -86,7 +86,7 @@ class AnswerClassificationControllerTest { .setDefaultOutcome(OUTCOME_2) .build() val state1 = createTestState("Numeric input", interaction1) - answerClassificationController.classify(state1, TEST_INT_121_ANSWER) + answerClassificationController.classify(state1, TEST_SIGNED_INT_121_ANSWER) val state2 = createTestState("Things you can do", interaction2) val outcome = answerClassificationController.classify(state2, TEST_STRING_ANSWER) 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 207355a7c40..0738bfdbbd0 100644 --- a/domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/exploration/ExplorationProgressControllerTest.kt @@ -14,7 +14,6 @@ import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.TestCoroutineDispatcher import kotlinx.coroutines.test.runBlockingTest -import org.junit.Assert.fail import org.junit.Before import org.junit.Rule import org.junit.Test @@ -29,20 +28,23 @@ import org.mockito.junit.MockitoJUnit import org.mockito.junit.MockitoRule import org.oppia.app.model.AnswerOutcome import org.oppia.app.model.EphemeralState +import org.oppia.app.model.EphemeralState.StateTypeCase.COMPLETED_STATE import org.oppia.app.model.EphemeralState.StateTypeCase.PENDING_STATE +import org.oppia.app.model.EphemeralState.StateTypeCase.TERMINAL_STATE import org.oppia.app.model.Exploration import org.oppia.app.model.InteractionObject 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 import javax.inject.Qualifier import javax.inject.Singleton import kotlin.coroutines.EmptyCoroutineContext -import kotlin.reflect.KClass -import kotlin.reflect.full.cast + +// For context: +// https://github.com/oppia/oppia/blob/37285a/extensions/interactions/Continue/directives/oppia-interactive-continue.directive.ts. +private const val DEFAULT_CONTINUE_INTERACTION_TEXT_ANSWER = "Please continue." /** Tests for [ExplorationProgressController]. */ @RunWith(AndroidJUnit4::class) @@ -50,6 +52,13 @@ import kotlin.reflect.full.cast class ExplorationProgressControllerTest { // TODO(#114): Add much more thorough tests for the integration pathway. + // TODO(#59): Once AsyncDataSubscriptionManager can be replaced with a fake, add the following tests once careful + // testing timing can be controlled: + // - testMoveToNext_whileSubmittingAnswer_failsWithError + // - testGetCurrentState_whileSubmittingCorrectMultiChoiceAnswer_updatesToPending + // - testSubmitAnswer_whileSubmittingAnotherAnswer_failsWithError + // - testMoveToPrevious_whileSubmittingAnswer_failsWithError + @Rule @JvmField val mockitoRule: MockitoRule = MockitoJUnit.rule() @@ -71,6 +80,9 @@ class ExplorationProgressControllerTest { @Mock lateinit var mockCurrentStateLiveDataObserver: Observer> + @Mock + lateinit var mockCurrentStateLiveDataObserver2: Observer> + @Mock lateinit var mockAsyncResultLiveDataObserver: Observer> @@ -131,8 +143,7 @@ class ExplorationProgressControllerTest { val currentStateLiveData = explorationProgressController.getCurrentState() currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) - explorationDataController.startPlayingExploration("invalid_exp_id") - advanceUntilIdle() + playExploration("invalid_exp_id") verify(mockCurrentStateLiveDataObserver, atLeastOnce()).onChanged(currentStateResultCaptor.capture()) assertThat(currentStateResultCaptor.value.isFailure()).isTrue() @@ -161,8 +172,7 @@ class ExplorationProgressControllerTest { currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) advanceUntilIdle() - explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) - advanceUntilIdle() + playExploration(TEST_EXPLORATION_ID_5) // The second-to-latest result stays pending since the exploration was loading (the actual result is the fully // loaded exploration). This is only true if the observer begins before starting to load the exploration. @@ -177,7 +187,7 @@ class ExplorationProgressControllerTest { coroutineContext ) { val exploration = getTestExploration5() - explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) + playExploration(TEST_EXPLORATION_ID_5) val currentStateLiveData = explorationProgressController.getCurrentState() currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) @@ -197,11 +207,11 @@ class ExplorationProgressControllerTest { ) { val exploration = getTestExploration5() // Start with playing an invalid exploration. - explorationDataController.startPlayingExploration("invalid_exp_id") - explorationDataController.stopPlayingExploration() + playExploration("invalid_exp_id") + endExploration() // Then a valid one. - explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) + playExploration(TEST_EXPLORATION_ID_5) val currentStateLiveData = explorationProgressController.getCurrentState() currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) advanceUntilIdle() @@ -231,7 +241,8 @@ class ExplorationProgressControllerTest { @Test @ExperimentalCoroutinesApi fun testPlayExploration_withoutFinishingPrevious_failsWithError() = runBlockingTest(coroutineContext) { - explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) + subscribeToCurrentStateToAllowExplorationToLoad() + playExploration(TEST_EXPLORATION_ID_5) // Try playing another exploration without finishing the previous one. val resultLiveData = explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) @@ -253,13 +264,11 @@ class ExplorationProgressControllerTest { val currentStateLiveData = explorationProgressController.getCurrentState() currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) // Start with playing a valid exploration, then stop. - explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) - explorationDataController.stopPlayingExploration() - advanceUntilIdle() + playExploration(TEST_EXPLORATION_ID_5) + endExploration() // Then another valid one. - explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_6) - advanceUntilIdle() + playExploration(TEST_EXPLORATION_ID_6) // The latest result should correspond to the valid ID, and the progress controller should gracefully recover. verify(mockCurrentStateLiveDataObserver, atLeastOnce()).onChanged(currentStateResultCaptor.capture()) @@ -271,16 +280,45 @@ class ExplorationProgressControllerTest { @Test @ExperimentalCoroutinesApi - fun testSubmitAnswer_forMultipleChoice_correctAnswer_succeeds() = runBlockingTest( - coroutineContext - ) { - val currentStateLiveData = explorationProgressController.getCurrentState() - currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) + fun testSubmitAnswer_beforePlaying_failsWithError() = runBlockingTest(coroutineContext) { + val result = explorationProgressController.submitAnswer(createMultipleChoiceAnswer(0)) + result.observeForever(mockAsyncAnswerOutcomeObserver) + advanceUntilIdle() + + // Verify that the answer submission failed. + verify(mockAsyncAnswerOutcomeObserver, atLeastOnce()).onChanged(asyncAnswerOutcomeCaptor.capture()) + assertThat(asyncAnswerOutcomeCaptor.value.isFailure()).isTrue() + assertThat(asyncAnswerOutcomeCaptor.value.getErrorOrNull()) + .hasMessageThat() + .contains("Cannot submit an answer if an exploration is not being played.") + } + + @Test + @ExperimentalCoroutinesApi + fun testSubmitAnswer_whileLoading_failsWithError() = runBlockingTest(coroutineContext) { + // Start playing an exploration, but don't wait for it to complete. + subscribeToCurrentStateToAllowExplorationToLoad() explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) + + val result = explorationProgressController.submitAnswer(createMultipleChoiceAnswer(0)) + result.observeForever(mockAsyncAnswerOutcomeObserver) advanceUntilIdle() - // The current interaction is multiple choice. - val result = explorationProgressController.submitAnswer(InteractionObject.newBuilder().setNonNegativeInt(0).build()) + // Verify that the answer submission failed. + verify(mockAsyncAnswerOutcomeObserver, atLeastOnce()).onChanged(asyncAnswerOutcomeCaptor.capture()) + assertThat(asyncAnswerOutcomeCaptor.value.isFailure()).isTrue() + assertThat(asyncAnswerOutcomeCaptor.value.getErrorOrNull()) + .hasMessageThat() + .contains("Cannot submit an answer while the exploration is being loaded.") + } + + @Test + @ExperimentalCoroutinesApi + fun testSubmitAnswer_forMultipleChoice_correctAnswer_succeeds() = runBlockingTest(coroutineContext) { + subscribeToCurrentStateToAllowExplorationToLoad() + playExploration(TEST_EXPLORATION_ID_5) + + val result = explorationProgressController.submitAnswer(createMultipleChoiceAnswer(0)) result.observeForever(mockAsyncAnswerOutcomeObserver) advanceUntilIdle() @@ -294,13 +332,10 @@ class ExplorationProgressControllerTest { fun testSubmitAnswer_forMultipleChoice_correctAnswer_returnsOutcomeWithTransition() = runBlockingTest( coroutineContext ) { - val currentStateLiveData = explorationProgressController.getCurrentState() - currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) - explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) - advanceUntilIdle() + subscribeToCurrentStateToAllowExplorationToLoad() + playExploration(TEST_EXPLORATION_ID_5) - // The current interaction is multiple choice. - val result = explorationProgressController.submitAnswer(InteractionObject.newBuilder().setNonNegativeInt(0).build()) + val result = explorationProgressController.submitAnswer(createMultipleChoiceAnswer(0)) result.observeForever(mockAsyncAnswerOutcomeObserver) advanceUntilIdle() @@ -316,33 +351,484 @@ class ExplorationProgressControllerTest { fun testSubmitAnswer_forMultipleChoice_wrongAnswer_succeeds() = runBlockingTest( coroutineContext ) { + subscribeToCurrentStateToAllowExplorationToLoad() + playExploration(TEST_EXPLORATION_ID_5) + + val result = explorationProgressController.submitAnswer(createMultipleChoiceAnswer(0)) + result.observeForever(mockAsyncAnswerOutcomeObserver) + advanceUntilIdle() + + // Verify that the answer submission was successful. + verify(mockAsyncAnswerOutcomeObserver, atLeastOnce()).onChanged(asyncAnswerOutcomeCaptor.capture()) + assertThat(asyncAnswerOutcomeCaptor.value.isSuccess()).isTrue() + } + + @Test + @ExperimentalCoroutinesApi + fun testSubmitAnswer_forMultipleChoice_wrongAnswer_providesDefaultFeedbackAndNewStateTransition() = runBlockingTest( + coroutineContext + ) { + subscribeToCurrentStateToAllowExplorationToLoad() + playExploration(TEST_EXPLORATION_ID_5) + + val result = explorationProgressController.submitAnswer(createMultipleChoiceAnswer(1)) + result.observeForever(mockAsyncAnswerOutcomeObserver) + advanceUntilIdle() + + // Verify that the answer submission was successful. + verify(mockAsyncAnswerOutcomeObserver, atLeastOnce()).onChanged(asyncAnswerOutcomeCaptor.capture()) + val answerOutcome = asyncAnswerOutcomeCaptor.value.getOrThrow() + assertThat(answerOutcome.destinationCase).isEqualTo(AnswerOutcome.DestinationCase.STATE_NAME) + assertThat(answerOutcome.feedback.html).contains("Hm, it certainly looks like it") + } + + @Test + @ExperimentalCoroutinesApi + fun testGetCurrentState_afterSubmittingCorrectMultiChoiceAnswer_becomesCompletedState() = runBlockingTest( + coroutineContext + ) { + val currentStateLiveData = explorationProgressController.getCurrentState() + currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) + playExploration(TEST_EXPLORATION_ID_5) + + submitMultipleChoiceAnswer(0) + + // Verify that the current state updates. It should stay pending, and the wrong answer should be appended. + verify(mockCurrentStateLiveDataObserver, atLeastOnce()).onChanged(currentStateResultCaptor.capture()) + assertThat(currentStateResultCaptor.value.isSuccess()).isTrue() + val currentState = currentStateResultCaptor.value.getOrThrow() + assertThat(currentState.stateTypeCase).isEqualTo(COMPLETED_STATE) + assertThat(currentState.completedState.answerCount).isEqualTo(1) + assertThat(currentState.completedState.getAnswer(0).userAnswer.nonNegativeInt).isEqualTo(0) + assertThat(currentState.completedState.getAnswer(0).feedback.html).isEqualTo("Yes!") + } + + @Test + @ExperimentalCoroutinesApi + fun testGetCurrentState_afterSubmittingWrongMultiChoiceAnswer_updatesPendingState() = runBlockingTest( + coroutineContext + ) { + val currentStateLiveData = explorationProgressController.getCurrentState() + currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) + playExploration(TEST_EXPLORATION_ID_5) + + submitMultipleChoiceAnswer(2) + + // Verify that the current state updates. It should now be completed with the correct answer. + verify(mockCurrentStateLiveDataObserver, atLeastOnce()).onChanged(currentStateResultCaptor.capture()) + assertThat(currentStateResultCaptor.value.isSuccess()).isTrue() + val currentState = currentStateResultCaptor.value.getOrThrow() + assertThat(currentState.stateTypeCase).isEqualTo(PENDING_STATE) + assertThat(currentState.pendingState.wrongAnswerCount).isEqualTo(1) + assertThat(currentState.pendingState.getWrongAnswer(0).userAnswer.nonNegativeInt).isEqualTo(2) + assertThat(currentState.pendingState.getWrongAnswer(0).feedback.html).contains("Have another go?") + } + + @Test + @ExperimentalCoroutinesApi + fun testGetCurrentState_afterSubmittingWrongThenRightAnswer_updatesToStateWithBothAnswers() = runBlockingTest( + coroutineContext + ) { + subscribeToCurrentStateToAllowExplorationToLoad() + playExploration(TEST_EXPLORATION_ID_5) + submitMultipleChoiceAnswer(2) + + submitMultipleChoiceAnswer(0) + + // Verify that the current state updates. It should now be completed with both the wrong and correct answers. + verify(mockCurrentStateLiveDataObserver, atLeastOnce()).onChanged(currentStateResultCaptor.capture()) + assertThat(currentStateResultCaptor.value.isSuccess()).isTrue() + val currentState = currentStateResultCaptor.value.getOrThrow() + assertThat(currentState.stateTypeCase).isEqualTo(COMPLETED_STATE) + assertThat(currentState.completedState.answerCount).isEqualTo(2) + assertThat(currentState.completedState.getAnswer(0).userAnswer.nonNegativeInt).isEqualTo(2) + assertThat(currentState.completedState.getAnswer(0).feedback.html).contains("Have another go?") + assertThat(currentState.completedState.getAnswer(1).userAnswer.nonNegativeInt).isEqualTo(0) + assertThat(currentState.completedState.getAnswer(1).feedback.html).isEqualTo("Yes!") + } + + @Test + @ExperimentalCoroutinesApi + fun testMoveToNext_beforePlaying_failsWithError() = runBlockingTest(coroutineContext) { + val moveToStateResult = explorationProgressController.moveToNextState() + moveToStateResult.observeForever(mockAsyncResultLiveDataObserver) + + verify(mockAsyncResultLiveDataObserver, atLeastOnce()).onChanged(asyncResultCaptor.capture()) + assertThat(asyncResultCaptor.value.isFailure()).isTrue() + assertThat(asyncResultCaptor.value.getErrorOrNull()) + .hasMessageThat() + .contains("Cannot navigate to a next state if an exploration is not being played.") + } + + @Test + @ExperimentalCoroutinesApi + fun testMoveToNext_whileLoadingExploration_failsWithError() = runBlockingTest(coroutineContext) { + // Start playing an exploration, but don't wait for it to complete. + subscribeToCurrentStateToAllowExplorationToLoad() + explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) + + val moveToStateResult = explorationProgressController.moveToNextState() + moveToStateResult.observeForever(mockAsyncResultLiveDataObserver) + + verify(mockAsyncResultLiveDataObserver, atLeastOnce()).onChanged(asyncResultCaptor.capture()) + assertThat(asyncResultCaptor.value.isFailure()).isTrue() + assertThat(asyncResultCaptor.value.getErrorOrNull()) + .hasMessageThat() + .contains("Cannot navigate to a next state if an exploration is being loaded.") + } + + @Test + @ExperimentalCoroutinesApi + fun testMoveToNext_forPendingInitialState_failsWithError() = runBlockingTest(coroutineContext) { + subscribeToCurrentStateToAllowExplorationToLoad() + playExploration(TEST_EXPLORATION_ID_5) + + val moveToStateResult = explorationProgressController.moveToNextState() + moveToStateResult.observeForever(mockAsyncResultLiveDataObserver) + advanceUntilIdle() + + // Verify that we can't move ahead since the current state isn't yet completed. + verify(mockAsyncResultLiveDataObserver, atLeastOnce()).onChanged(asyncResultCaptor.capture()) + assertThat(asyncResultCaptor.value.isFailure()).isTrue() + assertThat(asyncResultCaptor.value.getErrorOrNull()) + .hasMessageThat() + .contains("Cannot navigate to next state; at most recent state.") + } + + @Test + @ExperimentalCoroutinesApi + fun testMoveToNext_forCompletedState_succeeds() = runBlockingTest(coroutineContext) { + subscribeToCurrentStateToAllowExplorationToLoad() + playExploration(TEST_EXPLORATION_ID_5) + submitMultipleChoiceAnswer(0) + + val moveToStateResult = explorationProgressController.moveToNextState() + moveToStateResult.observeForever(mockAsyncResultLiveDataObserver) + advanceUntilIdle() + + verify(mockAsyncResultLiveDataObserver, atLeastOnce()).onChanged(asyncResultCaptor.capture()) + assertThat(asyncResultCaptor.value.isSuccess()).isTrue() + } + + @Test + @ExperimentalCoroutinesApi + fun testMoveToNext_forCompletedState_movesToNextState() = runBlockingTest(coroutineContext) { val currentStateLiveData = explorationProgressController.getCurrentState() currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) + playExploration(TEST_EXPLORATION_ID_5) + submitMultipleChoiceAnswer(0) + + moveToNextState() + + verify(mockCurrentStateLiveDataObserver, atLeastOnce()).onChanged(currentStateResultCaptor.capture()) + assertThat(currentStateResultCaptor.value.isSuccess()).isTrue() + val currentState = currentStateResultCaptor.value.getOrThrow() + assertThat(currentState.state.name).isEqualTo("What language") + assertThat(currentState.stateTypeCase).isEqualTo(PENDING_STATE) + } + + @Test + @ExperimentalCoroutinesApi + fun testMoveToNext_afterMovingFromCompletedState_failsWithError() = runBlockingTest(coroutineContext) { + subscribeToCurrentStateToAllowExplorationToLoad() + playExploration(TEST_EXPLORATION_ID_5) + submitMultipleChoiceAnswer(0) + moveToNextState() + + // Try skipping past the current state. + val moveToStateResult = explorationProgressController.moveToNextState() + moveToStateResult.observeForever(mockAsyncResultLiveDataObserver) + advanceUntilIdle() + + // Verify we can't move ahead since the new state isn't yet completed. + verify(mockAsyncResultLiveDataObserver, atLeastOnce()).onChanged(asyncResultCaptor.capture()) + assertThat(asyncResultCaptor.value.isFailure()).isTrue() + assertThat(asyncResultCaptor.value.getErrorOrNull()) + .hasMessageThat() + .contains("Cannot navigate to next state; at most recent state.") + } + + @Test + @ExperimentalCoroutinesApi + fun testMoveToPrevious_beforePlaying_failsWithError() = runBlockingTest(coroutineContext) { + val moveToStateResult = explorationProgressController.moveToPreviousState() + moveToStateResult.observeForever(mockAsyncResultLiveDataObserver) + advanceUntilIdle() + + verify(mockAsyncResultLiveDataObserver, atLeastOnce()).onChanged(asyncResultCaptor.capture()) + assertThat(asyncResultCaptor.value.isFailure()).isTrue() + assertThat(asyncResultCaptor.value.getErrorOrNull()) + .hasMessageThat() + .contains("Cannot navigate to a previous state if an exploration is not being played.") + } + + @Test + @ExperimentalCoroutinesApi + fun testMoveToPrevious_whileLoadingExploration_failsWithError() = runBlockingTest(coroutineContext) { + // Start playing an exploration, but don't wait for it to complete. + subscribeToCurrentStateToAllowExplorationToLoad() explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) + + val moveToStateResult = explorationProgressController.moveToPreviousState() + moveToStateResult.observeForever(mockAsyncResultLiveDataObserver) + advanceUntilIdle() + + verify(mockAsyncResultLiveDataObserver, atLeastOnce()).onChanged(asyncResultCaptor.capture()) + assertThat(asyncResultCaptor.value.isFailure()).isTrue() + assertThat(asyncResultCaptor.value.getErrorOrNull()) + .hasMessageThat() + .contains("Cannot navigate to a previous state if an exploration is being loaded.") + } + + @Test + @ExperimentalCoroutinesApi + fun testMoveToPrevious_onPendingInitialState_failsWithError() = runBlockingTest(coroutineContext) { + subscribeToCurrentStateToAllowExplorationToLoad() + playExploration(TEST_EXPLORATION_ID_5) + + val moveToStateResult = explorationProgressController.moveToPreviousState() + moveToStateResult.observeForever(mockAsyncResultLiveDataObserver) + advanceUntilIdle() + + // Verify we can't move behind since the current state is the initial exploration state. + verify(mockAsyncResultLiveDataObserver, atLeastOnce()).onChanged(asyncResultCaptor.capture()) + assertThat(asyncResultCaptor.value.isFailure()).isTrue() + assertThat(asyncResultCaptor.value.getErrorOrNull()) + .hasMessageThat() + .contains("Cannot navigate to previous state; at initial state.") + } + + @Test + @ExperimentalCoroutinesApi + fun testMoveToPrevious_onCompletedInitialState_failsWithError() = runBlockingTest(coroutineContext) { + subscribeToCurrentStateToAllowExplorationToLoad() + playExploration(TEST_EXPLORATION_ID_5) + submitMultipleChoiceAnswer(0) + + val moveToStateResult = explorationProgressController.moveToPreviousState() + moveToStateResult.observeForever(mockAsyncResultLiveDataObserver) + advanceUntilIdle() + + // Still can't navigate behind for a completed initial state since there's no previous state. + verify(mockAsyncResultLiveDataObserver, atLeastOnce()).onChanged(asyncResultCaptor.capture()) + assertThat(asyncResultCaptor.value.isFailure()).isTrue() + assertThat(asyncResultCaptor.value.getErrorOrNull()) + .hasMessageThat() + .contains("Cannot navigate to previous state; at initial state.") + } + + @Test + @ExperimentalCoroutinesApi + fun testMoveToPrevious_forStateWithCompletedPreviousState_succeeds() = runBlockingTest(coroutineContext) { + subscribeToCurrentStateToAllowExplorationToLoad() + playExploration(TEST_EXPLORATION_ID_5) + submitMultipleChoiceAnswerAndMoveToNextState(0) + + val moveToStateResult = explorationProgressController.moveToPreviousState() + moveToStateResult.observeForever(mockAsyncResultLiveDataObserver) + advanceUntilIdle() + + // Verify that we can navigate to the previous state since the current state is complete and not initial. + verify(mockAsyncResultLiveDataObserver, atLeastOnce()).onChanged(asyncResultCaptor.capture()) + assertThat(asyncResultCaptor.value.isSuccess()).isTrue() + } + + @Test + @ExperimentalCoroutinesApi + fun testMoveToPrevious_forCompletedState_movesToPreviousState() = runBlockingTest(coroutineContext) { + val currentStateLiveData = explorationProgressController.getCurrentState() + currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) + playExploration(TEST_EXPLORATION_ID_5) + submitMultipleChoiceAnswerAndMoveToNextState(0) + + moveToPreviousState() + + // Since the answer submission and forward navigation should work (see earlier tests), verify that the move to the + // previous state does return us back to the initial exploration state (which is now completed). + verify(mockCurrentStateLiveDataObserver, atLeastOnce()).onChanged(currentStateResultCaptor.capture()) + assertThat(currentStateResultCaptor.value.isSuccess()).isTrue() + val currentState = currentStateResultCaptor.value.getOrThrow() + assertThat(currentState.state.name).isEqualTo("Welcome!") + assertThat(currentState.stateTypeCase).isEqualTo(COMPLETED_STATE) + } + + @Test + @ExperimentalCoroutinesApi + fun testMoveToPrevious_navigatedForwardThenBackToInitial_failsWithError() = runBlockingTest(coroutineContext) { + subscribeToCurrentStateToAllowExplorationToLoad() + playExploration(TEST_EXPLORATION_ID_5) + submitMultipleChoiceAnswerAndMoveToNextState(0) + moveToPreviousState() + + val moveToStateResult = explorationProgressController.moveToPreviousState() + moveToStateResult.observeForever(mockAsyncResultLiveDataObserver) advanceUntilIdle() - // The current interaction is multiple choice. - val result = explorationProgressController.submitAnswer(InteractionObject.newBuilder().setNonNegativeInt(0).build()) + // The first previous navigation should succeed (see above), but the second will fail since we're back at the + // initial state. + verify(mockAsyncResultLiveDataObserver, atLeastOnce()).onChanged(asyncResultCaptor.capture()) + assertThat(asyncResultCaptor.value.isFailure()).isTrue() + assertThat(asyncResultCaptor.value.getErrorOrNull()) + .hasMessageThat() + .contains("Cannot navigate to previous state; at initial state.") + } + + @Test + @ExperimentalCoroutinesApi + fun testSubmitAnswer_forTextInput_correctAnswer_returnsOutcomeWithTransition() = runBlockingTest(coroutineContext) { + subscribeToCurrentStateToAllowExplorationToLoad() + playExploration(TEST_EXPLORATION_ID_5) + submitMultipleChoiceAnswerAndMoveToNextState(0) + + val result = explorationProgressController.submitAnswer(createTextInputAnswer("Finnish")) result.observeForever(mockAsyncAnswerOutcomeObserver) advanceUntilIdle() // Verify that the answer submission was successful. verify(mockAsyncAnswerOutcomeObserver, atLeastOnce()).onChanged(asyncAnswerOutcomeCaptor.capture()) - assertThat(asyncAnswerOutcomeCaptor.value.isSuccess()).isTrue() + val answerOutcome = asyncAnswerOutcomeCaptor.value.getOrThrow() + assertThat(answerOutcome.destinationCase).isEqualTo(AnswerOutcome.DestinationCase.STATE_NAME) + assertThat(answerOutcome.feedback.html).contains("Yes! Oppia is the Finnish word for learn.") } @Test @ExperimentalCoroutinesApi - fun testSubmitAnswer_forMultipleChoice_wrongAnswer_providesDefaultFeedbackAndNewStateTransition() = runBlockingTest( + fun testSubmitAnswer_forTextInput_wrongAnswer_returnsDefaultOutcome() = runBlockingTest(coroutineContext) { + subscribeToCurrentStateToAllowExplorationToLoad() + playExploration(TEST_EXPLORATION_ID_5) + submitMultipleChoiceAnswerAndMoveToNextState(0) + + val result = explorationProgressController.submitAnswer(createTextInputAnswer("Klingon")) + result.observeForever(mockAsyncAnswerOutcomeObserver) + advanceUntilIdle() + + // Verify that the answer was wrong, and that there's no handler for it so the default outcome is returned. + verify(mockAsyncAnswerOutcomeObserver, atLeastOnce()).onChanged(asyncAnswerOutcomeCaptor.capture()) + val answerOutcome = asyncAnswerOutcomeCaptor.value.getOrThrow() + assertThat(answerOutcome.destinationCase).isEqualTo(AnswerOutcome.DestinationCase.SAME_STATE) + assertThat(answerOutcome.feedback.html).contains("Sorry, nope") + } + + @Test + @ExperimentalCoroutinesApi + fun testGetCurrentState_secondState_submitRightAnswer_pendingStateBecomesCompleted() = runBlockingTest( coroutineContext ) { + subscribeToCurrentStateToAllowExplorationToLoad() + playExploration(TEST_EXPLORATION_ID_5) + submitMultipleChoiceAnswerAndMoveToNextState(0) + + val result = explorationProgressController.submitAnswer(createTextInputAnswer("Finnish")) + result.observeForever(mockAsyncAnswerOutcomeObserver) + advanceUntilIdle() + + // Verify that the current state updates. It should stay pending, and the wrong answer should be appended. + verify(mockCurrentStateLiveDataObserver, atLeastOnce()).onChanged(currentStateResultCaptor.capture()) + assertThat(currentStateResultCaptor.value.isSuccess()).isTrue() + val currentState = currentStateResultCaptor.value.getOrThrow() + assertThat(currentState.stateTypeCase).isEqualTo(COMPLETED_STATE) + assertThat(currentState.completedState.answerCount).isEqualTo(1) + assertThat(currentState.completedState.getAnswer(0).userAnswer.normalizedString).isEqualTo("Finnish") + assertThat(currentState.completedState.getAnswer(0).feedback.html).contains("Yes! Oppia is the Finnish word") + } + + @Test + @ExperimentalCoroutinesApi + fun testGetCurrentState_secondState_submitWrongAnswer_updatePendingState() = runBlockingTest( + coroutineContext + ) { + subscribeToCurrentStateToAllowExplorationToLoad() + playExploration(TEST_EXPLORATION_ID_5) + submitMultipleChoiceAnswerAndMoveToNextState(0) + + val result = explorationProgressController.submitAnswer(createTextInputAnswer("Klingon")) + result.observeForever(mockAsyncAnswerOutcomeObserver) + advanceUntilIdle() + + // Verify that the current state updates. It should now be completed with the correct answer. + verify(mockCurrentStateLiveDataObserver, atLeastOnce()).onChanged(currentStateResultCaptor.capture()) + assertThat(currentStateResultCaptor.value.isSuccess()).isTrue() + val currentState = currentStateResultCaptor.value.getOrThrow() + assertThat(currentState.stateTypeCase).isEqualTo(PENDING_STATE) + assertThat(currentState.pendingState.wrongAnswerCount).isEqualTo(1) + assertThat(currentState.pendingState.getWrongAnswer(0).userAnswer.normalizedString).isEqualTo("Klingon") + assertThat(currentState.pendingState.getWrongAnswer(0).feedback.html).contains("Sorry, nope") + } + + @Test + @ExperimentalCoroutinesApi + fun testGetCurrentState_afterMovePreviousAndNext_returnsCurrentState() = runBlockingTest(coroutineContext) { val currentStateLiveData = explorationProgressController.getCurrentState() currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) - explorationDataController.startPlayingExploration(TEST_EXPLORATION_ID_5) + playExploration(TEST_EXPLORATION_ID_5) + submitMultipleChoiceAnswerAndMoveToNextState(0) + + moveToPreviousState() + moveToNextState() + + // The current state should stay the same. + verify(mockCurrentStateLiveDataObserver, atLeastOnce()).onChanged(currentStateResultCaptor.capture()) + assertThat(currentStateResultCaptor.value.isSuccess()).isTrue() + val currentState = currentStateResultCaptor.value.getOrThrow() + assertThat(currentState.state.name).isEqualTo("What language") + assertThat(currentState.stateTypeCase).isEqualTo(PENDING_STATE) + } + + @Test + @ExperimentalCoroutinesApi + fun testGetCurrentState_afterMoveNextAndPrevious_returnsCurrentState() = runBlockingTest(coroutineContext) { + val currentStateLiveData = explorationProgressController.getCurrentState() + currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) + playExploration(TEST_EXPLORATION_ID_5) + submitMultipleChoiceAnswerAndMoveToNextState(0) + submitTextInputAnswer("Finnish") // Submit the answer but do not proceed to the next state. + + moveToNextState() + moveToPreviousState() + + // The current state should stay the same. + verify(mockCurrentStateLiveDataObserver, atLeastOnce()).onChanged(currentStateResultCaptor.capture()) + assertThat(currentStateResultCaptor.value.isSuccess()).isTrue() + val currentState = currentStateResultCaptor.value.getOrThrow() + assertThat(currentState.state.name).isEqualTo("What language") + assertThat(currentState.stateTypeCase).isEqualTo(COMPLETED_STATE) + } + + @Test + @ExperimentalCoroutinesApi + fun testGetCurrentState_afterMoveToPrevious_onThirdState_newObserver_receivesCompletedSecondState() = runBlockingTest( + coroutineContext + ) { + subscribeToCurrentStateToAllowExplorationToLoad() + playExploration(TEST_EXPLORATION_ID_5) + submitMultipleChoiceAnswerAndMoveToNextState(0) // First state -> second + submitTextInputAnswerAndMoveToNextState("Finnish") // Second state -> third + + // Move to the previous state and register a new observer. + moveToPreviousState() // Third state -> second + val currentStateLiveData = explorationProgressController.getCurrentState() + currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver2) advanceUntilIdle() - // The current interaction is multiple choice. - val result = explorationProgressController.submitAnswer(InteractionObject.newBuilder().setNonNegativeInt(1).build()) + // The new observer should observe the completed second state since it's the current pending state. + verify(mockCurrentStateLiveDataObserver2, atLeastOnce()).onChanged(currentStateResultCaptor.capture()) + assertThat(currentStateResultCaptor.value.isSuccess()).isTrue() + val currentState = currentStateResultCaptor.value.getOrThrow() + assertThat(currentState.state.name).isEqualTo("What language") + assertThat(currentState.stateTypeCase).isEqualTo(COMPLETED_STATE) + } + + @Test + @ExperimentalCoroutinesApi + fun testSubmitAnswer_forNumericInput_correctAnswer_returnsOutcomeWithTransition() = runBlockingTest( + coroutineContext + ) { + subscribeToCurrentStateToAllowExplorationToLoad() + playExploration(TEST_EXPLORATION_ID_5) + submitMultipleChoiceAnswerAndMoveToNextState(0) + submitTextInputAnswerAndMoveToNextState("Finnish") + + val result = explorationProgressController.submitAnswer(createNumericInputAnswer(121)) result.observeForever(mockAsyncAnswerOutcomeObserver) advanceUntilIdle() @@ -350,41 +836,175 @@ class ExplorationProgressControllerTest { verify(mockAsyncAnswerOutcomeObserver, atLeastOnce()).onChanged(asyncAnswerOutcomeCaptor.capture()) val answerOutcome = asyncAnswerOutcomeCaptor.value.getOrThrow() assertThat(answerOutcome.destinationCase).isEqualTo(AnswerOutcome.DestinationCase.STATE_NAME) - assertThat(answerOutcome.feedback.html).contains("Hm, it certainly looks like it") + assertThat(answerOutcome.feedback.html).contains("Yes, that's correct: 11 times 11 is 121.") + } + + @Test + @ExperimentalCoroutinesApi + fun testSubmitAnswer_forNumericInput_wrongAnswer_returnsDefaultOutcome() = runBlockingTest(coroutineContext) { + subscribeToCurrentStateToAllowExplorationToLoad() + playExploration(TEST_EXPLORATION_ID_5) + submitMultipleChoiceAnswerAndMoveToNextState(0) + submitTextInputAnswerAndMoveToNextState("Finnish") + + val result = explorationProgressController.submitAnswer(createNumericInputAnswer(0)) + result.observeForever(mockAsyncAnswerOutcomeObserver) + advanceUntilIdle() + + // Verify that the answer was wrong, and that there's no handler for it so the default outcome is returned. + // TODO(#114): Update this test to target a non-default outcome since the default outcome *should* be impossible to + // encounter. + verify(mockAsyncAnswerOutcomeObserver, atLeastOnce()).onChanged(asyncAnswerOutcomeCaptor.capture()) + val answerOutcome = asyncAnswerOutcomeCaptor.value.getOrThrow() + assertThat(answerOutcome.destinationCase).isEqualTo(AnswerOutcome.DestinationCase.SAME_STATE) + assertThat(answerOutcome.feedback.html).contains("If you got here, something's gone a bit wrong") + } + + @Test + @ExperimentalCoroutinesApi + fun testSubmitAnswer_forContinue_returnsOutcomeWithTransition() = runBlockingTest(coroutineContext) { + subscribeToCurrentStateToAllowExplorationToLoad() + playExploration(TEST_EXPLORATION_ID_5) + submitMultipleChoiceAnswerAndMoveToNextState(0) + submitTextInputAnswerAndMoveToNextState("Finnish") + submitNumericInputAnswerAndMoveToNextState(121) + + val result = explorationProgressController.submitAnswer(createContinueButtonAnswer()) + result.observeForever(mockAsyncAnswerOutcomeObserver) + advanceUntilIdle() + + // Verify that the continue button succeeds by default. + verify(mockAsyncAnswerOutcomeObserver, atLeastOnce()).onChanged(asyncAnswerOutcomeCaptor.capture()) + val answerOutcome = asyncAnswerOutcomeCaptor.value.getOrThrow() + assertThat(answerOutcome.destinationCase).isEqualTo(AnswerOutcome.DestinationCase.STATE_NAME) + assertThat(answerOutcome.feedback.html).isEmpty() + } + + @Test + @ExperimentalCoroutinesApi + fun testGetCurrentState_fifthState_isTerminalState() = runBlockingTest(coroutineContext) { + val currentStateLiveData = explorationProgressController.getCurrentState() + currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) + playExploration(TEST_EXPLORATION_ID_5) + submitMultipleChoiceAnswerAndMoveToNextState(0) + submitTextInputAnswerAndMoveToNextState("Finnish") + submitNumericInputAnswerAndMoveToNextState(121) + + submitContinueButtonAnswerAndMoveToNextState() + + // Verify that the fifth state is terminal. + verify(mockCurrentStateLiveDataObserver, atLeastOnce()).onChanged(currentStateResultCaptor.capture()) + assertThat(currentStateResultCaptor.value.isSuccess()).isTrue() + val currentState = currentStateResultCaptor.value.getOrThrow() + assertThat(currentState.stateTypeCase).isEqualTo(TERMINAL_STATE) } - // testGetCurrentState_whileSubmittingAnswer_becomesPending - // testGetCurrentState_afterSubmittingWrongAnswer_updatesPendingState - // testGetCurrentState_afterSubmittingCorrectAnswer_becomesCompletedState - // 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_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 + @Test + @ExperimentalCoroutinesApi + fun testGetCurrentState_afterMoveToPrevious_onThirdState_updatesToCompletedSecondState() = runBlockingTest( + coroutineContext + ) { + val currentStateLiveData = explorationProgressController.getCurrentState() + currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) + playExploration(TEST_EXPLORATION_ID_5) + submitMultipleChoiceAnswerAndMoveToNextState(0) + submitTextInputAnswerAndMoveToNextState("Finnish") + + moveToPreviousState() + + // Verify that the current state is the second state, and is completed. It should also have the previously submitted + // answer, allowing learners to potentially view past answers. + verify(mockCurrentStateLiveDataObserver, atLeastOnce()).onChanged(currentStateResultCaptor.capture()) + assertThat(currentStateResultCaptor.value.isSuccess()).isTrue() + val currentState = currentStateResultCaptor.value.getOrThrow() + assertThat(currentState.stateTypeCase).isEqualTo(COMPLETED_STATE) + assertThat(currentState.state.name).isEqualTo("What language") + assertThat(currentState.completedState.getAnswer(0).userAnswer.normalizedString).isEqualTo("Finnish") + } + + @Test + @ExperimentalCoroutinesApi + fun testMoveToNext_onFinalState_failsWithError() = runBlockingTest(coroutineContext) { + subscribeToCurrentStateToAllowExplorationToLoad() + playExploration(TEST_EXPLORATION_ID_5) + submitMultipleChoiceAnswerAndMoveToNextState(0) + submitTextInputAnswerAndMoveToNextState("Finnish") + submitNumericInputAnswerAndMoveToNextState(121) + submitContinueButtonAnswerAndMoveToNextState() + + val moveToStateResult = explorationProgressController.moveToNextState() + moveToStateResult.observeForever(mockAsyncResultLiveDataObserver) + advanceUntilIdle() + + // Verify we can't navigate past the last state of the exploration. + verify(mockAsyncResultLiveDataObserver, atLeastOnce()).onChanged(asyncResultCaptor.capture()) + assertThat(asyncResultCaptor.value.isFailure()).isTrue() + assertThat(asyncResultCaptor.value.getErrorOrNull()) + .hasMessageThat() + .contains("Cannot navigate to next state; at most recent state.") + } + + @Test + @ExperimentalCoroutinesApi + fun testGetCurrentState_afterPlayingFullSecondExploration_returnsTerminalState() = runBlockingTest(coroutineContext) { + val currentStateLiveData = explorationProgressController.getCurrentState() + currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) + + playExploration(TEST_EXPLORATION_ID_6) + submitContinueButtonAnswerAndMoveToNextState() + submitMultipleChoiceAnswerAndMoveToNextState(3) // Those were all the questions I had! + submitContinueButtonAnswerAndMoveToNextState() + + // Verify that we're now on the final state. + verify(mockCurrentStateLiveDataObserver, atLeastOnce()).onChanged(currentStateResultCaptor.capture()) + assertThat(currentStateResultCaptor.value.isSuccess()).isTrue() + val currentState = currentStateResultCaptor.value.getOrThrow() + assertThat(currentState.stateTypeCase).isEqualTo(TERMINAL_STATE) + } + + @Test + @ExperimentalCoroutinesApi + fun testGetCurrentState_afterPlayingFullSecondExploration_diffPath_returnsTerminalState() = runBlockingTest( + coroutineContext + ) { + val currentStateLiveData = explorationProgressController.getCurrentState() + currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) + + playExploration(TEST_EXPLORATION_ID_6) + submitContinueButtonAnswerAndMoveToNextState() + submitMultipleChoiceAnswerAndMoveToNextState(0) // How do your explorations work? + submitTextInputAnswerAndMoveToNextState("Oppia Otter") // Can I ask your name? + submitContinueButtonAnswerAndMoveToNextState() + submitMultipleChoiceAnswerAndMoveToNextState(3) // Those were all the questions I had! + submitContinueButtonAnswerAndMoveToNextState() + + // Verify that a different path can also result in reaching the end state. + verify(mockCurrentStateLiveDataObserver, atLeastOnce()).onChanged(currentStateResultCaptor.capture()) + assertThat(currentStateResultCaptor.value.isSuccess()).isTrue() + val currentState = currentStateResultCaptor.value.getOrThrow() + assertThat(currentState.stateTypeCase).isEqualTo(TERMINAL_STATE) + } + + @Test + @ExperimentalCoroutinesApi + fun testGetCurrentState_afterPlayingThroughPreviousExplorations_returnsStateFromSecondExp() = runBlockingTest( + coroutineContext + ) { + val currentStateLiveData = explorationProgressController.getCurrentState() + currentStateLiveData.observeForever(mockCurrentStateLiveDataObserver) + playThroughExploration5() + + playExploration(TEST_EXPLORATION_ID_6) + submitContinueButtonAnswerAndMoveToNextState() + submitMultipleChoiceAnswerAndMoveToNextState(3) // Those were all the questions I had! + + // Verify that we're on the second-to-last state of the second exploration. + verify(mockCurrentStateLiveDataObserver, atLeastOnce()).onChanged(currentStateResultCaptor.capture()) + assertThat(currentStateResultCaptor.value.isSuccess()).isTrue() + val currentState = currentStateResultCaptor.value.getOrThrow() + assertThat(currentState.stateTypeCase).isEqualTo(PENDING_STATE) + assertThat(currentState.state.name).isEqualTo("End Card") // This state is not in the other test exp. + } private suspend fun getTestExploration5(): Exploration { return explorationRetriever.loadExploration(TEST_EXPLORATION_ID_5) @@ -401,22 +1021,111 @@ class ExplorationProgressControllerTest { .inject(this) } - // TODO(#89): Move to a common test library. - /** A replacement to JUnit5's assertThrows(). */ - private fun assertThrows(type: KClass, operation: () -> Unit): T { - try { - operation() - fail("Expected to encounter exception of $type") - throw IllegalStateException("JUnit failed to interrupt execution flow.") - } catch (t: Throwable) { - if (type.isInstance(t)) { - return type.cast(t) - } - // Unexpected exception; throw it. - throw t - } + /** + * Creates a blank subscription to the current state to ensure that requests to load the exploration complete, + * otherwise post-load operations may fail. An observer is required since the current mediator live data + * implementation will only lazily load data based on whether there's an active subscription. + */ + private fun subscribeToCurrentStateToAllowExplorationToLoad() { + explorationProgressController.getCurrentState().observeForever(mockCurrentStateLiveDataObserver) + } + + @ExperimentalCoroutinesApi + private fun playExploration(explorationId: String) { + explorationDataController.startPlayingExploration(explorationId) + testDispatcher.advanceUntilIdle() + } + + @ExperimentalCoroutinesApi + private fun submitMultipleChoiceAnswer(choiceIndex: Int) { + explorationProgressController.submitAnswer(createMultipleChoiceAnswer(choiceIndex)) + testDispatcher.advanceUntilIdle() + } + + @ExperimentalCoroutinesApi + private fun submitTextInputAnswer(textAnswer: String) { + explorationProgressController.submitAnswer(createTextInputAnswer(textAnswer)) + testDispatcher.advanceUntilIdle() + } + + @ExperimentalCoroutinesApi + private fun submitNumericInputAnswer(numericAnswer: Int) { + explorationProgressController.submitAnswer(createNumericInputAnswer(numericAnswer)) + testDispatcher.advanceUntilIdle() + } + + @ExperimentalCoroutinesApi + private fun submitContinueButtonAnswer() { + explorationProgressController.submitAnswer(createContinueButtonAnswer()) + testDispatcher.advanceUntilIdle() + } + + @ExperimentalCoroutinesApi + private fun submitMultipleChoiceAnswerAndMoveToNextState(choiceIndex: Int) { + submitMultipleChoiceAnswer(choiceIndex) + moveToNextState() + } + + @ExperimentalCoroutinesApi + private fun submitTextInputAnswerAndMoveToNextState(textAnswer: String) { + submitTextInputAnswer(textAnswer) + moveToNextState() } + @ExperimentalCoroutinesApi + private fun submitNumericInputAnswerAndMoveToNextState(numericAnswer: Int) { + submitNumericInputAnswer(numericAnswer) + moveToNextState() + } + + @ExperimentalCoroutinesApi + private fun submitContinueButtonAnswerAndMoveToNextState() { + submitContinueButtonAnswer() + moveToNextState() + } + + @ExperimentalCoroutinesApi + private fun moveToNextState() { + explorationProgressController.moveToNextState() + testDispatcher.advanceUntilIdle() + } + + @ExperimentalCoroutinesApi + private fun moveToPreviousState() { + explorationProgressController.moveToPreviousState() + testDispatcher.advanceUntilIdle() + } + + @ExperimentalCoroutinesApi + private fun endExploration() { + explorationDataController.stopPlayingExploration() + testDispatcher.advanceUntilIdle() + } + + @ExperimentalCoroutinesApi + private fun playThroughExploration5() { + playExploration(TEST_EXPLORATION_ID_5) + submitMultipleChoiceAnswerAndMoveToNextState(0) + submitTextInputAnswerAndMoveToNextState("Finnish") + submitNumericInputAnswerAndMoveToNextState(121) + submitContinueButtonAnswerAndMoveToNextState() + endExploration() + } + + private fun createMultipleChoiceAnswer(choiceIndex: Int): InteractionObject { + return InteractionObject.newBuilder().setNonNegativeInt(choiceIndex).build() + } + + private fun createTextInputAnswer(textAnswer: String): InteractionObject { + return InteractionObject.newBuilder().setNormalizedString(textAnswer).build() + } + + private fun createNumericInputAnswer(numericAnswer: Int): InteractionObject { + return InteractionObject.newBuilder().setSignedInt(numericAnswer).build() + } + + private fun createContinueButtonAnswer() = createTextInputAnswer(DEFAULT_CONTINUE_INTERACTION_TEXT_ANSWER) + @Qualifier annotation class TestDispatcher // TODO(#89): Move this to a common test application component. From a2da218417b8fc7e903ef4f25ec4aeb2ba120806 Mon Sep 17 00:00:00 2001 From: veena Date: Fri, 4 Oct 2019 17:12:09 +0530 Subject: [PATCH 008/146] craeted image parser --- .../player/image_parser/UrlImageParserTest.kt | 75 +++++++++++++++++ .../java/org/oppia/util/parser/HtmlParser.kt | 38 +++++++++ .../org/oppia/util/parser/ImageLoader.java | 20 +++++ .../oppia/util/parser/ImageParsingFactory.kt | 15 ++++ .../oppia/util/parser/ImageParsingModule.kt | 28 +++++++ .../org/oppia/util/parser/UrlImageParser.kt | 80 +++++++++++++++++++ 6 files changed, 256 insertions(+) create mode 100755 app/src/sharedTest/java/org/oppia/app/player/image_parser/UrlImageParserTest.kt create mode 100755 utility/src/main/java/org/oppia/util/parser/HtmlParser.kt create mode 100755 utility/src/main/java/org/oppia/util/parser/ImageLoader.java create mode 100755 utility/src/main/java/org/oppia/util/parser/ImageParsingFactory.kt create mode 100755 utility/src/main/java/org/oppia/util/parser/ImageParsingModule.kt create mode 100755 utility/src/main/java/org/oppia/util/parser/UrlImageParser.kt diff --git a/app/src/sharedTest/java/org/oppia/app/player/image_parser/UrlImageParserTest.kt b/app/src/sharedTest/java/org/oppia/app/player/image_parser/UrlImageParserTest.kt new file mode 100755 index 00000000000..76a38770eff --- /dev/null +++ b/app/src/sharedTest/java/org/oppia/app/player/image_parser/UrlImageParserTest.kt @@ -0,0 +1,75 @@ +package org.oppia.app.player.image_parser + +import android.app.Application +import android.content.Context +import androidx.test.core.app.ActivityScenario +import androidx.test.espresso.Espresso.onView +import androidx.test.espresso.assertion.ViewAssertions.matches +import androidx.test.espresso.matcher.ViewMatchers.isDisplayed +import androidx.test.espresso.matcher.ViewMatchers.withId +import androidx.test.ext.junit.runners.AndroidJUnit4 +import dagger.BindsInstance +import dagger.Component +import dagger.Module +import dagger.Provides +import kotlinx.coroutines.CoroutineDispatcher +import org.junit.Test +import org.junit.runner.RunWith +import org.oppia.app.player.exploration.ExplorationActivity +import org.oppia.util.parser.UrlImageParser +import org.oppia.util.threading.BackgroundDispatcher +import org.oppia.util.threading.BlockingDispatcher +import javax.inject.Singleton + +/** Tests for [UrlImageParser]. */ +@RunWith(AndroidJUnit4::class) +class UrlImageParserTest { + + + @Test + fun testUrlImageParser_loadHtmlContent_isDisplayed() { + ActivityScenario.launch(ExplorationActivity::class.java).use { + onView(withId(org.oppia.app.R.id.recyclerview)).check(matches(isDisplayed())) + } + } + + fun pauseTestFor(milliseconds: Long) { + try { + Thread.sleep(milliseconds) + } catch (e: InterruptedException) { + e.printStackTrace(); + } + } + + @Module + class TestModule { + @Provides + @Singleton + fun provideContext(application: Application): Context { + return application + } + + // TODO(#89): Introduce a proper IdlingResource for background dispatchers to ensure they all complete before + // proceeding in an Espresso test. This solution should also be interoperative with Robolectric contexts by using a + // test coroutine dispatcher. + + @Singleton + @Provides + @BackgroundDispatcher + fun provideBackgroundDispatcher(@BlockingDispatcher blockingDispatcher: CoroutineDispatcher): CoroutineDispatcher { + return blockingDispatcher + } + } + + @Singleton + @Component(modules = [TestModule::class]) + interface TestApplicationComponent { + @Component.Builder + interface Builder { + @BindsInstance + fun setApplication(application: Application): Builder + + fun build(): TestApplicationComponent + } + } +} diff --git a/utility/src/main/java/org/oppia/util/parser/HtmlParser.kt b/utility/src/main/java/org/oppia/util/parser/HtmlParser.kt new file mode 100755 index 00000000000..86bd83ca480 --- /dev/null +++ b/utility/src/main/java/org/oppia/util/parser/HtmlParser.kt @@ -0,0 +1,38 @@ +package org.oppia.util.parser; + +import android.content.Context +import android.text.Html +import android.text.Spannable +import android.widget.TextView + +const val CUSTOM_TAG = "oppia-noninteractive-image" +const val HTML_TAG = "img" +const val CUSTOM_ATTRIBUTE = "filepath-with-value" +const val HTML_ATTRIBUTE = "src" + +/** Html Parser for android TextView to parse Html tag. */ +class HtmlParser( + internal var context: Context, + val entity_type: String, + val entity_id: String +) { + + fun parseHtml(rawString: String?, tvContents: TextView): Spannable { + val html: Spannable + var htmlContent = rawString + if (htmlContent!!.contains(CUSTOM_TAG)) { + htmlContent = htmlContent.replace(CUSTOM_TAG, HTML_TAG, false); + htmlContent = htmlContent.replace( + CUSTOM_ATTRIBUTE, + HTML_ATTRIBUTE, false); + htmlContent = htmlContent.replace("&quot;", "") + } + var imageGetter = UrlImageParser(tvContents, context, entity_type, entity_id) + if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.N) { + html = Html.fromHtml(htmlContent, Html.FROM_HTML_MODE_LEGACY, imageGetter, null) as Spannable + } else { + html = Html.fromHtml(htmlContent, imageGetter, null) as Spannable + } + return html + } +} diff --git a/utility/src/main/java/org/oppia/util/parser/ImageLoader.java b/utility/src/main/java/org/oppia/util/parser/ImageLoader.java new file mode 100755 index 00000000000..9f2271c4e32 --- /dev/null +++ b/utility/src/main/java/org/oppia/util/parser/ImageLoader.java @@ -0,0 +1,20 @@ +package org.oppia.util.parser; + +import android.content.Context; +import android.graphics.Bitmap; +import androidx.annotation.DrawableRes; +import com.bumptech.glide.Glide; +import com.bumptech.glide.request.target.SimpleTarget; + +import java.lang.annotation.Target; + +public interface ImageLoader { + + static void load(Context context, String path, int placeholder, SimpleTarget target) { + Glide.with(context) + .asBitmap() + .load(path) + .placeholder(placeholder) + .into(target); + } +} diff --git a/utility/src/main/java/org/oppia/util/parser/ImageParsingFactory.kt b/utility/src/main/java/org/oppia/util/parser/ImageParsingFactory.kt new file mode 100755 index 00000000000..7b5f90e7be8 --- /dev/null +++ b/utility/src/main/java/org/oppia/util/parser/ImageParsingFactory.kt @@ -0,0 +1,15 @@ +//package org.oppia.util.parser +// +//import javax.inject.Inject +//import javax.inject.Singleton +// +// +// @Singleton +// class Factory @Inject constructor( +// gcs_prefix: String, gcs_resource_bucket_name: String, image_download_url_template: String +// ) { +// fun create(gcs_prefix: String, gcs_resource_bucket_name: String, image_download_url_template: String): { +// return gcs_prefix+gcs_resource_bucket_name+image_download_url_template +// } +// } +// diff --git a/utility/src/main/java/org/oppia/util/parser/ImageParsingModule.kt b/utility/src/main/java/org/oppia/util/parser/ImageParsingModule.kt new file mode 100755 index 00000000000..25880d4eadf --- /dev/null +++ b/utility/src/main/java/org/oppia/util/parser/ImageParsingModule.kt @@ -0,0 +1,28 @@ +package org.oppia.util.parser + +import dagger.Module +import dagger.Provides +import javax.inject.Singleton + + +/** Provides image-extraction URL dependencies. */ +@Module +class ImageParsingModule { + @Provides + @Singleton + fun provide_GCS_PREFIX(): String { + return "https://storage.googleapis.com/" + } + + @Provides + @Singleton + fun provide_GCS_RESOURCE_BUCKET_NAME(): String { + return "oppiaserver-resources/" + } + + @Provides + @Singleton + fun provide_IMAGE_DOWNLOAD_URL_TEMPLATE(): String { + return "%s/%s/assets/image/%s" + } +} diff --git a/utility/src/main/java/org/oppia/util/parser/UrlImageParser.kt b/utility/src/main/java/org/oppia/util/parser/UrlImageParser.kt new file mode 100755 index 00000000000..4a2f431bd7c --- /dev/null +++ b/utility/src/main/java/org/oppia/util/parser/UrlImageParser.kt @@ -0,0 +1,80 @@ +package org.oppia.util.parser + +import android.content.Context +import android.graphics.Bitmap +import android.graphics.Canvas +import android.graphics.Rect +import android.graphics.drawable.BitmapDrawable +import android.graphics.drawable.Drawable +import android.text.Html +import android.widget.TextView +import com.bumptech.glide.request.target.SimpleTarget +import com.bumptech.glide.request.transition.Transition +import org.oppia.util.R + +// TODO (#169) : Replace this with exploration asset downloader +/** UrlImage Parser for android TextView to load Html Image tag. */ +class UrlImageParser( + internal var tvContents: TextView, + internal var context: Context, + private val entity_type: String, + private val entity_id: String +) : Html.ImageGetter { + + val GCS_PREFIX: String = "https://storage.googleapis.com/" + val GCS_RESOURCE_BUCKET_NAME = "oppiaserver-resources/" + var IMAGE_DOWNLOAD_URL_TEMPLATE = "%s/%s/assets/image/%s" + + /*** + * This method is called when the HTML parser encounters an tag + * @param urlString : urlString argument is the string from the "src" attribute + * @return Drawable : Drawable representation of the image + */ + override fun getDrawable(urlString: String): Drawable { + + IMAGE_DOWNLOAD_URL_TEMPLATE = String.format(IMAGE_DOWNLOAD_URL_TEMPLATE, entity_type, entity_id, urlString) + val urlDrawable = UrlDrawable() + val target = BitmapTarget(urlDrawable) + ImageLoader.load( + context, + GCS_PREFIX + GCS_RESOURCE_BUCKET_NAME + IMAGE_DOWNLOAD_URL_TEMPLATE, + R.drawable.abc_ab_share_pack_mtrl_alpha, + target + ); + return urlDrawable + } + + inner class BitmapTarget(private val urlDrawable: UrlDrawable) : SimpleTarget() { + internal var drawable: Drawable? = null + override fun onResourceReady(resource: Bitmap, transition: Transition?) { + drawable = BitmapDrawable(context.resources, resource) + tvContents.post { + val textViewWidth = tvContents.width + val drawableHeight = (drawable as BitmapDrawable).intrinsicHeight + val drawableWidth = (drawable as BitmapDrawable).intrinsicWidth + // To resize the image keeping aspect ratio. + if (drawableWidth > textViewWidth) { + val calculatedHeight = textViewWidth * drawableHeight / drawableWidth; + val rect = Rect(0, 0, textViewWidth, calculatedHeight) + (drawable as BitmapDrawable).bounds = rect + urlDrawable.bounds = rect + } else { + val rect = Rect(0, 0, drawableWidth, drawableHeight) + (drawable as BitmapDrawable).bounds = rect + urlDrawable.bounds = rect + } + urlDrawable.drawable = drawable + tvContents.text = tvContents.text + tvContents.invalidate() + } + } + } + + inner class UrlDrawable : BitmapDrawable() { + var drawable: Drawable? = null + override fun draw(canvas: Canvas) { + if (drawable != null) + drawable!!.draw(canvas) + } + } +} From 4a9aad14f35f30bd7bc9382fe72d7342db80c06c Mon Sep 17 00:00:00 2001 From: Rajat Talesra Date: Mon, 7 Oct 2019 14:34:43 +0530 Subject: [PATCH 009/146] ObservableViewModel --- .../app/viewmodel/ObservableViewModel.kt | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 app/src/main/java/org/oppia/app/viewmodel/ObservableViewModel.kt diff --git a/app/src/main/java/org/oppia/app/viewmodel/ObservableViewModel.kt b/app/src/main/java/org/oppia/app/viewmodel/ObservableViewModel.kt new file mode 100644 index 00000000000..4fa7ae7c2a6 --- /dev/null +++ b/app/src/main/java/org/oppia/app/viewmodel/ObservableViewModel.kt @@ -0,0 +1,30 @@ +package org.oppia.app.viewmodel + +import androidx.databinding.Observable +import androidx.databinding.PropertyChangeRegistry +import androidx.lifecycle.ViewModel + +/** A [ViewModel] that behaves the same as [androidx.databinding.BaseObservable]. */ +open class ObservableViewModel: ViewModel(), Observable { + private val callbacks by lazy { + PropertyChangeRegistry() + } + + override fun removeOnPropertyChangedCallback(callback: Observable.OnPropertyChangedCallback?) { + callbacks.remove(callback) + } + + override fun addOnPropertyChangedCallback(callback: Observable.OnPropertyChangedCallback?) { + callbacks.add(callback) + } + + /** See [androidx.databinding.BaseObservable.notifyChange]. */ + fun notifyChange() { + notifyPropertyChanged(/* fieldId= */ 0) + } + + /** See [androidx.databinding.BaseObservable.notifyPropertyChanged]. */ + fun notifyPropertyChanged(fieldId: Int) { + callbacks.notifyChange(this, fieldId) + } +} From 07055864a3dbe42242adce4afd8cfa5fa000a0f2 Mon Sep 17 00:00:00 2001 From: Rajat Talesra Date: Mon, 7 Oct 2019 16:11:47 +0530 Subject: [PATCH 010/146] Load exploration and open ExplorationActivity --- .../java/org/oppia/app/home/HomeActivity.kt | 8 ++++- .../oppia/app/home/HomeFragmentController.kt | 33 ++++++++++++++++- .../app/home/RouteToExplorationListener.kt | 6 ++++ .../player/exploration/ExplorationActivity.kt | 13 ++++++- .../player/state/StateFragmentPresenter.kt | 35 +++++++++++++++++-- app/src/main/res/layout/home_fragment.xml | 17 ++++++--- 6 files changed, 103 insertions(+), 9 deletions(-) create mode 100755 app/src/main/java/org/oppia/app/home/RouteToExplorationListener.kt diff --git a/app/src/main/java/org/oppia/app/home/HomeActivity.kt b/app/src/main/java/org/oppia/app/home/HomeActivity.kt index d5fe94ca133..c5707d3e4d6 100644 --- a/app/src/main/java/org/oppia/app/home/HomeActivity.kt +++ b/app/src/main/java/org/oppia/app/home/HomeActivity.kt @@ -1,11 +1,13 @@ package org.oppia.app.home import android.os.Bundle +import android.util.Log import org.oppia.app.activity.InjectableAppCompatActivity +import org.oppia.app.player.exploration.ExplorationActivity import javax.inject.Inject /** The central activity for all users entering the app. */ -class HomeActivity : InjectableAppCompatActivity() { +class HomeActivity : InjectableAppCompatActivity(), RouteToExplorationListener { @Inject lateinit var homeActivityController: HomeActivityController override fun onCreate(savedInstanceState: Bundle?) { @@ -13,4 +15,8 @@ class HomeActivity : InjectableAppCompatActivity() { activityComponent.inject(this) homeActivityController.handleOnCreate() } + + override fun routeToExploration() { + startActivity(ExplorationActivity.createExplorationActivityIntent(this)) + } } diff --git a/app/src/main/java/org/oppia/app/home/HomeFragmentController.kt b/app/src/main/java/org/oppia/app/home/HomeFragmentController.kt index d8f19de8685..052f08fc542 100644 --- a/app/src/main/java/org/oppia/app/home/HomeFragmentController.kt +++ b/app/src/main/java/org/oppia/app/home/HomeFragmentController.kt @@ -1,28 +1,44 @@ package org.oppia.app.home +import android.util.Log import android.view.LayoutInflater import android.view.View import android.view.ViewGroup +import androidx.appcompat.app.AppCompatActivity import androidx.fragment.app.Fragment +import androidx.lifecycle.Observer import org.oppia.app.databinding.HomeFragmentBinding import org.oppia.app.fragment.FragmentScope import org.oppia.app.viewmodel.ViewModelProvider import org.oppia.domain.UserAppHistoryController +import org.oppia.domain.exploration.ExplorationDataController +import org.oppia.domain.exploration.TEST_EXPLORATION_ID_5 +import org.oppia.util.data.AsyncResult +import org.oppia.util.logging.Logger import javax.inject.Inject +private const val TEST_EXPLORATION_ID = TEST_EXPLORATION_ID_5 + /** The controller for [HomeFragment]. */ @FragmentScope class HomeFragmentController @Inject constructor( + activity: AppCompatActivity, private val fragment: Fragment, private val viewModelProvider: ViewModelProvider, - private val userAppHistoryController: UserAppHistoryController + private val userAppHistoryController: UserAppHistoryController, + private val explorationDataController: ExplorationDataController, + private val logger: Logger ) { + + private val routeToExplorationListener = activity as RouteToExplorationListener + fun handleCreateView(inflater: LayoutInflater, container: ViewGroup?): View? { val binding = HomeFragmentBinding.inflate(inflater, container, /* attachToRoot= */ false) // NB: Both the view model and lifecycle owner must be set in order to correctly bind LiveData elements to // data-bound view models. binding.let { it.viewModel = getUserAppHistoryViewModel() + it.presenter = this it.lifecycleOwner = fragment } @@ -34,4 +50,19 @@ class HomeFragmentController @Inject constructor( private fun getUserAppHistoryViewModel(): UserAppHistoryViewModel { return viewModelProvider.getForFragment(fragment, UserAppHistoryViewModel::class.java) } + + fun goToExplorationButtonClicked(v: View) { + explorationDataController.startPlayingExploration( + TEST_EXPLORATION_ID + ).observe(fragment, Observer> { result -> + when { + result.isPending() -> logger.d("HomeFragmentController", "Loading exploration") + result.isFailure() -> logger.e("HomeFragmentController", "Failed to load exploration", result.getErrorOrNull()!!) + else -> { + logger.d("HomeFragmentController", "Successfully loaded exploration") + routeToExplorationListener.routeToExploration() + } + } + }) + } } diff --git a/app/src/main/java/org/oppia/app/home/RouteToExplorationListener.kt b/app/src/main/java/org/oppia/app/home/RouteToExplorationListener.kt new file mode 100755 index 00000000000..c853c876e8c --- /dev/null +++ b/app/src/main/java/org/oppia/app/home/RouteToExplorationListener.kt @@ -0,0 +1,6 @@ +package org.oppia.app.home + +/** Listener for when an activity should route to a exploration. */ +interface RouteToExplorationListener { + fun routeToExploration() +} diff --git a/app/src/main/java/org/oppia/app/player/exploration/ExplorationActivity.kt b/app/src/main/java/org/oppia/app/player/exploration/ExplorationActivity.kt index 5df1bd8884c..72a149e2943 100755 --- a/app/src/main/java/org/oppia/app/player/exploration/ExplorationActivity.kt +++ b/app/src/main/java/org/oppia/app/player/exploration/ExplorationActivity.kt @@ -1,16 +1,27 @@ package org.oppia.app.player.exploration +import android.content.Context +import android.content.Intent import android.os.Bundle +import android.util.Log import org.oppia.app.activity.InjectableAppCompatActivity import javax.inject.Inject /** The starting point for exploration. */ class ExplorationActivity : InjectableAppCompatActivity() { - @Inject lateinit var explorationActivityPresenter: ExplorationActivityPresenter + @Inject + lateinit var explorationActivityPresenter: ExplorationActivityPresenter override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) activityComponent.inject(this) explorationActivityPresenter.handleOnCreate() } + + companion object { + /** Returns a new [Intent] to route to [ExplorationActivity] for a specified exploration ID. */ + fun createExplorationActivityIntent(context: Context): Intent { + return Intent(context, ExplorationActivity::class.java) + } + } } diff --git a/app/src/main/java/org/oppia/app/player/state/StateFragmentPresenter.kt b/app/src/main/java/org/oppia/app/player/state/StateFragmentPresenter.kt index f5a2d663fd0..7c7b90c37d2 100755 --- a/app/src/main/java/org/oppia/app/player/state/StateFragmentPresenter.kt +++ b/app/src/main/java/org/oppia/app/player/state/StateFragmentPresenter.kt @@ -4,14 +4,19 @@ import android.view.LayoutInflater import android.view.View import android.view.ViewGroup import androidx.fragment.app.Fragment +import androidx.lifecycle.LiveData import androidx.lifecycle.Observer +import androidx.lifecycle.Transformations import org.oppia.app.databinding.StateFragmentBinding import org.oppia.app.fragment.FragmentScope import org.oppia.app.model.CellularDataPreference +import org.oppia.app.model.EphemeralState import org.oppia.app.player.audio.CellularDataDialogFragment import org.oppia.app.viewmodel.ViewModelProvider import org.oppia.domain.audio.CellularDialogController +import org.oppia.domain.exploration.ExplorationProgressController import org.oppia.util.data.AsyncResult +import org.oppia.util.logging.Logger import javax.inject.Inject private const val TAG_CELLULAR_DATA_DIALOG = "CELLULAR_DATA_DIALOG" @@ -21,7 +26,9 @@ private const val TAG_CELLULAR_DATA_DIALOG = "CELLULAR_DATA_DIALOG" class StateFragmentPresenter @Inject constructor( private val fragment: Fragment, private val cellularDialogController: CellularDialogController, - private val viewModelProvider: ViewModelProvider + private val viewModelProvider: ViewModelProvider, + private val explorationProgressController: ExplorationProgressController, + private val logger: Logger ) { private var showCellularDataDialog = true @@ -29,7 +36,7 @@ class StateFragmentPresenter @Inject constructor( fun handleCreateView(inflater: LayoutInflater, container: ViewGroup?): View? { cellularDialogController.getCellularDataPreference() - .observe(fragment, Observer>{ + .observe(fragment, Observer> { if (it.isSuccess()) { val prefs = it.getOrDefault(CellularDataPreference.getDefaultInstance()) showCellularDataDialog = !(prefs.hideDialog) @@ -42,6 +49,9 @@ class StateFragmentPresenter @Inject constructor( it.stateFragment = fragment as StateFragment it.viewModel = getStateViewModel() } + + getCurrentState() + return binding.root } @@ -81,4 +91,25 @@ class StateFragmentPresenter @Inject constructor( fun setAudioFragmentVisible(isVisible: Boolean) { getStateViewModel().setAudioFragmentVisible(isVisible) } + + private fun getCurrentState() { + ephemeralStateLiveData.observe(fragment, Observer { result -> + logger.d("TAG", "getCurrentState: " + result.state.name) + }) + } + + private val ephemeralStateLiveData: LiveData by lazy { + getEphemeralState() + } + + private fun getEphemeralState(): LiveData { + return Transformations.map(explorationProgressController.getCurrentState(), ::processCurrentState) + } + + private fun processCurrentState(ephemeralStateResult: AsyncResult): EphemeralState { + if (ephemeralStateResult.isFailure()) { + logger.e("StateFragmentPresenter", "Failed to retrieve ephemeral state", ephemeralStateResult.getErrorOrNull()!!) + } + return ephemeralStateResult.getOrDefault(EphemeralState.getDefaultInstance()) + } } diff --git a/app/src/main/res/layout/home_fragment.xml b/app/src/main/res/layout/home_fragment.xml index 45e60f50205..7479c99e42f 100644 --- a/app/src/main/res/layout/home_fragment.xml +++ b/app/src/main/res/layout/home_fragment.xml @@ -2,18 +2,18 @@ - + - - - +