-
Notifications
You must be signed in to change notification settings - Fork 532
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #120: Introduce question data controller API #217
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
27 changes: 27 additions & 0 deletions
27
domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
package org.oppia.domain.question | ||
|
||
import androidx.lifecycle.LiveData | ||
import org.oppia.app.model.Question | ||
import org.oppia.util.data.AsyncResult | ||
import javax.inject.Inject | ||
import javax.inject.Singleton | ||
|
||
/** | ||
* Controller that tracks and reports the learner's ephemeral/non-persisted progress through a question training | ||
* session. Note that this controller only supports one active training session at a time. | ||
* | ||
* The current training session session is started via the question training controller. | ||
* | ||
* 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 QuestionAssessmentProgressController @Inject constructor( | ||
) { | ||
fun beginQuestionTrainingSession(questionsList: LiveData<AsyncResult<List<Question>>>) { | ||
} | ||
|
||
fun finishQuestionTrainingSession() { | ||
|
||
} | ||
} |
101 changes: 101 additions & 0 deletions
101
domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,101 @@ | ||
package org.oppia.domain.question | ||
|
||
import androidx.lifecycle.LiveData | ||
import androidx.lifecycle.MutableLiveData | ||
import org.oppia.app.model.Question | ||
import org.oppia.domain.topic.TEST_SKILL_ID_0 | ||
import org.oppia.domain.topic.TEST_SKILL_ID_1 | ||
import org.oppia.domain.topic.TEST_SKILL_ID_2 | ||
import org.oppia.util.data.AsyncResult | ||
import org.oppia.util.data.DataProviders | ||
import javax.inject.Inject | ||
import javax.inject.Singleton | ||
|
||
private const val QUESTION_DATA_PROVIDER_ID = "QuestionDataProvider" | ||
const val TEST_QUESTION_ID_0 = "question_id_0" | ||
const val TEST_QUESTION_ID_1 = "question_id_1" | ||
const val TEST_QUESTION_ID_2 = "question_id_2" | ||
|
||
/** Controller for retrieving a set of questions. */ | ||
@Singleton | ||
class QuestionTrainingController @Inject constructor( | ||
private val questionAssessmentProgressController: QuestionAssessmentProgressController, | ||
private val dataProviders: DataProviders | ||
) { | ||
/** | ||
* Begins a question training session given a list of skill Ids and a total number of questions. | ||
* | ||
* This method is not expected to fail. [QuestionAssessmentProgressController] should be used to manage the | ||
* play state, and monitor the load success/failure of the training session. | ||
* | ||
* Questions will be shuffled and then the training session will begin. | ||
* | ||
* @return a one-time [LiveData] to observe whether initiating the play request succeeded. | ||
* The training session may still fail to load, but this provides early-failure detection. | ||
*/ | ||
fun startQuestionTrainingSession(skillIdsList: List<String>): LiveData<AsyncResult<Any?>> { | ||
return try { | ||
val questionsList = retrieveQuestionsForSkillIds(skillIdsList) | ||
questionAssessmentProgressController.beginQuestionTrainingSession(questionsList) | ||
MutableLiveData(AsyncResult.success<Any?>(null)) | ||
} catch (e: Exception) { | ||
MutableLiveData(AsyncResult.failed(e)) | ||
} | ||
} | ||
|
||
/** | ||
* Finishes the most recent training session started by [startQuestionTrainingSession]. | ||
* This method should only be called if there is a training session is being played, | ||
* otherwise an exception will be thrown. | ||
*/ | ||
fun stopQuestionTrainingSession(): LiveData<AsyncResult<Any?>> { | ||
return try { | ||
questionAssessmentProgressController.finishQuestionTrainingSession() | ||
MutableLiveData(AsyncResult.success<Any?>(null)) | ||
} catch (e: Exception) { | ||
MutableLiveData(AsyncResult.failed(e)) | ||
} | ||
} | ||
|
||
private fun retrieveQuestionsForSkillIds(skillIdsList: List<String>): LiveData<AsyncResult<List<Question>>> { | ||
val dataProvider = dataProviders.createInMemoryDataProviderAsync(QUESTION_DATA_PROVIDER_ID) { | ||
loadQuestionsForSkillIds(skillIdsList) | ||
} | ||
return dataProviders.convertToLiveData(dataProvider) | ||
} | ||
|
||
// Loads and returns the questions given a list of skill ids. | ||
@Suppress("RedundantSuspendModifier") // DataProviders expects this function to be a suspend function. | ||
private suspend fun loadQuestionsForSkillIds(skillIdsList: List<String>): AsyncResult<List<Question>> { | ||
return try { | ||
AsyncResult.success(loadQuestions(skillIdsList)) | ||
} catch (e: Exception) { | ||
AsyncResult.failed(e) | ||
} | ||
} | ||
|
||
@Suppress("RedundantSuspendModifier") // Force callers to call this on a background thread. | ||
private suspend fun loadQuestions(skillIdsList: List<String>): List<Question> { | ||
val questionsList = mutableListOf<Question>() | ||
for (skillId in skillIdsList) { | ||
when (skillId) { | ||
TEST_SKILL_ID_0 -> questionsList.add( | ||
Question.newBuilder() | ||
.setQuestionId(TEST_QUESTION_ID_0) | ||
.build()) | ||
TEST_SKILL_ID_1 -> questionsList.add( | ||
Question.newBuilder() | ||
.setQuestionId(TEST_QUESTION_ID_1) | ||
.build()) | ||
TEST_SKILL_ID_2 -> questionsList.add( | ||
Question.newBuilder() | ||
.setQuestionId(TEST_QUESTION_ID_2) | ||
.build()) | ||
else -> { | ||
throw IllegalStateException("Invalid skill ID: $skillId") | ||
} | ||
} | ||
} | ||
return questionsList | ||
} | ||
} |
182 changes: 182 additions & 0 deletions
182
domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,182 @@ | ||
package org.oppia.domain.question | ||
|
||
import android.app.Application | ||
import android.content.Context | ||
import androidx.arch.core.executor.testing.InstantTaskExecutorRule | ||
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.Dispatchers | ||
import kotlinx.coroutines.ExperimentalCoroutinesApi | ||
import kotlinx.coroutines.ObsoleteCoroutinesApi | ||
import kotlinx.coroutines.newSingleThreadContext | ||
import kotlinx.coroutines.test.TestCoroutineDispatcher | ||
import kotlinx.coroutines.test.resetMain | ||
import kotlinx.coroutines.test.runBlockingTest | ||
import kotlinx.coroutines.test.setMain | ||
import org.junit.After | ||
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.atLeastOnce | ||
import org.mockito.Mockito.verify | ||
import org.mockito.junit.MockitoJUnit | ||
import org.mockito.junit.MockitoRule | ||
import org.oppia.domain.topic.TEST_SKILL_ID_0 | ||
import org.oppia.domain.topic.TEST_SKILL_ID_1 | ||
import org.oppia.util.data.AsyncResult | ||
import org.oppia.util.logging.EnableConsoleLog | ||
import org.oppia.util.logging.EnableFileLog | ||
import org.oppia.util.logging.GlobalLogLevel | ||
import org.oppia.util.logging.LogLevel | ||
import org.oppia.util.threading.BackgroundDispatcher | ||
import org.oppia.util.threading.BlockingDispatcher | ||
import org.robolectric.annotation.Config | ||
import javax.inject.Inject | ||
import javax.inject.Qualifier | ||
import javax.inject.Singleton | ||
import kotlin.coroutines.EmptyCoroutineContext | ||
|
||
const val TEST_TOPIC_ID_0 = "test_topic_id_0" | ||
|
||
/** Tests for [QuestionTrainingController]. */ | ||
@RunWith(AndroidJUnit4::class) | ||
@Config(manifest = Config.NONE) | ||
class QuestionTrainingControllerTest { | ||
@Rule | ||
@JvmField | ||
val mockitoRule: MockitoRule = MockitoJUnit.rule() | ||
|
||
@Rule | ||
@JvmField | ||
val executorRule = InstantTaskExecutorRule() | ||
|
||
@Inject | ||
lateinit var questionTrainingController: QuestionTrainingController | ||
|
||
@Mock | ||
lateinit var mockQuestionListObserver: Observer<AsyncResult<Any?>> | ||
|
||
@Captor | ||
lateinit var questionListResultCaptor: ArgumentCaptor<AsyncResult<Any?>> | ||
|
||
@Inject | ||
@field:TestDispatcher | ||
lateinit var testDispatcher: CoroutineDispatcher | ||
|
||
private val coroutineContext by lazy { | ||
EmptyCoroutineContext + testDispatcher | ||
} | ||
|
||
// https://kotlin.github.io/kotlinx.coroutines/kotlinx-coroutines-test/ | ||
@ObsoleteCoroutinesApi | ||
private val testThread = newSingleThreadContext("TestMain") | ||
|
||
@Before | ||
@ExperimentalCoroutinesApi | ||
@ObsoleteCoroutinesApi | ||
fun setUp() { | ||
Dispatchers.setMain(testThread) | ||
setUpTestApplicationComponent() | ||
} | ||
|
||
@After | ||
@ExperimentalCoroutinesApi | ||
@ObsoleteCoroutinesApi | ||
fun tearDown() { | ||
Dispatchers.resetMain() | ||
testThread.close() | ||
} | ||
|
||
private fun setUpTestApplicationComponent() { | ||
DaggerQuestionTrainingControllerTest_TestApplicationComponent.builder() | ||
.setApplication(ApplicationProvider.getApplicationContext()) | ||
.build() | ||
.inject(this) | ||
} | ||
|
||
@Test | ||
@ExperimentalCoroutinesApi | ||
fun testController_successfullyStartsQuestionSessionForExistingSkillIds() = runBlockingTest(coroutineContext) { | ||
val questionListLiveData = questionTrainingController.startQuestionTrainingSession( | ||
listOf(TEST_SKILL_ID_0, TEST_SKILL_ID_1)) | ||
advanceUntilIdle() | ||
questionListLiveData.observeForever(mockQuestionListObserver) | ||
verify(mockQuestionListObserver, atLeastOnce()).onChanged(questionListResultCaptor.capture()) | ||
assertThat(questionListResultCaptor.value.isSuccess()).isTrue() | ||
} | ||
|
||
@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(): CoroutineDispatcher { | ||
return TestCoroutineDispatcher() | ||
} | ||
|
||
@Singleton | ||
@Provides | ||
@BackgroundDispatcher | ||
fun provideBackgroundDispatcher(@TestDispatcher testDispatcher: CoroutineDispatcher): CoroutineDispatcher { | ||
return testDispatcher | ||
} | ||
|
||
@Singleton | ||
@Provides | ||
@BlockingDispatcher | ||
fun provideBlockingDispatcher(@TestDispatcher testDispatcher: CoroutineDispatcher): CoroutineDispatcher { | ||
return testDispatcher | ||
} | ||
|
||
// TODO(#59): Either isolate these to their own shared test module, or use the real logging | ||
// module in tests to avoid needing to specify these settings for tests. | ||
@EnableConsoleLog | ||
@Provides | ||
fun provideEnableConsoleLog(): Boolean = true | ||
|
||
@EnableFileLog | ||
@Provides | ||
fun provideEnableFileLog(): Boolean = false | ||
|
||
@GlobalLogLevel | ||
@Provides | ||
fun provideGlobalLogLevel(): LogLevel = LogLevel.VERBOSE | ||
} | ||
|
||
// 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(questionTrainingControllerTest: QuestionTrainingControllerTest) | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
syntax = "proto3"; | ||
|
||
package model; | ||
|
||
import "exploration.proto"; | ||
|
||
option java_package = "org.oppia.app.model"; | ||
option java_multiple_files = true; | ||
|
||
// Structure for a single question. | ||
message Question { | ||
string question_id = 1; | ||
State question_state = 2; | ||
string language_code = 3; | ||
int32 version = 4; | ||
repeated string linked_skill_ids = 5; | ||
int64 created_on_timestamp_ms = 6; | ||
int64 updated_on_timestamp_ms = 7; | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, how does routing work for questions? Presumably, the
Outcome
you match to an answer won't have a destination state name. Is this what thelabelled_as_correct
should be used for? Can you also route to an exploration from a question via the refresher exploration ID?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it doesnt have a destination state, it only uses labelled_as_correct, and it moves to the next question if the labelled_as_correct value is true (https://github.com/oppia/oppia/blob/40cb6adca9101f46209cecd4c83bd26a4f112bf9/core/templates/dev/head/pages/exploration-player-page/services/question-player-engine.service.ts#L206)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, then I think I modeled this correctly downstream. Thanks!