Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #120: Introduce question data controller API #217

Merged
merged 4 commits into from
Oct 10, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
package org.oppia.domain.question

import android.content.Context
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: unused import (I noticed this in Android Studio after branching off of your PR). Please remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: for retrieving a Question

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Singleton
class QuestionDataController @Inject constructor(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: per #120 this should be QuestionTrainingController

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

private val questionProgressController: QuestionProgressController,
private val dataProviders: DataProviders
) {

/**
* Returns a list of [Question] objects given a topic ID.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this indicate that it's returning all questions for a given topic ID?

Also, do we really need this method? If not, suggest removing it since we should rely on the progress controller to retrieve specific state during a practice session.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if we need this only for testing and not prod cases I suggest removing it & instead verify that calling begin/end below doesn't fail.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up: I don't know what we would use this method for. The TopicController is used to retrieve the list of skills to display on the training page, so we should just be operating based on that list here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to fetch a list of questions given topic id / list of skill ids right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm doing the fetch within startTrainingSession itself

*/
fun getQuestionsForTopic(topicId: String): LiveData<AsyncResult<List<Question>>> {
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<Question>): LiveData<AsyncResult<Any?>> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this late, but shouldn't this take a list of skill IDs? I expect the controller to take a list of skill IDs and generate a list of questions from that. Please sync with me if my assumption is wrong, otherwise please update this method accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay got it, I have changed the methods to be this way

return try {
questionProgressController.beginQuestionTrainingSession(questionsList.shuffled())
MutableLiveData(AsyncResult.success<Any?>(null))
} catch (e: Exception) {
MutableLiveData(AsyncResult.failed(e))
}
}

/**
* Finishes the most recent training session started by [startQuestionTrainingSession].
* This method should only be called if there is a training session is being played,
* otherwise an exception will be thrown.
*/
fun stopQuestionTrainingSession(): LiveData<AsyncResult<Any?>> {
return try {
questionProgressController.finishQuestionTrainingSession()
MutableLiveData(AsyncResult.success<Any?>(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<List<Question>> {
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<Question> {
when(topicId) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: when (topicId) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

TEST_TOPIC_ID_0 -> return emptyList()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fill in a few sample questions otherwise we can't test the progress controller or downstream UI code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added questions just with a question ID set, I will create a follow up PR that adds proper question data

TEST_TOPIC_ID_1 -> return emptyList()
else -> throw IllegalStateException("Invalid topic ID: $topicId")
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package org.oppia.domain.question

import org.oppia.app.model.Question
import javax.inject.Inject
import javax.inject.Singleton


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: please remove extra newline.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

/** Controller for retrieving an exploration. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: please update to be question-specific (I suggest using the high-level summary of ExplorationProgressController for an example, but feel free to leave out the additional bits--I'll fill those in my interface introduction PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Singleton
class QuestionProgressController @Inject constructor(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was intending to call this QuestionAssessmentProgressController per #111, since a single 'question' doesn't really have progress. Please update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

) {
fun beginQuestionTrainingSession(questionsList: List<Question>) {
}

fun finishQuestionTrainingSession() {

}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit here & elsewhere: please make sure all files end with 1 blank newline.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Original file line number Diff line number Diff line change
@@ -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<AsyncResult<List<Question>>>

@Captor
lateinit var questionListResultCaptor: ArgumentCaptor<AsyncResult<List<Question>>>

@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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Please space out test to follow:

// Arrange

// Act

// Assert

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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)
}
}
21 changes: 21 additions & 0 deletions model/src/main/proto/question.proto
Original file line number Diff line number Diff line change
@@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, how does routing work for questions? Presumably, the Outcome you match to an answer won't have a destination state name. Is this what the labelled_as_correct should be used for? Can you also route to an exploration from a question via the refresher exploration ID?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it doesnt have a destination state, it only uses labelled_as_correct, and it moves to the next question if the labelled_as_correct value is true (https://github.com/oppia/oppia/blob/40cb6adca9101f46209cecd4c83bd26a4f112bf9/core/templates/dev/head/pages/exploration-player-page/services/question-player-engine.service.ts#L206)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, then I think I modeled this correctly downstream. Thanks!

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;
}


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: please remove extra blank newline (only one should terminate the file).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done