Fix #120: Introduce question data controller API #217
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BenHenning
approved these changes
Oct 9, 2019
There was a problem hiding this comment.
Thanks @vinitamurthi! I have a few high-level suggestions and a bunch of nits. The more important things to take from my comments are:
- We should name the progress controller based on Implement QuestionAssessmentProgressController [Blocked: #112] #111
- We should add some data to properly stub out the implementation otherwise we can't test the progress controller
Once these & my other comments are resolved, please submit this.
model/src/main/proto/question.proto
Outdated
| int64 updated_on_timestamp_ms = 7; | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Nit: please remove extra blank newline (only one should terminate the file).
| // Structure for a single question. | ||
| message Question { | ||
| string question_id = 1; | ||
| State question_state = 2; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes it doesnt have a destination state, it only uses labelled_as_correct, and it moves to the next question if the labelled_as_correct value is true (https://github.com/oppia/oppia/blob/40cb6adca9101f46209cecd4c83bd26a4f112bf9/core/templates/dev/head/pages/exploration-player-page/services/question-player-engine.service.ts#L206)
There was a problem hiding this comment.
Perfect, then I think I modeled this correctly downstream. Thanks!
| import javax.inject.Inject | ||
| import javax.inject.Singleton | ||
|
|
||
|
|
There was a problem hiding this comment.
Nit: please remove extra newline.
| fun finishQuestionTrainingSession() { | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
Nit here & elsewhere: please make sure all files end with 1 blank newline.
|
|
||
| /** Controller for retrieving an exploration. */ | ||
| @Singleton | ||
| class QuestionProgressController @Inject constructor( |
There was a problem hiding this comment.
I was intending to call this QuestionAssessmentProgressController per #111, since a single 'question' doesn't really have progress. Please update.
| import javax.inject.Singleton | ||
|
|
||
|
|
||
| /** Controller for retrieving an exploration. */ |
There was a problem hiding this comment.
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).
|
|
||
| // Loads and returns the questions given a topic id. | ||
| private fun loadQuestions(topicId: String): List<Question> { | ||
| when(topicId) { |
| // Loads and returns the questions given a topic id. | ||
| private fun loadQuestions(topicId: String): List<Question> { | ||
| when(topicId) { | ||
| TEST_TOPIC_ID_0 -> return emptyList() |
There was a problem hiding this comment.
Please fill in a few sample questions otherwise we can't test the progress controller or downstream UI code.
There was a problem hiding this comment.
I have added questions just with a question ID set, I will create a follow up PR that adds proper question data
| ) { | ||
|
|
||
| /** | ||
| * Returns a list of [Question] objects given a topic ID. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We still need to fetch a list of questions given topic id / list of skill ids right?
There was a problem hiding this comment.
I'm doing the fetch within startTrainingSession itself
| @ExperimentalCoroutinesApi | ||
| fun testController_returnsFailureForNonExistentTopic() = runBlockingTest(coroutineContext) { | ||
| val questionListLiveData = questionDataController.getQuestionsForTopic("NON_EXISTENT_TOPIC") | ||
| advanceUntilIdle() |
There was a problem hiding this comment.
Nit: Please space out test to follow:
// Arrange
// Act
// Assert
|
Also I marked this to fix #120 since this is introducing the interface. |
BenHenning
changed the title
BenHenning
approved these changes
Oct 9, 2019
|
|
||
| /** Controller for retrieving an exploration. */ | ||
| @Singleton | ||
| class QuestionDataController @Inject constructor( |
There was a problem hiding this comment.
Nit: per #120 this should be QuestionTrainingController
|
Please note @vinitamurthi that I did 2 reviews on this because I didn't realize that the data controller should be named training controller until after I submitted the review. Please address the comments from both reviews before submitting. |
BenHenning
reviewed
Oct 9, 2019
| @@ -0,0 +1,83 @@ | |||
| package org.oppia.domain.question | |||
|
|
|||
| import android.content.Context | |||
There was a problem hiding this comment.
Nit: unused import (I noticed this in Android Studio after branching off of your PR). Please remove.
BenHenning
reviewed
Oct 9, 2019
| * @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?>> { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah okay got it, I have changed the methods to be this way
BenHenning
added a commit
that referenced
this pull request
Oct 9, 2019
BenHenning
approved these changes
Oct 10, 2019
There was a problem hiding this comment.
Thanks @vinitamurthi, this LGTM! After addressing my current comments, please go ahead and submit this.
| const val TEST_QUESTION_ID_1 = "question_id_1" | ||
| const val TEST_QUESTION_ID_2 = "question_id_2" | ||
|
|
||
|
|
There was a problem hiding this comment.
Nit: please remove blank newline
| ) { | ||
| /** | ||
| * Begins a question training session given a list of skill Ids and a total number of questions. | ||
| * This method is not expected to fail. |
There was a problem hiding this comment.
Please add empty lines between separate paragraphs in the KDoc, e.g.:
/**
* Begins a question...
*
* This method is not expected to fail.
*
* [QuestionAssessment...(where relevant, my view of the code might be messing up line wrapping)
| * @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<String>): LiveData<AsyncResult<Any?>> { |
There was a problem hiding this comment.
This shouldn't be a suspend function since it's returning a LiveData (suspend functions won't be easily callable from the UI, and the LiveData w/ async result allows us to trigger an async method to return the response for here).
| */ | ||
| suspend fun startQuestionTrainingSession(skillIdsList: List<String>): LiveData<AsyncResult<Any?>> { | ||
| return try { | ||
| val questionsList = retrieveQuestionsForSkillIds(skillIdsList).getOrThrow() |
There was a problem hiding this comment.
I think that retrieve*() will probably return a LiveData since it will depend on TopicController. We could update TopicController in your next PR to have a separate method that returns the underlying DataProvider, then transform it here to return the list of questions. We could then transform that and call the begin method, then return the transformed data provider as a LiveData here.
Let me know if this isn't clear. It's partially a new pattern.
There was a problem hiding this comment.
I am not 100% sure I got what you are saying, this is what I understood:
TopicController will pass the underlying dataProvider which will be used in retrieve*(). The retrieve() function returns a LiveData that is called in startQuestion...(). And then startQuestion...() passes the same LiveData to begin*() within the assessmentprogress controller. Is that correct?
In this PR what I am doing is having retrieve*() return a LiveData() then pass that LiveData itself instead of the actual questions into the begin method in the assessment controller, is that what you mean?
|
@BenHenning I am submitting this PR as is. If my understanding of the last comment was wrong I will fix that in the follow up PR! |
BenHenning
added a commit
that referenced
this pull request
Nov 20, 2019
* Initial check in for the question data controller * Removed progress controller test * Address the renames suggested in #217. * Introduce thoroughly stubbed QuestionAssessmentProgressController + empty test suite. * Review changes * Review changes * Pass data providers to the assessment controller, and set caps in the training controller * Removed unnecessary imports/variables * Add fake question data for stubbed interfaces * Remove duplicate questions while fetching in training controller * Comment explaining the filter function * Improve duplicate checking - check it while filtering instead of after filtering * Add linked skill id values * Review changes * add a new module for questions constants * Review changes * add a test to verify questions were fetched properly * reformatted code * Re-add QuestionTrainingController removed in merge. * Finalize question progress controller interface. * Fix typo in QuestionTrainingController. * Post-merge fixes and adjustments. * Post-merge fixes & address reviewer comments. * Remove unneeded files. * Remove legacy code from tests.
BenHenning
added a commit
that referenced
this pull request
May 28, 2020
* Initial check in for the question data controller * Removed progress controller test * Address the renames suggested in #217. * Introduce thoroughly stubbed QuestionAssessmentProgressController + empty test suite. * Review changes * Review changes * Pass data providers to the assessment controller, and set caps in the training controller * Removed unnecessary imports/variables * Add fake question data for stubbed interfaces * Remove duplicate questions while fetching in training controller * Comment explaining the filter function * Improve duplicate checking - check it while filtering instead of after filtering * Add linked skill id values * Review changes * add a new module for questions constants * Review changes * add a test to verify questions were fetched properly * reformatted code * Re-add QuestionTrainingController removed in merge. * Finalize question progress controller interface. * Implement QuestionAssessmentProgressController. This includes some basic refactoring of internal structures used by the exploration progress controller to share common functionality between the two progress controllers. There's still some duplication, but this seems like a reasonable split since there's likely to be further differences in the progress controllers in the future. The question assessment progress controller tests pass, but no new ones have yet been added to thoroughly test the implementation. * Add initial phase of tests for the progress controller based on ExplorationProgressController. They haven't yet been verified as correct. * Fix typo in QuestionTrainingController. * Post-merge fixes and adjustments. * Fix broken ProfileManagementControllerTest. * Post-merge fixes. * Update tests to build, but not pass. * Fix question session not properly being reinitialized for new sessions. Also, ensure that notifications do not regenerate the training session. * Fix index accounting for questions. Before, the index incremented upon correct answer submission rather than navigation. * Remove support for backward navigation in questions since it isn't allowed. * Add support for a synthetic ephemeral question to represent the end of a training session. * Finish tests for Question{Training,AssessmentProgress}Controllers. Note that the nature of the assessment progress controller is such that a new, dynamic DataProvider is needed since the data provider for the current assessment question is actually a transformation of different data providers depending on which questions are passed to the controller. * Initial introduction of test coroutine dispatchers to replace Kotlin's test coroutine dispatcher. This includes introducing a test-only module to contain testing dependencies. * Introduce a new LiveData mechanism to bridge coroutines from DataProviders in a way that doesn't have the same limitations as the previous MutableLiveData-based bridge. * Introduce new nested data provider that effectively allows a DataProvider to be set up like transformAsync() but with the ability to change the root DataProvider. * Early work at introducing FakeSystemClock tests (not yet complete). * Remove unnecessary meta file for Mockito. * Minor import cleanup. * Remove infeasible testing structures, add documentation, and clean up implementation to prepare for code review. * Add notice that the dispatchers utility is temporary. * Cleanup new LiveData bridge, add tests for it, and migrate other DataProviders tests to using TestCoroutineDispatchers utility. * Add AsyncResult tests, fix & re-enable an earlier test in PersistentCacheStoreTest, and fix FakeSystemClock so that it works properly in test environments. * Use ktlint to reformat TestCoroutineDispatchers per reviewer comment thread. * Reformat files failing linter check. * Reformat new DataProviders code. * Add new tests for the NestedTransformedDataProvider. * Address reviewer comment. * Address reviewer comments. Co-authored-by: vinitamurthi <[email protected]>
BenHenning
added a commit
that referenced
this pull request
Jun 10, 2020
* Initial check in for the question data controller * Removed progress controller test * Address the renames suggested in #217. * Introduce thoroughly stubbed QuestionAssessmentProgressController + empty test suite. * Review changes * Review changes * Pass data providers to the assessment controller, and set caps in the training controller * Removed unnecessary imports/variables * Add fake question data for stubbed interfaces * Remove duplicate questions while fetching in training controller * Comment explaining the filter function * Improve duplicate checking - check it while filtering instead of after filtering * Add linked skill id values * Review changes * add a new module for questions constants * Review changes * add a test to verify questions were fetched properly * reformatted code * Re-add QuestionTrainingController removed in merge. * Finalize question progress controller interface. * Implement QuestionAssessmentProgressController. This includes some basic refactoring of internal structures used by the exploration progress controller to share common functionality between the two progress controllers. There's still some duplication, but this seems like a reasonable split since there's likely to be further differences in the progress controllers in the future. The question assessment progress controller tests pass, but no new ones have yet been added to thoroughly test the implementation. * Add initial phase of tests for the progress controller based on ExplorationProgressController. They haven't yet been verified as correct. * Fix typo in QuestionTrainingController. * Post-merge fixes and adjustments. * Fix broken ProfileManagementControllerTest. * Post-merge fixes. * Update tests to build, but not pass. * Started UI * setup ViewModel * added binding adapter * Handles basic functionality * Moved code around * Added replay button * Make questions partially work. This introduces back button behavior to ensure training sessions can be reset. This also attempts to introduce proper asset loading, but it doesn't seem to be working currently. * Temporarily remove check in StateDeck that breaks questions. * Ensure all state navigation buttons are on the same line and the same size. This required reworking how nav button binding works in the state fragment such that multiple models are used depending on which type of navigation scenario the fragment is currently in. * Fix dp adjustment that caused previous button to disappear. Fix previous button showing up twice for continue interaction. * Undo .idea/misc.xml change that snuck in. * Update & fix StateFragmentTest (at least for Espresso). See #495 description for details on the changes necessary to make these tests work. * Post-merge fixes with some temporary stop-gap solutions. * Fix questions image rendering and consolidate GCS resource bucket names. * Fix question session not properly being reinitialized for new sessions. Also, ensure that notifications do not regenerate the training session. * Fix index accounting for questions. Before, the index incremented upon correct answer submission rather than navigation. * Remove support for backward navigation in questions since it isn't allowed. * Set title of question activity to 'Train Mode'. * Add support for a synthetic ephemeral question to represent the end of a training session. * Update question progress tracking to be translatable and to ensure that the synthetic terminal state shows the correct progress: 'completed'. * Move most state player functional support into a new configurable assembler class to enable sharing between the exploration and question players. * Hook up StatePlayerRecyclerViewAssembler to the question player fragment presenter. This also enables the congratulations text. * Introduce proper terminal page and disable overscroll. Note that the end session page introduced here has not been mocked yet, so it's mostly a placeholder until a mock is available. * Add support & button for training session replay. * Update/add TODOs to correspond to GitHub tracked issues. * Fix broken import. * Finish tests for Question{Training,AssessmentProgress}Controllers. Note that the nature of the assessment progress controller is such that a new, dynamic DataProvider is needed since the data provider for the current assessment question is actually a transformation of different data providers depending on which questions are passed to the controller. * Initial introduction of test coroutine dispatchers to replace Kotlin's test coroutine dispatcher. This includes introducing a test-only module to contain testing dependencies. * Introduce a new LiveData mechanism to bridge coroutines from DataProviders in a way that doesn't have the same limitations as the previous MutableLiveData-based bridge. * Introduce new nested data provider that effectively allows a DataProvider to be set up like transformAsync() but with the ability to change the root DataProvider. * question player progress-bar hifi * nit * revert * revert * Early work at introducing FakeSystemClock tests (not yet complete). * Post-merge fixes. * Fix the submit button disabled states: with the new refactor, the submit button is now properly disabled when an invalid answer is pending. * Revert accidental changes to .idea/misc.xml. * Address reviewer comments. * Remove unnecessary meta file for Mockito. * Minor import cleanup. * Post-merge fixes. * Fix audio playback & generalize it to facilitate enabling it for questions in the future. * Address reviewer comments & fix broken tests. * Fix hints & solutions functionality after simplification (some bugs & regressions were introduced due to the simplification; these have been fixed). * Revert the assembler delays to match the intended durations rather than the test-only values used for development. * Address reviewer comments. * Remove infeasible testing structures, add documentation, and clean up implementation to prepare for code review. * Add notice that the dispatchers utility is temporary. * Cleanup new LiveData bridge, add tests for it, and migrate other DataProviders tests to using TestCoroutineDispatchers utility. * Add AsyncResult tests, fix & re-enable an earlier test in PersistentCacheStoreTest, and fix FakeSystemClock so that it works properly in test environments. * Use ktlint to reformat TestCoroutineDispatchers per reviewer comment thread. * Reformat files failing linter check. * Reformat new DataProviders code. * Add new tests for the NestedTransformedDataProvider. * Address reviewer comment. * Address reviewer comments. * Address reviewer comments & fix lint issue. * Update hints & solutions handling to more closely follow the intended behavior graph (and added an actual visual representation of this graph in code to simplify maintaining the solution in the future). Tests will be added as part of solving #1273. Co-authored-by: vinitamurthi <[email protected]> Co-authored-by: James Xu <[email protected]> Co-authored-by: marysoloman <[email protected]>
nikitamarysolomanpvt
added a commit
that referenced
this pull request
Jun 12, 2020
* Initial check in for the question data controller * Removed progress controller test * Address the renames suggested in #217. * Introduce thoroughly stubbed QuestionAssessmentProgressController + empty test suite. * Review changes * Review changes * Pass data providers to the assessment controller, and set caps in the training controller * Removed unnecessary imports/variables * Add fake question data for stubbed interfaces * Remove duplicate questions while fetching in training controller * Comment explaining the filter function * Improve duplicate checking - check it while filtering instead of after filtering * Add linked skill id values * Review changes * add a new module for questions constants * Review changes * add a test to verify questions were fetched properly * reformatted code * Re-add QuestionTrainingController removed in merge. * Finalize question progress controller interface. * Implement QuestionAssessmentProgressController. This includes some basic refactoring of internal structures used by the exploration progress controller to share common functionality between the two progress controllers. There's still some duplication, but this seems like a reasonable split since there's likely to be further differences in the progress controllers in the future. The question assessment progress controller tests pass, but no new ones have yet been added to thoroughly test the implementation. * Add initial phase of tests for the progress controller based on ExplorationProgressController. They haven't yet been verified as correct. * Fix typo in QuestionTrainingController. * Post-merge fixes and adjustments. * Fix broken ProfileManagementControllerTest. * Post-merge fixes. * Update tests to build, but not pass. * Started UI * setup ViewModel * added binding adapter * Handles basic functionality * Moved code around * Added replay button * Make questions partially work. This introduces back button behavior to ensure training sessions can be reset. This also attempts to introduce proper asset loading, but it doesn't seem to be working currently. * Temporarily remove check in StateDeck that breaks questions. * Ensure all state navigation buttons are on the same line and the same size. This required reworking how nav button binding works in the state fragment such that multiple models are used depending on which type of navigation scenario the fragment is currently in. * Fix dp adjustment that caused previous button to disappear. Fix previous button showing up twice for continue interaction. * Undo .idea/misc.xml change that snuck in. * Update & fix StateFragmentTest (at least for Espresso). See #495 description for details on the changes necessary to make these tests work. * Post-merge fixes with some temporary stop-gap solutions. * Fix questions image rendering and consolidate GCS resource bucket names. * Fix question session not properly being reinitialized for new sessions. Also, ensure that notifications do not regenerate the training session. * Fix index accounting for questions. Before, the index incremented upon correct answer submission rather than navigation. * Remove support for backward navigation in questions since it isn't allowed. * Set title of question activity to 'Train Mode'. * Add support for a synthetic ephemeral question to represent the end of a training session. * Update question progress tracking to be translatable and to ensure that the synthetic terminal state shows the correct progress: 'completed'. * Move most state player functional support into a new configurable assembler class to enable sharing between the exploration and question players. * Hook up StatePlayerRecyclerViewAssembler to the question player fragment presenter. This also enables the congratulations text. * Introduce proper terminal page and disable overscroll. Note that the end session page introduced here has not been mocked yet, so it's mostly a placeholder until a mock is available. * Add support & button for training session replay. * Update/add TODOs to correspond to GitHub tracked issues. * Fix broken import. * Finish tests for Question{Training,AssessmentProgress}Controllers. Note that the nature of the assessment progress controller is such that a new, dynamic DataProvider is needed since the data provider for the current assessment question is actually a transformation of different data providers depending on which questions are passed to the controller. * Initial introduction of test coroutine dispatchers to replace Kotlin's test coroutine dispatcher. This includes introducing a test-only module to contain testing dependencies. * Introduce a new LiveData mechanism to bridge coroutines from DataProviders in a way that doesn't have the same limitations as the previous MutableLiveData-based bridge. * Introduce new nested data provider that effectively allows a DataProvider to be set up like transformAsync() but with the ability to change the root DataProvider. * question player toolbar shadow * question player progress-bar hifi * nit * revert * revert * Early work at introducing FakeSystemClock tests (not yet complete). * Post-merge fixes. * Fix the submit button disabled states: with the new refactor, the submit button is now properly disabled when an invalid answer is pending. * Revert accidental changes to .idea/misc.xml. * Address reviewer comments. * Remove unnecessary meta file for Mockito. * Minor import cleanup. * Post-merge fixes. * Fix audio playback & generalize it to facilitate enabling it for questions in the future. * Address reviewer comments & fix broken tests. * Fix hints & solutions functionality after simplification (some bugs & regressions were introduced due to the simplification; these have been fixed). * Revert the assembler delays to match the intended durations rather than the test-only values used for development. * Address reviewer comments. * Remove infeasible testing structures, add documentation, and clean up implementation to prepare for code review. * Add notice that the dispatchers utility is temporary. * Cleanup new LiveData bridge, add tests for it, and migrate other DataProviders tests to using TestCoroutineDispatchers utility. * Add AsyncResult tests, fix & re-enable an earlier test in PersistentCacheStoreTest, and fix FakeSystemClock so that it works properly in test environments. * Use ktlint to reformat TestCoroutineDispatchers per reviewer comment thread. * Reformat files failing linter check. * Reformat new DataProviders code. * Add new tests for the NestedTransformedDataProvider. * Address reviewer comment. * Address reviewer comments. * Address reviewer comments & fix lint issue. * Update hints & solutions handling to more closely follow the intended behavior graph (and added an actual visual representation of this graph in code to simplify maintaining the solution in the future). Tests will be added as part of solving #1273. Co-authored-by: vinitamurthi <[email protected]> Co-authored-by: Ben Henning <[email protected]> Co-authored-by: James Xu <[email protected]>
nikitamarysolomanpvt
added a commit
that referenced
this pull request
Jun 16, 2020
* Initial check in for the question data controller * Removed progress controller test * Address the renames suggested in #217. * Introduce thoroughly stubbed QuestionAssessmentProgressController + empty test suite. * Review changes * Review changes * Pass data providers to the assessment controller, and set caps in the training controller * Removed unnecessary imports/variables * Add fake question data for stubbed interfaces * Remove duplicate questions while fetching in training controller * Comment explaining the filter function * Improve duplicate checking - check it while filtering instead of after filtering * Add linked skill id values * Review changes * add a new module for questions constants * Review changes * add a test to verify questions were fetched properly * reformatted code * Re-add QuestionTrainingController removed in merge. * Finalize question progress controller interface. * Implement QuestionAssessmentProgressController. This includes some basic refactoring of internal structures used by the exploration progress controller to share common functionality between the two progress controllers. There's still some duplication, but this seems like a reasonable split since there's likely to be further differences in the progress controllers in the future. The question assessment progress controller tests pass, but no new ones have yet been added to thoroughly test the implementation. * Add initial phase of tests for the progress controller based on ExplorationProgressController. They haven't yet been verified as correct. * Fix typo in QuestionTrainingController. * Post-merge fixes and adjustments. * Fix broken ProfileManagementControllerTest. * Post-merge fixes. * Update tests to build, but not pass. * Started UI * setup ViewModel * added binding adapter * Handles basic functionality * Moved code around * Added replay button * Make questions partially work. This introduces back button behavior to ensure training sessions can be reset. This also attempts to introduce proper asset loading, but it doesn't seem to be working currently. * Temporarily remove check in StateDeck that breaks questions. * Ensure all state navigation buttons are on the same line and the same size. This required reworking how nav button binding works in the state fragment such that multiple models are used depending on which type of navigation scenario the fragment is currently in. * Fix dp adjustment that caused previous button to disappear. Fix previous button showing up twice for continue interaction. * Undo .idea/misc.xml change that snuck in. * Update & fix StateFragmentTest (at least for Espresso). See #495 description for details on the changes necessary to make these tests work. * Post-merge fixes with some temporary stop-gap solutions. * Fix questions image rendering and consolidate GCS resource bucket names. * Fix question session not properly being reinitialized for new sessions. Also, ensure that notifications do not regenerate the training session. * Fix index accounting for questions. Before, the index incremented upon correct answer submission rather than navigation. * Remove support for backward navigation in questions since it isn't allowed. * Set title of question activity to 'Train Mode'. * Add support for a synthetic ephemeral question to represent the end of a training session. * Update question progress tracking to be translatable and to ensure that the synthetic terminal state shows the correct progress: 'completed'. * Move most state player functional support into a new configurable assembler class to enable sharing between the exploration and question players. * Hook up StatePlayerRecyclerViewAssembler to the question player fragment presenter. This also enables the congratulations text. * Introduce proper terminal page and disable overscroll. Note that the end session page introduced here has not been mocked yet, so it's mostly a placeholder until a mock is available. * Add support & button for training session replay. * Update/add TODOs to correspond to GitHub tracked issues. * Fix broken import. * Finish tests for Question{Training,AssessmentProgress}Controllers. Note that the nature of the assessment progress controller is such that a new, dynamic DataProvider is needed since the data provider for the current assessment question is actually a transformation of different data providers depending on which questions are passed to the controller. * Initial introduction of test coroutine dispatchers to replace Kotlin's test coroutine dispatcher. This includes introducing a test-only module to contain testing dependencies. * Introduce a new LiveData mechanism to bridge coroutines from DataProviders in a way that doesn't have the same limitations as the previous MutableLiveData-based bridge. * Introduce new nested data provider that effectively allows a DataProvider to be set up like transformAsync() but with the ability to change the root DataProvider. * question player progress-bar hifi * nit * revert * revert * Early work at introducing FakeSystemClock tests (not yet complete). * hifi lanscape question player * Post-merge fixes. * Fix the submit button disabled states: with the new refactor, the submit button is now properly disabled when an invalid answer is pending. * Revert accidental changes to .idea/misc.xml. * Address reviewer comments. * Remove unnecessary meta file for Mockito. * Minor import cleanup. * Post-merge fixes. * Fix audio playback & generalize it to facilitate enabling it for questions in the future. * Address reviewer comments & fix broken tests. * Fix hints & solutions functionality after simplification (some bugs & regressions were introduced due to the simplification; these have been fixed). * Revert the assembler delays to match the intended durations rather than the test-only values used for development. * Address reviewer comments. * Remove infeasible testing structures, add documentation, and clean up implementation to prepare for code review. * Add notice that the dispatchers utility is temporary. * Cleanup new LiveData bridge, add tests for it, and migrate other DataProviders tests to using TestCoroutineDispatchers utility. * Add AsyncResult tests, fix & re-enable an earlier test in PersistentCacheStoreTest, and fix FakeSystemClock so that it works properly in test environments. * Use ktlint to reformat TestCoroutineDispatchers per reviewer comment thread. * Reformat files failing linter check. * Reformat new DataProviders code. * Add new tests for the NestedTransformedDataProvider. * Address reviewer comment. * Address reviewer comments. * Address reviewer comments & fix lint issue. * Update hints & solutions handling to more closely follow the intended behavior graph (and added an actual visual representation of this graph in code to simplify maintaining the solution in the future). Tests will be added as part of solving #1273. * nit * nit Co-authored-by: vinitamurthi <[email protected]> Co-authored-by: Ben Henning <[email protected]> Co-authored-by: James Xu <[email protected]>
BenHenning
added a commit
that referenced
this pull request
Jul 17, 2020
* Initial check in for the question data controller * Removed progress controller test * Address the renames suggested in #217. * Introduce thoroughly stubbed QuestionAssessmentProgressController + empty test suite. * Review changes * Review changes * Pass data providers to the assessment controller, and set caps in the training controller * Removed unnecessary imports/variables * Add fake question data for stubbed interfaces * Remove duplicate questions while fetching in training controller * Comment explaining the filter function * Improve duplicate checking - check it while filtering instead of after filtering * Add linked skill id values * Review changes * add a new module for questions constants * Review changes * add a test to verify questions were fetched properly * reformatted code * Re-add QuestionTrainingController removed in merge. * Finalize question progress controller interface. * Implement QuestionAssessmentProgressController. This includes some basic refactoring of internal structures used by the exploration progress controller to share common functionality between the two progress controllers. There's still some duplication, but this seems like a reasonable split since there's likely to be further differences in the progress controllers in the future. The question assessment progress controller tests pass, but no new ones have yet been added to thoroughly test the implementation. * Add initial phase of tests for the progress controller based on ExplorationProgressController. They haven't yet been verified as correct. * Fix typo in QuestionTrainingController. * Post-merge fixes and adjustments. * Fix broken ProfileManagementControllerTest. * Post-merge fixes. * Update tests to build, but not pass. * Started UI * setup ViewModel * added binding adapter * Handles basic functionality * Moved code around * Added replay button * Make questions partially work. This introduces back button behavior to ensure training sessions can be reset. This also attempts to introduce proper asset loading, but it doesn't seem to be working currently. * Temporarily remove check in StateDeck that breaks questions. * Ensure all state navigation buttons are on the same line and the same size. This required reworking how nav button binding works in the state fragment such that multiple models are used depending on which type of navigation scenario the fragment is currently in. * Fix dp adjustment that caused previous button to disappear. Fix previous button showing up twice for continue interaction. * Undo .idea/misc.xml change that snuck in. * Update & fix StateFragmentTest (at least for Espresso). See #495 description for details on the changes necessary to make these tests work. * Post-merge fixes with some temporary stop-gap solutions. * Fix questions image rendering and consolidate GCS resource bucket names. * Fix question session not properly being reinitialized for new sessions. Also, ensure that notifications do not regenerate the training session. * Fix index accounting for questions. Before, the index incremented upon correct answer submission rather than navigation. * Remove support for backward navigation in questions since it isn't allowed. * Set title of question activity to 'Train Mode'. * Add support for a synthetic ephemeral question to represent the end of a training session. * Update question progress tracking to be translatable and to ensure that the synthetic terminal state shows the correct progress: 'completed'. * Move most state player functional support into a new configurable assembler class to enable sharing between the exploration and question players. * Hook up StatePlayerRecyclerViewAssembler to the question player fragment presenter. This also enables the congratulations text. * Introduce proper terminal page and disable overscroll. Note that the end session page introduced here has not been mocked yet, so it's mostly a placeholder until a mock is available. * Add support & button for training session replay. * Update/add TODOs to correspond to GitHub tracked issues. * Fix broken import. * Finish tests for Question{Training,AssessmentProgress}Controllers. Note that the nature of the assessment progress controller is such that a new, dynamic DataProvider is needed since the data provider for the current assessment question is actually a transformation of different data providers depending on which questions are passed to the controller. * Initial introduction of test coroutine dispatchers to replace Kotlin's test coroutine dispatcher. This includes introducing a test-only module to contain testing dependencies. * Introduce a new LiveData mechanism to bridge coroutines from DataProviders in a way that doesn't have the same limitations as the previous MutableLiveData-based bridge. * Introduce new nested data provider that effectively allows a DataProvider to be set up like transformAsync() but with the ability to change the root DataProvider. * question player progress-bar hifi * nit * revert * revert * Early work at introducing FakeSystemClock tests (not yet complete). * Post-merge fixes. * Fix the submit button disabled states: with the new refactor, the submit button is now properly disabled when an invalid answer is pending. * Revert accidental changes to .idea/misc.xml. * Address reviewer comments. * Remove unnecessary meta file for Mockito. * Minor import cleanup. * Post-merge fixes. * Fix audio playback & generalize it to facilitate enabling it for questions in the future. * Address reviewer comments & fix broken tests. * Fix hints & solutions functionality after simplification (some bugs & regressions were introduced due to the simplification; these have been fixed). * Revert the assembler delays to match the intended durations rather than the test-only values used for development. * Address reviewer comments. * Remove infeasible testing structures, add documentation, and clean up implementation to prepare for code review. * Add notice that the dispatchers utility is temporary. * Cleanup new LiveData bridge, add tests for it, and migrate other DataProviders tests to using TestCoroutineDispatchers utility. * Add AsyncResult tests, fix & re-enable an earlier test in PersistentCacheStoreTest, and fix FakeSystemClock so that it works properly in test environments. * Use ktlint to reformat TestCoroutineDispatchers per reviewer comment thread. * Reformat files failing linter check. * Reformat new DataProviders code. * Add new tests for the NestedTransformedDataProvider. * Address reviewer comment. * Address reviewer comments. * Address reviewer comments & fix lint issue. * Update hints & solutions handling to more closely follow the intended behavior graph (and added an actual visual representation of this graph in code to simplify maintaining the solution in the future). Tests will be added as part of solving #1273. * Add support for sharing the test application component between UI dependencies and tests. Also, fix the test coroutine dispatcher to properly manage time in tests. This enables the test coroutine dispatcher for app module tests. * Clean up the documentation for test utilities. * Add tests for hints & solutions. These are Robolectric-only tests since we need to carefully coordinate timing and very quickly drive through a bunch of different UI screens. * Address reviewer comment. * Address reviewer comments & fix broken tests after merging in develop. * Post-merge fixes. * Fix linter errors.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
This PR introduces the question data controller API and also adds the interface for the question progress controller. It also adds the proto file for questions.
A follow up PR will introduce the stubbed interface for the question list data.
Checklist