New state structure. #5007

DubeySandeep · 2018-05-31T14:54:12Z

Explanation

This PR changes the state structure.

Replaces audio_translations to content_id in SubtitledHtml.
Adds Content_ids_to _audio_translation to connect audio translations with SubtitledHtml.

(Doc can be found here)

CheckList:

seanlip · 2018-05-31T20:03:36Z

Hi @DubeySandeep, please note merge conflicts.

Also, might be worth making a note of what remaining work is needed on this PR in the form of a checklist? This helps the reviewer understand what the current status is, so they don't comment on stuff you are planning to do anyway.

…o new_state

tjiang11

Thanks @DubeySandeep ! I left some comments, I think it looks pretty good overall, but it's pretty clear this needs some tests.

I also tried running it and it errors out. It seems like a circular dependency has been introduced by the id services. Could you take a look at that? Thanks!

tjiang11 · 2018-06-02T06:03:18Z

core/domain/exp_domain.py

        """Initializes a SubtitledHtml domain object.

        Args:
+            content_id: str. An id for the object.


This description is extremely vague. Perhaps something more like "A unique id referring to the audio translations for this content."?

tjiang11 · 2018-06-02T06:08:19Z

core/domain/exp_domain.py

@@ -934,7 +910,7 @@ def __init__(self, hint_content):
        """Constructs a Hint domain object.

        Args:
-            hint_content: SubtitledHtml. The hint text and audio translations.
+            hint_content: SubtitledHtml. The hint text and content_id.


Maybe "The hint text and ID referring to the audio translations for this content."?

tjiang11 · 2018-06-02T06:11:04Z

core/domain/exp_domain.py

@@ -504,23 +506,20 @@ class SubtitledHtml(object):
    """Value object representing subtitled HTML."""

    DEFAULT_SUBTITLED_HTML_DICT = {
-        'html': '',
-        'audio_translations': {}
+        'content_id': feconf.DEFAULT_OUTCOME_CONTENT_ID,


Why is this DEFAULT_OUTCOME_CONTENT_ID specifically?

tjiang11 · 2018-06-02T06:13:09Z

core/domain/exp_domain.py

@@ -987,8 +963,8 @@ def __init__(
                False if is one of possible answer.
            correct_answer: str. The correct answer; this answer enables the
                learner to progress to the next card.
-            explanation: SubtitledHtml. Contains text and audio translations for
-                the solution's explanation.
+            explanation: SubtitledHtml. Contains text and text id to link audio


Make this comment similar/consistent with the other ones, such as hint_content in hint

tjiang11 · 2018-06-02T06:49:15Z

core/domain/exp_services.py

-                if default_outcome:
-                    default_outcome.feedback.audio_translations = {}
+            for content_id in state.content_ids_to_audio_translations:
+                state.content_ids_to_audio_translations[content_id] = {}


Would it matter that the content_id is still present within the different components?

Sorry, I didn't understand this one. Can you please explain a bit more?

Ah nevermind, realized this shouldn't matter, I think

tjiang11 · 2018-06-02T07:06:54Z

core/templates/dev/head/pages/exploration_editor/exploration_editor.html

@@ -221,6 +221,7 @@
  <script src="{{TEMPLATE_DIR_PREFIX}}/services/CodeNormalizerService.js"></script>
  <script src="{{TEMPLATE_DIR_PREFIX}}/services/ComputeGraphService.js"></script>
  <script src="{{TEMPLATE_DIR_PREFIX}}/services/EditabilityService.js"></script>
+  <script src="{{TEMPLATE_DIR_PREFIX}}/services/GenerateContendIdService.js"></script>


ContentIdService*

Ah, I have also misspelled the filename.
Done!

tjiang11 · 2018-06-02T07:08:19Z

core/templates/dev/head/services/GenerateContendIdService.js

+  function(stateContentIdsToAudioTranslationsService) {
+    var generateRandomString = function() {
+      randomString = '';
+      while (randomStringlength != 6){


Where is randomStringLength defined?

Also capitalize the l in length

tjiang11 · 2018-06-02T07:09:16Z

core/templates/dev/head/services/GenerateContendIdService.js

+      }
+    };
+    var _generateUniqueId = function() {
+      var ContentIdList = Object.keys(


contentIdList (camel case)

tjiang11 · 2018-06-02T07:10:49Z

core/templates/dev/head/services/GenerateContendIdService.js

+      while (ContentIdList.indexOf(uniqueId) >= 0) {
+        uniqueId = generateRandomString();
+      }
+    };


Shouldn't this function return something?

tjiang11 · 2018-06-02T07:11:20Z

utils.py

+        old_ids.
+    """
+    new_id = generate_random_string(6)
+    while(new_id in old_id):


DubeySandeep · 2018-06-03T05:33:26Z

core/templates/dev/head/components/hint_editor_directive.html

@@ -54,7 +54,8 @@

  <div ng-if="stateHintsService.savedMemento[getIndexPlusOne() - 1] && !stateHintsService.savedMemento[getIndexPlusOne() - 1].isEmpty()">
    <audio-translations-editor component-name="<[COMPONENT_NAME_HINT]>"
-                               subtitled-html="stateHintsService.displayed[getIndexPlusOne() - 1].hintContent"
+                               content-ids-to-audio-translations="stateContentIdsToAudioTranslationsService.displayed"


@seanlip, Can we remove this (also on-change) as this will be same in all other places?

DubeySandeep · 2018-06-03T05:39:24Z

core/templates/dev/head/components/hint_editor_directive.html

@@ -54,7 +54,8 @@

  <div ng-if="stateHintsService.savedMemento[getIndexPlusOne() - 1] && !stateHintsService.savedMemento[getIndexPlusOne() - 1].isEmpty()">
    <audio-translations-editor component-name="<[COMPONENT_NAME_HINT]>"
-                               subtitled-html="stateHintsService.displayed[getIndexPlusOne() - 1].hintContent"
+                               content-ids-to-audio-translations="stateContentIdsToAudioTranslationsService.displayed"
+                               conten-id="stateHintsService.displayed[getIndexPlusOne() - 1].hintContent.getContentId()"
                               on-start-edit="onAudioTranslationsStartEditAction"
                               on-change="onAudioTranslationsEdited">


@seanlip, onAudioTranslationsEdited we'll have to save the changes in stateContentIdsToAudioTranslationsService.saveDisplayedValue(); everywhere.

DubeySandeep · 2018-06-04T20:23:16Z

core/templates/dev/head/pages/exploration_editor/editor_tab/StateInteraction.js

            $scope.cancel = function() {
              $uibModalInstance.dismiss('cancel');
              AlertsService.clearWarnings();
            };
          }
        ]
      }).result.then(function() {
+        // TODO(SD): Clean Hints and Responses to resolve #4634.


@seanlip, is this an expected behavior?
We don't clean the stateHintsService while deleting an interaction? (I'll have to do the same thing for the audio part)

(Hm, looks like my comment showed up in the main thread. I'll reply inline.)

Yep I think it is expected. Scenario: creator wants to ask exactly the same question, but using a different interaction (eg number with units instead of just a number).

@seanlip,

Create an exploration add textInput interaction.

Add hint.

Delete Interaction

Add end exploration interaction.

Preview

Hint appears in "End exploration" card.

Is this expected?

Oh, that's a good point. I didn't think of that scenario. It makes sense for hints not to appear when the state does not have an interaction to which answers are submittable in the first place.

Could you please file a bug for this? I am open to fixing it in this PR or fixing it in a separate PR, and I think the latter might be better since this PR is getting large, but your call.

Sure! (I have created an issue for now #5063 I'll fix this in another PR)

DubeySandeep · 2018-06-04T20:34:00Z

@tjiang11, can you PTAL? I'll be modifying/writing tests after this.

seanlip · 2018-06-04T21:31:56Z

Yep I think it is expected. Scenario: creator wants to ask exactly the same question, but using a different interaction (eg number with units instead of just a number).

…

codecov-io · 2018-06-09T15:16:44Z

Codecov Report

Merging #5007 into develop will increase coverage by 0.02%.
The diff coverage is 58.69%.

@@             Coverage Diff             @@
##           develop    #5007      +/-   ##
===========================================
+ Coverage     45.2%   45.22%   +0.02%     
===========================================
  Files          409      411       +2     
  Lines        23891    23993     +102     
  Branches      3861     3858       -3     
===========================================
+ Hits         10800    10851      +51     
- Misses       13091    13142      +51

Impacted Files	Coverage Δ
...head/pages/exploration_editor/ChangeListService.js	`74.19% <ø> (ø)`	⬆️
...v/head/domain/exploration/SolutionObjectFactory.js	`81.39% <ø> (ø)`	⬆️
...mplates/dev/head/components/HintEditorDirective.js	`2.43% <0%> (+0.05%)`	⬆️
.../pages/exploration_editor/editor_tab/StateHints.js	`0.95% <0%> (-0.12%)`	⬇️
.../exploration_player/HintAndSolutionModalService.js	`2.43% <0%> (-0.27%)`	⬇️
...ates/dev/head/components/OutcomeEditorDirective.js	`1.03% <0%> (ø)`	⬆️
...d/components/SolutionExplanationEditorDirective.js	`2.56% <0%> (-0.22%)`	⬇️
...es/exploration_editor/editor_tab/StateResponses.js	`2.59% <0%> (-0.07%)`	⬇️
...ev/head/pages/exploration_player/PlayerServices.js	`0.88% <0%> (-0.02%)`	⬇️
...ges/exploration_editor/ExplorationStatesService.js	`44.62% <0%> (-1.49%)`	⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2edb01...be7480a. Read the comment docs.

tjiang11 · 2018-06-12T03:45:37Z

core/domain/exp_domain.py

+        if not set(content_id_list) <= set(available_content_ids):
+            raise utils.ValidationError(
+                'Expected state content_ids_to_audio_translations to have all '
+                'of the listed content ids %s' % content_id_list)


Might be worth making a test to test this case? Same with the duplicate content ids case

tjiang11 · 2018-06-12T03:52:47Z

core/templates/dev/head/domain/exploration/StatesObjectFactorySpec.js

@@ -203,7 +239,10 @@ describe('States object factory', function() {
            customization_args: {},
            default_outcome: {
              dest: 'new state',
-              feedback: [],
+              feedback: {
+                contnet_id: 'default_outcome',


Here and in about 20 other places, content*

tjiang11 · 2018-06-12T04:01:42Z

core/templates/dev/head/components/HintEditorDirective.js

@@ -29,9 +29,9 @@ oppia.directive('hintEditor', [
        '/components/hint_editor_directive.html'),
      controller: [
        '$scope', '$uibModal', 'EditabilityService', 'stateHintsService',
-        'COMPONENT_NAME_HINT',
+        'stateContentIdsToAudioTranslationsService', 'COMPONENT_NAME_HINT',


Are you still planning on renaming to AudioAssets?

Yeah, but I didn't find any other name, as these are not the audio assets these are just the metadata for audio files and their status. So maybe we can leave it as-is.

tjiang11 · 2018-06-12T04:11:02Z

core/templates/dev/head/domain/exploration/OutcomeObjectFactory.js

      return new Outcome(
        dest,
-        SubtitledHtmlObjectFactory.createDefault(feedbackText),
+        SubtitledHtmlObjectFactory.createDefault(feedbackText, feedbacktextId),


feedbackTextId (camel case)

tjiang11 · 2018-06-12T04:22:01Z

core/templates/dev/head/pages/exploration_editor/editor_tab/StateHints.js

@@ -115,10 +117,13 @@ oppia.controller('StateHints', [
            $scope.hintIndex = stateHintsService.displayed.length + 1;

            $scope.saveHint = function() {
+              var contentId = GenerateContentIdService.generateUniqueId(


Maybe instead of generateUniqueId, we should call it something more like getNextId? Unique Id makes it sound like it's a random string. Up to you though I'm ultimately okay with either.

* Changes in exp_domain for new state structure * Made changes in exp_services * changes made in SubtitledHtmlObjectFactory * changes made in SubtitledHtmlObjectFactory * Made changes in backend exp_domain for state validation and conversion * Added new GenerateContendIdService * Added domain object for contentIdsToAudioTranslations * Added changes to support exploration player for new changes * Fixed lint issues andmade few changes. * Exploration editor and player started working * Minor changes to make AudioTranslationBar double binded. * Added review changes. * Change branch * exp_services_test fix * Fixed other backend test from core.domain * Fixed frontend tests * Lint fix * Fixed e2e test(1) * Fixed backend tests * Added Spec files for ContentIdsToAudioTranslationsObjectFactory * Added Spec file for GenerateContentIdService * Added validators for citat in sate.validator and fixed tests * Removed DEFAULT_OUTCOME_CONTENT_ID * minor changes * Added e2e review comments changes * Changed all contnet to content * Added review changes * resolved lint errors for docsting

DubeySandeep added 4 commits May 29, 2018 14:14

Changes in exp_domain for new state structure

8565966

Made changes in exp_services

00be632

changes made in SubtitledHtmlObjectFactory

3d3b14a

changes made in SubtitledHtmlObjectFactory

2167bbe

DubeySandeep requested a review from tjiang11 May 31, 2018 14:54

Merge branch 'develop' of https://github.com/oppia/oppia into new_state

28e8b48

seanlip assigned tjiang11 May 31, 2018

DubeySandeep added 3 commits June 1, 2018 02:35

Removed merge conflicts from exp_services.py

6586813

Merge branch 'new_state' of https://github.com/DubeySandeep/oppia int…

6f60e98

…o new_state

Made changes in backend exp_domain for state validation and conversion

99c5f6e

DubeySandeep force-pushed the new_state branch from 8186489 to 99c5f6e Compare June 1, 2018 00:02

DubeySandeep added 2 commits June 1, 2018 11:53

Added new GenerateContendIdService

4ce40b9

Added domain object for contentIdsToAudioTranslations

6a20c5f

DubeySandeep force-pushed the new_state branch from cf96029 to 6a20c5f Compare June 2, 2018 01:30

tjiang11 suggested changes Jun 2, 2018

View reviewed changes

Added changes to support exploration player for new changes

6772e6a

DubeySandeep force-pushed the new_state branch from 38cf766 to 6772e6a Compare June 2, 2018 19:43

DubeySandeep commented Jun 3, 2018

View reviewed changes

DubeySandeep added 2 commits June 3, 2018 16:00

Merge branch 'develop' of https://github.com/oppia/oppia into new_state

aa6cd17

Fixed lint issues andmade few changes.

6b2af0d

DubeySandeep force-pushed the new_state branch from 37ef5ab to 0362f9b Compare June 4, 2018 20:16

DubeySandeep commented Jun 4, 2018

View reviewed changes

DubeySandeep force-pushed the new_state branch from 0362f9b to 3759006 Compare June 4, 2018 20:25

Exploration editor and player started working

5c1aff0

DubeySandeep force-pushed the new_state branch from 3759006 to 5c1aff0 Compare June 4, 2018 20:31

Minor changes to make AudioTranslationBar double binded.

a9956c8

DubeySandeep added 6 commits June 8, 2018 17:05

exp_services_test fix

3018fad

Resolved merge conflicts

de0fc2d

Fixed other backend test from core.domain

c85da1e

Fixed frontend tests

cb7151c

Merge branch 'develop' of https://github.com/oppia/oppia into new_state

e4c2dda

Lint fix

f368267

DubeySandeep changed the title ~~[WIP] New state structure.~~ New state structure. Jun 9, 2018

Fixed e2e test(1)

59d7f4d

DubeySandeep added 2 commits June 10, 2018 19:59

Fixed backend tests

e9c4939

Resolve merge conflicts

55e1b15

DubeySandeep force-pushed the new_state branch from 5d2870a to 55e1b15 Compare June 10, 2018 16:15

DubeySandeep added 7 commits June 12, 2018 02:10

Added Spec files for ContentIdsToAudioTranslationsObjectFactory

c4f3b31

Added Spec file for GenerateContentIdService

acb6d53

Added validators for citat in sate.validator and fixed tests

79067af

Updated with upstream develop

7c69f12

Removed DEFAULT_OUTCOME_CONTENT_ID

43b4077

minor changes

d7e1106

Added e2e review comments changes

72ebcfa

tjiang11 reviewed Jun 12, 2018

View reviewed changes

Changed all contnet to content

bd77e78

tjiang11 reviewed Jun 12, 2018

View reviewed changes

DubeySandeep added 2 commits June 12, 2018 16:25

Added review changes

5f10315

resolved lint errors for docsting

be7480a

tjiang11 approved these changes Jun 12, 2018

View reviewed changes

tjiang11 merged commit 150a076 into oppia:develop Jun 12, 2018

New state structure. #5007

New state structure. #5007

Conversation

DubeySandeep commented May 31, 2018 • edited

Explanation

CheckList:

seanlip commented May 31, 2018

tjiang11 left a comment • edited

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

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

DubeySandeep Jun 3, 2018 • edited

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

DubeySandeep commented Jun 4, 2018

seanlip commented Jun 4, 2018 via email

codecov-io commented Jun 9, 2018 • edited

Codecov Report

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

DubeySandeep Jun 12, 2018 • edited

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

DubeySandeep commented May 31, 2018 •

edited

tjiang11 left a comment •

edited

DubeySandeep Jun 3, 2018 •

edited

codecov-io commented Jun 9, 2018 •

edited

DubeySandeep Jun 12, 2018 •

edited