Fix #110: Filter questions required for assessment in QuestionTrainingController #227

vinitamurthi · 2019-10-11T21:35:47Z

Explanation

This PR fetches a specific amount of questions, spread across each skill in the QuestionTrainingController and passes on the data provider to the QuestionAssessmentProgressController.
This also adds some fake question data with different interaction types - multiple choice, text input, and numeric input (these are the only ones that are currently supported even in the web question player) that can be used in a training session

Fixes #110

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The PR follows the style guide.
The PR does not contain any unnecessary auto-generated code from Android Studio.
The PR is made from a branch that's not called "develop".
The PR is assigned to an appropriate reviewer.

… training controller

…r filtering

vinitamurthi · 2019-10-11T23:19:58Z

@BenHenning PTAL, I have also added a json with 3 sample questions.

BenHenning

Thanks @vinitamurthi.

One top-level question: is this actually fixing #120? That issue appears to be closed. Is this fixing part of #110 or all of #110, instead?

Otherwise, my feedback is primarily around ensuring that we're generating this as expected. I was expecting more randomness, and it'd be good to make sure that we're testing multiple aspects of the controller.

domain/src/main/assets/sample_questions.json

domain/src/main/java/org/oppia/domain/exploration/ExplorationRetriever.kt

domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt

domain/src/main/java/org/oppia/domain/topic/TopicController.kt

domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt

BenHenning · 2019-10-15T04:35:21Z

Also if there is missing functionality before we can fully finish #110, what is that functionality? Can we just implement it here to consider the training controller finished?

vinitamurthi · 2019-10-15T22:00:42Z

#110 is implemented completely by this PR, sorry I meant to write #110 in this PR but accidentally wrote that it fixes #120

BenHenning

Thanks @vinitamurthi--took another pass on this. Please let me know if you have any questions regarding my comments.

domain/src/main/assets/sample_questions.json

domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt

BenHenning · 2019-10-18T07:49:01Z

domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt

+    val jsonContents = assetManager.open(assetName).bufferedReader().use { it.readText() }
+    return JSONObject(jsonContents)
+  }
+}


Please add EOF newline here & elsewhere in this PR where one is missing.

This appears to still be missing. Please make sure all new files have EOF newlines.

domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt

domain/src/main/java/org/oppia/domain/util/StateRetriever.kt

domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt

domain/src/test/java/org/oppia/domain/topic/TopicControllerTest.kt

vinitamurthi · 2019-10-18T18:35:08Z

I have made all the changes requested, PTAL @BenHenning !

BenHenning

Thanks @vinitamurthi! Did you also address my question regarding the random number generation?

Overall this PR LGTM, but my main open question is around the randomness. Shouldn't we be randomly sorting the questions so that assessments are different each time? If so, we should also be thoroughly testing these flows in a deterministic way.

BenHenning · 2019-10-23T04:54:04Z

domain/src/main/assets/sample_questions.json

+      "solicit_answer_details": false
+    }
+  ]
+}


Nit: please add EOF newline.

BenHenning · 2019-10-23T04:56:04Z

domain/src/main/java/org/oppia/domain/question/QuestionTrainingConstantsProvider.kt

+class QuestionTrainingConstantsProvider @Inject constructor(
+) {
+  fun getQuestionCountPerTrainingSession(): Int {
+    return QUESTION_COUNT_PER_TRAINING_SESSION


Suggest providing this via Dagger instead. That would look something like:

Annotation file:

@Qualifier annotation class QuestionCountPerTrainingSession

Module:

@Module class QuestionModule { @Provides @QuestionCountPerTrainingSession fun provideQuestionCountPerTrainingSession(): Int = 10 }

When using it:

class Dependency @Inject(@QuestionCountPerTrainingSession private val int questionCountPerSession) { ... }

This approach allows us to switch out the count in test modules to make it easier to perform testing.

BenHenning · 2019-10-23T04:58:16Z

domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt

+import java.io.IOException
+import javax.inject.Inject
+
+/** Utility that helps retrieving JSON assets and converting them to a JSON object. */


Nit on verb tense: Utility that retrieves JSON assets and converts them to JSON objects.

BenHenning · 2019-10-23T04:58:37Z

domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt

+    val jsonContents = assetManager.open(assetName).bufferedReader().use { it.readText() }
+    return JSONObject(jsonContents)
+  }
+}


This appears to still be missing. Please make sure all new files have EOF newlines.

BenHenning · 2019-10-23T04:59:13Z

domain/src/main/java/org/oppia/domain/util/JsonAssetRetriever.kt

+class JsonAssetRetriever @Inject constructor(private val context: Context) {
+
+  /** Loads the JSON string from an asset and converts it to a JSONObject */
+  @Throws(IOException::class)


Is this actually necessary? If not, prefer to remove since exceptions are always optional in Kotlin (this annotation should only be needed when calling this method from Java code, and we're not doing that).

BenHenning · 2019-10-23T05:01:09Z

domain/src/test/java/org/oppia/domain/question/QuestionTrainingControllerTest.kt

+    @Provides
+    @Singleton
+    fun provideQuestionTrainingConstantsProvider(): QuestionTrainingConstantsProvider {
+      MockitoAnnotations.initMocks(this)


This is a bit hacky and ought to be done only for test classes. However we can avoid using Mockito here altogether if you move the constant directly to the dagger graph.

Got it, I have moved it to the dagger graph now

BenHenning · 2019-10-23T05:01:33Z

domain/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker

@@ -0,0 +1 @@
+mock-maker-inline


Why do we need this? Prefer to remove.

I had kept this because mockito doesnt allow mocking final classes unless you do this and all kotlin classes are final by default. But since I dont need to use mockito anymore I removed it

…seed changes

BenHenning

Thanks!

BenHenning · 2019-10-25T21:41:01Z

domain/src/main/java/org/oppia/domain/question/QuestionConstantsProvider.kt

+import dagger.Provides
+import javax.inject.Qualifier
+
+/** Provider to return any constants required during the training session. */


Perhaps this should be on the Module instead of this annotation?

BenHenning · 2019-10-25T21:41:21Z

domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt

+    }
+    return trainingQuestions.take(questionCountPerSession)
+  }
+


Nit: please remove extra 2 newlines.

BenHenning · 2019-10-25T21:41:53Z

domain/src/main/java/org/oppia/domain/question/QuestionTrainingController.kt

+    val questionsDataProvider = topicController.retrieveQuestionsForSkillIds(skillIdsList)
+    return dataProviders.transform(TRAINING_QUESTIONS_PROVIDER, questionsDataProvider) {
+      getFilteredQuestionsForTraining(
+        skillIdsList, it.shuffled(Random(questionTrainingSeed)),


Won't this cause the same order for subsequent assessments? I'm guessing we need to actually store the random object in a field to make sure it's shared across multiple shuffles.

Yes that's right my mistake. I am storing the random object as a member variable and reusing the same object

vinitamurthi · 2019-10-25T22:23:21Z

I verified through a manual test that the random generator does send out a different assessment when called by the same object multiple times. I will submit this PR now and we can make any subsequent changes in a follow up!

vinitamurthi added 7 commits October 7, 2019 20:07

Initial check in for the question data controller

9594954

Removed progress controller test

6ee7aec

Review changes

4596e86

Review changes

f5c23c8

Pass data providers to the assessment controller, and set caps in the…

53d367d

… training controller

Fixed merge conflicts

86630b6

Removed unnecessary imports/variables

a8adf93

vinitamurthi requested a review from BenHenning October 11, 2019 21:35

vinitamurthi added 5 commits October 11, 2019 16:06

Add fake question data for stubbed interfaces

a7a10f3

Remove duplicate questions while fetching in training controller

459e3e9

Comment explaining the filter function

73c49df

Improve duplicate checking - check it while filtering instead of afte…

91cf8e6

…r filtering

Add linked skill id values

54546d7

vinitamurthi assigned BenHenning Oct 11, 2019

BenHenning requested changes Oct 15, 2019

View reviewed changes

BenHenning assigned vinitamurthi and unassigned BenHenning Oct 15, 2019

vinitamurthi changed the title ~~Fix #120: Filter questions required for assessment in QuestionTrainingController~~ Fix #110: Filter questions required for assessment in QuestionTrainingController Oct 15, 2019

Review changes

be375fd

vinitamurthi assigned BenHenning and unassigned vinitamurthi Oct 15, 2019

BenHenning requested changes Oct 18, 2019

View reviewed changes

BenHenning assigned vinitamurthi and unassigned BenHenning Oct 18, 2019

vinitamurthi added 3 commits October 18, 2019 10:48

add a new module for questions constants

64b8265

fix merge conflicts

75405a2

Review changes

4878d10

vinitamurthi added 2 commits October 18, 2019 11:30

add a test to verify questions were fetched properly

c37f14b

reformatted code

d1bd4a6

BenHenning mentioned this pull request Oct 20, 2019

Fix #112: Introduce question progress controller interface #218

Merged

vinitamurthi assigned BenHenning and unassigned vinitamurthi Oct 21, 2019

BenHenning requested changes Oct 23, 2019

View reviewed changes

BenHenning assigned vinitamurthi and unassigned BenHenning Oct 23, 2019

vinitamurthi added 4 commits October 25, 2019 09:13

Merge remote-tracking branch 'origin/develop' into question_data

1f96f0c

Review changes

38147cb

remove mockmaker

0d1d65e

new line for questionstrainingconstants

10dbeff

vinitamurthi assigned BenHenning and unassigned vinitamurthi Oct 25, 2019

Add another test to ensure that a different test is created when the …

5b768b2

…seed changes

BenHenning approved these changes Oct 25, 2019

View reviewed changes

BenHenning assigned vinitamurthi and unassigned BenHenning Oct 25, 2019

Review changes

3cb73e1

Remove unused imports

f2b5bc1

vinitamurthi merged commit 904c9f4 into develop Oct 25, 2019

vinitamurthi deleted the question_data branch November 5, 2019 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #110: Filter questions required for assessment in QuestionTrainingController #227

Fix #110: Filter questions required for assessment in QuestionTrainingController #227

vinitamurthi commented Oct 11, 2019 •

edited

Loading

vinitamurthi commented Oct 11, 2019

BenHenning left a comment

BenHenning commented Oct 15, 2019

vinitamurthi commented Oct 15, 2019

BenHenning left a comment

BenHenning Oct 18, 2019

vinitamurthi Oct 18, 2019

BenHenning Oct 23, 2019

vinitamurthi Oct 25, 2019

vinitamurthi commented Oct 18, 2019

BenHenning left a comment

BenHenning Oct 23, 2019

vinitamurthi Oct 25, 2019

BenHenning Oct 23, 2019

vinitamurthi Oct 25, 2019

BenHenning Oct 23, 2019

vinitamurthi Oct 25, 2019

BenHenning Oct 23, 2019

BenHenning Oct 23, 2019

vinitamurthi Oct 25, 2019

BenHenning Oct 23, 2019

vinitamurthi Oct 25, 2019

BenHenning Oct 23, 2019

vinitamurthi Oct 25, 2019

BenHenning left a comment

BenHenning Oct 25, 2019

vinitamurthi Oct 25, 2019

BenHenning Oct 25, 2019

vinitamurthi Oct 25, 2019

BenHenning Oct 25, 2019

vinitamurthi Oct 25, 2019

vinitamurthi commented Oct 25, 2019

		@@ -0,0 +1 @@
		mock-maker-inline

Fix #110: Filter questions required for assessment in QuestionTrainingController #227

Fix #110: Filter questions required for assessment in QuestionTrainingController #227

Conversation

vinitamurthi commented Oct 11, 2019 • edited Loading

Explanation

Checklist

vinitamurthi commented Oct 11, 2019

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning commented Oct 15, 2019

vinitamurthi commented Oct 15, 2019

BenHenning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinitamurthi commented Oct 18, 2019

BenHenning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinitamurthi commented Oct 25, 2019

vinitamurthi commented Oct 11, 2019 •

edited

Loading