From 95949549dde1c26baa83ec082630524a253a9fea Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Mon, 7 Oct 2019 20:07:48 -0700 Subject: [PATCH 01/25] Initial check in for the question data controller --- .../domain/question/QuestionDataController.kt | 83 ++++++++ .../question/QuestionProgressController.kt | 18 ++ .../question/QuestionDataControllerTest.kt | 194 ++++++++++++++++++ .../QuestionProgressControllerTest.kt | 194 ++++++++++++++++++ model/src/main/proto/question.proto | 21 ++ 5 files changed, 510 insertions(+) create mode 100644 domain/src/main/java/org/oppia/domain/question/QuestionDataController.kt create mode 100644 domain/src/main/java/org/oppia/domain/question/QuestionProgressController.kt create mode 100644 domain/src/test/java/org/oppia/domain/question/QuestionDataControllerTest.kt create mode 100644 domain/src/test/java/org/oppia/domain/question/QuestionProgressControllerTest.kt create mode 100644 model/src/main/proto/question.proto diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionDataController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionDataController.kt new file mode 100644 index 00000000000..31261ff1e32 --- /dev/null +++ b/domain/src/main/java/org/oppia/domain/question/QuestionDataController.kt @@ -0,0 +1,83 @@ +package org.oppia.domain.question + +import android.content.Context +import androidx.lifecycle.LiveData +import androidx.lifecycle.MutableLiveData +import org.oppia.app.model.Question +import org.oppia.domain.topic.TEST_TOPIC_ID_0 +import org.oppia.domain.topic.TEST_TOPIC_ID_1 +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" + +/** Controller for retrieving an exploration. */ +@Singleton +class QuestionDataController @Inject constructor( + private val questionProgressController: QuestionProgressController, + private val dataProviders: DataProviders +) { + + /** + * Returns a list of [Question] objects given a topic ID. + */ + fun getQuestionsForTopic(topicId: String): LiveData>> { + val dataProvider = dataProviders.createInMemoryDataProviderAsync(QUESTION_DATA_PROVIDER_ID) { + retrieveQuestionsForTopic(topicId) + } + return dataProviders.convertToLiveData(dataProvider) + } + + /** + * Begins a question training session given a list of questions. This method is not expected to fail. + * [QuestionProgressController] 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(questionsList: List): LiveData> { + return try { + questionProgressController.beginQuestionTrainingSession(questionsList.shuffled()) + MutableLiveData(AsyncResult.success(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> { + return try { + questionProgressController.finishQuestionTrainingSession() + MutableLiveData(AsyncResult.success(null)) + } catch (e: Exception) { + MutableLiveData(AsyncResult.failed(e)) + } + } + + @Suppress("RedundantSuspendModifier") // DataProviders expects this function to be a suspend function. + private suspend fun retrieveQuestionsForTopic(topicId: String): AsyncResult> { + return try { + AsyncResult.success(loadQuestions(topicId)) + } catch (e: Exception) { + AsyncResult.failed(e) + } + } + + // Loads and returns the questions given a topic id. + private fun loadQuestions(topicId: String): List { + when(topicId) { + TEST_TOPIC_ID_0 -> return emptyList() + TEST_TOPIC_ID_1 -> return emptyList() + else -> throw IllegalStateException("Invalid topic ID: $topicId") + } + } +} \ No newline at end of file diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionProgressController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionProgressController.kt new file mode 100644 index 00000000000..55e34661bd0 --- /dev/null +++ b/domain/src/main/java/org/oppia/domain/question/QuestionProgressController.kt @@ -0,0 +1,18 @@ +package org.oppia.domain.question + +import org.oppia.app.model.Question +import javax.inject.Inject +import javax.inject.Singleton + + +/** Controller for retrieving an exploration. */ +@Singleton +class QuestionProgressController @Inject constructor( +) { + fun beginQuestionTrainingSession(questionsList: List) { + } + + fun finishQuestionTrainingSession() { + + } +} \ No newline at end of file diff --git a/domain/src/test/java/org/oppia/domain/question/QuestionDataControllerTest.kt b/domain/src/test/java/org/oppia/domain/question/QuestionDataControllerTest.kt new file mode 100644 index 00000000000..63c19d2cf47 --- /dev/null +++ b/domain/src/test/java/org/oppia/domain/question/QuestionDataControllerTest.kt @@ -0,0 +1,194 @@ +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.app.model.Question +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 [QuestionDataController]. */ +@RunWith(AndroidJUnit4::class) +@Config(manifest = Config.NONE) +class QuestionDataControllerTest { + @Rule + @JvmField + val mockitoRule: MockitoRule = MockitoJUnit.rule() + + @Rule + @JvmField + val executorRule = InstantTaskExecutorRule() + + @Inject + lateinit var questionDataController: QuestionDataController + + @Mock + lateinit var mockQuestionListObserver: Observer>> + + @Captor + lateinit var questionListResultCaptor: ArgumentCaptor>> + + @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() { + DaggerQuestionDataControllerTest_TestApplicationComponent.builder() + .setApplication(ApplicationProvider.getApplicationContext()) + .build() + .inject(this) + } + + @Test + @ExperimentalCoroutinesApi + fun testController_providesInitialLiveDataForTopicId0() = runBlockingTest(coroutineContext) { + val questionListLiveData = questionDataController.getQuestionsForTopic(TEST_TOPIC_ID_0) + advanceUntilIdle() + questionListLiveData.observeForever(mockQuestionListObserver) + + verify(mockQuestionListObserver, atLeastOnce()).onChanged(questionListResultCaptor.capture()) + assertThat(questionListResultCaptor.value.isSuccess()).isTrue() + assertThat(questionListResultCaptor.value.getOrThrow()).isNotNull() + val questionList = questionListResultCaptor.value.getOrThrow(); + assertThat(questionList.size).isEqualTo(0) + } + + @Test + @ExperimentalCoroutinesApi + fun testController_returnsFailureForNonExistentTopic() = runBlockingTest(coroutineContext) { + val questionListLiveData = questionDataController.getQuestionsForTopic("NON_EXISTENT_TOPIC") + advanceUntilIdle() + questionListLiveData.observeForever(mockQuestionListObserver) + verify(mockQuestionListObserver, atLeastOnce()).onChanged(questionListResultCaptor.capture()) + assertThat(questionListResultCaptor.value.isFailure()).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(questionDataControllerTest: QuestionDataControllerTest) + } +} diff --git a/domain/src/test/java/org/oppia/domain/question/QuestionProgressControllerTest.kt b/domain/src/test/java/org/oppia/domain/question/QuestionProgressControllerTest.kt new file mode 100644 index 00000000000..63c19d2cf47 --- /dev/null +++ b/domain/src/test/java/org/oppia/domain/question/QuestionProgressControllerTest.kt @@ -0,0 +1,194 @@ +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.app.model.Question +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 [QuestionDataController]. */ +@RunWith(AndroidJUnit4::class) +@Config(manifest = Config.NONE) +class QuestionDataControllerTest { + @Rule + @JvmField + val mockitoRule: MockitoRule = MockitoJUnit.rule() + + @Rule + @JvmField + val executorRule = InstantTaskExecutorRule() + + @Inject + lateinit var questionDataController: QuestionDataController + + @Mock + lateinit var mockQuestionListObserver: Observer>> + + @Captor + lateinit var questionListResultCaptor: ArgumentCaptor>> + + @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() { + DaggerQuestionDataControllerTest_TestApplicationComponent.builder() + .setApplication(ApplicationProvider.getApplicationContext()) + .build() + .inject(this) + } + + @Test + @ExperimentalCoroutinesApi + fun testController_providesInitialLiveDataForTopicId0() = runBlockingTest(coroutineContext) { + val questionListLiveData = questionDataController.getQuestionsForTopic(TEST_TOPIC_ID_0) + advanceUntilIdle() + questionListLiveData.observeForever(mockQuestionListObserver) + + verify(mockQuestionListObserver, atLeastOnce()).onChanged(questionListResultCaptor.capture()) + assertThat(questionListResultCaptor.value.isSuccess()).isTrue() + assertThat(questionListResultCaptor.value.getOrThrow()).isNotNull() + val questionList = questionListResultCaptor.value.getOrThrow(); + assertThat(questionList.size).isEqualTo(0) + } + + @Test + @ExperimentalCoroutinesApi + fun testController_returnsFailureForNonExistentTopic() = runBlockingTest(coroutineContext) { + val questionListLiveData = questionDataController.getQuestionsForTopic("NON_EXISTENT_TOPIC") + advanceUntilIdle() + questionListLiveData.observeForever(mockQuestionListObserver) + verify(mockQuestionListObserver, atLeastOnce()).onChanged(questionListResultCaptor.capture()) + assertThat(questionListResultCaptor.value.isFailure()).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(questionDataControllerTest: QuestionDataControllerTest) + } +} diff --git a/model/src/main/proto/question.proto b/model/src/main/proto/question.proto new file mode 100644 index 00000000000..230373f37aa --- /dev/null +++ b/model/src/main/proto/question.proto @@ -0,0 +1,21 @@ +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; +} + + From 6ee7aec072a63b768687bf313dd6cc8e0b79fcc4 Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Mon, 7 Oct 2019 20:20:42 -0700 Subject: [PATCH 02/25] Removed progress controller test --- .../QuestionProgressControllerTest.kt | 194 ------------------ 1 file changed, 194 deletions(-) delete mode 100644 domain/src/test/java/org/oppia/domain/question/QuestionProgressControllerTest.kt diff --git a/domain/src/test/java/org/oppia/domain/question/QuestionProgressControllerTest.kt b/domain/src/test/java/org/oppia/domain/question/QuestionProgressControllerTest.kt deleted file mode 100644 index 63c19d2cf47..00000000000 --- a/domain/src/test/java/org/oppia/domain/question/QuestionProgressControllerTest.kt +++ /dev/null @@ -1,194 +0,0 @@ -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.app.model.Question -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 [QuestionDataController]. */ -@RunWith(AndroidJUnit4::class) -@Config(manifest = Config.NONE) -class QuestionDataControllerTest { - @Rule - @JvmField - val mockitoRule: MockitoRule = MockitoJUnit.rule() - - @Rule - @JvmField - val executorRule = InstantTaskExecutorRule() - - @Inject - lateinit var questionDataController: QuestionDataController - - @Mock - lateinit var mockQuestionListObserver: Observer>> - - @Captor - lateinit var questionListResultCaptor: ArgumentCaptor>> - - @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() { - DaggerQuestionDataControllerTest_TestApplicationComponent.builder() - .setApplication(ApplicationProvider.getApplicationContext()) - .build() - .inject(this) - } - - @Test - @ExperimentalCoroutinesApi - fun testController_providesInitialLiveDataForTopicId0() = runBlockingTest(coroutineContext) { - val questionListLiveData = questionDataController.getQuestionsForTopic(TEST_TOPIC_ID_0) - advanceUntilIdle() - questionListLiveData.observeForever(mockQuestionListObserver) - - verify(mockQuestionListObserver, atLeastOnce()).onChanged(questionListResultCaptor.capture()) - assertThat(questionListResultCaptor.value.isSuccess()).isTrue() - assertThat(questionListResultCaptor.value.getOrThrow()).isNotNull() - val questionList = questionListResultCaptor.value.getOrThrow(); - assertThat(questionList.size).isEqualTo(0) - } - - @Test - @ExperimentalCoroutinesApi - fun testController_returnsFailureForNonExistentTopic() = runBlockingTest(coroutineContext) { - val questionListLiveData = questionDataController.getQuestionsForTopic("NON_EXISTENT_TOPIC") - advanceUntilIdle() - questionListLiveData.observeForever(mockQuestionListObserver) - verify(mockQuestionListObserver, atLeastOnce()).onChanged(questionListResultCaptor.capture()) - assertThat(questionListResultCaptor.value.isFailure()).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(questionDataControllerTest: QuestionDataControllerTest) - } -} From 848f172638b2376972854fa68816ae09c2803694 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Tue, 8 Oct 2019 19:27:37 -0700 Subject: [PATCH 03/25] Address the renames suggested in #217. --- ...r.kt => QuestionAssessmentProgressController.kt} | 4 ++-- ...aController.kt => QuestionTrainingController.kt} | 13 ++++++------- .../domain/question/QuestionDataControllerTest.kt | 8 ++++---- 3 files changed, 12 insertions(+), 13 deletions(-) rename domain/src/main/java/org/oppia/domain/question/{QuestionProgressController.kt => QuestionAssessmentProgressController.kt} (82%) rename domain/src/main/java/org/oppia/domain/question/{QuestionDataController.kt => QuestionTrainingController.kt} (85%) diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionProgressController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt similarity index 82% rename from domain/src/main/java/org/oppia/domain/question/QuestionProgressController.kt rename to domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt index 55e34661bd0..f7c08ad1c53 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionProgressController.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt @@ -7,7 +7,7 @@ import javax.inject.Singleton /** Controller for retrieving an exploration. */ @Singleton -class QuestionProgressController @Inject constructor( +class QuestionAssessmentProgressController @Inject constructor( ) { fun beginQuestionTrainingSession(questionsList: List) { } @@ -15,4 +15,4 @@ class QuestionProgressController @Inject constructor( fun finishQuestionTrainingSession() { } -} \ No newline at end of file +} diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionDataController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt similarity index 85% rename from domain/src/main/java/org/oppia/domain/question/QuestionDataController.kt rename to domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt index 31261ff1e32..8049480d35f 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionDataController.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt @@ -1,6 +1,5 @@ package org.oppia.domain.question -import android.content.Context import androidx.lifecycle.LiveData import androidx.lifecycle.MutableLiveData import org.oppia.app.model.Question @@ -15,8 +14,8 @@ private const val QUESTION_DATA_PROVIDER_ID = "QuestionDataProvider" /** Controller for retrieving an exploration. */ @Singleton -class QuestionDataController @Inject constructor( - private val questionProgressController: QuestionProgressController, +class QuestionTrainingController @Inject constructor( + private val questionAssessmentProgressController: QuestionAssessmentProgressController, private val dataProviders: DataProviders ) { @@ -32,8 +31,8 @@ class QuestionDataController @Inject constructor( /** * Begins a question training session given a list of questions. This method is not expected to fail. - * [QuestionProgressController] should be used to manage the play state, and monitor the load success/failure of - * the training session. + * [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. * @@ -42,7 +41,7 @@ class QuestionDataController @Inject constructor( */ fun startQuestionTrainingSession(questionsList: List): LiveData> { return try { - questionProgressController.beginQuestionTrainingSession(questionsList.shuffled()) + questionAssessmentProgressController.beginQuestionTrainingSession(questionsList.shuffled()) MutableLiveData(AsyncResult.success(null)) } catch (e: Exception) { MutableLiveData(AsyncResult.failed(e)) @@ -56,7 +55,7 @@ class QuestionDataController @Inject constructor( */ fun stopQuestionTrainingSession(): LiveData> { return try { - questionProgressController.finishQuestionTrainingSession() + questionAssessmentProgressController.finishQuestionTrainingSession() MutableLiveData(AsyncResult.success(null)) } catch (e: Exception) { MutableLiveData(AsyncResult.failed(e)) diff --git a/domain/src/test/java/org/oppia/domain/question/QuestionDataControllerTest.kt b/domain/src/test/java/org/oppia/domain/question/QuestionDataControllerTest.kt index 63c19d2cf47..4f02e3740fa 100644 --- a/domain/src/test/java/org/oppia/domain/question/QuestionDataControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/question/QuestionDataControllerTest.kt @@ -48,7 +48,7 @@ import kotlin.coroutines.EmptyCoroutineContext const val TEST_TOPIC_ID_0 = "test_topic_id_0" -/** Tests for [QuestionDataController]. */ +/** Tests for [QuestionTrainingController]. */ @RunWith(AndroidJUnit4::class) @Config(manifest = Config.NONE) class QuestionDataControllerTest { @@ -61,7 +61,7 @@ class QuestionDataControllerTest { val executorRule = InstantTaskExecutorRule() @Inject - lateinit var questionDataController: QuestionDataController + lateinit var questionTrainingController: QuestionTrainingController @Mock lateinit var mockQuestionListObserver: Observer>> @@ -107,7 +107,7 @@ class QuestionDataControllerTest { @Test @ExperimentalCoroutinesApi fun testController_providesInitialLiveDataForTopicId0() = runBlockingTest(coroutineContext) { - val questionListLiveData = questionDataController.getQuestionsForTopic(TEST_TOPIC_ID_0) + val questionListLiveData = questionTrainingController.getQuestionsForTopic(TEST_TOPIC_ID_0) advanceUntilIdle() questionListLiveData.observeForever(mockQuestionListObserver) @@ -121,7 +121,7 @@ class QuestionDataControllerTest { @Test @ExperimentalCoroutinesApi fun testController_returnsFailureForNonExistentTopic() = runBlockingTest(coroutineContext) { - val questionListLiveData = questionDataController.getQuestionsForTopic("NON_EXISTENT_TOPIC") + val questionListLiveData = questionTrainingController.getQuestionsForTopic("NON_EXISTENT_TOPIC") advanceUntilIdle() questionListLiveData.observeForever(mockQuestionListObserver) verify(mockQuestionListObserver, atLeastOnce()).onChanged(questionListResultCaptor.capture()) From 4147857fd5f268c4dadc2b60bac80aefb894e9e0 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Tue, 8 Oct 2019 20:40:17 -0700 Subject: [PATCH 04/25] Introduce thoroughly stubbed QuestionAssessmentProgressController + empty test suite. --- .../ExplorationProgressController.kt | 13 +- .../QuestionAssessmentProgressController.kt | 133 +++++++++++++++++- ...uestionAssessmentProgressControllerTest.kt | 111 +++++++++++++++ model/src/main/proto/question.proto | 57 +++++++- 4 files changed, 295 insertions(+), 19 deletions(-) create mode 100644 domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.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 index 8269b6b073f..bfc89639ef1 100644 --- a/domain/src/main/java/org/oppia/domain/exploration/ExplorationProgressController.kt +++ b/domain/src/main/java/org/oppia/domain/exploration/ExplorationProgressController.kt @@ -149,21 +149,13 @@ class ExplorationProgressController @Inject constructor( } } - /** - * 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 and [getCurrentState] returns a non-pending result or - * an exception will be thrown. - */ /** * 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 navigation was attempted at an invalid time in the state graph (e.g. if currently viewing 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. */ @@ -228,9 +220,6 @@ class ExplorationProgressController @Inject constructor( * 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 diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt index f7c08ad1c53..8991a7a1690 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt @@ -1,18 +1,139 @@ package org.oppia.domain.question +import androidx.lifecycle.LiveData +import androidx.lifecycle.MutableLiveData +import org.oppia.app.model.AnsweredQuestionOutcome +import org.oppia.app.model.EphemeralQuestion +import org.oppia.app.model.EphemeralState +import org.oppia.app.model.InteractionObject +import org.oppia.app.model.PendingState import org.oppia.app.model.Question +import org.oppia.app.model.SubtitledHtml +import org.oppia.util.data.AsyncResult import javax.inject.Inject import javax.inject.Singleton - -/** Controller for retrieving an exploration. */ +/** + * Controller that tracks and reports the learner's ephemeral/non-persisted progress through an practice training + * session. Note that this controller only supports one active training session at a time. + * + * The current training 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: List) { +class QuestionAssessmentProgressController @Inject constructor() { + private lateinit var inProgressQuestionList: List + private var playing: Boolean = false + + internal fun beginQuestionTrainingSession(questionsList: List) { + check(!playing) { "Cannot start a new training session until the previous one is completed" } + check(questionsList.isNotEmpty()) { "Cannot start a training session with zero questions." } + inProgressQuestionList = questionsList } - fun finishQuestionTrainingSession() { + internal fun finishQuestionTrainingSession() { + check(playing) { "Cannot stop a new training session which wasn't started" } + } + + /** + * Submits an answer to the current question 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 [getCurrentQuestion] + * 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 should result in the learner staying in the current question or moving to a new question in + * the training session. Note that once a correct answer is processed, the current state reported to + * [getCurrentQuestion] will change from a pending question to a completed question since the learner completed that + * question card. The learner can then proceed from the current completed question to the next pending question using + * [moveToNextQuestion]. + * + * This method cannot be called until a training session has started and [getCurrentQuestion] returns a non-pending + * result 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. + * + * No assumptions should be made about the completion order of the returned [LiveData] vs. the [LiveData] from + * [getCurrentQuestion]. Also note that the returned [LiveData] will only have a single value and not be reused after + * that point. + */ + fun submitAnswer(answer: InteractionObject): LiveData> { + val outcome = AnsweredQuestionOutcome.newBuilder() + .setFeedback(SubtitledHtml.newBuilder().setHtml("Response to answer: $answer")) + .setIsCorrectAnswer(true) + .build() + return MutableLiveData(AsyncResult.success(outcome)) + } + + /** + * Navigates to the previous question already answered. If the learner is currently on the first question, 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 question was successful, or a failure + * if question navigation was attempted at an invalid time (such as when viewing the first question). It's + * recommended that calling code only listen to this result for failures, and instead rely on [getCurrentQuestion] + * for observing a successful transition to another state. + */ + fun moveToPreviousQuestion(): LiveData> { + check(playing) { "Cannot move to the previous question unless an active training session is ongoing" } + return MutableLiveData(AsyncResult.success(null)) + } + + /** + * Navigates to the next question in the assessment. This method is only valid if the current [EphemeralQuestion] + * reported by [getCurrentQuestion] is a completed question. Calling code is responsible for ensuring this method is + * only called when it's possible to navigate forward. + * + * Note that if the current question is pending, the user needs to submit a correct answer via [submitAnswer] before + * forward navigation can occur. + * + * @return a one-time [LiveData] indicating whether the movement to the next question was successful, or a failure if + * question navigation was attempted at an invalid time (such as if the current question is pending or terminal). + * It's recommended that calling code only listen to this result for failures, and instead rely on + * [getCurrentQuestion] for observing a successful transition to another question. + */ + fun moveToNextQuestion(): LiveData> { + check(playing) { "Cannot move to the next question unless an active training session is ongoing" } + return MutableLiveData(AsyncResult.success(null)) + } + /** + * Returns a [LiveData] monitoring the current [EphemeralQuestion] the learner is currently viewing. If this state + * corresponds to a a terminal state, then the learner has completed the training session. Note that + * [moveToPreviousQuestion] and [moveToNextQuestion] will automatically update observers of this live data when the + * next question is navigated to. + * + * This [LiveData] may 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 question 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 UI reconstitution + * after configuration changes. + * + * The underlying question returned by this function can only be changed by calls to [moveToNextQuestion] and + * [moveToPreviousQuestion], or the question training controller if another question session begins. UI code can be + * confident only calls from the UI layer will trigger changes here to ensure atomicity between receiving and making + * question state changes. + * + * This method is safe to be called before a training session has started. If there is no ongoing session, it should + * return a pending state. + */ + fun getCurrentQuestion(): LiveData> { + val currentQuestion = inProgressQuestionList.first() + val ephemeralQuestion = EphemeralQuestion.newBuilder() + .setEphemeralState(EphemeralState.newBuilder() + .setState(currentQuestion.questionState) + .setPendingState(PendingState.getDefaultInstance())) + .setCurrentQuestionIndex(0) + .setTotalQuestionCount(inProgressQuestionList.size) + .setInitialTotalQuestionCount(inProgressQuestionList.size) + .build() + return MutableLiveData(AsyncResult.success(ephemeralQuestion)) } } diff --git a/domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.kt b/domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.kt new file mode 100644 index 00000000000..100942e3a8a --- /dev/null +++ b/domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.kt @@ -0,0 +1,111 @@ +package org.oppia.domain.question + +import android.app.Application +import android.content.Context +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 org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +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 + +/** Tests for [QuestionAssessmentProgressController]. */ +@RunWith(AndroidJUnit4::class) +@Config(manifest = Config.NONE) +class QuestionAssessmentProgressControllerTest { + @Inject + lateinit var questionTrainingController: QuestionTrainingController + + @Inject + lateinit var questionAssessmentProgressController: QuestionAssessmentProgressController + + @Inject + @field:TestDispatcher + lateinit var testDispatcher: CoroutineDispatcher + + private val coroutineContext by lazy { + EmptyCoroutineContext + testDispatcher + } + + @Before + fun setUp() { + setUpTestApplicationComponent() + } + + // TODO(#111): Add tests for the controller once there's a real controller to test. + @Test + fun testNothing_succeeds() { + assertThat(true).isTrue() + } + + private fun setUpTestApplicationComponent() { + DaggerQuestionAssessmentProgressControllerTest_TestApplicationComponent.builder() + .setApplication(ApplicationProvider.getApplicationContext()) + .build() + .inject(this) + } + + @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(#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(questionAssessmentProgressControllerTest: QuestionAssessmentProgressControllerTest) + } +} diff --git a/model/src/main/proto/question.proto b/model/src/main/proto/question.proto index 230373f37aa..e933665dcf6 100644 --- a/model/src/main/proto/question.proto +++ b/model/src/main/proto/question.proto @@ -3,19 +3,74 @@ syntax = "proto3"; package model; import "exploration.proto"; +import "subtitled_html.proto"; option java_package = "org.oppia.app.model"; option java_multiple_files = true; -// Structure for a single question. +// Represents a question that can be used to determine how well a learner understands specific skills. message Question { + // The ID of the question. string question_id = 1; + + // The [State] representing the Q&A structure of the question. State question_state = 2; + + // The language code to which this question is localized. string language_code = 3; + + // The version of the question. int32 version = 4; + + // The IDs of skills to which this question is associated. repeated string linked_skill_ids = 5; + + // Number of milliseconds since the Unix epoch corresponding to when this question was created. int64 created_on_timestamp_ms = 6; + + // Number of milliseconds since the Unix epoch corresponding to when this question was most recently updated. int64 updated_on_timestamp_ms = 7; } +// Corresponds to a question that has been answered in an assessment, or is in the process of being answered, and will +// disappear once the user finishes the assessment or navigates away from the training session. This has strong +// correlation with the EphemeralState structure used when playing explorations, and contains an instance of that due to +// the internal structure of Question also relying on a State. This also contains additional information to help report +// the user's progress through the assessment. +message EphemeralQuestion { + // Corresponds to the current ephemeral state the question is in (including indicating whether the question has been + // completed, or if the learner has reached the end of the assessment). Note that a valid question can also be + // terminal (such as if it's the last question in the assessment). This differs from explorations which have a + // dedicated terminal state. + EphemeralState ephemeral_state = 1; + + // Corresponds the index of the current question in the assessment, starting at 0. This index is guaranteed to be + // unique for a specific question being assessed, even as different EphemeralQuestions are being dispatched to + // represent different states the question is going through. + int32 current_question_index = 2; + + // Corresponds to the number of questions in the assessment. This value will change across EphemeralQuestion instances + // for a single assessment if the controller decides to add/remove questions mid-assessment. + int32 total_question_count = 3; + + // Corresponds to the number of questions in the assessment when it started. This value will never change across + // EphemeralQuestion instances for the same assessment. It can be compared with total_question_count to detect if the + // learner required extra questions during the assessment, or completed it early. + int32 initial_total_question_count = 4; + + // Corresponds to skill IDs that the learner should review. This is expected only to be filled at the end of the + // assessment (for the terminal question), but this should not be used as an indicator of whether the assessment + // ended. If this has values outside the terminal state, they should be ignored. There's no guarantee any skill IDs + // will be suggested to review at the end of the assessment. + repeated string skill_ids_to_review = 5; +} + +// The outcome of submitting an answer to a question during a training session. +message AnsweredQuestionOutcome { + // Oppia's feedback to the learner's most recent answer. + SubtitledHtml feedback = 1; + + // Whether the answer the learner submitted is the correct answer. + bool is_correct_answer = 2; +} From 4596e86f5e4b564f1f76f60aba8a10b681cf99ea Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Wed, 9 Oct 2019 13:07:37 -0700 Subject: [PATCH 05/25] Review changes --- .../QuestionAssessmentProgressController.kt | 25 +++++ .../domain/question/QuestionDataController.kt | 83 ---------------- .../question/QuestionProgressController.kt | 18 ---- .../question/QuestionTrainingController.kt | 96 +++++++++++++++++++ ...t.kt => QuestionTrainingControllerTest.kt} | 33 +++---- model/src/main/proto/question.proto | 2 - 6 files changed, 138 insertions(+), 119 deletions(-) create mode 100644 domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt delete mode 100644 domain/src/main/java/org/oppia/domain/question/QuestionDataController.kt delete mode 100644 domain/src/main/java/org/oppia/domain/question/QuestionProgressController.kt create mode 100644 domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt rename domain/src/test/java/org/oppia/domain/question/{QuestionDataControllerTest.kt => QuestionTrainingControllerTest.kt} (83%) diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt new file mode 100644 index 00000000000..69f71cb010d --- /dev/null +++ b/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt @@ -0,0 +1,25 @@ +package org.oppia.domain.question + +import org.oppia.app.model.Question +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: List) { + } + + fun finishQuestionTrainingSession() { + + } +} diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionDataController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionDataController.kt deleted file mode 100644 index 31261ff1e32..00000000000 --- a/domain/src/main/java/org/oppia/domain/question/QuestionDataController.kt +++ /dev/null @@ -1,83 +0,0 @@ -package org.oppia.domain.question - -import android.content.Context -import androidx.lifecycle.LiveData -import androidx.lifecycle.MutableLiveData -import org.oppia.app.model.Question -import org.oppia.domain.topic.TEST_TOPIC_ID_0 -import org.oppia.domain.topic.TEST_TOPIC_ID_1 -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" - -/** Controller for retrieving an exploration. */ -@Singleton -class QuestionDataController @Inject constructor( - private val questionProgressController: QuestionProgressController, - private val dataProviders: DataProviders -) { - - /** - * Returns a list of [Question] objects given a topic ID. - */ - fun getQuestionsForTopic(topicId: String): LiveData>> { - val dataProvider = dataProviders.createInMemoryDataProviderAsync(QUESTION_DATA_PROVIDER_ID) { - retrieveQuestionsForTopic(topicId) - } - return dataProviders.convertToLiveData(dataProvider) - } - - /** - * Begins a question training session given a list of questions. This method is not expected to fail. - * [QuestionProgressController] 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(questionsList: List): LiveData> { - return try { - questionProgressController.beginQuestionTrainingSession(questionsList.shuffled()) - MutableLiveData(AsyncResult.success(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> { - return try { - questionProgressController.finishQuestionTrainingSession() - MutableLiveData(AsyncResult.success(null)) - } catch (e: Exception) { - MutableLiveData(AsyncResult.failed(e)) - } - } - - @Suppress("RedundantSuspendModifier") // DataProviders expects this function to be a suspend function. - private suspend fun retrieveQuestionsForTopic(topicId: String): AsyncResult> { - return try { - AsyncResult.success(loadQuestions(topicId)) - } catch (e: Exception) { - AsyncResult.failed(e) - } - } - - // Loads and returns the questions given a topic id. - private fun loadQuestions(topicId: String): List { - when(topicId) { - TEST_TOPIC_ID_0 -> return emptyList() - TEST_TOPIC_ID_1 -> return emptyList() - else -> throw IllegalStateException("Invalid topic ID: $topicId") - } - } -} \ No newline at end of file diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionProgressController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionProgressController.kt deleted file mode 100644 index 55e34661bd0..00000000000 --- a/domain/src/main/java/org/oppia/domain/question/QuestionProgressController.kt +++ /dev/null @@ -1,18 +0,0 @@ -package org.oppia.domain.question - -import org.oppia.app.model.Question -import javax.inject.Inject -import javax.inject.Singleton - - -/** Controller for retrieving an exploration. */ -@Singleton -class QuestionProgressController @Inject constructor( -) { - fun beginQuestionTrainingSession(questionsList: List) { - } - - fun finishQuestionTrainingSession() { - - } -} \ No newline at end of file diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt new file mode 100644 index 00000000000..4c0bcdc2c06 --- /dev/null +++ b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt @@ -0,0 +1,96 @@ +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. + */ + suspend fun startQuestionTrainingSession(skillIdsList: List): LiveData> { + return try { + val questionsList = retrieveQuestionsForSkillIds(skillIdsList).getOrThrow() + questionAssessmentProgressController.beginQuestionTrainingSession(questionsList.shuffled()) + MutableLiveData(AsyncResult.success(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> { + return try { + questionAssessmentProgressController.finishQuestionTrainingSession() + MutableLiveData(AsyncResult.success(null)) + } catch (e: Exception) { + MutableLiveData(AsyncResult.failed(e)) + } + } + + + @Suppress("RedundantSuspendModifier") // DataProviders expects this function to be a suspend function. + private suspend fun retrieveQuestionsForSkillIds(skillIdsList: List): AsyncResult> { + return try { + AsyncResult.success(loadQuestions(skillIdsList)) + } catch (e: Exception) { + AsyncResult.failed(e) + } + } + + // Loads and returns the questions given a list of skill ids. + private fun loadQuestions(skillIdsList: List): List { + val questionsList = mutableListOf() + 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 + } +} diff --git a/domain/src/test/java/org/oppia/domain/question/QuestionDataControllerTest.kt b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt similarity index 83% rename from domain/src/test/java/org/oppia/domain/question/QuestionDataControllerTest.kt rename to domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt index 63c19d2cf47..18eb34ecf59 100644 --- a/domain/src/test/java/org/oppia/domain/question/QuestionDataControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt @@ -32,7 +32,8 @@ 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.Question +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 @@ -48,10 +49,10 @@ import kotlin.coroutines.EmptyCoroutineContext const val TEST_TOPIC_ID_0 = "test_topic_id_0" -/** Tests for [QuestionDataController]. */ +/** Tests for [QuestionTrainingController]. */ @RunWith(AndroidJUnit4::class) @Config(manifest = Config.NONE) -class QuestionDataControllerTest { +class QuestionTrainingControllerTest { @Rule @JvmField val mockitoRule: MockitoRule = MockitoJUnit.rule() @@ -61,13 +62,13 @@ class QuestionDataControllerTest { val executorRule = InstantTaskExecutorRule() @Inject - lateinit var questionDataController: QuestionDataController + lateinit var questionTrainingController: QuestionTrainingController @Mock - lateinit var mockQuestionListObserver: Observer>> + lateinit var mockQuestionListObserver: Observer> @Captor - lateinit var questionListResultCaptor: ArgumentCaptor>> + lateinit var questionListResultCaptor: ArgumentCaptor> @Inject @field:TestDispatcher @@ -98,7 +99,7 @@ class QuestionDataControllerTest { } private fun setUpTestApplicationComponent() { - DaggerQuestionDataControllerTest_TestApplicationComponent.builder() + DaggerQuestionTrainingControllerTest_TestApplicationComponent.builder() .setApplication(ApplicationProvider.getApplicationContext()) .build() .inject(this) @@ -106,25 +107,25 @@ class QuestionDataControllerTest { @Test @ExperimentalCoroutinesApi - fun testController_providesInitialLiveDataForTopicId0() = runBlockingTest(coroutineContext) { - val questionListLiveData = questionDataController.getQuestionsForTopic(TEST_TOPIC_ID_0) + 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() - assertThat(questionListResultCaptor.value.getOrThrow()).isNotNull() - val questionList = questionListResultCaptor.value.getOrThrow(); - assertThat(questionList.size).isEqualTo(0) } @Test @ExperimentalCoroutinesApi - fun testController_returnsFailureForNonExistentTopic() = runBlockingTest(coroutineContext) { - val questionListLiveData = questionDataController.getQuestionsForTopic("NON_EXISTENT_TOPIC") + fun testController_failsToStartQuestionSessionForNonExistentSkillId() = runBlockingTest(coroutineContext) { + val questionListLiveData = questionTrainingController.startQuestionTrainingSession( + listOf(TEST_SKILL_ID_0, TEST_SKILL_ID_1, "NON_EXISTENT_SKILL_ID")) advanceUntilIdle() questionListLiveData.observeForever(mockQuestionListObserver) verify(mockQuestionListObserver, atLeastOnce()).onChanged(questionListResultCaptor.capture()) + assertThat(questionListResultCaptor.value.isFailure()).isTrue() } @@ -189,6 +190,6 @@ class QuestionDataControllerTest { fun build(): TestApplicationComponent } - fun inject(questionDataControllerTest: QuestionDataControllerTest) + fun inject(questionTrainingControllerTest: QuestionTrainingControllerTest) } } diff --git a/model/src/main/proto/question.proto b/model/src/main/proto/question.proto index 230373f37aa..f4aa01a3f2d 100644 --- a/model/src/main/proto/question.proto +++ b/model/src/main/proto/question.proto @@ -17,5 +17,3 @@ message Question { int64 created_on_timestamp_ms = 6; int64 updated_on_timestamp_ms = 7; } - - From f5c23c8d129a2011cdd310087981826533b0883a Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Thu, 10 Oct 2019 14:56:56 -0700 Subject: [PATCH 06/25] Review changes --- .../QuestionAssessmentProgressController.kt | 4 ++- .../question/QuestionTrainingController.kt | 27 +++++++++++-------- .../QuestionTrainingControllerTest.kt | 13 --------- 3 files changed, 19 insertions(+), 25 deletions(-) diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt index 69f71cb010d..509913b03ac 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt @@ -1,6 +1,8 @@ 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 @@ -16,7 +18,7 @@ import javax.inject.Singleton @Singleton class QuestionAssessmentProgressController @Inject constructor( ) { - fun beginQuestionTrainingSession(questionsList: List) { + fun beginQuestionTrainingSession(questionsList: LiveData>>) { } fun finishQuestionTrainingSession() { diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt index 4c0bcdc2c06..b65ddb693d1 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt @@ -16,7 +16,6 @@ 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( @@ -25,21 +24,20 @@ class QuestionTrainingController @Inject constructor( ) { /** * 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. + * + * 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. */ - suspend fun startQuestionTrainingSession(skillIdsList: List): LiveData> { + fun startQuestionTrainingSession(skillIdsList: List): LiveData> { return try { - val questionsList = retrieveQuestionsForSkillIds(skillIdsList).getOrThrow() - questionAssessmentProgressController.beginQuestionTrainingSession(questionsList.shuffled()) + val questionsList = retrieveQuestionsForSkillIds(skillIdsList) + questionAssessmentProgressController.beginQuestionTrainingSession(questionsList) MutableLiveData(AsyncResult.success(null)) - } catch (e: Exception) { MutableLiveData(AsyncResult.failed(e)) } @@ -59,9 +57,16 @@ class QuestionTrainingController @Inject constructor( } } + private fun retrieveQuestionsForSkillIds(skillIdsList: List): LiveData>> { + 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 retrieveQuestionsForSkillIds(skillIdsList: List): AsyncResult> { + private suspend fun loadQuestionsForSkillIds(skillIdsList: List): AsyncResult> { return try { AsyncResult.success(loadQuestions(skillIdsList)) } catch (e: Exception) { @@ -69,8 +74,8 @@ class QuestionTrainingController @Inject constructor( } } - // Loads and returns the questions given a list of skill ids. - private fun loadQuestions(skillIdsList: List): List { + @Suppress("RedundantSuspendModifier") // Force callers to call this on a background thread. + private suspend fun loadQuestions(skillIdsList: List): List { val questionsList = mutableListOf() for (skillId in skillIdsList) { when (skillId) { diff --git a/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt index 18eb34ecf59..3eb95a2fbd0 100644 --- a/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt @@ -113,22 +113,9 @@ class QuestionTrainingControllerTest { advanceUntilIdle() questionListLiveData.observeForever(mockQuestionListObserver) verify(mockQuestionListObserver, atLeastOnce()).onChanged(questionListResultCaptor.capture()) - assertThat(questionListResultCaptor.value.isSuccess()).isTrue() } - @Test - @ExperimentalCoroutinesApi - fun testController_failsToStartQuestionSessionForNonExistentSkillId() = runBlockingTest(coroutineContext) { - val questionListLiveData = questionTrainingController.startQuestionTrainingSession( - listOf(TEST_SKILL_ID_0, TEST_SKILL_ID_1, "NON_EXISTENT_SKILL_ID")) - advanceUntilIdle() - questionListLiveData.observeForever(mockQuestionListObserver) - verify(mockQuestionListObserver, atLeastOnce()).onChanged(questionListResultCaptor.capture()) - - assertThat(questionListResultCaptor.value.isFailure()).isTrue() - } - @Qualifier annotation class TestDispatcher From 53d367d11ad3535281ff597cc0af706c4bf9cee3 Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Fri, 11 Oct 2019 13:52:50 -0700 Subject: [PATCH 07/25] Pass data providers to the assessment controller, and set caps in the training controller --- .../QuestionAssessmentProgressController.kt | 3 +- .../question/QuestionTrainingController.kt | 73 ++++------- .../org/oppia/domain/topic/TopicController.kt | 53 +++++++- .../oppia/domain/topic/TopicControllerTest.kt | 117 ++++++++++++++++++ 4 files changed, 196 insertions(+), 50 deletions(-) diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt index 509913b03ac..667a10ae59c 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt @@ -3,6 +3,7 @@ package org.oppia.domain.question import androidx.lifecycle.LiveData import org.oppia.app.model.Question import org.oppia.util.data.AsyncResult +import org.oppia.util.data.DataProvider import javax.inject.Inject import javax.inject.Singleton @@ -18,7 +19,7 @@ import javax.inject.Singleton @Singleton class QuestionAssessmentProgressController @Inject constructor( ) { - fun beginQuestionTrainingSession(questionsList: LiveData>>) { + fun beginQuestionTrainingSession(questionsList: DataProvider>) { } fun finishQuestionTrainingSession() { diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt index b65ddb693d1..280ee82a735 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt @@ -6,20 +6,21 @@ 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.domain.topic.TopicController import org.oppia.util.data.AsyncResult +import org.oppia.util.data.DataProvider 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" +const val TOTAL_QUESTIONS_PER_TOPIC = 10 +const val TRAINING_QUESTIONS_PROVIDER = "TrainingQuestionsProvider" /** Controller for retrieving a set of questions. */ @Singleton class QuestionTrainingController @Inject constructor( private val questionAssessmentProgressController: QuestionAssessmentProgressController, + private val topicController: TopicController, private val dataProviders: DataProviders ) { /** @@ -35,14 +36,32 @@ class QuestionTrainingController @Inject constructor( */ fun startQuestionTrainingSession(skillIdsList: List): LiveData> { return try { - val questionsList = retrieveQuestionsForSkillIds(skillIdsList) - questionAssessmentProgressController.beginQuestionTrainingSession(questionsList) + questionAssessmentProgressController.beginQuestionTrainingSession( + retrieveQuestionsForSkillIds(skillIdsList)) MutableLiveData(AsyncResult.success(null)) } catch (e: Exception) { MutableLiveData(AsyncResult.failed(e)) } } + private fun retrieveQuestionsForSkillIds(skillIdsList: List): DataProvider> { + val questionsDataProvider = topicController.retrieveQuestionsForSkillIds(skillIdsList) + return dataProviders.transform(TRAINING_QUESTIONS_PROVIDER, questionsDataProvider) { + getFilteredQuestionsForTraining(skillIdsList, it, TOTAL_QUESTIONS_PER_TOPIC/skillIdsList.size) + } + } + + private fun getFilteredQuestionsForTraining( + skillIdsList: List, questionsList: List, numQuestionsPerSkill: Int): List{ + val trainingQuestions = mutableListOf() + for (skillId in skillIdsList) { + trainingQuestions.addAll(questionsList.filter { + it.linkedSkillIdsList.contains(skillId) + }.take(numQuestionsPerSkill + 1)) + } + return trainingQuestions.take(TOTAL_QUESTIONS_PER_TOPIC) + } + /** * Finishes the most recent training session started by [startQuestionTrainingSession]. * This method should only be called if there is a training session is being played, @@ -56,46 +75,4 @@ class QuestionTrainingController @Inject constructor( MutableLiveData(AsyncResult.failed(e)) } } - - private fun retrieveQuestionsForSkillIds(skillIdsList: List): LiveData>> { - 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): AsyncResult> { - 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): List { - val questionsList = mutableListOf() - 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 - } } diff --git a/domain/src/main/java/org/oppia/domain/topic/TopicController.kt b/domain/src/main/java/org/oppia/domain/topic/TopicController.kt index f3419feba35..e0751509e74 100644 --- a/domain/src/main/java/org/oppia/domain/topic/TopicController.kt +++ b/domain/src/main/java/org/oppia/domain/topic/TopicController.kt @@ -10,6 +10,7 @@ import org.oppia.app.model.ChapterSummary import org.oppia.app.model.ConceptCard import org.oppia.app.model.LessonThumbnail import org.oppia.app.model.LessonThumbnailGraphic +import org.oppia.app.model.Question import org.oppia.app.model.SkillSummary import org.oppia.app.model.StorySummary import org.oppia.app.model.SubtitledHtml @@ -19,16 +20,25 @@ import org.oppia.app.model.TranslationMapping import org.oppia.app.model.Voiceover import org.oppia.app.model.VoiceoverMapping import org.oppia.util.data.AsyncResult +import org.oppia.util.data.DataProvider +import org.oppia.util.data.DataProviders const val TEST_SKILL_ID_0 = "test_skill_id_0" const val TEST_SKILL_ID_1 = "test_skill_id_1" const val TEST_SKILL_ID_2 = "test_skill_id_2" const val TEST_SKILL_CONTENT_ID_0 = "test_skill_content_id_0" const val TEST_SKILL_CONTENT_ID_1 = "test_skill_content_id_1" +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" + +private const val QUESTION_DATA_PROVIDER_ID = "QuestionDataProvider" /** Controller for retrieving all aspects of a topic. */ @Singleton -class TopicController @Inject constructor() { +class TopicController @Inject constructor( + private val dataProviders: DataProviders +) { /** Returns the [Topic] corresponding to the specified topic ID, or a failed result if no such topic exists. */ fun getTopic(topicId: String): LiveData> { return MutableLiveData( @@ -66,6 +76,47 @@ class TopicController @Inject constructor() { ) } + fun retrieveQuestionsForSkillIds(skillIdsList: List): DataProvider> { + return dataProviders.createInMemoryDataProviderAsync(QUESTION_DATA_PROVIDER_ID) { + loadQuestionsForSkillIds(skillIdsList) + } + } + + // 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): AsyncResult> { + 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): List { + val questionsList = mutableListOf() + 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 + } + private fun createTestTopic0(): Topic { return Topic.newBuilder() .setTopicId(TEST_TOPIC_ID_0) diff --git a/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt b/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt index 5a14ea6b38f..aab80a2cc5e 100644 --- a/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt @@ -2,6 +2,8 @@ package org.oppia.domain.topic 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 @@ -9,18 +11,46 @@ 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.verify +import org.mockito.junit.MockitoJUnit +import org.mockito.junit.MockitoRule import org.oppia.app.model.ChapterPlayState import org.oppia.app.model.ChapterSummary import org.oppia.app.model.LessonThumbnailGraphic +import org.oppia.app.model.Question import org.oppia.app.model.SkillSummary import org.oppia.app.model.StorySummary import org.oppia.app.model.Topic +import org.oppia.util.data.AsyncResult +import org.oppia.util.data.DataProviders +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 /** Tests for [TopicController]. */ @RunWith(AndroidJUnit4::class) @@ -29,11 +59,49 @@ class TopicControllerTest { @Inject lateinit var topicController: TopicController + @Rule + @JvmField + val mockitoRule: MockitoRule = MockitoJUnit.rule() + + @Rule + @JvmField + val executorRule = InstantTaskExecutorRule() + + @Mock + lateinit var mockQuestionListObserver: Observer>> + + @Captor + lateinit var questionListResultCaptor: ArgumentCaptor>> + + @Inject + lateinit var dataProviders: DataProviders + + @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 fun setUp() { + Dispatchers.setMain(testThread) setUpTestApplicationComponent() } + @After + @ExperimentalCoroutinesApi + @ObsoleteCoroutinesApi + fun tearDown() { + Dispatchers.resetMain() + testThread.close() + } + @Test fun testRetrieveTopic_validTopic_isSuccessful() { val topicLiveData = topicController.getTopic(TEST_TOPIC_ID_0) @@ -414,6 +482,30 @@ class TopicControllerTest { assertThat(conceptCardLiveData.value!!.isFailure()).isTrue() } + @Test + fun testRetrieveQuestionsForSkillIds_returnsAllQuestions() = runBlockingTest(coroutineContext) { + val questionsListProvider = topicController.retrieveQuestionsForSkillIds( + listOf(TEST_SKILL_ID_0, TEST_SKILL_ID_1)) + dataProviders.convertToLiveData(questionsListProvider).observeForever(mockQuestionListObserver) + + verify(mockQuestionListObserver).onChanged(questionListResultCaptor.capture()) + assertThat(questionListResultCaptor.value.isSuccess()).isTrue() + val questionsList = questionListResultCaptor.value.getOrThrow() + assertThat(questionsList.size).isEqualTo(2) + assertThat(questionsList[0].questionId).isEqualTo(TEST_QUESTION_ID_0) + assertThat(questionsList[1].questionId).isEqualTo(TEST_QUESTION_ID_1) + } + + @Test + fun testRetrieveQuestionsForInvalidSkillIds_returnsFailure() = runBlockingTest(coroutineContext) { + val questionsListProvider = topicController.retrieveQuestionsForSkillIds( + listOf(TEST_SKILL_ID_0, TEST_SKILL_ID_1, "NON_EXISTENT_SKILL_ID")) + dataProviders.convertToLiveData(questionsListProvider).observeForever(mockQuestionListObserver) + + verify(mockQuestionListObserver).onChanged(questionListResultCaptor.capture()) + assertThat(questionListResultCaptor.value.isFailure()).isTrue() + } + private fun setUpTestApplicationComponent() { DaggerTopicControllerTest_TestApplicationComponent.builder() .setApplication(ApplicationProvider.getApplicationContext()) @@ -433,6 +525,9 @@ class TopicControllerTest { return story.chapterList.map(ChapterSummary::getExplorationId) } + @Qualifier + annotation class TestDispatcher + // TODO(#89): Move this to a common test application component. @Module class TestModule { @@ -441,6 +536,28 @@ class TopicControllerTest { 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(#89): Move this to a common test application component. From a8adf93452f6056300af17305423c35ff900cb7f Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Fri, 11 Oct 2019 14:34:15 -0700 Subject: [PATCH 08/25] Removed unnecessary imports/variables --- .../question/QuestionAssessmentProgressController.kt | 3 --- .../domain/question/QuestionTrainingControllerTest.kt | 1 - .../java/org/oppia/domain/topic/TopicControllerTest.kt | 8 ++------ 3 files changed, 2 insertions(+), 10 deletions(-) diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt index f0dc0f93056..1c91397dd0d 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt @@ -1,8 +1,6 @@ package org.oppia.domain.question -import androidx.lifecycle.LiveData import org.oppia.app.model.Question -import org.oppia.util.data.AsyncResult import org.oppia.util.data.DataProvider import javax.inject.Inject import javax.inject.Singleton @@ -20,7 +18,6 @@ import javax.inject.Singleton class QuestionAssessmentProgressController @Inject constructor( ) { fun beginQuestionTrainingSession(questionsList: DataProvider>) { - } fun finishQuestionTrainingSession() { diff --git a/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt index 3eb95a2fbd0..abc670c6080 100644 --- a/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt @@ -47,7 +47,6 @@ 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) diff --git a/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt b/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt index aab80a2cc5e..1d8642286cf 100644 --- a/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt @@ -40,10 +40,6 @@ import org.oppia.app.model.StorySummary import org.oppia.app.model.Topic import org.oppia.util.data.AsyncResult import org.oppia.util.data.DataProviders -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 @@ -487,8 +483,8 @@ class TopicControllerTest { val questionsListProvider = topicController.retrieveQuestionsForSkillIds( listOf(TEST_SKILL_ID_0, TEST_SKILL_ID_1)) dataProviders.convertToLiveData(questionsListProvider).observeForever(mockQuestionListObserver) - verify(mockQuestionListObserver).onChanged(questionListResultCaptor.capture()) + assertThat(questionListResultCaptor.value.isSuccess()).isTrue() val questionsList = questionListResultCaptor.value.getOrThrow() assertThat(questionsList.size).isEqualTo(2) @@ -501,8 +497,8 @@ class TopicControllerTest { val questionsListProvider = topicController.retrieveQuestionsForSkillIds( listOf(TEST_SKILL_ID_0, TEST_SKILL_ID_1, "NON_EXISTENT_SKILL_ID")) dataProviders.convertToLiveData(questionsListProvider).observeForever(mockQuestionListObserver) - verify(mockQuestionListObserver).onChanged(questionListResultCaptor.capture()) + assertThat(questionListResultCaptor.value.isFailure()).isTrue() } From a7a10f364215cf2b90b1a6fd6afc9c7999f8259f Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Fri, 11 Oct 2019 16:06:11 -0700 Subject: [PATCH 09/25] Add fake question data for stubbed interfaces --- domain/src/main/assets/sample_questions.json | 263 ++++++++++++++++++ .../exploration/ExplorationRetriever.kt | 4 +- .../org/oppia/domain/topic/TopicController.kt | 12 +- 3 files changed, 276 insertions(+), 3 deletions(-) create mode 100644 domain/src/main/assets/sample_questions.json diff --git a/domain/src/main/assets/sample_questions.json b/domain/src/main/assets/sample_questions.json new file mode 100644 index 00000000000..5024906fb64 --- /dev/null +++ b/domain/src/main/assets/sample_questions.json @@ -0,0 +1,263 @@ +{ "questions": +[{ + "name":"question", + "classifier_model_id":null, + "content":{ + "html":"

What fraction does 'quarter' represent? 

", + "content_id":"content" + }, + "interaction":{ + "confirmed_unclassified_answers": [], + "answer_groups":[ + { + "rule_specs":[ + { + "rule_type":"Contains", + "inputs":{ + "x":"1/4" + } + } + ], + "outcome":{ + "dest":null, + "feedback":{ + "html":"

That's correct!

", + "content_id":"feedback_1" + }, + "labelled_as_correct":true, + "param_changes":[ + + ], + "refresher_exploration_id":null, + "missing_prerequisite_skill_id":null + }, + "trainingData":[ + + ], + "taggedSkillMisconceptionId":null + } + ], + "confirmedUnclassifiedAnswers":[ + + ], + "customizationArgs":{ + "placeholder":{ + "value":"Write your fraction as x/y" + }, + "rows":{ + "value":1 + } + }, + "defaultOutcome":{ + "dest":null, + "feedback":{ + "html":"", + "content_id":"default_outcome" + }, + "labelled_as_correct":false, + "param_changes":[ + + ], + "refresher_exploration_id":null, + "missing_prerequisite_skill_id":null + }, + "hints":[ + { + "hintContent":{ + "html":"

Hint text will appear here

", + "content_id":"hint_1" + } + } + ], + "id":"TextInput", + "solution":{ + "answer_is_exclusive":false, + "correct_answer":"1/4", + "explanation":{ + "html":"

A quarter is 1/4th of something

", + "content_id":"solution" + } + } + }, + "param_changes":[ + + ], + "solicit_answer_details":false, + "writtenTranslations":{ + "translationsMapping":{ + "content":{ + + }, + "hint_1":{ + + }, + "feedback_1":{ + + }, + "default_outcome":{ + + }, + "solution":{ + + } + }, + "_writtenTranslationObjectFactory":{ + + } + } +}, +{ + "name":"question", + "classifier_model_id":null, + "content":{ + "html":"

If we talk about wanting  of a cake, what does the 7 represent?

", + "content_id":"content" + }, + "interaction":{ + "confirmed_unclassified_answers": [], + "answer_groups":[ + { + "rule_specs":[ + { + "rule_type":"Equals", + "inputs":{ + "x":1 + } + } + ], + "outcome":{ + "dest":null, + "feedback":{ + "html":"

That's correct!

", + "content_id":"feedback_1" + }, + "labelled_as_correct":true, + "param_changes":[ + + ], + "refresher_exploration_id":null, + "missing_prerequisite_skill_id":null + }, + "trainingData":[ + + ], + "taggedSkillMisconceptionId":null + } + ], + "confirmedUnclassifiedAnswers":[ + + ], + "customizationArgs":{ + "choices":{ + "value":[ + "

The number of pieces of cake I want.

", + "

The number of pieces the whole cake is cut into.

", + "

None of the above.

" + ] + } + }, + "defaultOutcome":{ + "dest":null, + "feedback":{ + "html":"", + "content_id":"default_outcome" + }, + "labelled_as_correct":false, + "param_changes":[ + + ], + "refresher_exploration_id":null, + "missing_prerequisite_skill_id":null + }, + "hints":[ + { + "hintContent":{ + "html":"

Hint text will appear here

", + "content_id":"hint_1" + } + } + ], + "id":"MultipleChoiceInput", + "solution":null + }, + "param_changes":[ + + ], + "solicit_answer_details":false +}, + { + "name":"question", + "classifier_model_id":null, + "content":{ + "html":"

What is the numerator of  equal to?

", + "content_id":"content" + }, + "interaction":{ + "confirmed_unclassified_answers": [], + "answer_groups":[ + { + "rule_specs":[ + { + "rule_type":"IsInclusivelyBetween", + "inputs":{ + "a":3, + "b":3 + } + } + ], + "outcome":{ + "dest":null, + "feedback": "

That's correct!

", + "labelled_as_correct":true, + "param_changes":[], + "refresher_exploration_id":null, + "missing_prerequisite_skill_id":null + }, + "trainingData":[ + + ], + "taggedSkillMisconceptionId":null + } + ], + "confirmedUnclassifiedAnswers":[ + + ], + "customizationArgs":{ + + }, + "defaultOutcome":{ + "dest":null, + "feedback":{ + "html":"", + "content_id":"default_outcome" + }, + "labelled_as_correct":false, + "param_changes":[ + + ], + "refresher_exploration_id":null, + "missing_prerequisite_skill_id":null + }, + "hints":[ + { + "hintContent":{ + "html":"

Hint text will appear here

", + "content_id":"hint_1" + } + } + ], + "id":"NumericInput", + "solution":{ + "answer_is_exclusive":false, + "correct_answer":3, + "explanation":{ + "html":"

The solution comes out to be  and the numerator of that is 3.

", + "content_id":"solution" + } + } + }, + "param_changes":[ + ], + "solicit_answer_details":false + }] +} 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 2962eaa8276..bc55c66c3d3 100644 --- a/domain/src/main/java/org/oppia/domain/exploration/ExplorationRetriever.kt +++ b/domain/src/main/java/org/oppia/domain/exploration/ExplorationRetriever.kt @@ -56,7 +56,7 @@ class ExplorationRetriever @Inject constructor(private val context: Context) { // Loads the JSON string from an asset and converts it to a JSONObject @Throws(IOException::class) - private fun loadJsonFromAsset(assetName: String): JSONObject? { + fun loadJsonFromAsset(assetName: String): JSONObject? { val assetManager = context.assets val jsonContents = assetManager.open(assetName).bufferedReader().use { it.readText() } return JSONObject(jsonContents) @@ -75,7 +75,7 @@ class ExplorationRetriever @Inject constructor(private val context: Context) { } // Creates a single state object from JSON - private fun createStateFromJson(stateName: String, stateJson: JSONObject?): State { + fun createStateFromJson(stateName: String, stateJson: JSONObject?): State { return State.newBuilder() .setName(stateName) .setContent( diff --git a/domain/src/main/java/org/oppia/domain/topic/TopicController.kt b/domain/src/main/java/org/oppia/domain/topic/TopicController.kt index e0751509e74..43184f83c75 100644 --- a/domain/src/main/java/org/oppia/domain/topic/TopicController.kt +++ b/domain/src/main/java/org/oppia/domain/topic/TopicController.kt @@ -19,6 +19,7 @@ import org.oppia.app.model.Translation import org.oppia.app.model.TranslationMapping import org.oppia.app.model.Voiceover import org.oppia.app.model.VoiceoverMapping +import org.oppia.domain.exploration.ExplorationRetriever import org.oppia.util.data.AsyncResult import org.oppia.util.data.DataProvider import org.oppia.util.data.DataProviders @@ -37,7 +38,8 @@ private const val QUESTION_DATA_PROVIDER_ID = "QuestionDataProvider" /** Controller for retrieving all aspects of a topic. */ @Singleton class TopicController @Inject constructor( - private val dataProviders: DataProviders + private val dataProviders: DataProviders, + private val explorationRetriever: ExplorationRetriever ) { /** Returns the [Topic] corresponding to the specified topic ID, or a failed result if no such topic exists. */ fun getTopic(topicId: String): LiveData> { @@ -95,19 +97,27 @@ class TopicController @Inject constructor( @Suppress("RedundantSuspendModifier") // Force callers to call this on a background thread. private suspend fun loadQuestions(skillIdsList: List): List { val questionsList = mutableListOf() + val questionsJSON = explorationRetriever.loadJsonFromAsset( + "sample_questions.json")?.getJSONArray("questions") for (skillId in skillIdsList) { when (skillId) { TEST_SKILL_ID_0 -> questionsList.add( Question.newBuilder() .setQuestionId(TEST_QUESTION_ID_0) + .setQuestionState(explorationRetriever.createStateFromJson( + "question",questionsJSON?.getJSONObject(0))) .build()) TEST_SKILL_ID_1 -> questionsList.add( Question.newBuilder() .setQuestionId(TEST_QUESTION_ID_1) + .setQuestionState(explorationRetriever.createStateFromJson( + "question",questionsJSON?.getJSONObject(1))) .build()) TEST_SKILL_ID_2 -> questionsList.add( Question.newBuilder() .setQuestionId(TEST_QUESTION_ID_2) + .setQuestionState(explorationRetriever.createStateFromJson( + "question",questionsJSON?.getJSONObject(2))) .build()) else -> { throw IllegalStateException("Invalid skill ID: $skillId") From 459e3e93dace4a6536c30f4a802b6a6b4ff2e4ac Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Fri, 11 Oct 2019 16:10:32 -0700 Subject: [PATCH 10/25] Remove duplicate questions while fetching in training controller --- .../org/oppia/domain/question/QuestionTrainingController.kt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt index 4491b1ba282..df8fd6596dd 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt @@ -54,7 +54,9 @@ class QuestionTrainingController @Inject constructor( for (skillId in skillIdsList) { trainingQuestions.addAll(questionsList.filter { it.linkedSkillIdsList.contains(skillId) - }.take(numQuestionsPerSkill + 1)) + }.distinctBy { + it.questionId} + .take(numQuestionsPerSkill + 1)) } return trainingQuestions.take(TOTAL_QUESTIONS_PER_TOPIC) } From 73c49dffdcf3f1e113af09bc2f626e22fbe2e257 Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Fri, 11 Oct 2019 16:12:43 -0700 Subject: [PATCH 11/25] Comment explaining the filter function --- .../domain/question/QuestionTrainingController.kt | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt index df8fd6596dd..3a10e2d15ab 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt @@ -48,17 +48,19 @@ class QuestionTrainingController @Inject constructor( } } + // Attempts to fetch equal number of questions per skill. Removes any duplicates and limits the questions to be + // equal to TOTAL_QUESTIONS_PER_TOPIC questions. private fun getFilteredQuestionsForTraining( skillIdsList: List, questionsList: List, numQuestionsPerSkill: Int): List{ val trainingQuestions = mutableListOf() for (skillId in skillIdsList) { trainingQuestions.addAll(questionsList.filter { it.linkedSkillIdsList.contains(skillId) - }.distinctBy { - it.questionId} - .take(numQuestionsPerSkill + 1)) + }.take(numQuestionsPerSkill + 1)) } - return trainingQuestions.take(TOTAL_QUESTIONS_PER_TOPIC) + return trainingQuestions + .distinctBy {it.questionId} + .take(TOTAL_QUESTIONS_PER_TOPIC) } /** From 91cf8e6b0810b6d369d4aeff74aa0d75dc7afb71 Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Fri, 11 Oct 2019 16:15:49 -0700 Subject: [PATCH 12/25] Improve duplicate checking - check it while filtering instead of after filtering --- .../org/oppia/domain/question/QuestionTrainingController.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt index 3a10e2d15ab..1d687774a0c 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt @@ -55,11 +55,11 @@ class QuestionTrainingController @Inject constructor( val trainingQuestions = mutableListOf() for (skillId in skillIdsList) { trainingQuestions.addAll(questionsList.filter { - it.linkedSkillIdsList.contains(skillId) + it.linkedSkillIdsList.contains(skillId) && + !trainingQuestions.contains(it) }.take(numQuestionsPerSkill + 1)) } return trainingQuestions - .distinctBy {it.questionId} .take(TOTAL_QUESTIONS_PER_TOPIC) } From 54546d78cc4913ebe460becc0a968b48ae30e120 Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Fri, 11 Oct 2019 16:18:42 -0700 Subject: [PATCH 13/25] Add linked skill id values --- domain/src/main/java/org/oppia/domain/topic/TopicController.kt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/domain/src/main/java/org/oppia/domain/topic/TopicController.kt b/domain/src/main/java/org/oppia/domain/topic/TopicController.kt index 43184f83c75..a7993071992 100644 --- a/domain/src/main/java/org/oppia/domain/topic/TopicController.kt +++ b/domain/src/main/java/org/oppia/domain/topic/TopicController.kt @@ -106,18 +106,21 @@ class TopicController @Inject constructor( .setQuestionId(TEST_QUESTION_ID_0) .setQuestionState(explorationRetriever.createStateFromJson( "question",questionsJSON?.getJSONObject(0))) + .addLinkedSkillIds(TEST_SKILL_ID_0) .build()) TEST_SKILL_ID_1 -> questionsList.add( Question.newBuilder() .setQuestionId(TEST_QUESTION_ID_1) .setQuestionState(explorationRetriever.createStateFromJson( "question",questionsJSON?.getJSONObject(1))) + .addLinkedSkillIds(TEST_SKILL_ID_1) .build()) TEST_SKILL_ID_2 -> questionsList.add( Question.newBuilder() .setQuestionId(TEST_QUESTION_ID_2) .setQuestionState(explorationRetriever.createStateFromJson( "question",questionsJSON?.getJSONObject(2))) + .addLinkedSkillIds(TEST_SKILL_ID_2) .build()) else -> { throw IllegalStateException("Invalid skill ID: $skillId") From be375fd375515a2af9fab3d61000249cb30e8190 Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Tue, 15 Oct 2019 14:58:50 -0700 Subject: [PATCH 14/25] Review changes --- domain/src/main/assets/sample_questions.json | 681 +++++++++++++----- .../exploration/ExplorationRetriever.kt | 215 +----- .../question/QuestionTrainingController.kt | 7 +- .../org/oppia/domain/topic/TopicController.kt | 118 ++- .../oppia/domain/util/JsonAssetRetriever.kt | 17 + .../org/oppia/domain/util/StateRetriever.kt | 205 ++++++ .../oppia/domain/topic/TopicControllerTest.kt | 7 +- 7 files changed, 800 insertions(+), 450 deletions(-) create mode 100644 domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt create mode 100644 domain/src/main/java/org/oppia/domain/util/StateRetriever.kt diff --git a/domain/src/main/assets/sample_questions.json b/domain/src/main/assets/sample_questions.json index 5024906fb64..102bdcaef41 100644 --- a/domain/src/main/assets/sample_questions.json +++ b/domain/src/main/assets/sample_questions.json @@ -1,263 +1,544 @@ -{ "questions": -[{ - "name":"question", - "classifier_model_id":null, - "content":{ - "html":"

What fraction does 'quarter' represent? 

", - "content_id":"content" - }, - "interaction":{ - "confirmed_unclassified_answers": [], - "answer_groups":[ - { - "rule_specs":[ +{ + "questions":[ + { + "name":"question", + "classifier_model_id":null, + "content":{ + "html":"

What fraction does 'quarter' represent? 

", + "content_id":"content" + }, + "interaction":{ + "confirmed_unclassified_answers":[ + + ], + "answer_groups":[ { - "rule_type":"Contains", - "inputs":{ - "x":"1/4" - } + "rule_specs":[ + { + "rule_type":"Contains", + "inputs":{ + "x":"1/4" + } + } + ], + "outcome":{ + "dest":null, + "feedback":{ + "html":"

That's correct!

", + "content_id":"feedback_1" + }, + "labelled_as_correct":true, + "param_changes":[ + + ], + "refresher_exploration_id":null, + "missing_prerequisite_skill_id":null + }, + "trainingData":[ + + ], + "taggedSkillMisconceptionId":null } ], - "outcome":{ + "confirmedUnclassifiedAnswers":[ + + ], + "customizationArgs":{ + "placeholder":{ + "value":"Write your fraction as x/y" + }, + "rows":{ + "value":1 + } + }, + "defaultOutcome":{ "dest":null, "feedback":{ - "html":"

That's correct!

", - "content_id":"feedback_1" + "html":"", + "content_id":"default_outcome" }, - "labelled_as_correct":true, + "labelled_as_correct":false, "param_changes":[ ], "refresher_exploration_id":null, "missing_prerequisite_skill_id":null }, - "trainingData":[ - + "hints":[ + { + "hintContent":{ + "html":"

Hint text will appear here

", + "content_id":"hint_1" + } + } ], - "taggedSkillMisconceptionId":null - } - ], - "confirmedUnclassifiedAnswers":[ - - ], - "customizationArgs":{ - "placeholder":{ - "value":"Write your fraction as x/y" - }, - "rows":{ - "value":1 - } - }, - "defaultOutcome":{ - "dest":null, - "feedback":{ - "html":"", - "content_id":"default_outcome" + "id":"TextInput", + "solution":{ + "answer_is_exclusive":false, + "correct_answer":"1/4", + "explanation":{ + "html":"

A quarter is 1/4th of something

", + "content_id":"solution" + } + } }, - "labelled_as_correct":false, "param_changes":[ ], - "refresher_exploration_id":null, - "missing_prerequisite_skill_id":null - }, - "hints":[ - { - "hintContent":{ - "html":"

Hint text will appear here

", - "content_id":"hint_1" + "solicit_answer_details":false, + "writtenTranslations":{ + "translationsMapping":{ + "content":{ + + }, + "hint_1":{ + + }, + "feedback_1":{ + + }, + "default_outcome":{ + + }, + "solution":{ + + } + }, + "_writtenTranslationObjectFactory":{ + } } - ], - "id":"TextInput", - "solution":{ - "answer_is_exclusive":false, - "correct_answer":"1/4", - "explanation":{ - "html":"

A quarter is 1/4th of something

", - "content_id":"solution" - } - } - }, - "param_changes":[ - - ], - "solicit_answer_details":false, - "writtenTranslations":{ - "translationsMapping":{ + }, + { + "name":"question", + "classifier_model_id":null, "content":{ - + "html":"

If we talk about wanting  of a cake, what does the 7 represent?

", + "content_id":"content" }, - "hint_1":{ + "interaction":{ + "confirmed_unclassified_answers":[ + + ], + "answer_groups":[ + { + "rule_specs":[ + { + "rule_type":"Equals", + "inputs":{ + "x":1 + } + } + ], + "outcome":{ + "dest":null, + "feedback":{ + "html":"

That's correct!

", + "content_id":"feedback_1" + }, + "labelled_as_correct":true, + "param_changes":[ + ], + "refresher_exploration_id":null, + "missing_prerequisite_skill_id":null + }, + "trainingData":[ + + ], + "taggedSkillMisconceptionId":null + } + ], + "confirmedUnclassifiedAnswers":[ + + ], + "customizationArgs":{ + "choices":{ + "value":[ + "

The number of pieces of cake I want.

", + "

The number of pieces the whole cake is cut into.

", + "

None of the above.

" + ] + } + }, + "defaultOutcome":{ + "dest":null, + "feedback":{ + "html":"", + "content_id":"default_outcome" + }, + "labelled_as_correct":false, + "param_changes":[ + + ], + "refresher_exploration_id":null, + "missing_prerequisite_skill_id":null + }, + "hints":[ + { + "hintContent":{ + "html":"

Hint text will appear here

", + "content_id":"hint_1" + } + } + ], + "id":"MultipleChoiceInput", + "solution":null }, - "feedback_1":{ + "param_changes":[ + ], + "solicit_answer_details":false + }, + { + "name":"question", + "classifier_model_id":null, + "content":{ + "html":"

What is the numerator of  equal to?

", + "content_id":"content" }, - "default_outcome":{ + "interaction":{ + "confirmed_unclassified_answers":[ + + ], + "answer_groups":[ + { + "rule_specs":[ + { + "rule_type":"IsInclusivelyBetween", + "inputs":{ + "a":3, + "b":3 + } + } + ], + "outcome":{ + "dest":null, + "feedback":"

That's correct!

", + "labelled_as_correct":true, + "param_changes":[ + ], + "refresher_exploration_id":null, + "missing_prerequisite_skill_id":null + }, + "trainingData":[ + + ], + "taggedSkillMisconceptionId":null + } + ], + "confirmedUnclassifiedAnswers":[ + + ], + "customizationArgs":{ + + }, + "defaultOutcome":{ + "dest":null, + "feedback":{ + "html":"", + "content_id":"default_outcome" + }, + "labelled_as_correct":false, + "param_changes":[ + + ], + "refresher_exploration_id":null, + "missing_prerequisite_skill_id":null + }, + "hints":[ + { + "hintContent":{ + "html":"

Hint text will appear here

", + "content_id":"hint_1" + } + } + ], + "id":"NumericInput", + "solution":{ + "answer_is_exclusive":false, + "correct_answer":3, + "explanation":{ + "html":"

The solution comes out to be  and the numerator of that is 3.

", + "content_id":"solution" + } + } }, - "solution":{ + "param_changes":[ - } + ], + "solicit_answer_details":false }, - "_writtenTranslationObjectFactory":{ + { + "name":"question", + "classifier_model_id":null, + "content":{ + "html":"

What fraction does 'half' represent? 

", + "content_id":"content" + }, + "interaction":{ + "confirmed_unclassified_answers":[ - } - } -}, -{ - "name":"question", - "classifier_model_id":null, - "content":{ - "html":"

If we talk about wanting  of a cake, what does the 7 represent?

", - "content_id":"content" - }, - "interaction":{ - "confirmed_unclassified_answers": [], - "answer_groups":[ - { - "rule_specs":[ + ], + "answer_groups":[ { - "rule_type":"Equals", - "inputs":{ - "x":1 - } + "rule_specs":[ + { + "rule_type":"Contains", + "inputs":{ + "x":"1/2" + } + } + ], + "outcome":{ + "dest":null, + "feedback":{ + "html":"

That's correct!

", + "content_id":"feedback_1" + }, + "labelled_as_correct":true, + "param_changes":[ + + ], + "refresher_exploration_id":null, + "missing_prerequisite_skill_id":null + }, + "trainingData":[ + + ], + "taggedSkillMisconceptionId":null } ], - "outcome":{ + "confirmedUnclassifiedAnswers":[ + + ], + "customizationArgs":{ + "placeholder":{ + "value":"Write your fraction as x/y" + }, + "rows":{ + "value":1 + } + }, + "defaultOutcome":{ "dest":null, "feedback":{ - "html":"

That's correct!

", - "content_id":"feedback_1" + "html":"", + "content_id":"default_outcome" }, - "labelled_as_correct":true, + "labelled_as_correct":false, "param_changes":[ ], "refresher_exploration_id":null, "missing_prerequisite_skill_id":null }, - "trainingData":[ - + "hints":[ + { + "hintContent":{ + "html":"

Hint text will appear here

", + "content_id":"hint_1" + } + } ], - "taggedSkillMisconceptionId":null - } - ], - "confirmedUnclassifiedAnswers":[ - - ], - "customizationArgs":{ - "choices":{ - "value":[ - "

The number of pieces of cake I want.

", - "

The number of pieces the whole cake is cut into.

", - "

None of the above.

" - ] - } - }, - "defaultOutcome":{ - "dest":null, - "feedback":{ - "html":"", - "content_id":"default_outcome" + "id":"TextInput", + "solution":{ + "answer_is_exclusive":false, + "correct_answer":"1/2", + "explanation":{ + "html":"

A half is 1/2 of something

", + "content_id":"solution" + } + } }, - "labelled_as_correct":false, "param_changes":[ ], - "refresher_exploration_id":null, - "missing_prerequisite_skill_id":null - }, - "hints":[ - { - "hintContent":{ - "html":"

Hint text will appear here

", - "content_id":"hint_1" + "solicit_answer_details":false, + "writtenTranslations":{ + "translationsMapping":{ + "content":{ + + }, + "hint_1":{ + + }, + "feedback_1":{ + + }, + "default_outcome":{ + + }, + "solution":{ + + } + }, + "_writtenTranslationObjectFactory":{ + } } - ], - "id":"MultipleChoiceInput", - "solution":null - }, - "param_changes":[ - - ], - "solicit_answer_details":false -}, - { - "name":"question", - "classifier_model_id":null, - "content":{ - "html":"

What is the numerator of  equal to?

", - "content_id":"content" }, - "interaction":{ - "confirmed_unclassified_answers": [], - "answer_groups":[ - { - "rule_specs":[ - { - "rule_type":"IsInclusivelyBetween", - "inputs":{ - "a":3, - "b":3 + { + "name":"question", + "classifier_model_id":null, + "content":{ + "html":"

If we talk about wanting  of a cake, what does the 10 represent?

", + "content_id":"content" + }, + "interaction":{ + "confirmed_unclassified_answers":[ + + ], + "answer_groups":[ + { + "rule_specs":[ + { + "rule_type":"Equals", + "inputs":{ + "x":1 + } } - } - ], - "outcome":{ - "dest":null, - "feedback": "

That's correct!

", - "labelled_as_correct":true, - "param_changes":[], - "refresher_exploration_id":null, - "missing_prerequisite_skill_id":null + ], + "outcome":{ + "dest":null, + "feedback":{ + "html":"

That's correct!

", + "content_id":"feedback_1" + }, + "labelled_as_correct":true, + "param_changes":[ + + ], + "refresher_exploration_id":null, + "missing_prerequisite_skill_id":null + }, + "trainingData":[ + + ], + "taggedSkillMisconceptionId":null + } + ], + "confirmedUnclassifiedAnswers":[ + + ], + "customizationArgs":{ + "choices":{ + "value":[ + "

The number of pieces of cake I want.

", + "

The number of pieces the whole cake is cut into.

", + "

None of the above.

" + ] + } + }, + "defaultOutcome":{ + "dest":null, + "feedback":{ + "html":"", + "content_id":"default_outcome" }, - "trainingData":[ + "labelled_as_correct":false, + "param_changes":[ ], - "taggedSkillMisconceptionId":null - } - ], - "confirmedUnclassifiedAnswers":[ + "refresher_exploration_id":null, + "missing_prerequisite_skill_id":null + }, + "hints":[ + { + "hintContent":{ + "html":"

Hint text will appear here

", + "content_id":"hint_1" + } + } + ], + "id":"MultipleChoiceInput", + "solution":null + }, + "param_changes":[ ], - "customizationArgs":{ - + "solicit_answer_details":false + }, + { + "name":"question", + "classifier_model_id":null, + "content":{ + "html":"

What is the numerator of  equal to?

", + "content_id":"content" }, - "defaultOutcome":{ - "dest":null, - "feedback":{ - "html":"", - "content_id":"default_outcome" + "interaction":{ + "confirmed_unclassified_answers":[ + + ], + "answer_groups":[ + { + "rule_specs":[ + { + "rule_type":"IsInclusivelyBetween", + "inputs":{ + "a":5, + "b":5 + } + } + ], + "outcome":{ + "dest":null, + "feedback":"

That's correct!

", + "labelled_as_correct":true, + "param_changes":[ + + ], + "refresher_exploration_id":null, + "missing_prerequisite_skill_id":null + }, + "trainingData":[ + + ], + "taggedSkillMisconceptionId":null + } + ], + "confirmedUnclassifiedAnswers":[ + + ], + "customizationArgs":{ + }, - "labelled_as_correct":false, - "param_changes":[ + "defaultOutcome":{ + "dest":null, + "feedback":{ + "html":"", + "content_id":"default_outcome" + }, + "labelled_as_correct":false, + "param_changes":[ + ], + "refresher_exploration_id":null, + "missing_prerequisite_skill_id":null + }, + "hints":[ + { + "hintContent":{ + "html":"

Hint text will appear here

", + "content_id":"hint_1" + } + } ], - "refresher_exploration_id":null, - "missing_prerequisite_skill_id":null - }, - "hints":[ - { - "hintContent":{ - "html":"

Hint text will appear here

", - "content_id":"hint_1" + "id":"NumericInput", + "solution":{ + "answer_is_exclusive":false, + "correct_answer":5, + "explanation":{ + "html":"

The solution comes out to be  and the numerator of that is 5.

", + "content_id":"solution" } } + }, + "param_changes":[ + ], - "id":"NumericInput", - "solution":{ - "answer_is_exclusive":false, - "correct_answer":3, - "explanation":{ - "html":"

The solution comes out to be  and the numerator of that is 3.

", - "content_id":"solution" - } - } - }, - "param_changes":[ - ], - "solicit_answer_details":false - }] -} + "solicit_answer_details":false + } + ] +} \ No newline at end of file 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 bc55c66c3d3..eebbd9b2e6b 100644 --- a/domain/src/main/java/org/oppia/domain/exploration/ExplorationRetriever.kt +++ b/domain/src/main/java/org/oppia/domain/exploration/ExplorationRetriever.kt @@ -1,17 +1,10 @@ package org.oppia.domain.exploration -import android.content.Context -import org.json.JSONArray import org.json.JSONObject -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.StringList -import org.oppia.app.model.SubtitledHtml +import org.oppia.domain.util.JsonAssetRetriever +import org.oppia.domain.util.StateRetriever import java.io.IOException import javax.inject.Inject @@ -22,7 +15,9 @@ const val TEST_EXPLORATION_ID_6 = "test_exp_id_6" // 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) { +class ExplorationRetriever @Inject constructor( + private val jsonAssetRetriever: JsonAssetRetriever, + private val stateRetriever: StateRetriever) { /** 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 { @@ -36,7 +31,7 @@ class ExplorationRetriever @Inject constructor(private val context: Context) { // Returns an exploration given an assetName private fun loadExplorationFromAsset(assetName: String): Exploration { try { - val explorationObject = loadJsonFromAsset(assetName) ?: return Exploration.getDefaultInstance() + val explorationObject = jsonAssetRetriever.loadJsonFromAsset(assetName) ?: return Exploration.getDefaultInstance() return Exploration.newBuilder() .setTitle(explorationObject.getString("title")) .setLanguageCode(explorationObject.getString("language_code")) @@ -49,19 +44,6 @@ 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.optJSONObject(key) - } - - // Loads the JSON string from an asset and converts it to a JSONObject - @Throws(IOException::class) - fun loadJsonFromAsset(assetName: String): JSONObject? { - val assetManager = context.assets - val jsonContents = assetManager.open(assetName).bufferedReader().use { it.readText() } - return JSONObject(jsonContents) - } - // Creates the states map from JSON private fun createStatesFromJsonObject(statesJsonObject: JSONObject?): MutableMap { val statesMap: MutableMap = mutableMapOf() @@ -69,193 +51,10 @@ class ExplorationRetriever @Inject constructor(private val context: Context) { val statesIterator = statesKeys.iterator() while (statesIterator.hasNext()) { val key = statesIterator.next() - statesMap[key] = createStateFromJson(key, statesJsonObject.getJSONObject(key)) + statesMap[key] = stateRetriever.createStateFromJson(key, statesJsonObject.getJSONObject(key)) } return statesMap } - // Creates a single state object from JSON - fun createStateFromJson(stateName: String, stateJson: JSONObject?): State { - return State.newBuilder() - .setName(stateName) - .setContent( - SubtitledHtml.newBuilder().setHtml( - stateJson?.getJSONObject("content")?.getString("html") - ) - ) - .setInteraction(createInteractionFromJson(stateJson?.getJSONObject("interaction"))) - .build() - } - - // Creates an interaction from JSON - private fun createInteractionFromJson(interactionJson: JSONObject?): Interaction { - if (interactionJson == null) { - return Interaction.getDefaultInstance() - } - return Interaction.newBuilder() - .setId(interactionJson.getString("id")) - .addAllAnswerGroups( - createAnswerGroupsFromJson( - interactionJson.getJSONArray("answer_groups"), - interactionJson.getString("id") - ) - ) - .addAllConfirmedUnclassifiedAnswers( - createAnswerGroupsFromJson( - interactionJson.getJSONArray("confirmed_unclassified_answers"), - interactionJson.getString("id") - ) - ) - .setDefaultOutcome( - createOutcomeFromJson( - getJsonObject(interactionJson, "default_outcome") - ) - ) - .putAllCustomizationArgs( - createCustomizationArgsMapFromJson( - getJsonObject(interactionJson, "customization_args") - ) - ) - .build() - } - // Creates the list of answer group objects from JSON - private fun createAnswerGroupsFromJson( - answerGroupsJson: JSONArray?, interactionId: String - ): MutableList { - val answerGroups = mutableListOf() - if (answerGroupsJson == null) { - return answerGroups - } - for (i in 0 until answerGroupsJson.length()) { - answerGroups.add( - createSingleAnswerGroupFromJson( - answerGroupsJson.getJSONObject(i), interactionId - ) - ) - } - return answerGroups - } - - // Creates a single answer group object from JSON - private fun createSingleAnswerGroupFromJson( - answerGroupJson: JSONObject, interactionId: String - ): AnswerGroup { - return AnswerGroup.newBuilder() - .setOutcome( - createOutcomeFromJson(answerGroupJson.getJSONObject("outcome")) - ) - .addAllRuleSpecs( - createRuleSpecsFromJson( - answerGroupJson.getJSONArray("rule_specs"), interactionId - ) - ) - .build() - } - - // Creates an outcome object from JSON - private fun createOutcomeFromJson(outcomeJson: JSONObject?): Outcome { - if (outcomeJson == null) { - return Outcome.getDefaultInstance() - } - return Outcome.newBuilder() - .setDestStateName(outcomeJson.getString("dest")) - .setFeedback( - SubtitledHtml.newBuilder() - .setHtml(outcomeJson.getString("feedback")) - ) - .setLabelledAsCorrect(outcomeJson.getBoolean("labelled_as_correct")) - .build() - } - - // Creates the list of rule spec objects from JSON - private fun createRuleSpecsFromJson( - ruleSpecJson: JSONArray?, interactionId: String - ): MutableList { - val ruleSpecList = mutableListOf() - if (ruleSpecJson == null) { - return ruleSpecList - } - for (i in 0 until ruleSpecJson.length()) { - val ruleSpecBuilder = RuleSpec.newBuilder() - ruleSpecBuilder.ruleType = ruleSpecJson.getJSONObject(i).getString("rule_type") - val inputsJson = ruleSpecJson.getJSONObject(i).getJSONObject("inputs") - val inputKeysIterator = inputsJson.keys() - while (inputKeysIterator.hasNext()) { - val inputName = inputKeysIterator.next() - ruleSpecBuilder.putInput(inputName, createInputFromJson(inputsJson, inputName, interactionId)) - } - ruleSpecList.add(ruleSpecBuilder.build()) - } - return ruleSpecList - } - - // Creates an input interaction object from JSON - private fun createInputFromJson( - inputJson: JSONObject?, keyName: String, interactionId: String - ): InteractionObject { - if (inputJson == null) { - return InteractionObject.getDefaultInstance() - } - return when (interactionId) { - "MultipleChoiceInput" -> InteractionObject.newBuilder() - .setNonNegativeInt(inputJson.getInt(keyName)) - .build() - "TextInput" -> InteractionObject.newBuilder() - .setNormalizedString(inputJson.getString(keyName)) - .build() - "NumericInput" -> InteractionObject.newBuilder() - .setReal(inputJson.getDouble(keyName)) - .build() - else -> throw IllegalStateException("Encountered unexpected interaction ID: $interactionId") - } - } - - // Creates a customization arg mapping from JSON - private fun createCustomizationArgsMapFromJson( - customizationArgsJson: JSONObject? - ): MutableMap { - val customizationArgsMap: MutableMap = mutableMapOf() - if (customizationArgsJson == null) { - return customizationArgsMap - } - val customizationArgsKeys = customizationArgsJson.keys() ?: return customizationArgsMap - val customizationArgsIterator = customizationArgsKeys.iterator() - while (customizationArgsIterator.hasNext()) { - val key = customizationArgsIterator.next() - customizationArgsMap[key] = createCustomizationArgValueFromJson( - customizationArgsJson.getJSONObject(key).get("value") - ) - } - return customizationArgsMap - } - - // Creates a customization arg value interaction object from JSON - private fun createCustomizationArgValueFromJson(customizationArgValue: Any): InteractionObject { - val interactionObjectBuilder = InteractionObject.newBuilder() - when (customizationArgValue) { - is String -> return interactionObjectBuilder - .setNormalizedString(customizationArgValue).build() - is Int -> return interactionObjectBuilder - .setSignedInt(customizationArgValue).build() - is Double -> return interactionObjectBuilder - .setReal(customizationArgValue).build() - is List<*> -> if (customizationArgValue.size > 0) { - return interactionObjectBuilder.setSetOfHtmlString( - createStringList(customizationArgValue) - ).build() - } - } - return InteractionObject.getDefaultInstance() - } - - @Suppress("UNCHECKED_CAST") // Checked cast in the if statement - private fun createStringList(value: List<*>): StringList { - val stringList = mutableListOf() - if (value[0] is String) { - stringList.addAll(value as List) - return StringList.newBuilder().addAllHtml(stringList).build() - } - return StringList.getDefaultInstance() - } } diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt index 1d687774a0c..401d14ea50b 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt @@ -10,7 +10,7 @@ import org.oppia.util.data.DataProviders import javax.inject.Inject import javax.inject.Singleton -const val TOTAL_QUESTIONS_PER_TOPIC = 10 +const val QUESTION_COUNT_PER_TRAINING_SESSION = 10 const val TRAINING_QUESTIONS_PROVIDER = "TrainingQuestionsProvider" /** Controller for retrieving a set of questions. */ @@ -44,7 +44,8 @@ class QuestionTrainingController @Inject constructor( private fun retrieveQuestionsForSkillIds(skillIdsList: List): DataProvider> { val questionsDataProvider = topicController.retrieveQuestionsForSkillIds(skillIdsList) return dataProviders.transform(TRAINING_QUESTIONS_PROVIDER, questionsDataProvider) { - getFilteredQuestionsForTraining(skillIdsList, it, TOTAL_QUESTIONS_PER_TOPIC/skillIdsList.size) + getFilteredQuestionsForTraining(skillIdsList, it, + QUESTION_COUNT_PER_TRAINING_SESSION / skillIdsList.size) } } @@ -60,7 +61,7 @@ class QuestionTrainingController @Inject constructor( }.take(numQuestionsPerSkill + 1)) } return trainingQuestions - .take(TOTAL_QUESTIONS_PER_TOPIC) + .take(QUESTION_COUNT_PER_TRAINING_SESSION) } /** diff --git a/domain/src/main/java/org/oppia/domain/topic/TopicController.kt b/domain/src/main/java/org/oppia/domain/topic/TopicController.kt index a7993071992..2a1ee209ccb 100644 --- a/domain/src/main/java/org/oppia/domain/topic/TopicController.kt +++ b/domain/src/main/java/org/oppia/domain/topic/TopicController.kt @@ -2,6 +2,7 @@ package org.oppia.domain.topic import androidx.lifecycle.LiveData import androidx.lifecycle.MutableLiveData +import org.json.JSONArray import org.oppia.app.model.ChapterPlayState import java.lang.IllegalArgumentException import javax.inject.Inject @@ -19,7 +20,8 @@ import org.oppia.app.model.Translation import org.oppia.app.model.TranslationMapping import org.oppia.app.model.Voiceover import org.oppia.app.model.VoiceoverMapping -import org.oppia.domain.exploration.ExplorationRetriever +import org.oppia.domain.util.JsonAssetRetriever +import org.oppia.domain.util.StateRetriever import org.oppia.util.data.AsyncResult import org.oppia.util.data.DataProvider import org.oppia.util.data.DataProviders @@ -32,6 +34,9 @@ const val TEST_SKILL_CONTENT_ID_1 = "test_skill_content_id_1" 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" +const val TEST_QUESTION_ID_3 = "question_id_3" +const val TEST_QUESTION_ID_4 = "question_id_4" +const val TEST_QUESTION_ID_5 = "question_id_5" private const val QUESTION_DATA_PROVIDER_ID = "QuestionDataProvider" @@ -39,7 +44,8 @@ private const val QUESTION_DATA_PROVIDER_ID = "QuestionDataProvider" @Singleton class TopicController @Inject constructor( private val dataProviders: DataProviders, - private val explorationRetriever: ExplorationRetriever + private val jsonAssetRetriever: JsonAssetRetriever, + private val stateRetriever: StateRetriever ) { /** Returns the [Topic] corresponding to the specified topic ID, or a failed result if no such topic exists. */ fun getTopic(topicId: String): LiveData> { @@ -79,49 +85,33 @@ class TopicController @Inject constructor( } fun retrieveQuestionsForSkillIds(skillIdsList: List): DataProvider> { - return dataProviders.createInMemoryDataProviderAsync(QUESTION_DATA_PROVIDER_ID) { - loadQuestionsForSkillIds(skillIdsList) - } + return dataProviders.createInMemoryDataProvider(QUESTION_DATA_PROVIDER_ID) { + loadQuestionsForSkillIds(skillIdsList) + } } // 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): AsyncResult> { - return try { - AsyncResult.success(loadQuestions(skillIdsList)) - } catch (e: Exception) { - AsyncResult.failed(e) - } + private fun loadQuestionsForSkillIds(skillIdsList: List): List { + return loadQuestions(skillIdsList) } - @Suppress("RedundantSuspendModifier") // Force callers to call this on a background thread. - private suspend fun loadQuestions(skillIdsList: List): List { + private fun loadQuestions(skillIdsList: List): List { val questionsList = mutableListOf() - val questionsJSON = explorationRetriever.loadJsonFromAsset( + val questionsJSON = jsonAssetRetriever.loadJsonFromAsset( "sample_questions.json")?.getJSONArray("questions") for (skillId in skillIdsList) { when (skillId) { - TEST_SKILL_ID_0 -> questionsList.add( - Question.newBuilder() - .setQuestionId(TEST_QUESTION_ID_0) - .setQuestionState(explorationRetriever.createStateFromJson( - "question",questionsJSON?.getJSONObject(0))) - .addLinkedSkillIds(TEST_SKILL_ID_0) - .build()) - TEST_SKILL_ID_1 -> questionsList.add( - Question.newBuilder() - .setQuestionId(TEST_QUESTION_ID_1) - .setQuestionState(explorationRetriever.createStateFromJson( - "question",questionsJSON?.getJSONObject(1))) - .addLinkedSkillIds(TEST_SKILL_ID_1) - .build()) - TEST_SKILL_ID_2 -> questionsList.add( - Question.newBuilder() - .setQuestionId(TEST_QUESTION_ID_2) - .setQuestionState(explorationRetriever.createStateFromJson( - "question",questionsJSON?.getJSONObject(2))) - .addLinkedSkillIds(TEST_SKILL_ID_2) - .build()) + TEST_SKILL_ID_0 -> questionsList.addAll(mutableListOf( + createTestQuestion0(questionsJSON), + createTestQuestion1(questionsJSON), + createTestQuestion2(questionsJSON))) + TEST_SKILL_ID_1 -> questionsList.addAll(mutableListOf( + createTestQuestion0(questionsJSON), + createTestQuestion3(questionsJSON))) + TEST_SKILL_ID_2 -> questionsList.addAll(mutableListOf( + createTestQuestion2(questionsJSON), + createTestQuestion4(questionsJSON), + createTestQuestion5(questionsJSON))) else -> { throw IllegalStateException("Invalid skill ID: $skillId") } @@ -130,6 +120,62 @@ class TopicController @Inject constructor( return questionsList } + private fun createTestQuestion0(questionsJSON: JSONArray?): Question { + return Question.newBuilder() + .setQuestionId(TEST_QUESTION_ID_0) + .setQuestionState(stateRetriever.createStateFromJson( + "question",questionsJSON?.getJSONObject(0))) + .addAllLinkedSkillIds(mutableListOf(TEST_SKILL_ID_0, TEST_SKILL_ID_1)) + .build() + } + + private fun createTestQuestion1(questionsJSON: JSONArray?): Question { + return Question.newBuilder() + .setQuestionId(TEST_QUESTION_ID_1) + .setQuestionState(stateRetriever.createStateFromJson( + "question",questionsJSON?.getJSONObject(1))) + .addAllLinkedSkillIds(mutableListOf(TEST_SKILL_ID_0)) + .build() + } + + private fun createTestQuestion2(questionsJSON: JSONArray?): Question { + return Question.newBuilder() + .setQuestionId(TEST_QUESTION_ID_2) + .setQuestionState(stateRetriever.createStateFromJson( + "question",questionsJSON?.getJSONObject(2))) + .addAllLinkedSkillIds(mutableListOf(TEST_SKILL_ID_0, TEST_SKILL_ID_2)) + .build() + } + + private fun createTestQuestion3(questionsJSON: JSONArray?): Question { + return Question.newBuilder() + .setQuestionId(TEST_QUESTION_ID_3) + .setQuestionState(stateRetriever.createStateFromJson( + "question",questionsJSON?.getJSONObject(0))) + .addAllLinkedSkillIds(mutableListOf(TEST_SKILL_ID_1)) + .build() + } + + private fun createTestQuestion4(questionsJSON: JSONArray?): Question { + return Question.newBuilder() + .setQuestionId(TEST_QUESTION_ID_4) + .setQuestionState(stateRetriever.createStateFromJson( + "question",questionsJSON?.getJSONObject(1))) + .addAllLinkedSkillIds(mutableListOf(TEST_SKILL_ID_2)) + .build() + } + + private fun createTestQuestion5(questionsJSON: JSONArray?): Question { + return Question.newBuilder() + .setQuestionId(TEST_QUESTION_ID_5) + .setQuestionState(stateRetriever.createStateFromJson( + "question",questionsJSON?.getJSONObject(2))) + .addAllLinkedSkillIds(mutableListOf(TEST_SKILL_ID_2)) + .build() + } + + + private fun createTestTopic0(): Topic { return Topic.newBuilder() .setTopicId(TEST_TOPIC_ID_0) diff --git a/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt b/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt new file mode 100644 index 00000000000..22428b6b9f3 --- /dev/null +++ b/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt @@ -0,0 +1,17 @@ +package org.oppia.domain.util + +import android.content.Context +import org.json.JSONObject +import java.io.IOException +import javax.inject.Inject + +class JsonAssetRetriever @Inject constructor(private val context: Context) { + + // Loads the JSON string from an asset and converts it to a JSONObject + @Throws(IOException::class) + public fun loadJsonFromAsset(assetName: String): JSONObject? { + val assetManager = context.assets + val jsonContents = assetManager.open(assetName).bufferedReader().use { it.readText() } + return JSONObject(jsonContents) + } +} \ No newline at end of file diff --git a/domain/src/main/java/org/oppia/domain/util/StateRetriever.kt b/domain/src/main/java/org/oppia/domain/util/StateRetriever.kt new file mode 100644 index 00000000000..138fe1c2acc --- /dev/null +++ b/domain/src/main/java/org/oppia/domain/util/StateRetriever.kt @@ -0,0 +1,205 @@ +package org.oppia.domain.util + +import org.json.JSONArray +import org.json.JSONObject +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.RuleSpec +import org.oppia.app.model.State +import org.oppia.app.model.StringList +import org.oppia.app.model.SubtitledHtml +import javax.inject.Inject + +class StateRetriever @Inject constructor() { + // Creates a single state object from JSON + public fun createStateFromJson(stateName: String, stateJson: JSONObject?): State { + return State.newBuilder() + .setName(stateName) + .setContent( + SubtitledHtml.newBuilder().setHtml( + stateJson?.getJSONObject("content")?.getString("html") + ) + ) + .setInteraction(createInteractionFromJson(stateJson?.getJSONObject("interaction"))) + .build() + } + + // Creates an interaction from JSON + private fun createInteractionFromJson(interactionJson: JSONObject?): Interaction { + if (interactionJson == null) { + return Interaction.getDefaultInstance() + } + return Interaction.newBuilder() + .setId(interactionJson.getString("id")) + .addAllAnswerGroups( + createAnswerGroupsFromJson( + interactionJson.getJSONArray("answer_groups"), + interactionJson.getString("id") + ) + ) + .addAllConfirmedUnclassifiedAnswers( + createAnswerGroupsFromJson( + interactionJson.getJSONArray("confirmed_unclassified_answers"), + interactionJson.getString("id") + ) + ) + .setDefaultOutcome( + createOutcomeFromJson( + getJsonObject(interactionJson, "default_outcome") + ) + ) + .putAllCustomizationArgs( + createCustomizationArgsMapFromJson( + getJsonObject(interactionJson, "customization_args") + ) + ) + .build() + } + + // Returns a JSON Object if it exists, else returns null + private fun getJsonObject(parentObject: JSONObject, key: String): JSONObject? { + return parentObject.optJSONObject(key) + } + + // Creates the list of answer group objects from JSON + private fun createAnswerGroupsFromJson( + answerGroupsJson: JSONArray?, interactionId: String + ): MutableList { + val answerGroups = mutableListOf() + if (answerGroupsJson == null) { + return answerGroups + } + for (i in 0 until answerGroupsJson.length()) { + answerGroups.add( + createSingleAnswerGroupFromJson( + answerGroupsJson.getJSONObject(i), interactionId + ) + ) + } + return answerGroups + } + + // Creates a single answer group object from JSON + private fun createSingleAnswerGroupFromJson( + answerGroupJson: JSONObject, interactionId: String + ): AnswerGroup { + return AnswerGroup.newBuilder() + .setOutcome( + createOutcomeFromJson(answerGroupJson.getJSONObject("outcome")) + ) + .addAllRuleSpecs( + createRuleSpecsFromJson( + answerGroupJson.getJSONArray("rule_specs"), interactionId + ) + ) + .build() + } + + // Creates an outcome object from JSON + private fun createOutcomeFromJson(outcomeJson: JSONObject?): Outcome { + if (outcomeJson == null) { + return Outcome.getDefaultInstance() + } + return Outcome.newBuilder() + .setDestStateName(outcomeJson.getString("dest")) + .setFeedback( + SubtitledHtml.newBuilder() + .setHtml(outcomeJson.getString("feedback")) + ) + .setLabelledAsCorrect(outcomeJson.getBoolean("labelled_as_correct")) + .build() + } + + // Creates the list of rule spec objects from JSON + private fun createRuleSpecsFromJson( + ruleSpecJson: JSONArray?, interactionId: String + ): MutableList { + val ruleSpecList = mutableListOf() + if (ruleSpecJson == null) { + return ruleSpecList + } + for (i in 0 until ruleSpecJson.length()) { + val ruleSpecBuilder = RuleSpec.newBuilder() + ruleSpecBuilder.ruleType = ruleSpecJson.getJSONObject(i).getString("rule_type") + val inputsJson = ruleSpecJson.getJSONObject(i).getJSONObject("inputs") + val inputKeysIterator = inputsJson.keys() + while (inputKeysIterator.hasNext()) { + val inputName = inputKeysIterator.next() + ruleSpecBuilder.putInput(inputName, createInputFromJson(inputsJson, inputName, interactionId)) + } + ruleSpecList.add(ruleSpecBuilder.build()) + } + return ruleSpecList + } + + // Creates an input interaction object from JSON + private fun createInputFromJson( + inputJson: JSONObject?, keyName: String, interactionId: String + ): InteractionObject { + if (inputJson == null) { + return InteractionObject.getDefaultInstance() + } + return when (interactionId) { + "MultipleChoiceInput" -> InteractionObject.newBuilder() + .setNonNegativeInt(inputJson.getInt(keyName)) + .build() + "TextInput" -> InteractionObject.newBuilder() + .setNormalizedString(inputJson.getString(keyName)) + .build() + "NumericInput" -> InteractionObject.newBuilder() + .setReal(inputJson.getDouble(keyName)) + .build() + else -> throw IllegalStateException("Encountered unexpected interaction ID: $interactionId") + } + } + + // Creates a customization arg mapping from JSON + private fun createCustomizationArgsMapFromJson( + customizationArgsJson: JSONObject? + ): MutableMap { + val customizationArgsMap: MutableMap = mutableMapOf() + if (customizationArgsJson == null) { + return customizationArgsMap + } + val customizationArgsKeys = customizationArgsJson.keys() ?: return customizationArgsMap + val customizationArgsIterator = customizationArgsKeys.iterator() + while (customizationArgsIterator.hasNext()) { + val key = customizationArgsIterator.next() + customizationArgsMap[key] = createCustomizationArgValueFromJson( + customizationArgsJson.getJSONObject(key).get("value") + ) + } + return customizationArgsMap + } + + // Creates a customization arg value interaction object from JSON + private fun createCustomizationArgValueFromJson(customizationArgValue: Any): InteractionObject { + val interactionObjectBuilder = InteractionObject.newBuilder() + when (customizationArgValue) { + is String -> return interactionObjectBuilder + .setNormalizedString(customizationArgValue).build() + is Int -> return interactionObjectBuilder + .setSignedInt(customizationArgValue).build() + is Double -> return interactionObjectBuilder + .setReal(customizationArgValue).build() + is List<*> -> if (customizationArgValue.size > 0) { + return interactionObjectBuilder.setSetOfHtmlString( + createStringList(customizationArgValue) + ).build() + } + } + return InteractionObject.getDefaultInstance() + } + + @Suppress("UNCHECKED_CAST") // Checked cast in the if statement + private fun createStringList(value: List<*>): StringList { + val stringList = mutableListOf() + if (value[0] is String) { + stringList.addAll(value as List) + return StringList.newBuilder().addAllHtml(stringList).build() + } + return StringList.getDefaultInstance() + } +} \ No newline at end of file diff --git a/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt b/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt index 1d8642286cf..a7019dffb2b 100644 --- a/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt @@ -487,9 +487,10 @@ class TopicControllerTest { assertThat(questionListResultCaptor.value.isSuccess()).isTrue() val questionsList = questionListResultCaptor.value.getOrThrow() - assertThat(questionsList.size).isEqualTo(2) - assertThat(questionsList[0].questionId).isEqualTo(TEST_QUESTION_ID_0) - assertThat(questionsList[1].questionId).isEqualTo(TEST_QUESTION_ID_1) + assertThat(questionsList.size).isEqualTo(5) + val questionIds = questionsList.map { it -> it.questionId } + assertThat(questionIds).containsExactlyElementsIn(mutableListOf(TEST_QUESTION_ID_0, TEST_QUESTION_ID_1, + TEST_QUESTION_ID_2, TEST_QUESTION_ID_0, TEST_QUESTION_ID_3)) } @Test From 64b82658cf31518c703932efc85b0a02689fb7c7 Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Fri, 18 Oct 2019 10:48:11 -0700 Subject: [PATCH 15/25] add a new module for questions constants --- domain/src/main/assets/sample_questions.json | 698 ++++++++---------- .../QuestionTrainingConstantsProvider.kt | 17 + 2 files changed, 316 insertions(+), 399 deletions(-) create mode 100644 domain/src/main/java/org/oppia/domain/question/QuestionTrainingConstantsProvider.kt diff --git a/domain/src/main/assets/sample_questions.json b/domain/src/main/assets/sample_questions.json index 102bdcaef41..592a66ff015 100644 --- a/domain/src/main/assets/sample_questions.json +++ b/domain/src/main/assets/sample_questions.json @@ -1,544 +1,444 @@ { - "questions":[ + "questions": [ { - "name":"question", - "classifier_model_id":null, - "content":{ - "html":"

What fraction does 'quarter' represent? 

", - "content_id":"content" + "name": "question", + "classifier_model_id": null, + "content": { + "html": "

What fraction does 'quarter' represent? 

", + "content_id": "content" }, - "interaction":{ - "confirmed_unclassified_answers":[ - - ], - "answer_groups":[ + "interaction": { + "confirmed_unclassified_answers": [], + "answer_groups": [ { - "rule_specs":[ + "rule_specs": [ { - "rule_type":"Contains", - "inputs":{ - "x":"1/4" + "rule_type": "Contains", + "inputs": { + "x": "1/4" } } ], - "outcome":{ - "dest":null, - "feedback":{ - "html":"

That's correct!

", - "content_id":"feedback_1" + "outcome": { + "dest": null, + "feedback": { + "html": "

That's correct!

", + "content_id": "feedback_1" }, - "labelled_as_correct":true, - "param_changes":[ - - ], - "refresher_exploration_id":null, - "missing_prerequisite_skill_id":null + "labelled_as_correct": true, + "param_changes": [], + "refresher_exploration_id": null, + "missing_prerequisite_skill_id": null }, - "trainingData":[ - - ], - "taggedSkillMisconceptionId":null + "trainingData": [], + "taggedSkillMisconceptionId": null } ], - "confirmedUnclassifiedAnswers":[ - - ], - "customizationArgs":{ - "placeholder":{ - "value":"Write your fraction as x/y" + "confirmedUnclassifiedAnswers": [], + "customizationArgs": { + "placeholder": { + "value": "Write your fraction as x/y" }, - "rows":{ - "value":1 + "rows": { + "value": 1 } }, - "defaultOutcome":{ - "dest":null, - "feedback":{ - "html":"", - "content_id":"default_outcome" + "defaultOutcome": { + "dest": null, + "feedback": { + "html": "", + "content_id": "default_outcome" }, - "labelled_as_correct":false, - "param_changes":[ - - ], - "refresher_exploration_id":null, - "missing_prerequisite_skill_id":null + "labelled_as_correct": false, + "param_changes": [], + "refresher_exploration_id": null, + "missing_prerequisite_skill_id": null }, - "hints":[ + "hints": [ { - "hintContent":{ - "html":"

Hint text will appear here

", - "content_id":"hint_1" + "hintContent": { + "html": "

Hint text will appear here

", + "content_id": "hint_1" } } ], - "id":"TextInput", - "solution":{ - "answer_is_exclusive":false, - "correct_answer":"1/4", - "explanation":{ - "html":"

A quarter is 1/4th of something

", - "content_id":"solution" + "id": "TextInput", + "solution": { + "answer_is_exclusive": false, + "correct_answer": "1/4", + "explanation": { + "html": "

A quarter is 1/4th of something

", + "content_id": "solution" } } }, - "param_changes":[ - - ], - "solicit_answer_details":false, - "writtenTranslations":{ - "translationsMapping":{ - "content":{ - - }, - "hint_1":{ - - }, - "feedback_1":{ - - }, - "default_outcome":{ - - }, - "solution":{ - - } + "param_changes": [], + "solicit_answer_details": false, + "writtenTranslations": { + "translationsMapping": { + "content": {}, + "hint_1": {}, + "feedback_1": {}, + "default_outcome": {}, + "solution": {} }, - "_writtenTranslationObjectFactory":{ - - } + "_writtenTranslationObjectFactory": {} } }, { - "name":"question", - "classifier_model_id":null, - "content":{ - "html":"

If we talk about wanting  of a cake, what does the 7 represent?

", - "content_id":"content" + "name": "question", + "classifier_model_id": null, + "content": { + "html": "

If we talk about wanting  of a cake, what does the 7 represent?

", + "content_id": "content" }, - "interaction":{ - "confirmed_unclassified_answers":[ - - ], - "answer_groups":[ + "interaction": { + "confirmed_unclassified_answers": [], + "answer_groups": [ { - "rule_specs":[ + "rule_specs": [ { - "rule_type":"Equals", - "inputs":{ - "x":1 + "rule_type": "Equals", + "inputs": { + "x": 1 } } ], - "outcome":{ - "dest":null, - "feedback":{ - "html":"

That's correct!

", - "content_id":"feedback_1" + "outcome": { + "dest": null, + "feedback": { + "html": "

That's correct!

", + "content_id": "feedback_1" }, - "labelled_as_correct":true, - "param_changes":[ - - ], - "refresher_exploration_id":null, - "missing_prerequisite_skill_id":null + "labelled_as_correct": true, + "param_changes": [], + "refresher_exploration_id": null, + "missing_prerequisite_skill_id": null }, - "trainingData":[ - - ], - "taggedSkillMisconceptionId":null + "trainingData": [], + "taggedSkillMisconceptionId": null } ], - "confirmedUnclassifiedAnswers":[ - - ], - "customizationArgs":{ - "choices":{ - "value":[ + "confirmedUnclassifiedAnswers": [], + "customizationArgs": { + "choices": { + "value": [ "

The number of pieces of cake I want.

", "

The number of pieces the whole cake is cut into.

", "

None of the above.

" ] } }, - "defaultOutcome":{ - "dest":null, - "feedback":{ - "html":"", - "content_id":"default_outcome" + "defaultOutcome": { + "dest": null, + "feedback": { + "html": "", + "content_id": "default_outcome" }, - "labelled_as_correct":false, - "param_changes":[ - - ], - "refresher_exploration_id":null, - "missing_prerequisite_skill_id":null + "labelled_as_correct": false, + "param_changes": [], + "refresher_exploration_id": null, + "missing_prerequisite_skill_id": null }, - "hints":[ + "hints": [ { - "hintContent":{ - "html":"

Hint text will appear here

", - "content_id":"hint_1" + "hintContent": { + "html": "

Hint text will appear here

", + "content_id": "hint_1" } } ], - "id":"MultipleChoiceInput", - "solution":null + "id": "MultipleChoiceInput", + "solution": null }, - "param_changes":[ - - ], - "solicit_answer_details":false + "param_changes": [], + "solicit_answer_details": false }, { - "name":"question", - "classifier_model_id":null, - "content":{ - "html":"

What is the numerator of  equal to?

", - "content_id":"content" + "name": "question", + "classifier_model_id": null, + "content": { + "html": "

What is the numerator of  equal to?

", + "content_id": "content" }, - "interaction":{ - "confirmed_unclassified_answers":[ - - ], - "answer_groups":[ + "interaction": { + "confirmed_unclassified_answers": [], + "answer_groups": [ { - "rule_specs":[ + "rule_specs": [ { - "rule_type":"IsInclusivelyBetween", - "inputs":{ - "a":3, - "b":3 + "rule_type": "IsInclusivelyBetween", + "inputs": { + "a": 3, + "b": 3 } } ], - "outcome":{ - "dest":null, - "feedback":"

That's correct!

", - "labelled_as_correct":true, - "param_changes":[ - - ], - "refresher_exploration_id":null, - "missing_prerequisite_skill_id":null + "outcome": { + "dest": null, + "feedback": "

That's correct!

", + "labelled_as_correct": true, + "param_changes": [], + "refresher_exploration_id": null, + "missing_prerequisite_skill_id": null }, - "trainingData":[ - - ], - "taggedSkillMisconceptionId":null + "trainingData": [], + "taggedSkillMisconceptionId": null } ], - "confirmedUnclassifiedAnswers":[ - - ], - "customizationArgs":{ - - }, - "defaultOutcome":{ - "dest":null, - "feedback":{ - "html":"", - "content_id":"default_outcome" + "confirmedUnclassifiedAnswers": [], + "customizationArgs": {}, + "defaultOutcome": { + "dest": null, + "feedback": { + "html": "", + "content_id": "default_outcome" }, - "labelled_as_correct":false, - "param_changes":[ - - ], - "refresher_exploration_id":null, - "missing_prerequisite_skill_id":null + "labelled_as_correct": false, + "param_changes": [], + "refresher_exploration_id": null, + "missing_prerequisite_skill_id": null }, - "hints":[ + "hints": [ { - "hintContent":{ - "html":"

Hint text will appear here

", - "content_id":"hint_1" + "hintContent": { + "html": "

Hint text will appear here

", + "content_id": "hint_1" } } ], - "id":"NumericInput", - "solution":{ - "answer_is_exclusive":false, - "correct_answer":3, - "explanation":{ - "html":"

The solution comes out to be  and the numerator of that is 3.

", - "content_id":"solution" + "id": "NumericInput", + "solution": { + "answer_is_exclusive": false, + "correct_answer": 3, + "explanation": { + "html": "

The solution comes out to be  and the numerator of that is 3.

", + "content_id": "solution" } } }, - "param_changes":[ - - ], - "solicit_answer_details":false + "param_changes": [], + "solicit_answer_details": false }, { - "name":"question", - "classifier_model_id":null, - "content":{ - "html":"

What fraction does 'half' represent? 

", - "content_id":"content" + "name": "question", + "classifier_model_id": null, + "content": { + "html": "

What fraction does 'half' represent? 

", + "content_id": "content" }, - "interaction":{ - "confirmed_unclassified_answers":[ - - ], - "answer_groups":[ + "interaction": { + "confirmed_unclassified_answers": [], + "answer_groups": [ { - "rule_specs":[ + "rule_specs": [ { - "rule_type":"Contains", - "inputs":{ - "x":"1/2" + "rule_type": "Contains", + "inputs": { + "x": "1/2" } } ], - "outcome":{ - "dest":null, - "feedback":{ - "html":"

That's correct!

", - "content_id":"feedback_1" + "outcome": { + "dest": null, + "feedback": { + "html": "

That's correct!

", + "content_id": "feedback_1" }, - "labelled_as_correct":true, - "param_changes":[ - - ], - "refresher_exploration_id":null, - "missing_prerequisite_skill_id":null + "labelled_as_correct": true, + "param_changes": [], + "refresher_exploration_id": null, + "missing_prerequisite_skill_id": null }, - "trainingData":[ - - ], - "taggedSkillMisconceptionId":null + "trainingData": [], + "taggedSkillMisconceptionId": null } ], - "confirmedUnclassifiedAnswers":[ - - ], - "customizationArgs":{ - "placeholder":{ - "value":"Write your fraction as x/y" + "confirmedUnclassifiedAnswers": [], + "customizationArgs": { + "placeholder": { + "value": "Write your fraction as x/y" }, - "rows":{ - "value":1 + "rows": { + "value": 1 } }, - "defaultOutcome":{ - "dest":null, - "feedback":{ - "html":"", - "content_id":"default_outcome" + "defaultOutcome": { + "dest": null, + "feedback": { + "html": "", + "content_id": "default_outcome" }, - "labelled_as_correct":false, - "param_changes":[ - - ], - "refresher_exploration_id":null, - "missing_prerequisite_skill_id":null + "labelled_as_correct": false, + "param_changes": [], + "refresher_exploration_id": null, + "missing_prerequisite_skill_id": null }, - "hints":[ + "hints": [ { - "hintContent":{ - "html":"

Hint text will appear here

", - "content_id":"hint_1" + "hintContent": { + "html": "

Hint text will appear here

", + "content_id": "hint_1" } } ], - "id":"TextInput", - "solution":{ - "answer_is_exclusive":false, - "correct_answer":"1/2", - "explanation":{ - "html":"

A half is 1/2 of something

", - "content_id":"solution" + "id": "TextInput", + "solution": { + "answer_is_exclusive": false, + "correct_answer": "1/2", + "explanation": { + "html": "

A half is 1/2 of something

", + "content_id": "solution" } } }, - "param_changes":[ - - ], - "solicit_answer_details":false, - "writtenTranslations":{ - "translationsMapping":{ - "content":{ - - }, - "hint_1":{ - - }, - "feedback_1":{ - - }, - "default_outcome":{ - - }, - "solution":{ - - } + "param_changes": [], + "solicit_answer_details": false, + "writtenTranslations": { + "translationsMapping": { + "content": {}, + "hint_1": {}, + "feedback_1": {}, + "default_outcome": {}, + "solution": {} }, - "_writtenTranslationObjectFactory":{ - - } + "_writtenTranslationObjectFactory": {} } }, { - "name":"question", - "classifier_model_id":null, - "content":{ - "html":"

If we talk about wanting  of a cake, what does the 10 represent?

", - "content_id":"content" + "name": "question", + "classifier_model_id": null, + "content": { + "html": "

If we talk about wanting  of a cake, what does the 10 represent?

", + "content_id": "content" }, - "interaction":{ - "confirmed_unclassified_answers":[ - - ], - "answer_groups":[ + "interaction": { + "confirmed_unclassified_answers": [], + "answer_groups": [ { - "rule_specs":[ + "rule_specs": [ { - "rule_type":"Equals", - "inputs":{ - "x":1 + "rule_type": "Equals", + "inputs": { + "x": 1 } } ], - "outcome":{ - "dest":null, - "feedback":{ - "html":"

That's correct!

", - "content_id":"feedback_1" + "outcome": { + "dest": null, + "feedback": { + "html": "

That's correct!

", + "content_id": "feedback_1" }, - "labelled_as_correct":true, - "param_changes":[ - - ], - "refresher_exploration_id":null, - "missing_prerequisite_skill_id":null + "labelled_as_correct": true, + "param_changes": [], + "refresher_exploration_id": null, + "missing_prerequisite_skill_id": null }, - "trainingData":[ - - ], - "taggedSkillMisconceptionId":null + "trainingData": [], + "taggedSkillMisconceptionId": null } ], - "confirmedUnclassifiedAnswers":[ - - ], - "customizationArgs":{ - "choices":{ - "value":[ + "confirmedUnclassifiedAnswers": [], + "customizationArgs": { + "choices": { + "value": [ "

The number of pieces of cake I want.

", "

The number of pieces the whole cake is cut into.

", "

None of the above.

" ] } }, - "defaultOutcome":{ - "dest":null, - "feedback":{ - "html":"", - "content_id":"default_outcome" + "defaultOutcome": { + "dest": null, + "feedback": { + "html": "", + "content_id": "default_outcome" }, - "labelled_as_correct":false, - "param_changes":[ - - ], - "refresher_exploration_id":null, - "missing_prerequisite_skill_id":null + "labelled_as_correct": false, + "param_changes": [], + "refresher_exploration_id": null, + "missing_prerequisite_skill_id": null }, - "hints":[ + "hints": [ { - "hintContent":{ - "html":"

Hint text will appear here

", - "content_id":"hint_1" + "hintContent": { + "html": "

Hint text will appear here

", + "content_id": "hint_1" } } ], - "id":"MultipleChoiceInput", - "solution":null + "id": "MultipleChoiceInput", + "solution": null }, - "param_changes":[ - - ], - "solicit_answer_details":false + "param_changes": [], + "solicit_answer_details": false }, { - "name":"question", - "classifier_model_id":null, - "content":{ - "html":"

What is the numerator of  equal to?

", - "content_id":"content" + "name": "question", + "classifier_model_id": null, + "content": { + "html": "

What is the numerator of  equal to?

", + "content_id": "content" }, - "interaction":{ - "confirmed_unclassified_answers":[ - - ], - "answer_groups":[ + "interaction": { + "confirmed_unclassified_answers": [], + "answer_groups": [ { - "rule_specs":[ + "rule_specs": [ { - "rule_type":"IsInclusivelyBetween", - "inputs":{ - "a":5, - "b":5 + "rule_type": "IsInclusivelyBetween", + "inputs": { + "a": 5, + "b": 5 } } ], - "outcome":{ - "dest":null, - "feedback":"

That's correct!

", - "labelled_as_correct":true, - "param_changes":[ - - ], - "refresher_exploration_id":null, - "missing_prerequisite_skill_id":null + "outcome": { + "dest": null, + "feedback": "

That's correct!

", + "labelled_as_correct": true, + "param_changes": [], + "refresher_exploration_id": null, + "missing_prerequisite_skill_id": null }, - "trainingData":[ - - ], - "taggedSkillMisconceptionId":null + "trainingData": [], + "taggedSkillMisconceptionId": null } ], - "confirmedUnclassifiedAnswers":[ - - ], - "customizationArgs":{ - - }, - "defaultOutcome":{ - "dest":null, - "feedback":{ - "html":"", - "content_id":"default_outcome" + "confirmedUnclassifiedAnswers": [], + "customizationArgs": {}, + "defaultOutcome": { + "dest": null, + "feedback": { + "html": "", + "content_id": "default_outcome" }, - "labelled_as_correct":false, - "param_changes":[ - - ], - "refresher_exploration_id":null, - "missing_prerequisite_skill_id":null + "labelled_as_correct": false, + "param_changes": [], + "refresher_exploration_id": null, + "missing_prerequisite_skill_id": null }, - "hints":[ + "hints": [ { - "hintContent":{ - "html":"

Hint text will appear here

", - "content_id":"hint_1" + "hintContent": { + "html": "

Hint text will appear here

", + "content_id": "hint_1" } } ], - "id":"NumericInput", - "solution":{ - "answer_is_exclusive":false, - "correct_answer":5, - "explanation":{ - "html":"

The solution comes out to be  and the numerator of that is 5.

", - "content_id":"solution" + "id": "NumericInput", + "solution": { + "answer_is_exclusive": false, + "correct_answer": 5, + "explanation": { + "html": "

The solution comes out to be  and the numerator of that is 5.

", + "content_id": "solution" } } }, - "param_changes":[ - - ], - "solicit_answer_details":false + "param_changes": [], + "solicit_answer_details": false } ] } \ No newline at end of file diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingConstantsProvider.kt b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingConstantsProvider.kt new file mode 100644 index 00000000000..9d94d5e3e5d --- /dev/null +++ b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingConstantsProvider.kt @@ -0,0 +1,17 @@ +package org.oppia.domain.question + +import org.oppia.domain.topic.TopicController +import org.oppia.util.data.DataProviders +import javax.inject.Inject +import javax.inject.Singleton + +const val QUESTION_COUNT_PER_TRAINING_SESSION = 10 + +/** Provider to return any constants required during the training session. */ +@Singleton +class QuestionTrainingConstantsProvider @Inject constructor( +) { + fun getQuestionCountPerTrainingSession(): Int { + return QUESTION_COUNT_PER_TRAINING_SESSION + } +} \ No newline at end of file From 4878d10cb2e2661fe5279d091ce8f905dcebb7ce Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Fri, 18 Oct 2019 10:51:43 -0700 Subject: [PATCH 16/25] Review changes --- .../question/QuestionTrainingController.kt | 27 ++--- .../org/oppia/domain/topic/TopicController.kt | 98 ++++++++++++------- .../oppia/domain/util/JsonAssetRetriever.kt | 5 +- .../org/oppia/domain/util/StateRetriever.kt | 8 +- .../QuestionTrainingControllerTest.kt | 19 +++- 5 files changed, 103 insertions(+), 54 deletions(-) diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt index 401d14ea50b..35ea1a2d798 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt @@ -10,7 +10,6 @@ import org.oppia.util.data.DataProviders import javax.inject.Inject import javax.inject.Singleton -const val QUESTION_COUNT_PER_TRAINING_SESSION = 10 const val TRAINING_QUESTIONS_PROVIDER = "TrainingQuestionsProvider" /** Controller for retrieving a set of questions. */ @@ -18,7 +17,8 @@ const val TRAINING_QUESTIONS_PROVIDER = "TrainingQuestionsProvider" class QuestionTrainingController @Inject constructor( private val questionAssessmentProgressController: QuestionAssessmentProgressController, private val topicController: TopicController, - private val dataProviders: DataProviders + private val dataProviders: DataProviders, + private val questionTrainingConstantsProvider: QuestionTrainingConstantsProvider ) { /** * Begins a question training session given a list of skill Ids and a total number of questions. @@ -31,11 +31,13 @@ class QuestionTrainingController @Inject constructor( * @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): LiveData> { + fun startQuestionTrainingSession(skillIdsList: List): LiveData>> { return try { - questionAssessmentProgressController.beginQuestionTrainingSession( - retrieveQuestionsForSkillIds(skillIdsList)) - MutableLiveData(AsyncResult.success(null)) + val retrieveQuestionsDataProvider = retrieveQuestionsForSkillIds(skillIdsList) + questionAssessmentProgressController.beginQuestionTrainingSession( + retrieveQuestionsDataProvider + ) + dataProviders.convertToLiveData(retrieveQuestionsDataProvider) } catch (e: Exception) { MutableLiveData(AsyncResult.failed(e)) } @@ -44,24 +46,27 @@ class QuestionTrainingController @Inject constructor( private fun retrieveQuestionsForSkillIds(skillIdsList: List): DataProvider> { val questionsDataProvider = topicController.retrieveQuestionsForSkillIds(skillIdsList) return dataProviders.transform(TRAINING_QUESTIONS_PROVIDER, questionsDataProvider) { - getFilteredQuestionsForTraining(skillIdsList, it, - QUESTION_COUNT_PER_TRAINING_SESSION / skillIdsList.size) + getFilteredQuestionsForTraining( + skillIdsList, it, + questionTrainingConstantsProvider.getQuestionCountPerTrainingSession() / skillIdsList.size + ) } } // Attempts to fetch equal number of questions per skill. Removes any duplicates and limits the questions to be // equal to TOTAL_QUESTIONS_PER_TOPIC questions. private fun getFilteredQuestionsForTraining( - skillIdsList: List, questionsList: List, numQuestionsPerSkill: Int): List{ + skillIdsList: List, questionsList: List, numQuestionsPerSkill: Int + ): List { val trainingQuestions = mutableListOf() for (skillId in skillIdsList) { trainingQuestions.addAll(questionsList.filter { it.linkedSkillIdsList.contains(skillId) && - !trainingQuestions.contains(it) + !trainingQuestions.contains(it) }.take(numQuestionsPerSkill + 1)) } return trainingQuestions - .take(QUESTION_COUNT_PER_TRAINING_SESSION) + .take(questionTrainingConstantsProvider.getQuestionCountPerTrainingSession()) } /** diff --git a/domain/src/main/java/org/oppia/domain/topic/TopicController.kt b/domain/src/main/java/org/oppia/domain/topic/TopicController.kt index 2a1ee209ccb..b621eb5c078 100644 --- a/domain/src/main/java/org/oppia/domain/topic/TopicController.kt +++ b/domain/src/main/java/org/oppia/domain/topic/TopicController.kt @@ -85,9 +85,9 @@ class TopicController @Inject constructor( } fun retrieveQuestionsForSkillIds(skillIdsList: List): DataProvider> { - return dataProviders.createInMemoryDataProvider(QUESTION_DATA_PROVIDER_ID) { - loadQuestionsForSkillIds(skillIdsList) - } + return dataProviders.createInMemoryDataProvider(QUESTION_DATA_PROVIDER_ID) { + loadQuestionsForSkillIds(skillIdsList) + } } // Loads and returns the questions given a list of skill ids. @@ -98,20 +98,30 @@ class TopicController @Inject constructor( private fun loadQuestions(skillIdsList: List): List { val questionsList = mutableListOf() val questionsJSON = jsonAssetRetriever.loadJsonFromAsset( - "sample_questions.json")?.getJSONArray("questions") + "sample_questions.json" + )?.getJSONArray("questions") for (skillId in skillIdsList) { when (skillId) { - TEST_SKILL_ID_0 -> questionsList.addAll(mutableListOf( - createTestQuestion0(questionsJSON), - createTestQuestion1(questionsJSON), - createTestQuestion2(questionsJSON))) - TEST_SKILL_ID_1 -> questionsList.addAll(mutableListOf( - createTestQuestion0(questionsJSON), - createTestQuestion3(questionsJSON))) - TEST_SKILL_ID_2 -> questionsList.addAll(mutableListOf( - createTestQuestion2(questionsJSON), - createTestQuestion4(questionsJSON), - createTestQuestion5(questionsJSON))) + TEST_SKILL_ID_0 -> questionsList.addAll( + mutableListOf( + createTestQuestion0(questionsJSON), + createTestQuestion1(questionsJSON), + createTestQuestion2(questionsJSON) + ) + ) + TEST_SKILL_ID_1 -> questionsList.addAll( + mutableListOf( + createTestQuestion0(questionsJSON), + createTestQuestion3(questionsJSON) + ) + ) + TEST_SKILL_ID_2 -> questionsList.addAll( + mutableListOf( + createTestQuestion2(questionsJSON), + createTestQuestion4(questionsJSON), + createTestQuestion5(questionsJSON) + ) + ) else -> { throw IllegalStateException("Invalid skill ID: $skillId") } @@ -120,62 +130,78 @@ class TopicController @Inject constructor( return questionsList } - private fun createTestQuestion0(questionsJSON: JSONArray?): Question { + private fun createTestQuestion0(questionsJson: JSONArray?): Question { return Question.newBuilder() .setQuestionId(TEST_QUESTION_ID_0) - .setQuestionState(stateRetriever.createStateFromJson( - "question",questionsJSON?.getJSONObject(0))) - .addAllLinkedSkillIds(mutableListOf(TEST_SKILL_ID_0, TEST_SKILL_ID_1)) + .setQuestionState( + stateRetriever.createStateFromJson( + "question", questionsJson?.getJSONObject(0) + ) + ) + .addAllLinkedSkillIds(mutableListOf(TEST_SKILL_ID_0, TEST_SKILL_ID_1)) .build() } - private fun createTestQuestion1(questionsJSON: JSONArray?): Question { + private fun createTestQuestion1(questionsJson: JSONArray?): Question { return Question.newBuilder() .setQuestionId(TEST_QUESTION_ID_1) - .setQuestionState(stateRetriever.createStateFromJson( - "question",questionsJSON?.getJSONObject(1))) + .setQuestionState( + stateRetriever.createStateFromJson( + "question", questionsJson?.getJSONObject(1) + ) + ) .addAllLinkedSkillIds(mutableListOf(TEST_SKILL_ID_0)) .build() } - private fun createTestQuestion2(questionsJSON: JSONArray?): Question { + private fun createTestQuestion2(questionsJson: JSONArray?): Question { return Question.newBuilder() .setQuestionId(TEST_QUESTION_ID_2) - .setQuestionState(stateRetriever.createStateFromJson( - "question",questionsJSON?.getJSONObject(2))) + .setQuestionState( + stateRetriever.createStateFromJson( + "question", questionsJson?.getJSONObject(2) + ) + ) .addAllLinkedSkillIds(mutableListOf(TEST_SKILL_ID_0, TEST_SKILL_ID_2)) .build() } - private fun createTestQuestion3(questionsJSON: JSONArray?): Question { + private fun createTestQuestion3(questionsJson: JSONArray?): Question { return Question.newBuilder() .setQuestionId(TEST_QUESTION_ID_3) - .setQuestionState(stateRetriever.createStateFromJson( - "question",questionsJSON?.getJSONObject(0))) + .setQuestionState( + stateRetriever.createStateFromJson( + "question", questionsJson?.getJSONObject(0) + ) + ) .addAllLinkedSkillIds(mutableListOf(TEST_SKILL_ID_1)) .build() } - private fun createTestQuestion4(questionsJSON: JSONArray?): Question { + private fun createTestQuestion4(questionsJson: JSONArray?): Question { return Question.newBuilder() .setQuestionId(TEST_QUESTION_ID_4) - .setQuestionState(stateRetriever.createStateFromJson( - "question",questionsJSON?.getJSONObject(1))) + .setQuestionState( + stateRetriever.createStateFromJson( + "question", questionsJson?.getJSONObject(1) + ) + ) .addAllLinkedSkillIds(mutableListOf(TEST_SKILL_ID_2)) .build() } - private fun createTestQuestion5(questionsJSON: JSONArray?): Question { + private fun createTestQuestion5(questionsJson: JSONArray?): Question { return Question.newBuilder() .setQuestionId(TEST_QUESTION_ID_5) - .setQuestionState(stateRetriever.createStateFromJson( - "question",questionsJSON?.getJSONObject(2))) + .setQuestionState( + stateRetriever.createStateFromJson( + "question", questionsJson?.getJSONObject(2) + ) + ) .addAllLinkedSkillIds(mutableListOf(TEST_SKILL_ID_2)) .build() } - - private fun createTestTopic0(): Topic { return Topic.newBuilder() .setTopicId(TEST_TOPIC_ID_0) diff --git a/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt b/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt index 22428b6b9f3..c035e0a1a58 100644 --- a/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt +++ b/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt @@ -5,11 +5,12 @@ import org.json.JSONObject import java.io.IOException import javax.inject.Inject +/** Utility that helps retrieving JSON assets and converting them to a JSON object. */ class JsonAssetRetriever @Inject constructor(private val context: Context) { - // Loads the JSON string from an asset and converts it to a JSONObject + /** Loads the JSON string from an asset and converts it to a JSONObject */ @Throws(IOException::class) - public fun loadJsonFromAsset(assetName: String): JSONObject? { + fun loadJsonFromAsset(assetName: String): JSONObject? { val assetManager = context.assets val jsonContents = assetManager.open(assetName).bufferedReader().use { it.readText() } return JSONObject(jsonContents) diff --git a/domain/src/main/java/org/oppia/domain/util/StateRetriever.kt b/domain/src/main/java/org/oppia/domain/util/StateRetriever.kt index 138fe1c2acc..29a331cba38 100644 --- a/domain/src/main/java/org/oppia/domain/util/StateRetriever.kt +++ b/domain/src/main/java/org/oppia/domain/util/StateRetriever.kt @@ -12,9 +12,11 @@ import org.oppia.app.model.StringList import org.oppia.app.model.SubtitledHtml import javax.inject.Inject +/** Utility that helps create a [State] object given its JSON representation. */ class StateRetriever @Inject constructor() { - // Creates a single state object from JSON - public fun createStateFromJson(stateName: String, stateJson: JSONObject?): State { + + /** Creates a single state object from JSON */ + fun createStateFromJson(stateName: String, stateJson: JSONObject?): State { return State.newBuilder() .setName(stateName) .setContent( @@ -202,4 +204,4 @@ class StateRetriever @Inject constructor() { } return StringList.getDefaultInstance() } -} \ No newline at end of file +} diff --git a/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt index abc670c6080..9a70caf16a3 100644 --- a/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt @@ -32,6 +32,7 @@ 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.Question import org.oppia.domain.topic.TEST_SKILL_ID_0 import org.oppia.domain.topic.TEST_SKILL_ID_1 import org.oppia.util.data.AsyncResult @@ -64,10 +65,10 @@ class QuestionTrainingControllerTest { lateinit var questionTrainingController: QuestionTrainingController @Mock - lateinit var mockQuestionListObserver: Observer> + lateinit var mockQuestionListObserver: Observer>> @Captor - lateinit var questionListResultCaptor: ArgumentCaptor> + lateinit var questionListResultCaptor: ArgumentCaptor>> @Inject @field:TestDispatcher @@ -112,7 +113,10 @@ class QuestionTrainingControllerTest { advanceUntilIdle() questionListLiveData.observeForever(mockQuestionListObserver) verify(mockQuestionListObserver, atLeastOnce()).onChanged(questionListResultCaptor.capture()) + assertThat(questionListResultCaptor.value.isSuccess()).isTrue() + val questionsList = questionListResultCaptor.value.getOrThrow() + assertThat(questionsList.size).isEqualTo(1) } @Qualifier @@ -121,12 +125,23 @@ class QuestionTrainingControllerTest { // TODO(#89): Move this to a common test application component. @Module class TestModule { + @Mock + lateinit var questionsTrainingConstantsProvider: QuestionTrainingConstantsProvider + @Provides @Singleton fun provideContext(application: Application): Context { return application } +// @Provides +// @Singleton +// fun provideQuestionTrainingConstantsProvider(): QuestionTrainingConstantsProvider { +// Mockito.`when`( +// questionsTrainingConstantsProvider.getQuestionCountPerTrainingSession()).thenReturn(1) +// return questionsTrainingConstantsProvider +// } + @ExperimentalCoroutinesApi @Singleton @Provides From c37f14ba9cab3703952a259d7522393602b50c8d Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Fri, 18 Oct 2019 11:30:40 -0700 Subject: [PATCH 17/25] add a test to verify questions were fetched properly --- .../question/QuestionTrainingController.kt | 2 +- .../QuestionTrainingControllerTest.kt | 30 +++++++++++++------ .../org.mockito.plugins.MockMaker | 1 + 3 files changed, 23 insertions(+), 10 deletions(-) create mode 100644 domain/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt index 35ea1a2d798..d0a84158584 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt @@ -63,7 +63,7 @@ class QuestionTrainingController @Inject constructor( trainingQuestions.addAll(questionsList.filter { it.linkedSkillIdsList.contains(skillId) && !trainingQuestions.contains(it) - }.take(numQuestionsPerSkill + 1)) + }.distinctBy { it.questionId }.take(numQuestionsPerSkill + 1)) } return trainingQuestions .take(questionTrainingConstantsProvider.getQuestionCountPerTrainingSession()) diff --git a/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt index 9a70caf16a3..52b86f3203a 100644 --- a/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt @@ -28,11 +28,17 @@ import org.junit.runner.RunWith import org.mockito.ArgumentCaptor import org.mockito.Captor import org.mockito.Mock +import org.mockito.Mockito import org.mockito.Mockito.atLeastOnce import org.mockito.Mockito.verify +import org.mockito.MockitoAnnotations import org.mockito.junit.MockitoJUnit import org.mockito.junit.MockitoRule import org.oppia.app.model.Question +import org.oppia.domain.topic.TEST_QUESTION_ID_0 +import org.oppia.domain.topic.TEST_QUESTION_ID_1 +import org.oppia.domain.topic.TEST_QUESTION_ID_2 +import org.oppia.domain.topic.TEST_QUESTION_ID_3 import org.oppia.domain.topic.TEST_SKILL_ID_0 import org.oppia.domain.topic.TEST_SKILL_ID_1 import org.oppia.util.data.AsyncResult @@ -116,9 +122,14 @@ class QuestionTrainingControllerTest { assertThat(questionListResultCaptor.value.isSuccess()).isTrue() val questionsList = questionListResultCaptor.value.getOrThrow() - assertThat(questionsList.size).isEqualTo(1) + assertThat(questionsList.size).isEqualTo(4) + val questionIds = questionsList.map{it.questionId} + assertThat(questionIds).containsExactlyElementsIn(mutableListOf( + TEST_QUESTION_ID_0, TEST_QUESTION_ID_1, TEST_QUESTION_ID_2, TEST_QUESTION_ID_3)) } + + @Qualifier annotation class TestDispatcher @@ -126,7 +137,7 @@ class QuestionTrainingControllerTest { @Module class TestModule { @Mock - lateinit var questionsTrainingConstantsProvider: QuestionTrainingConstantsProvider + lateinit var questionTrainingConstantsProvider: QuestionTrainingConstantsProvider @Provides @Singleton @@ -134,13 +145,14 @@ class QuestionTrainingControllerTest { return application } -// @Provides -// @Singleton -// fun provideQuestionTrainingConstantsProvider(): QuestionTrainingConstantsProvider { -// Mockito.`when`( -// questionsTrainingConstantsProvider.getQuestionCountPerTrainingSession()).thenReturn(1) -// return questionsTrainingConstantsProvider -// } + @Provides + @Singleton + fun provideQuestionTrainingConstantsProvider(): QuestionTrainingConstantsProvider { + MockitoAnnotations.initMocks(this) + Mockito.`when`( + questionTrainingConstantsProvider.getQuestionCountPerTrainingSession()).thenReturn(10) + return questionTrainingConstantsProvider + } @ExperimentalCoroutinesApi @Singleton diff --git a/domain/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker b/domain/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker new file mode 100644 index 00000000000..ca6ee9cea8e --- /dev/null +++ b/domain/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker @@ -0,0 +1 @@ +mock-maker-inline \ No newline at end of file From d1bd4a60c42e35e3b566b09cd7fbf27b29a887b2 Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Fri, 18 Oct 2019 11:34:08 -0700 Subject: [PATCH 18/25] reformatted code --- .../question/QuestionTrainingControllerTest.kt | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt index 52b86f3203a..6a950f75dfe 100644 --- a/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt @@ -54,7 +54,6 @@ import javax.inject.Qualifier import javax.inject.Singleton import kotlin.coroutines.EmptyCoroutineContext - /** Tests for [QuestionTrainingController]. */ @RunWith(AndroidJUnit4::class) @Config(manifest = Config.NONE) @@ -115,7 +114,8 @@ class QuestionTrainingControllerTest { @ExperimentalCoroutinesApi fun testController_successfullyStartsQuestionSessionForExistingSkillIds() = runBlockingTest(coroutineContext) { val questionListLiveData = questionTrainingController.startQuestionTrainingSession( - listOf(TEST_SKILL_ID_0, TEST_SKILL_ID_1)) + listOf(TEST_SKILL_ID_0, TEST_SKILL_ID_1) + ) advanceUntilIdle() questionListLiveData.observeForever(mockQuestionListObserver) verify(mockQuestionListObserver, atLeastOnce()).onChanged(questionListResultCaptor.capture()) @@ -123,13 +123,14 @@ class QuestionTrainingControllerTest { assertThat(questionListResultCaptor.value.isSuccess()).isTrue() val questionsList = questionListResultCaptor.value.getOrThrow() assertThat(questionsList.size).isEqualTo(4) - val questionIds = questionsList.map{it.questionId} - assertThat(questionIds).containsExactlyElementsIn(mutableListOf( - TEST_QUESTION_ID_0, TEST_QUESTION_ID_1, TEST_QUESTION_ID_2, TEST_QUESTION_ID_3)) + val questionIds = questionsList.map { it.questionId } + assertThat(questionIds).containsExactlyElementsIn( + mutableListOf( + TEST_QUESTION_ID_0, TEST_QUESTION_ID_1, TEST_QUESTION_ID_2, TEST_QUESTION_ID_3 + ) + ) } - - @Qualifier annotation class TestDispatcher @@ -150,7 +151,8 @@ class QuestionTrainingControllerTest { fun provideQuestionTrainingConstantsProvider(): QuestionTrainingConstantsProvider { MockitoAnnotations.initMocks(this) Mockito.`when`( - questionTrainingConstantsProvider.getQuestionCountPerTrainingSession()).thenReturn(10) + questionTrainingConstantsProvider.getQuestionCountPerTrainingSession() + ).thenReturn(10) return questionTrainingConstantsProvider } From 86df525d6d2329049f1d0a5121f1c9f0311918cb Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Sun, 20 Oct 2019 11:22:19 -0700 Subject: [PATCH 19/25] Re-add QuestionTrainingController removed in merge. --- .../question/QuestionTrainingController.kt | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt new file mode 100644 index 00000000000..d0a84158584 --- /dev/null +++ b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt @@ -0,0 +1,85 @@ +package org.oppia.domain.question + +import androidx.lifecycle.LiveData +import androidx.lifecycle.MutableLiveData +import org.oppia.app.model.Question +import org.oppia.domain.topic.TopicController +import org.oppia.util.data.AsyncResult +import org.oppia.util.data.DataProvider +import org.oppia.util.data.DataProviders +import javax.inject.Inject +import javax.inject.Singleton + +const val TRAINING_QUESTIONS_PROVIDER = "TrainingQuestionsProvider" + +/** Controller for retrieving a set of questions. */ +@Singleton +class QuestionTrainingController @Inject constructor( + private val questionAssessmentProgressController: QuestionAssessmentProgressController, + private val topicController: TopicController, + private val dataProviders: DataProviders, + private val questionTrainingConstantsProvider: QuestionTrainingConstantsProvider +) { + /** + * 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): LiveData>> { + return try { + val retrieveQuestionsDataProvider = retrieveQuestionsForSkillIds(skillIdsList) + questionAssessmentProgressController.beginQuestionTrainingSession( + retrieveQuestionsDataProvider + ) + dataProviders.convertToLiveData(retrieveQuestionsDataProvider) + } catch (e: Exception) { + MutableLiveData(AsyncResult.failed(e)) + } + } + + private fun retrieveQuestionsForSkillIds(skillIdsList: List): DataProvider> { + val questionsDataProvider = topicController.retrieveQuestionsForSkillIds(skillIdsList) + return dataProviders.transform(TRAINING_QUESTIONS_PROVIDER, questionsDataProvider) { + getFilteredQuestionsForTraining( + skillIdsList, it, + questionTrainingConstantsProvider.getQuestionCountPerTrainingSession() / skillIdsList.size + ) + } + } + + // Attempts to fetch equal number of questions per skill. Removes any duplicates and limits the questions to be + // equal to TOTAL_QUESTIONS_PER_TOPIC questions. + private fun getFilteredQuestionsForTraining( + skillIdsList: List, questionsList: List, numQuestionsPerSkill: Int + ): List { + val trainingQuestions = mutableListOf() + for (skillId in skillIdsList) { + trainingQuestions.addAll(questionsList.filter { + it.linkedSkillIdsList.contains(skillId) && + !trainingQuestions.contains(it) + }.distinctBy { it.questionId }.take(numQuestionsPerSkill + 1)) + } + return trainingQuestions + .take(questionTrainingConstantsProvider.getQuestionCountPerTrainingSession()) + } + + /** + * 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> { + return try { + questionAssessmentProgressController.finishQuestionTrainingSession() + MutableLiveData(AsyncResult.success(null)) + } catch (e: Exception) { + MutableLiveData(AsyncResult.failed(e)) + } + } +} From 54340e68c86891cb9587b2efccd6f4f6f84c8e44 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Sun, 20 Oct 2019 12:15:16 -0700 Subject: [PATCH 20/25] Finalize question progress controller interface. --- .../QuestionAssessmentProgressController.kt | 54 ++++++-- .../question/QuestionTrainingController.kt | 26 ++-- ...uestionAssessmentProgressControllerTest.kt | 131 +++++++++++++++++- .../QuestionTrainingControllerTest.kt | 23 ++- 4 files changed, 212 insertions(+), 22 deletions(-) diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt index ec392513291..da88acb2166 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt @@ -11,9 +11,13 @@ import org.oppia.app.model.Question import org.oppia.app.model.SubtitledHtml import org.oppia.util.data.AsyncResult import org.oppia.util.data.DataProvider +import org.oppia.util.data.DataProviders import javax.inject.Inject import javax.inject.Singleton +private const val EPHEMERAL_QUESTION_DATA_PROVIDER_ID = "EphemeralQuestionDataProvider" +private const val EMPTY_QUESTIONS_LIST_DATA_PROVIDER_ID = "EmptyQuestionsListDataProvider" + /** * Controller that tracks and reports the learner's ephemeral/non-persisted progress through a practice training * session. Note that this controller only supports one active training session at a time. @@ -24,18 +28,25 @@ import javax.inject.Singleton * that uses of this class do not specifically depend on ordering. */ @Singleton -class QuestionAssessmentProgressController @Inject constructor() { - private lateinit var inProgressQuestionList: List +class QuestionAssessmentProgressController @Inject constructor(private val dataProviders: DataProviders) { + private var inProgressQuestionsListDataProvider: DataProvider> = createEmptyQuestionsListDataProvider() private var playing: Boolean = false + private val ephemeralQuestionDataSource: DataProvider by lazy { + dataProviders.transformAsync( + EPHEMERAL_QUESTION_DATA_PROVIDER_ID, inProgressQuestionsListDataProvider, this::computeEphemeralQuestionStateAsync + ) + } - internal fun beginQuestionTrainingSession(questionsList: DataProvider>) { + internal fun beginQuestionTrainingSession(questionsListDataProvider: DataProvider>) { check(!playing) { "Cannot start a new training session until the previous one is completed" } - check(questionsList.isNotEmpty()) { "Cannot start a training session with zero questions." } - inProgressQuestionList = questionsList + inProgressQuestionsListDataProvider = questionsListDataProvider + playing = true } internal fun finishQuestionTrainingSession() { check(playing) { "Cannot stop a new training session which wasn't started" } + playing = false + inProgressQuestionsListDataProvider = createEmptyQuestionsListDataProvider() } /** @@ -126,15 +137,38 @@ class QuestionAssessmentProgressController @Inject constructor() { * return a pending state. */ fun getCurrentQuestion(): LiveData> { - val currentQuestion = inProgressQuestionList.first() - val ephemeralQuestion = EphemeralQuestion.newBuilder() + return dataProviders.convertToLiveData(ephemeralQuestionDataSource) + } + + @Suppress("RedundantSuspendModifier") // 'suspend' expected by DataProviders. + private suspend fun computeEphemeralQuestionStateAsync( + questionsList: List + ): AsyncResult { + if (!playing) { + return AsyncResult.pending() + } + return try { + AsyncResult.success(computeEphemeralQuestionState(questionsList)) + } catch (e: Exception) { + AsyncResult.failed(e) + } + } + + private fun computeEphemeralQuestionState(questionsList: List): EphemeralQuestion { + check(questionsList.isNotEmpty()) { "Cannot start a training session with zero questions." } + val currentQuestion = questionsList.first() + return EphemeralQuestion.newBuilder() .setEphemeralState(EphemeralState.newBuilder() .setState(currentQuestion.questionState) .setPendingState(PendingState.getDefaultInstance())) .setCurrentQuestionIndex(0) - .setTotalQuestionCount(inProgressQuestionList.size) - .setInitialTotalQuestionCount(inProgressQuestionList.size) + .setTotalQuestionCount(questionsList.size) + .setInitialTotalQuestionCount(questionsList.size) .build() - return MutableLiveData(AsyncResult.success(ephemeralQuestion)) + } + + /** Returns a temporary [DataProvider] that always provides an empty list of [Question]s. */ + private fun createEmptyQuestionsListDataProvider(): DataProvider> { + return dataProviders.createInMemoryDataProvider(EMPTY_QUESTIONS_LIST_DATA_PROVIDER_ID) { listOf() } } } diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt index d0a84158584..a16540377fc 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt @@ -10,7 +10,8 @@ import org.oppia.util.data.DataProviders import javax.inject.Inject import javax.inject.Singleton -const val TRAINING_QUESTIONS_PROVIDER = "TrainingQuestionsProvider" +private const val TRAINING_QUESTIONS_PROVIDER_ID = "TrainingQuestionsProvider" +private const val START_QUESTION_TRAINING_SESSION_DATA_PROVIDER_ID = "StartQuestionTrainingSessionDataProvider" /** Controller for retrieving a set of questions. */ @Singleton @@ -31,13 +32,16 @@ class QuestionTrainingController @Inject constructor( * @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): LiveData>> { + fun startQuestionTrainingSession(skillIdsList: List): LiveData> { return try { val retrieveQuestionsDataProvider = retrieveQuestionsForSkillIds(skillIdsList) questionAssessmentProgressController.beginQuestionTrainingSession( retrieveQuestionsDataProvider ) - dataProviders.convertToLiveData(retrieveQuestionsDataProvider) + val hiddenTypeDataProvider: DataProvider = dataProviders.transform( + START_QUESTION_TRAINING_SESSION_DATA_PROVIDER_ID, retrieveQuestionsDataProvider + ) { null } + dataProviders.convertToLiveData(hiddenTypeDataProvider) } catch (e: Exception) { MutableLiveData(AsyncResult.failed(e)) } @@ -45,14 +49,20 @@ class QuestionTrainingController @Inject constructor( private fun retrieveQuestionsForSkillIds(skillIdsList: List): DataProvider> { val questionsDataProvider = topicController.retrieveQuestionsForSkillIds(skillIdsList) - return dataProviders.transform(TRAINING_QUESTIONS_PROVIDER, questionsDataProvider) { - getFilteredQuestionsForTraining( - skillIdsList, it, - questionTrainingConstantsProvider.getQuestionCountPerTrainingSession() / skillIdsList.size - ) + return dataProviders.transform(TRAINING_QUESTIONS_PROVIDER_ID, questionsDataProvider) { + val questionsPerSkill = + if (skillIdsList.isNotEmpty()) + questionTrainingConstantsProvider.getQuestionCountPerTrainingSession() / skillIdsList.size + else 0 + getFilteredQuestionsForTraining(skillIdsList, it, questionsPerSkill) } } + /** Returns a [LiveData] representing a valid question assessment generated from the list of specified skill IDs. */ + fun generateQuestionTrainingSession(skillIdsList: List): LiveData>> { + return dataProviders.convertToLiveData(retrieveQuestionsForSkillIds(skillIdsList)) + } + // Attempts to fetch equal number of questions per skill. Removes any duplicates and limits the questions to be // equal to TOTAL_QUESTIONS_PER_TOPIC questions. private fun getFilteredQuestionsForTraining( diff --git a/domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.kt b/domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.kt index 100942e3a8a..e6a0557be5b 100644 --- a/domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.kt @@ -2,6 +2,7 @@ package org.oppia.domain.question 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 @@ -12,9 +13,24 @@ import dagger.Provides import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.TestCoroutineDispatcher +import kotlinx.coroutines.test.runBlockingTest 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.app.model.EphemeralQuestion +import org.oppia.app.model.EphemeralState.StateTypeCase.PENDING_STATE +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.threading.BackgroundDispatcher import org.oppia.util.threading.BlockingDispatcher import org.robolectric.annotation.Config @@ -27,6 +43,13 @@ import kotlin.coroutines.EmptyCoroutineContext @RunWith(AndroidJUnit4::class) @Config(manifest = Config.NONE) class QuestionAssessmentProgressControllerTest { + private val TEST_SKILL_ID_LIST_012 = listOf(TEST_SKILL_ID_0, TEST_SKILL_ID_1, TEST_SKILL_ID_2) + private val TEST_SKILL_ID_LIST_02 = listOf(TEST_SKILL_ID_0, TEST_SKILL_ID_2) + + @Rule + @JvmField + val mockitoRule: MockitoRule = MockitoJUnit.rule() + @Inject lateinit var questionTrainingController: QuestionTrainingController @@ -37,6 +60,18 @@ class QuestionAssessmentProgressControllerTest { @field:TestDispatcher lateinit var testDispatcher: CoroutineDispatcher + @Mock + lateinit var mockCurrentQuestionLiveDataObserver: Observer> + + @Mock + lateinit var mockAsyncResultLiveDataObserver: Observer> + + @Captor + lateinit var currentQuestionResultCaptor: ArgumentCaptor> + + @Captor + lateinit var asyncResultCaptor: ArgumentCaptor> + private val coroutineContext by lazy { EmptyCoroutineContext + testDispatcher } @@ -47,9 +82,101 @@ class QuestionAssessmentProgressControllerTest { } // TODO(#111): Add tests for the controller once there's a real controller to test. + + @Test + @ExperimentalCoroutinesApi + fun testStartTrainingSession_succeeds() = runBlockingTest(coroutineContext) { + val resultLiveData = questionTrainingController.startQuestionTrainingSession(TEST_SKILL_ID_LIST_012) + resultLiveData.observeForever(mockAsyncResultLiveDataObserver) + advanceUntilIdle() + + verify(mockAsyncResultLiveDataObserver).onChanged(asyncResultCaptor.capture()) + assertThat(asyncResultCaptor.value.isSuccess()).isTrue() + } + + @Test + @ExperimentalCoroutinesApi + fun testStopTrainingSession_afterStartingPreviousSession_succeeds() = runBlockingTest(coroutineContext) { + questionTrainingController.startQuestionTrainingSession(TEST_SKILL_ID_LIST_012) + + val resultLiveData = questionTrainingController.stopQuestionTrainingSession() + advanceUntilIdle() + + assertThat(resultLiveData.value).isNotNull() + assertThat(resultLiveData.value!!.isSuccess()).isTrue() + } + + @Test + @ExperimentalCoroutinesApi + fun testStartTrainingSession_afterStartingPreviousSession_fails() = runBlockingTest(coroutineContext) { + questionTrainingController.startQuestionTrainingSession(TEST_SKILL_ID_LIST_012) + + val resultLiveData = questionTrainingController.startQuestionTrainingSession(TEST_SKILL_ID_LIST_02) + advanceUntilIdle() + + assertThat(resultLiveData.value).isNotNull() + assertThat(resultLiveData.value!!.isFailure()).isTrue() + assertThat(resultLiveData.value!!.getErrorOrNull()) + .hasMessageThat() + .contains("Cannot start a new training session until the previous one is completed") + } + + @Test + @ExperimentalCoroutinesApi + fun testStopTrainingSession_withoutStartingSession_fails() = runBlockingTest(coroutineContext) { + val resultLiveData = questionTrainingController.stopQuestionTrainingSession() + advanceUntilIdle() + + assertThat(resultLiveData.value).isNotNull() + assertThat(resultLiveData.value!!.isFailure()).isTrue() + assertThat(resultLiveData.value!!.getErrorOrNull()) + .hasMessageThat() + .contains("Cannot stop a new training session which wasn't started") + } + + @Test + @ExperimentalCoroutinesApi + fun testGetCurrentQuestion_noSessionStarted_returnsPendingResult() = runBlockingTest(coroutineContext) { + val resultLiveData = questionAssessmentProgressController.getCurrentQuestion() + resultLiveData.observeForever(mockCurrentQuestionLiveDataObserver) + advanceUntilIdle() + + verify(mockCurrentQuestionLiveDataObserver).onChanged(currentQuestionResultCaptor.capture()) + assertThat(currentQuestionResultCaptor.value.isPending()).isTrue() + } + + @Test + @ExperimentalCoroutinesApi + fun testGetCurrentQuestion_sessionStarted_withEmptyQuestionList_fails() = runBlockingTest(coroutineContext) { + questionTrainingController.startQuestionTrainingSession(listOf()) + + val resultLiveData = questionAssessmentProgressController.getCurrentQuestion() + resultLiveData.observeForever(mockCurrentQuestionLiveDataObserver) + advanceUntilIdle() + + verify(mockCurrentQuestionLiveDataObserver, atLeastOnce()).onChanged(currentQuestionResultCaptor.capture()) + assertThat(currentQuestionResultCaptor.value.isFailure()).isTrue() + assertThat(currentQuestionResultCaptor.value.getErrorOrNull()) + .hasMessageThat() + .contains("Cannot start a training session with zero questions.") + } + @Test - fun testNothing_succeeds() { - assertThat(true).isTrue() + @ExperimentalCoroutinesApi + fun testGetCurrentQuestion_sessionStarted_returnsInitialQuestion() = runBlockingTest(coroutineContext) { + questionTrainingController.startQuestionTrainingSession(TEST_SKILL_ID_LIST_012) + + val resultLiveData = questionAssessmentProgressController.getCurrentQuestion() + resultLiveData.observeForever(mockCurrentQuestionLiveDataObserver) + advanceUntilIdle() + + verify(mockCurrentQuestionLiveDataObserver, atLeastOnce()).onChanged(currentQuestionResultCaptor.capture()) + assertThat(currentQuestionResultCaptor.value.isSuccess()).isTrue() + val ephemeralQuestion = currentQuestionResultCaptor.value.getOrThrow() + assertThat(ephemeralQuestion.currentQuestionIndex).isEqualTo(0) + assertThat(ephemeralQuestion.totalQuestionCount).isEqualTo(6) + assertThat(ephemeralQuestion.ephemeralState.stateTypeCase).isEqualTo(PENDING_STATE) + assertThat(ephemeralQuestion.ephemeralState.state.content.html).contains("What fraction does 'quarter'") } private fun setUpTestApplicationComponent() { diff --git a/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt index 6a950f75dfe..75d7fb400f2 100644 --- a/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt @@ -69,9 +69,15 @@ class QuestionTrainingControllerTest { @Inject lateinit var questionTrainingController: QuestionTrainingController + @Mock + lateinit var mockAsyncResultLiveDataObserver: Observer> + @Mock lateinit var mockQuestionListObserver: Observer>> + @Captor + lateinit var asyncResultCaptor: ArgumentCaptor> + @Captor lateinit var questionListResultCaptor: ArgumentCaptor>> @@ -112,14 +118,27 @@ class QuestionTrainingControllerTest { @Test @ExperimentalCoroutinesApi - fun testController_successfullyStartsQuestionSessionForExistingSkillIds() = runBlockingTest(coroutineContext) { + fun testStartQuestionSession_forExistingSkillIds_succeeds() = runBlockingTest(coroutineContext) { val questionListLiveData = questionTrainingController.startQuestionTrainingSession( listOf(TEST_SKILL_ID_0, TEST_SKILL_ID_1) ) + questionListLiveData.observeForever(mockAsyncResultLiveDataObserver) advanceUntilIdle() + + verify(mockAsyncResultLiveDataObserver, atLeastOnce()).onChanged(asyncResultCaptor.capture()) + assertThat(asyncResultCaptor.value.isSuccess()).isTrue() + } + + @Test + @ExperimentalCoroutinesApi + fun testGenerateQuestionSession_forExistingSkillIds_providesValidAssessment() = runBlockingTest(coroutineContext) { + val questionListLiveData = questionTrainingController.generateQuestionTrainingSession( + listOf(TEST_SKILL_ID_0, TEST_SKILL_ID_1) + ) questionListLiveData.observeForever(mockQuestionListObserver) - verify(mockQuestionListObserver, atLeastOnce()).onChanged(questionListResultCaptor.capture()) + advanceUntilIdle() + verify(mockQuestionListObserver, atLeastOnce()).onChanged(questionListResultCaptor.capture()) assertThat(questionListResultCaptor.value.isSuccess()).isTrue() val questionsList = questionListResultCaptor.value.getOrThrow() assertThat(questionsList.size).isEqualTo(4) From 73372fe975f9c7ef129eccb7aabf6b51f6914a35 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Sun, 27 Oct 2019 21:50:34 -0700 Subject: [PATCH 21/25] Fix typo in QuestionTrainingController. --- .../org/oppia/domain/question/QuestionTrainingController.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt index 0f84488da86..ac775d4fc5b 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt @@ -60,7 +60,7 @@ class QuestionTrainingController @Inject constructor( // Attempts to fetch equal number of questions per skill. Removes any duplicates and limits the questions to be // equal to TOTAL_QUESTIONS_PER_TOPIC questions. private fun getFilteredQuestionsForTraining( - skillIdsList: List, questionsList: List, numQuestionsPerSkill: Int + skillIdsList: List, questionsList: List, numQuestionsPerSkill: Int ): List { val trainingQuestions = mutableListOf() for (skillId in skillIdsList) { From 8d29dd1433d88e421552bda1c0133c86d17e7cae Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Sun, 27 Oct 2019 22:07:38 -0700 Subject: [PATCH 22/25] Post-merge fixes and adjustments. --- .../question/QuestionTrainingController.kt | 22 ++++++++++------ ...uestionAssessmentProgressControllerTest.kt | 20 ++++++++++++--- .../QuestionTrainingControllerTest.kt | 25 ++++++------------- 3 files changed, 38 insertions(+), 29 deletions(-) diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt index ac775d4fc5b..b4b8e4ac6bd 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt @@ -11,7 +11,8 @@ import javax.inject.Inject import javax.inject.Singleton import kotlin.random.Random -const val TRAINING_QUESTIONS_PROVIDER = "TrainingQuestionsProvider" +private const val TRAINING_QUESTIONS_PROVIDER = "TrainingQuestionsProvider" +private const val RETRIEVE_QUESTIONS_RESULT_DATA_PROVIDER = "RetrieveQuestionsResultsProvider" /** Controller for retrieving a set of questions. */ @Singleton @@ -35,13 +36,16 @@ class QuestionTrainingController @Inject constructor( * @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): LiveData>> { + fun startQuestionTrainingSession(skillIdsList: List): LiveData> { return try { val retrieveQuestionsDataProvider = retrieveQuestionsForSkillIds(skillIdsList) questionAssessmentProgressController.beginQuestionTrainingSession( retrieveQuestionsDataProvider ) - dataProviders.convertToLiveData(retrieveQuestionsDataProvider) + val erasedDataProvider: DataProvider = dataProviders.transform( + RETRIEVE_QUESTIONS_RESULT_DATA_PROVIDER, retrieveQuestionsDataProvider + ) { it } + dataProviders.convertToLiveData(erasedDataProvider) } catch (e: Exception) { MutableLiveData(AsyncResult.failed(e)) } @@ -50,10 +54,14 @@ class QuestionTrainingController @Inject constructor( private fun retrieveQuestionsForSkillIds(skillIdsList: List): DataProvider> { val questionsDataProvider = topicController.retrieveQuestionsForSkillIds(skillIdsList) return dataProviders.transform(TRAINING_QUESTIONS_PROVIDER, questionsDataProvider) { - getFilteredQuestionsForTraining( - skillIdsList, it.shuffled(random), - questionCountPerSession / skillIdsList.size - ) + if (skillIdsList.isEmpty()) { + listOf() + } else { + getFilteredQuestionsForTraining( + skillIdsList, it.shuffled(random), + questionCountPerSession / skillIdsList.size + ) + } } } diff --git a/domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.kt b/domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.kt index e6a0557be5b..7adca5ce22a 100644 --- a/domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.kt @@ -64,13 +64,13 @@ class QuestionAssessmentProgressControllerTest { lateinit var mockCurrentQuestionLiveDataObserver: Observer> @Mock - lateinit var mockAsyncResultLiveDataObserver: Observer> + lateinit var mockAsyncResultLiveDataObserver: Observer> @Captor lateinit var currentQuestionResultCaptor: ArgumentCaptor> @Captor - lateinit var asyncResultCaptor: ArgumentCaptor> + lateinit var asyncResultCaptor: ArgumentCaptor> private val coroutineContext by lazy { EmptyCoroutineContext + testDispatcher @@ -174,9 +174,9 @@ class QuestionAssessmentProgressControllerTest { assertThat(currentQuestionResultCaptor.value.isSuccess()).isTrue() val ephemeralQuestion = currentQuestionResultCaptor.value.getOrThrow() assertThat(ephemeralQuestion.currentQuestionIndex).isEqualTo(0) - assertThat(ephemeralQuestion.totalQuestionCount).isEqualTo(6) + assertThat(ephemeralQuestion.totalQuestionCount).isEqualTo(3) assertThat(ephemeralQuestion.ephemeralState.stateTypeCase).isEqualTo(PENDING_STATE) - assertThat(ephemeralQuestion.ephemeralState.state.content.html).contains("What fraction does 'quarter'") + assertThat(ephemeralQuestion.ephemeralState.state.content.html).contains("What is the numerator") } private fun setUpTestApplicationComponent() { @@ -192,6 +192,18 @@ class QuestionAssessmentProgressControllerTest { // TODO(#89): Move this to a common test application component. @Module class TestModule { + companion object { + var questionSeed = 0L + } + + @Provides + @QuestionCountPerTrainingSession + fun provideQuestionCountPerTrainingSession(): Int = 3 + + @Provides + @QuestionTrainingSeed + fun provideQuestionTrainingSeed(): Long = questionSeed ++ + @Provides @Singleton fun provideContext(application: Application): Context { diff --git a/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt index a7705333ee5..528ac7c1c4c 100644 --- a/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt @@ -70,10 +70,10 @@ class QuestionTrainingControllerTest { lateinit var questionTrainingController: QuestionTrainingController @Mock - lateinit var mockQuestionListObserver: Observer>> + lateinit var mockQuestionListObserver: Observer> @Captor - lateinit var questionListResultCaptor: ArgumentCaptor>> + lateinit var questionListResultCaptor: ArgumentCaptor> @Inject @field:TestDispatcher @@ -112,7 +112,7 @@ class QuestionTrainingControllerTest { @Test @ExperimentalCoroutinesApi - fun testStartQuestionSession_forExistingSkillIds_succeeds() = runBlockingTest(coroutineContext) { + fun testController_successfullyStartsQuestionSessionForExistingSkillIds() = runBlockingTest(coroutineContext) { val questionListLiveData = questionTrainingController.startQuestionTrainingSession( listOf(TEST_SKILL_ID_0, TEST_SKILL_ID_1) ) @@ -121,7 +121,8 @@ class QuestionTrainingControllerTest { verify(mockQuestionListObserver, atLeastOnce()).onChanged(questionListResultCaptor.capture()) assertThat(questionListResultCaptor.value.isSuccess()).isTrue() - val questionsList = questionListResultCaptor.value.getOrThrow() + @Suppress("UNCHECKED_CAST") // TODO(#111): Observe this via the progress controller, instead. + val questionsList = questionListResultCaptor.value.getOrThrow() as List assertThat(questionsList.size).isEqualTo(3) val questionIds = questionsList.map { it.questionId } assertThat(questionIds).containsExactlyElementsIn( @@ -138,24 +139,12 @@ class QuestionTrainingControllerTest { listOf(TEST_SKILL_ID_0, TEST_SKILL_ID_1) ) advanceUntilIdle() - - verify(mockAsyncResultLiveDataObserver, atLeastOnce()).onChanged(asyncResultCaptor.capture()) - assertThat(asyncResultCaptor.value.isSuccess()).isTrue() - } - - @Test - @ExperimentalCoroutinesApi - fun testGenerateQuestionSession_forExistingSkillIds_providesValidAssessment() = runBlockingTest(coroutineContext) { - val questionListLiveData = questionTrainingController.generateQuestionTrainingSession( - listOf(TEST_SKILL_ID_0, TEST_SKILL_ID_1) - ) questionListLiveData.observeForever(mockQuestionListObserver) - advanceUntilIdle() - verify(mockQuestionListObserver, atLeastOnce()).onChanged(questionListResultCaptor.capture()) assertThat(questionListResultCaptor.value.isSuccess()).isTrue() - val questionsList = questionListResultCaptor.value.getOrThrow() + @Suppress("UNCHECKED_CAST") // TODO(#111): Observe this via the progress controller, instead. + val questionsList = questionListResultCaptor.value.getOrThrow() as List assertThat(questionsList.size).isEqualTo(3) val questionIds = questionsList.map { it.questionId } assertThat(questionIds).containsExactlyElementsIn( From d0f848736c4fa50a02e798723d25b744632fb6d4 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Tue, 19 Nov 2019 16:28:08 -0800 Subject: [PATCH 23/25] Post-merge fixes & address reviewer comments. --- .../QuestionAssessmentProgressController.kt | 38 ++-- ...uestionAssessmentProgressControllerTest.kt | 12 ++ .../question/QuestionDataControllerTest.kt | 194 ------------------ .../QuestionTrainingControllerTest.kt | 12 +- 4 files changed, 42 insertions(+), 214 deletions(-) delete mode 100644 domain/src/test/java/org/oppia/domain/question/QuestionDataControllerTest.kt diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt index 9fab7ebe1c0..600b340deb5 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionAssessmentProgressController.kt @@ -10,9 +10,13 @@ import org.oppia.app.model.PendingState import org.oppia.app.model.Question import org.oppia.app.model.SubtitledHtml import org.oppia.util.data.AsyncResult +import org.oppia.util.data.DataProvider +import org.oppia.util.data.DataProviders import javax.inject.Inject import javax.inject.Singleton +private const val CURRENT_QUESTION_DATA_PROVIDER = "CurrentQuestionDataProvider" + /** * Controller that tracks and reports the learner's ephemeral/non-persisted progress through a practice training * session. Note that this controller only supports one active training session at a time. @@ -23,14 +27,13 @@ import javax.inject.Singleton * that uses of this class do not specifically depend on ordering. */ @Singleton -class QuestionAssessmentProgressController @Inject constructor() { - private lateinit var inProgressQuestionList: List +class QuestionAssessmentProgressController @Inject constructor(private val dataProviders: DataProviders) { + private lateinit var inProgressQuestionListDataProvider: DataProvider> private var playing: Boolean = false - internal fun beginQuestionTrainingSession(questionsList: List) { + internal fun beginQuestionTrainingSession(questionsListDataProvider: DataProvider>) { check(!playing) { "Cannot start a new training session until the previous one is completed" } - check(questionsList.isNotEmpty()) { "Cannot start a training session with zero questions." } - inProgressQuestionList = questionsList + inProgressQuestionListDataProvider = questionsListDataProvider } internal fun finishQuestionTrainingSession() { @@ -125,15 +128,20 @@ class QuestionAssessmentProgressController @Inject constructor() { * return a pending state. */ fun getCurrentQuestion(): LiveData> { - val currentQuestion = inProgressQuestionList.first() - val ephemeralQuestion = EphemeralQuestion.newBuilder() - .setEphemeralState(EphemeralState.newBuilder() - .setState(currentQuestion.questionState) - .setPendingState(PendingState.getDefaultInstance())) - .setCurrentQuestionIndex(0) - .setTotalQuestionCount(inProgressQuestionList.size) - .setInitialTotalQuestionCount(inProgressQuestionList.size) - .build() - return MutableLiveData(AsyncResult.success(ephemeralQuestion)) + val questionDataProvider = dataProviders.transform( + CURRENT_QUESTION_DATA_PROVIDER, inProgressQuestionListDataProvider) { questionsList -> + check(questionsList.isNotEmpty()) { "Cannot start a training session with zero questions." } + + val currentQuestion = questionsList.first() + EphemeralQuestion.newBuilder() + .setEphemeralState(EphemeralState.newBuilder() + .setState(currentQuestion.questionState) + .setPendingState(PendingState.getDefaultInstance())) + .setCurrentQuestionIndex(0) + .setTotalQuestionCount(questionsList.size) + .setInitialTotalQuestionCount(questionsList.size) + .build() + } + return dataProviders.convertToLiveData(questionDataProvider) } } diff --git a/domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.kt b/domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.kt index 100942e3a8a..e6391519df9 100644 --- a/domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.kt @@ -65,6 +65,18 @@ class QuestionAssessmentProgressControllerTest { // TODO(#89): Move this to a common test application component. @Module class TestModule { + companion object { + var questionSeed = 0L + } + + @Provides + @QuestionCountPerTrainingSession + fun provideQuestionCountPerTrainingSession(): Int = 3 + + @Provides + @QuestionTrainingSeed + fun provideQuestionTrainingSeed(): Long = questionSeed++ + @Provides @Singleton fun provideContext(application: Application): Context { diff --git a/domain/src/test/java/org/oppia/domain/question/QuestionDataControllerTest.kt b/domain/src/test/java/org/oppia/domain/question/QuestionDataControllerTest.kt deleted file mode 100644 index 4f02e3740fa..00000000000 --- a/domain/src/test/java/org/oppia/domain/question/QuestionDataControllerTest.kt +++ /dev/null @@ -1,194 +0,0 @@ -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.app.model.Question -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 QuestionDataControllerTest { - @Rule - @JvmField - val mockitoRule: MockitoRule = MockitoJUnit.rule() - - @Rule - @JvmField - val executorRule = InstantTaskExecutorRule() - - @Inject - lateinit var questionTrainingController: QuestionTrainingController - - @Mock - lateinit var mockQuestionListObserver: Observer>> - - @Captor - lateinit var questionListResultCaptor: ArgumentCaptor>> - - @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() { - DaggerQuestionDataControllerTest_TestApplicationComponent.builder() - .setApplication(ApplicationProvider.getApplicationContext()) - .build() - .inject(this) - } - - @Test - @ExperimentalCoroutinesApi - fun testController_providesInitialLiveDataForTopicId0() = runBlockingTest(coroutineContext) { - val questionListLiveData = questionTrainingController.getQuestionsForTopic(TEST_TOPIC_ID_0) - advanceUntilIdle() - questionListLiveData.observeForever(mockQuestionListObserver) - - verify(mockQuestionListObserver, atLeastOnce()).onChanged(questionListResultCaptor.capture()) - assertThat(questionListResultCaptor.value.isSuccess()).isTrue() - assertThat(questionListResultCaptor.value.getOrThrow()).isNotNull() - val questionList = questionListResultCaptor.value.getOrThrow(); - assertThat(questionList.size).isEqualTo(0) - } - - @Test - @ExperimentalCoroutinesApi - fun testController_returnsFailureForNonExistentTopic() = runBlockingTest(coroutineContext) { - val questionListLiveData = questionTrainingController.getQuestionsForTopic("NON_EXISTENT_TOPIC") - advanceUntilIdle() - questionListLiveData.observeForever(mockQuestionListObserver) - verify(mockQuestionListObserver, atLeastOnce()).onChanged(questionListResultCaptor.capture()) - assertThat(questionListResultCaptor.value.isFailure()).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(questionDataControllerTest: QuestionDataControllerTest) - } -} diff --git a/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt index 2b41311cc37..a7f07941824 100644 --- a/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt @@ -68,10 +68,10 @@ class QuestionTrainingControllerTest { lateinit var questionTrainingController: QuestionTrainingController @Mock - lateinit var mockQuestionListObserver: Observer>> + lateinit var mockQuestionListObserver: Observer> @Captor - lateinit var questionListResultCaptor: ArgumentCaptor>> + lateinit var questionListResultCaptor: ArgumentCaptor> @Inject @field:TestDispatcher @@ -119,7 +119,8 @@ class QuestionTrainingControllerTest { verify(mockQuestionListObserver, atLeastOnce()).onChanged(questionListResultCaptor.capture()) assertThat(questionListResultCaptor.value.isSuccess()).isTrue() - val questionsList = questionListResultCaptor.value.getOrThrow() + @Suppress("UNCHECKED_CAST") // TODO(#111): Observe this via the progress controller, instead. + val questionsList = questionListResultCaptor.value.getOrThrow() as List assertThat(questionsList.size).isEqualTo(3) val questionIds = questionsList.map { it.questionId } assertThat(questionIds).containsExactlyElementsIn( @@ -140,7 +141,8 @@ class QuestionTrainingControllerTest { verify(mockQuestionListObserver, atLeastOnce()).onChanged(questionListResultCaptor.capture()) assertThat(questionListResultCaptor.value.isSuccess()).isTrue() - val questionsList = questionListResultCaptor.value.getOrThrow() + @Suppress("UNCHECKED_CAST") // TODO(#111): Observe this via the progress controller, instead. + val questionsList = questionListResultCaptor.value.getOrThrow() as List assertThat(questionsList.size).isEqualTo(3) val questionIds = questionsList.map { it.questionId } assertThat(questionIds).containsExactlyElementsIn( @@ -211,7 +213,7 @@ class QuestionTrainingControllerTest { @Provides @QuestionTrainingSeed - fun provideQuestionTrainingSeed(): Long = questionSeed ++ + fun provideQuestionTrainingSeed(): Long = questionSeed++ } // TODO(#89): Move this to a common test application component. From 0c3be01fec76a2df763845a1b622c57d478c8b42 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Tue, 19 Nov 2019 16:34:49 -0800 Subject: [PATCH 24/25] Remove unneeded files. --- .../QuestionTrainingConstantsProvider.kt | 17 ----------------- .../org.mockito.plugins.MockMaker | 1 - 2 files changed, 18 deletions(-) delete mode 100644 domain/src/main/java/org/oppia/domain/question/QuestionTrainingConstantsProvider.kt delete mode 100644 domain/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingConstantsProvider.kt b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingConstantsProvider.kt deleted file mode 100644 index 9d94d5e3e5d..00000000000 --- a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingConstantsProvider.kt +++ /dev/null @@ -1,17 +0,0 @@ -package org.oppia.domain.question - -import org.oppia.domain.topic.TopicController -import org.oppia.util.data.DataProviders -import javax.inject.Inject -import javax.inject.Singleton - -const val QUESTION_COUNT_PER_TRAINING_SESSION = 10 - -/** Provider to return any constants required during the training session. */ -@Singleton -class QuestionTrainingConstantsProvider @Inject constructor( -) { - fun getQuestionCountPerTrainingSession(): Int { - return QUESTION_COUNT_PER_TRAINING_SESSION - } -} \ No newline at end of file diff --git a/domain/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker b/domain/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker deleted file mode 100644 index ca6ee9cea8e..00000000000 --- a/domain/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker +++ /dev/null @@ -1 +0,0 @@ -mock-maker-inline \ No newline at end of file From b6cd5727cd9f2c58a5efd952680055b9cb1fbac4 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Tue, 19 Nov 2019 16:36:09 -0800 Subject: [PATCH 25/25] Remove legacy code from tests. --- ...uestionAssessmentProgressControllerTest.kt | 29 ++++++++++--------- .../QuestionTrainingControllerTest.kt | 13 --------- 2 files changed, 16 insertions(+), 26 deletions(-) diff --git a/domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.kt b/domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.kt index fe90f01fff1..d6c902cb5fd 100644 --- a/domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/question/QuestionAssessmentProgressControllerTest.kt @@ -192,18 +192,6 @@ class QuestionAssessmentProgressControllerTest { // TODO(#89): Move this to a common test application component. @Module class TestModule { - companion object { - var questionSeed = 0L - } - - @Provides - @QuestionCountPerTrainingSession - fun provideQuestionCountPerTrainingSession(): Int = 3 - - @Provides - @QuestionTrainingSeed - fun provideQuestionTrainingSeed(): Long = questionSeed++ - @Provides @Singleton fun provideContext(application: Application): Context { @@ -233,9 +221,24 @@ class QuestionAssessmentProgressControllerTest { } } + @Module + class TestQuestionModule { + companion object { + var questionSeed = 0L + } + + @Provides + @QuestionCountPerTrainingSession + fun provideQuestionCountPerTrainingSession(): Int = 3 + + @Provides + @QuestionTrainingSeed + fun provideQuestionTrainingSeed(): Long = questionSeed++ + } + // TODO(#89): Move this to a common test application component. @Singleton - @Component(modules = [TestModule::class]) + @Component(modules = [TestModule::class,TestQuestionModule::class]) interface TestApplicationComponent { @Component.Builder interface Builder { diff --git a/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt index 681a77ccc7e..2826944900b 100644 --- a/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt @@ -160,25 +160,12 @@ class QuestionTrainingControllerTest { // TODO(#89): Move this to a common test application component. @Module class TestModule { - @Mock - lateinit var questionTrainingConstantsProvider: QuestionTrainingConstantsProvider - @Provides @Singleton fun provideContext(application: Application): Context { return application } - @Provides - @Singleton - fun provideQuestionTrainingConstantsProvider(): QuestionTrainingConstantsProvider { - MockitoAnnotations.initMocks(this) - Mockito.`when`( - questionTrainingConstantsProvider.getQuestionCountPerTrainingSession() - ).thenReturn(10) - return questionTrainingConstantsProvider - } - @ExperimentalCoroutinesApi @Singleton @Provides