From 95949549dde1c26baa83ec082630524a253a9fea Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Mon, 7 Oct 2019 20:07:48 -0700 Subject: [PATCH 01/22] 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/22] 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 4596e86f5e4b564f1f76f60aba8a10b681cf99ea Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Wed, 9 Oct 2019 13:07:37 -0700 Subject: [PATCH 03/22] 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 04/22] 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 05/22] 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 06/22] 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 07/22] 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 08/22] 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 09/22] 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 10/22] 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 11/22] 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 12/22] 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 13/22] 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 14/22] 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 15/22] 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 16/22] 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 38147cbb589e94ca580885c979e09e9a1dba271c Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Fri, 25 Oct 2019 10:32:57 -0700 Subject: [PATCH 17/22] Review changes --- domain/src/main/assets/sample_questions.json | 2 +- .../question/QuestionConstantsProvider.kt | 23 ++++++++++++++ .../QuestionTrainingConstantsProvider.kt | 17 ----------- .../question/QuestionTrainingController.kt | 14 +++++---- .../oppia/domain/util/JsonAssetRetriever.kt | 5 ++-- .../QuestionTrainingControllerTest.kt | 30 +++++++++---------- 6 files changed, 49 insertions(+), 42 deletions(-) create mode 100644 domain/src/main/java/org/oppia/domain/question/QuestionConstantsProvider.kt delete 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 592a66ff015..8ea79c3bf95 100644 --- a/domain/src/main/assets/sample_questions.json +++ b/domain/src/main/assets/sample_questions.json @@ -441,4 +441,4 @@ "solicit_answer_details": false } ] -} \ No newline at end of file +} diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionConstantsProvider.kt b/domain/src/main/java/org/oppia/domain/question/QuestionConstantsProvider.kt new file mode 100644 index 00000000000..912d3e8b88c --- /dev/null +++ b/domain/src/main/java/org/oppia/domain/question/QuestionConstantsProvider.kt @@ -0,0 +1,23 @@ +package org.oppia.domain.question + +import dagger.Module +import dagger.Provides +import javax.inject.Qualifier + +/** Provider to return any constants required during the training session. */ +@Qualifier +annotation class QuestionCountPerTrainingSession + +@Qualifier +annotation class QuestionTrainingSeed + +@Module +class QuestionModule { + @Provides + @QuestionCountPerTrainingSession + fun provideQuestionCountPerTrainingSession(): Int = 10 + + @Provides + @QuestionTrainingSeed + fun provideQuestionTrainingSeed(): Long = System.currentTimeMillis() +} \ 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 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/main/java/org/oppia/domain/question/QuestionTrainingController.kt b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt index d0a84158584..2ca20dea93c 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt @@ -7,8 +7,10 @@ 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 java.io.RandomAccessFile import javax.inject.Inject import javax.inject.Singleton +import kotlin.random.Random const val TRAINING_QUESTIONS_PROVIDER = "TrainingQuestionsProvider" @@ -18,7 +20,8 @@ class QuestionTrainingController @Inject constructor( private val questionAssessmentProgressController: QuestionAssessmentProgressController, private val topicController: TopicController, private val dataProviders: DataProviders, - private val questionTrainingConstantsProvider: QuestionTrainingConstantsProvider + @QuestionCountPerTrainingSession private val questionCountPerSession: Int, + @QuestionTrainingSeed private val questionTrainingSeed: Long ) { /** * Begins a question training session given a list of skill Ids and a total number of questions. @@ -47,8 +50,8 @@ class QuestionTrainingController @Inject constructor( val questionsDataProvider = topicController.retrieveQuestionsForSkillIds(skillIdsList) return dataProviders.transform(TRAINING_QUESTIONS_PROVIDER, questionsDataProvider) { getFilteredQuestionsForTraining( - skillIdsList, it, - questionTrainingConstantsProvider.getQuestionCountPerTrainingSession() / skillIdsList.size + skillIdsList, it.shuffled(Random(questionTrainingSeed)), + questionCountPerSession / skillIdsList.size ) } } @@ -65,10 +68,11 @@ class QuestionTrainingController @Inject constructor( !trainingQuestions.contains(it) }.distinctBy { it.questionId }.take(numQuestionsPerSkill + 1)) } - return trainingQuestions - .take(questionTrainingConstantsProvider.getQuestionCountPerTrainingSession()) + return trainingQuestions.take(questionCountPerSession) } + + /** * Finishes the most recent training session started by [startQuestionTrainingSession]. * This method should only be called if there is a training session is being played, 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 c035e0a1a58..a572a492461 100644 --- a/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt +++ b/domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt @@ -5,14 +5,13 @@ 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. */ +/** Utility that retrieves JSON assets and converts them to JSON objects. */ class JsonAssetRetriever @Inject constructor(private val context: Context) { /** 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) } -} \ 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 6a950f75dfe..86ffc207af0 100644 --- a/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt @@ -122,11 +122,11 @@ class QuestionTrainingControllerTest { assertThat(questionListResultCaptor.value.isSuccess()).isTrue() val questionsList = questionListResultCaptor.value.getOrThrow() - assertThat(questionsList.size).isEqualTo(4) + assertThat(questionsList.size).isEqualTo(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 + TEST_QUESTION_ID_2, TEST_QUESTION_ID_0, TEST_QUESTION_ID_3 ) ) } @@ -137,25 +137,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 @@ -193,9 +180,20 @@ class QuestionTrainingControllerTest { fun provideGlobalLogLevel(): LogLevel = LogLevel.VERBOSE } + @Module + class TestQuestionModule { + @Provides + @QuestionCountPerTrainingSession + fun provideQuestionCountPerTrainingSession(): Int = 3 + + @Provides + @QuestionTrainingSeed + fun provideQuestionTrainingSeed(): Long = 0L + } + // 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 { From 0d1d65efd726a2d4091f8b1300d41052252116b5 Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Fri, 25 Oct 2019 10:34:45 -0700 Subject: [PATCH 18/22] remove mockmaker --- .../resources/mockito-extensions/org.mockito.plugins.MockMaker | 1 - 1 file changed, 1 deletion(-) delete mode 100644 domain/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker 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 10dbeff8237874342a6df5fdf63a077068952106 Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Fri, 25 Oct 2019 10:36:07 -0700 Subject: [PATCH 19/22] new line for questionstrainingconstants --- .../java/org/oppia/domain/question/QuestionConstantsProvider.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionConstantsProvider.kt b/domain/src/main/java/org/oppia/domain/question/QuestionConstantsProvider.kt index 912d3e8b88c..02d9764bfaf 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionConstantsProvider.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionConstantsProvider.kt @@ -20,4 +20,4 @@ class QuestionModule { @Provides @QuestionTrainingSeed fun provideQuestionTrainingSeed(): Long = System.currentTimeMillis() -} \ No newline at end of file +} From 5b768b2f6c28fc4da27c8a192c06e7e2613a69fd Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Fri, 25 Oct 2019 13:37:43 -0700 Subject: [PATCH 20/22] Add another test to ensure that a different test is created when the seed changes --- .../QuestionTrainingControllerTest.kt | 26 ++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) 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 86ffc207af0..fbde1396117 100644 --- a/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt @@ -131,6 +131,27 @@ class QuestionTrainingControllerTest { ) } + @Test + @ExperimentalCoroutinesApi + fun testController_successfullyStartsDifferentQuestionSessionForExistingSkillIds() = 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() + val questionsList = questionListResultCaptor.value.getOrThrow() + assertThat(questionsList.size).isEqualTo(3) + val questionIds = questionsList.map { it.questionId } + assertThat(questionIds).containsExactlyElementsIn( + mutableListOf( + TEST_QUESTION_ID_0, TEST_QUESTION_ID_1, TEST_QUESTION_ID_3 + ) + ) + } + @Qualifier annotation class TestDispatcher @@ -182,13 +203,16 @@ class QuestionTrainingControllerTest { @Module class TestQuestionModule { + companion object { + var questionSeed = 0L + } @Provides @QuestionCountPerTrainingSession fun provideQuestionCountPerTrainingSession(): Int = 3 @Provides @QuestionTrainingSeed - fun provideQuestionTrainingSeed(): Long = 0L + fun provideQuestionTrainingSeed(): Long = questionSeed ++ } // TODO(#89): Move this to a common test application component. From 3cb73e1fd1996a1680483e2eb25dca195eb2a59c Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Fri, 25 Oct 2019 15:21:54 -0700 Subject: [PATCH 21/22] Review changes --- .../org/oppia/domain/question/QuestionConstantsProvider.kt | 2 +- .../oppia/domain/question/QuestionTrainingController.kt | 6 +++--- .../domain/question/QuestionTrainingControllerTest.kt | 7 ++++--- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/domain/src/main/java/org/oppia/domain/question/QuestionConstantsProvider.kt b/domain/src/main/java/org/oppia/domain/question/QuestionConstantsProvider.kt index 02d9764bfaf..bb05d27bf24 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionConstantsProvider.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionConstantsProvider.kt @@ -4,13 +4,13 @@ import dagger.Module import dagger.Provides import javax.inject.Qualifier -/** Provider to return any constants required during the training session. */ @Qualifier annotation class QuestionCountPerTrainingSession @Qualifier annotation class QuestionTrainingSeed +/** Provider to return any constants required during the training session. */ @Module class QuestionModule { @Provides 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 2ca20dea93c..6bc6fb2ec97 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt @@ -23,6 +23,8 @@ class QuestionTrainingController @Inject constructor( @QuestionCountPerTrainingSession private val questionCountPerSession: Int, @QuestionTrainingSeed private val questionTrainingSeed: Long ) { + + private val random = Random(questionTrainingSeed) /** * Begins a question training session given a list of skill Ids and a total number of questions. * @@ -50,7 +52,7 @@ class QuestionTrainingController @Inject constructor( val questionsDataProvider = topicController.retrieveQuestionsForSkillIds(skillIdsList) return dataProviders.transform(TRAINING_QUESTIONS_PROVIDER, questionsDataProvider) { getFilteredQuestionsForTraining( - skillIdsList, it.shuffled(Random(questionTrainingSeed)), + skillIdsList, it.shuffled(random), questionCountPerSession / skillIdsList.size ) } @@ -71,8 +73,6 @@ class QuestionTrainingController @Inject constructor( return trainingQuestions.take(questionCountPerSession) } - - /** * Finishes the most recent training session started by [startQuestionTrainingSession]. * This method should only be called if there is a training session is being played, 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 fbde1396117..9fdcb7a81fa 100644 --- a/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt @@ -126,14 +126,14 @@ class QuestionTrainingControllerTest { val questionIds = questionsList.map { it.questionId } assertThat(questionIds).containsExactlyElementsIn( mutableListOf( - TEST_QUESTION_ID_2, TEST_QUESTION_ID_0, TEST_QUESTION_ID_3 + TEST_QUESTION_ID_0, TEST_QUESTION_ID_1, TEST_QUESTION_ID_3 ) ) } @Test @ExperimentalCoroutinesApi - fun testController_successfullyStartsDifferentQuestionSessionForExistingSkillIds() = runBlockingTest(coroutineContext) { + fun testController_startsDifferentQuestionSessionForExistingSkillIds() = runBlockingTest(coroutineContext) { val questionListLiveData = questionTrainingController.startQuestionTrainingSession( listOf(TEST_SKILL_ID_0, TEST_SKILL_ID_1) ) @@ -147,7 +147,7 @@ class QuestionTrainingControllerTest { val questionIds = questionsList.map { it.questionId } assertThat(questionIds).containsExactlyElementsIn( mutableListOf( - TEST_QUESTION_ID_0, TEST_QUESTION_ID_1, TEST_QUESTION_ID_3 + TEST_QUESTION_ID_2, TEST_QUESTION_ID_0, TEST_QUESTION_ID_3 ) ) } @@ -206,6 +206,7 @@ class QuestionTrainingControllerTest { companion object { var questionSeed = 0L } + @Provides @QuestionCountPerTrainingSession fun provideQuestionCountPerTrainingSession(): Int = 3 From f2b5bc15765a2b25baf50b10418f9d0ce8c7237c Mon Sep 17 00:00:00 2001 From: vinitamurthi Date: Fri, 25 Oct 2019 15:25:25 -0700 Subject: [PATCH 22/22] Remove unused imports --- .../org/oppia/domain/question/QuestionTrainingController.kt | 1 - .../org/oppia/domain/question/QuestionTrainingControllerTest.kt | 2 -- 2 files changed, 3 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 6bc6fb2ec97..ac775d4fc5b 100644 --- a/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt +++ b/domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt @@ -7,7 +7,6 @@ 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 java.io.RandomAccessFile import javax.inject.Inject import javax.inject.Singleton import kotlin.random.Random 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 9fdcb7a81fa..2b41311cc37 100644 --- a/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt +++ b/domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt @@ -28,10 +28,8 @@ 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