Playthrough Visualisation: Milestone 1.2 (Part 2) #4946

pranavsid98 · 2018-05-16T13:50:27Z

This PR implements Milestone 1.2 of the Playthrough Visualisation project (can be found at Learner Playthrough Visualization ).

The following features are implemented in this PR:

StorePlaythrough controller along with tests.

@seanlip @brianrodri PTAL! This is the most complex controller with a lot of conditionals, so I'll make another commit after adding more tests.

Thanks a lot

codecov-io · 2018-05-16T21:31:18Z

Codecov Report

Merging #4946 into develop will increase coverage by 0.39%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #4946      +/-   ##
===========================================
+ Coverage    44.16%   44.56%   +0.39%     
===========================================
  Files          386      387       +1     
  Lines        23289    23345      +56     
  Branches      3789     3800      +11     
===========================================
+ Hits         10286    10403     +117     
+ Misses       13003    12942      -61

Impacted Files	Coverage Δ
...ev/head/components/CodemirrorMergeviewDirective.js	`5.55% <0%> (-0.33%)`	⬇️
...v/head/pages/learner_dashboard/LearnerDashboard.js	`2.27% <0%> (-0.02%)`	⬇️
...main/utilities/FileDownloadRequestObjectFactory.js	`100% <0%> (ø)`	⬆️
...ev/head/domain/utilities/AudioFileObjectFactory.js	`100% <0%> (ø)`	⬆️
...ead/domain/utilities/AudioLanguageObjectFactory.js	`100% <0%> (ø)`	⬆️
...ilities/AutogeneratedAudioLanguageObjectFactory.js	`100% <0%> (ø)`	⬆️
...ev/head/services/stateful/BackgroundMaskService.js	`100% <0%> (ø)`	⬆️
...ev/head/domain/utilities/ImageFileObjectFactory.js	`100% <0%> (ø)`
...ploration_player/AudioTranslationManagerService.js	`75.75% <0%> (+0.75%)`	⬆️
...ctions/InteractiveMap/directives/InteractiveMap.js	`27.84% <0%> (+0.92%)`	⬆️
... and 4 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 0828d8c...0f19145. Read the comment docs.

brianrodri

Thanks Pranav, I've taken my first pass! I see mostly stylistic issues, the logic looks good to me!

brianrodri · 2018-05-17T01:12:08Z

core/controllers/reader.py

+        # either 'state_name' or 'state_names'.
+        state_name_in_customization_args = (
+            True
+            if 'state_name' in customization_args else False)


This can be simplified to

state_name_in_customization_args = 'state_name' in customization_args

Ahh. right. Makes sense. Done.

brianrodri · 2018-05-17T01:14:44Z

core/controllers/reader.py

+                            customization_args['state_name']))) or (
+                                not state_name_in_customization_args and (
+                                    issue_customization_args['state_names'] == (
+                                        customization_args['state_names']))):


This expression is rather hard to read, here are some ideas for improvement:

Move it to a function with many simple return True or return False statements.

Split the expression into more variables (like you've done with state_name_in_customization_args)

Did the first point.

brianrodri · 2018-05-17T01:17:05Z

core/controllers/reader.py

+            exp_issues.unresolved_issues.append(issue)
+
+        stats_services.save_exp_issues_model_transactional(
+            exp_issues)


This statement should fit on one line?

Ah true. Done.

brianrodri · 2018-05-17T01:22:00Z

core/controllers/reader_test.py

+        self.playthrough_data['issue_customization_args']['state_name'][
+            'value'] = 'state_name2'
+
+        self.post_json('/explorehandler/store_playthrough/%s' % (


nit: prefer keeping expressions together on a single line, e.g.:

self.post_json( '/explorehandler/store_playthrough/%s' % (self.exp_id), { 'playthrough_data': self.playthrough_data, 'exp_issue': self.exp_issue })

core/controllers/reader_test.py

+        """Test that a new cyclic playthrough gets added to the correct
+        cyclic issue when it exists.
+        """
+        playthrough_id = stats_models.PlaythroughModel.create(


brianrodri · 2018-05-17T01:23:34Z

core/controllers/reader_test.py

+        })
+        model.put()
+
+


nit: only use 2 blank lines between classes.

brianrodri · 2018-05-17T03:12:00Z

core/controllers/reader.py

+            customization_args: dict. The customization dict for the issue of
+                the playthrough to be stored.
+        """
+        if state_name_present:


Since all the data you need to determine this value is already passed in as parameters, it'd make more sense to make it inline:

if 'state_name' in issue_customization_args: # use 'state_name' else: # use 'state_names'

Or maybe even

state_name_key = ( 'state_name' if 'state_name' in issue_customization_args else 'state_names') return issue_customization_args[state_name_key] == customization_args[state_name_key]

Agreed -- having state_name_present passed in as an arg is icky.

Also this is quite fragile. Better to do it based on type: if issue_type = ... then do a certain check; else if issue_type = ... then do another check, etc. That way it's easy to augment when new issue types are added in the future.

Even better: have a static dict mapping from issue_type to issue_keyname (either 'state_name' or 'state_names'). Then get the relevant keyname from that dict and look it up. Make sure to add comments to explain what an "issue keyname" is and what it's used for.

Also, if you're using state_names as a key, is it sorted? Is [a, b, c] considered the same key as [b, c, a]? That's worth a comment.

@brianrodri @seanlip The last comment seems sensible to implement. I've added a mapping for the same, and its significantly reduced lines of code too. Thanks for pointing it out.

Re the comment for state_names, ordering is important and Ive added a comment to state the same.

brianrodri · 2018-05-17T03:18:56Z

core/controllers/reader.py

+            if issue['issue_id'] == exp_issue_dict['issue_id']:
+                issue_customization_args = issue['issue_customization_args']
+                # This check checks if the state name or the list of state names
+                # (depending on the issue type) are identical.


This comment doesn't belong here, the function's name does a fine job of explaining what's happening here. Move it to the function's docstring, since it describes the implementation.

brianrodri

Getting close, thanks!

Also, I still see some lines that should fit on a single line. It really helps readability to have less eye-movement, so try to fit expressions together into a single line when possible!

e.g., prefer:

someFunctionCall(
    somethingElse('with string', 'and int:', 5),
    andThen(
        'some long string that takes up a lot of space',
        # '[32,' could fit on previous line, but should remain "tight" with entire list.
        [32, 55]),
    5)

brianrodri · 2018-05-17T03:25:10Z

core/controllers/reader.py

+class StorePlaythroughHandler(base.BaseHandler):
+    """Handles a useful playthrough coming in from the frontend to store it."""
+
+    REQUIRE_PAYLOAD_CSRF_CHECK = False


Please add a comment to explain what this is/what it is used for.

Actually @pranavsid98 this seems incorrect. We should have a CSRF check. Why bypass it?

brianrodri · 2018-05-17T03:27:48Z

core/controllers/reader.py

+                if self._does_exp_issue_exist_in_exp_issues_model(
+                        state_name_in_customization_args,
+                        issue_customization_args, customization_args):
+                    if len(issue['playthrough_ids']) < (


nit: move the parenthesis to wrap the entire expression:

if (len(issue['playthrough_ids']) < feconf.MAX_PLAYTHROUGHS_FOR_ISSUE):

brianrodri · 2018-05-17T03:30:06Z

core/controllers/reader_test.py

+
+        self.login(self.VIEWER_EMAIL)
+        self.signup(self.VIEWER_EMAIL, self.VIEWER_USERNAME)
+        exp_services.delete_demo(self.exp_id)


Does this need to be run for the very first test, or is it to clean-up just in case a demo already exists? In that case, maybe this belongs in def tearDown?

Actually, its not required. Ive removed it.

OK, please commit that then!

brianrodri · 2018-05-17T03:31:46Z

core/controllers/reader_test.py

+                        'value': 'state_name1'
+                    }
+                }
+            }], True)


nit: use keyword argument here so it's clearer "what" is going to be set to True.

brianrodri · 2018-05-17T03:33:35Z

core/controllers/reader_test.py

+                        'value': 'state_name1'
+                    }
+                }
+            }], True)


Same, please add keyword argument label here.

seanlip · 2018-05-17T04:28:34Z

core/domain/stats_domain.py

@@ -420,7 +420,7 @@ def __init__(self, exp_id, unresolved_issues):
                represents an issue along with the associated playthroughs.
                Each dict will be of the form:
                {
-                    issue_type: str. Type of the issue.
+                    issue_id: str. ID of the issue.


As an FYI I think issue_type is better. An ID uniquely identifies an issue.

(We got this wrong with interactions -- we should actually have used interaction_type instead of interaction_id, oh well. But let's not propagate that.)

pranavsid98 · 2018-05-18T08:24:44Z

@brianrodri @seanlip This is good to go for another pass. PTAL! Thanks :)

brianrodri

Excellent, looks good to me! Have a few more round of requested changes before we can merge :)

brianrodri · 2018-05-18T15:27:12Z

core/controllers/reader.py

+
+    @acl_decorators.can_play_exploration
+    def post(self, exploration_id):
+        """Handles POST requests.


Please also document what happens to the POST requests, e.g., "appends to existing list of playthroughs or deletes it if already full." It should give me an idea of what the code will do so it's easy to review when I actually read over the code (both today and in the future).

brianrodri · 2018-05-18T15:30:25Z

core/controllers/reader_test.py

+
+        self.login(self.VIEWER_EMAIL)
+        self.signup(self.VIEWER_EMAIL, self.VIEWER_USERNAME)
+        exp_services.delete_demo(self.exp_id)


OK, please commit that then!

brianrodri · 2018-05-18T15:34:14Z

core/controllers/reader_test.py

+        """Test that a new playthrough gets created and a new issue is created
+        for it.
+        """
+        self.exp_issue['customization_args']['state_name'][


nit: use parenthesis here since the indexing fits on one line:

self.exp_issue['customization_args']['state_name']['value'] = ( 'state_name2')

brianrodri · 2018-05-18T15:37:38Z

core/domain/stats_services.py

+    ExplorationIssues domain object.
+
+    Args:
+        exp_issues. ExplorationIssues. The exploration issues domain


should use a ":" here,

exp_issues: ExplorationIssues.

brianrodri · 2018-05-18T15:37:48Z

core/domain/stats_services.py

+    ExplorationIssues domain object in a transaction.
+
+    Args:
+        exp_issues. ExplorationIssues. The exploration issues domain


brianrodri

Whoops, forgot to hit Request changes

seanlip · 2018-05-18T22:54:20Z

@pranavsid98 FYI I'll take one more pass later this evening.

seanlip

Sorry for the delay, @pranavsid98. Took a careful pass, PTAL.

seanlip · 2018-05-19T08:05:01Z

core/controllers/reader.py

+class StorePlaythroughHandler(base.BaseHandler):
+    """Handles a useful playthrough coming in from the frontend to store it."""
+
+    @acl_decorators.can_play_exploration


Why "play"? Shouldn't it at least be edit access?

@seanlip Nope. A playthrough of a learner is what's being stored right? The learner user doesnt need to have edit access right?

Why do you want to allow learners to look at other learners' playthroughs?

@seanlip Im a bit confused here. What does this decorator mean? Doesnt it mean that the user that is sending the request should have that particular permission? So, the StorePlaythroughController is sent from the front end during a learner session and technically from the learner user. Why would he need edit access?
Please correct me if Im understanding this wrong here.

Oh, sorry! It's me who understood it wrongly. You're correct. Thank you for pushing back.

seanlip · 2018-05-19T08:07:25Z

core/controllers/reader.py

+                    else:
+                        # In this case, the playthrough instance created doesn't
+                        # need to be stored, so it is deleted.
+                        stats_models.PlaythroughModel.get(


I don't think it makes sense to speculatively create then delete (two datastore calls). Can we check whether we want to create it first, and then, if so, only create it then?

True. I modified it to do create only when required instead.

seanlip · 2018-05-19T08:09:00Z

core/controllers/reader.py

+
+        issue_found = False
+        for index, issue in enumerate(exp_issues.unresolved_issues):
+            if issue['issue_type'] == exp_issue_dict['issue_type']:


Is there any guarantee that each issue in the list of unresolved issues has a different (type, key) tuple? If so, where is this enforced?

@seanlip Whenever any issue is added to this list, it is done only if the combination of (type, key) doesnt already exist. Isn't that sufficient enough to know there are no duplicates?

Yes as long as this is the only mechanism for adding new issues.

seanlip · 2018-05-19T08:10:08Z

core/controllers/reader.py

+                    'issue_customization_args'],
+                'playthrough_ids': [playthrough_id]
+            }
+            exp_issues.unresolved_issues.append(issue)


Any problem if there's already a resolved issue with the same issue type and customization args? Will it lead to duplicates when the unresolved issue subsequently gets resolved?

@seanlip Our current system of handling issue resolving is to just remove the issue from the list and delete all associated playthroughs. So, if an issue has the same type and customization args and we've resolved an issue before of the same type, the new issue will still get recorded. This behaviour is desired since the issue is cropping up even after the creator resolves an issue.

OK, sounds good, thanks!

core/controllers/reader_test.py

+        """Test that a new cyclic playthrough gets added to the correct
+        cyclic issue when it exists.
+        """
+        playthrough_id = stats_models.PlaythroughModel.create(


seanlip · 2018-05-19T08:12:41Z

core/domain/issue_registry.py

@@ -35,7 +35,7 @@ def get_all_issue_ids(cls):
        Returns:
            list(str). The list of all allowed issue IDs.
        """
-        return feconf.ALLOWED_ISSUE_IDS
+        return feconf.ALLOWED_ISSUE_TYPES


docstring says ids, this says types, please standardize.

seanlip · 2018-05-19T08:13:44Z

core/storage/statistics/gae_models.py

-    issue_id = ndb.StringProperty(indexed=True)
-    # The customization args dict for the given issue_id.
+    # Type of the issue.
+    issue_type = ndb.StringProperty(indexed=True)


Probably want a choices parameter too? See e.g. #4943.

@seanlip Shouldn't we be trying to define all our issue/action types in feconf and use it from there? Is this actually required?

In general the more validation the better. So yes.

(In fact it seems to me like you'll want to add required=True in a bunch of places, too.)

seanlip · 2018-05-19T08:14:53Z

core/storage/statistics/gae_models.py

-    # The customization args dict for the given issue_id.
+    # Type of the issue.
+    issue_type = ndb.StringProperty(indexed=True)
+    # The customization args dict for the given issue_type.
    issue_customization_args = ndb.JsonProperty(default=None)
    # The playthrough actions for this playthrough. This will be a list of dicts


Should be ndb.JsonProperty(repeated=True) to signify list

seanlip · 2018-05-19T08:15:28Z

core/storage/statistics/gae_models.py

            issue_customization_args: dict. The customization args dict for the
-                given issue_id.
+                given issue_type.
            playthrough_actions: list(dict). The playthrough actions for this
                playthrough. This will be a list of dicts where each dict
                represents a single playthrough action. The list is ordered by


two lines below: bool not Bool.

seanlip · 2018-05-19T08:16:15Z

feconf.py

    'ExplorationStart',
    'AnswerSubmit',
    'ExplorationQuit'
 ]
-
+# Mapping from issue type to issue keyname in the issue customization dict. This
+# particular mapping is to distinguish between issues which have their keyname


This second sentence isn't great because new types of issues may be added in the future.

Instead, say something about how the field can be used to uniquely identify/characterize the issue.

@seanlip Does it seem better now?

seanlip · 2018-05-20T19:33:50Z

core/controllers/reader.py

+class StorePlaythroughHandler(base.BaseHandler):
+    """Handles a useful playthrough coming in from the frontend to store it."""
+
+    @acl_decorators.can_play_exploration


Oh, sorry! It's me who understood it wrongly. You're correct. Thank you for pushing back.

seanlip · 2018-05-20T19:34:13Z

core/controllers/reader.py

+
+        issue_found = False
+        for index, issue in enumerate(exp_issues.unresolved_issues):
+            if issue['issue_type'] == exp_issue_dict['issue_type']:


Yes as long as this is the only mechanism for adding new issues.

seanlip · 2018-05-20T19:34:36Z

core/controllers/reader.py

+                    'issue_customization_args'],
+                'playthrough_ids': [playthrough_id]
+            }
+            exp_issues.unresolved_issues.append(issue)


OK, sounds good, thanks!

seanlip · 2018-05-20T19:35:51Z

core/storage/statistics/gae_models.py

-    issue_id = ndb.StringProperty(indexed=True)
-    # The customization args dict for the given issue_id.
+    # Type of the issue.
+    issue_type = ndb.StringProperty(indexed=True)


In general the more validation the better. So yes.

(In fact it seems to me like you'll want to add required=True in a bunch of places, too.)

seanlip · 2018-05-20T19:36:43Z

core/storage/statistics/gae_models.py

    issue_customization_args = ndb.JsonProperty(default=None)
    # The playthrough actions for this playthrough. This will be a list of dicts
    # where each dict represents a single playthrough action. The list is
    # ordered by the time of occurence of the action.
-    playthrough_actions = ndb.JsonProperty(default=None)
+    playthrough_actions = ndb.JsonProperty(default=None, repeated=True)


I think if repeated is specified, you don't need default -- the default value will just be []. Having, additionally, a default of None seems confusing.

(Please double-check the relevant App Engine docs on ndb datastore properties before resolving this.)

seanlip · 2018-05-20T19:42:53Z

core/controllers/reader.py

-                        # In this case, the playthrough instance created doesn't
-                        # need to be stored, so it is deleted.
-                        stats_models.PlaythroughModel.get(
-                            playthrough_id).delete()
                    issue_found = True


nit: consider moving this statement up to line 331, since it is at that point in the logic that you are certain that the issue has been found. Easier to read and follow the code that way.

seanlip · 2018-05-20T19:47:45Z

core/storage/statistics/gae_models.py

@@ -1044,6 +1044,26 @@ def create(
        playthrough_instance.put()
        return instance_id

+    @classmethod
+    def create_from_dict(cls, playthrough_data):


See above warning about implicitly trusting the playthrough data dict.

Also the layering is a bit off. Stuff like to_dict() and from_dict() should go in the domain layer. See how other objects are saved/created; you shouldn't be passing raw dicts to the storage layer and expecting the storage layer to interpret those (unstructured) dicts. Passing individual args is fine, and calling the constructor directly (like we do elsewhere) is also fine.

seanlip · 2018-05-20T19:48:53Z

extensions/actions/base_test.py

-EXPLORATION_START_ID = 'ExplorationStart'
-ANSWER_SUBMIT_ID = 'AnswerSubmit'
-EXPLORATION_QUIT_ID = 'ExplorationQuit'
+EXPLORATION_START_TYPE = 'ExplorationStart'


ACTION_TYPE_EXPLORATION_START
ACTION_TYPE_ANSWER_SUBMIT
ACTION_TYPE_EXPLORATION_QUIT

Btw for this and the file below, why are these constants being defined in tests? Shouldn't we be using the canonical references for these constants (like feconf or something)?

Unless your explicit purpose is to mock these, but in that case, it should be something like ACTION_TYPE_FAKE_ACTION_FOR_TESTS to make the intention clear.

seanlip · 2018-05-20T19:49:05Z

extensions/issues/base_test.py

-EARLY_QUIT_ID = 'EarlyQuit'
-MULTIPLE_INCORRECT_SUBMISSIONS_ID = 'MultipleIncorrectSubmissions'
-CYCLIC_STATE_TRANSITIONS_ID = 'CyclicStateTransitions'
+EARLY_QUIT_TYPE = 'EarlyQuit'


ISSUE_TYPE_EARLY_QUIT, etc.

seanlip · 2018-05-20T19:51:29Z

feconf.py

-# IDs of allowed types of issues.
-ALLOWED_ISSUE_IDS = [
+# Types of allowed issues.
+ALLOWED_ISSUE_TYPES = [
    'EarlyQuit',


If you need to, you can also make these individual strings into named constants, and then have

ALLOWED_ISSUE_TYPES = [ISSUE_TYPE_EARLY_QUIT, ...]

Also do these constants really need to be in feconf? If they're being referenced by multiple modules or controllers then maybe that makes sense since that means they're in "shared" territory. But if their usage is fairly narrow then maybe they'd be better placed somewhere like stats_models instead.

seanlip · 2018-05-20T19:54:54Z

feconf.py

-
+# Mapping from issue type to issue keyname in the issue customization dict. This
+# mapping is useful to uniquely identify issues by the combination of their
+# issue type and the concerend state(s).


typo: concerned

Also, change last part to: ... of their issue type and other type-specific information (such as the list of states involved).

This gives more wiggle room in the future if we introduce keys for issues that are not related to states.

seanlip · 2018-05-21T02:10:15Z

@pranavsid98 I don't see any new changes to review. Did you push your commits?

seanlip · 2018-05-21T20:14:02Z

core/controllers/reader.py

+        for playthrough_property in playthrough_properties:
+            if playthrough_property not in playthrough_data:
+                raise self.InvalidInputException(
+                    '%s not in playthrough data dict.' % (playthrough_property))


no need parens around playthrough_property

seanlip · 2018-05-21T20:16:35Z

core/controllers/reader.py

+                playthrough_data['issue_type'],
+                playthrough_data['issue_customization_args'],
+                playthrough_data['playthrough_actions'],
+                playthrough_data['is_valid'])
            issue = {


This should be an Issue domain object, right? Not a dict? (Otherwise unresolved_issues should be unresolved_issues_dicts.)

Generally we convert to dicts only when storing as jsonproperty, but when retrieving then we convert everything (+ subdicts) into domain objects and work with domain objects throughout the logic layer and higher.

Done. Required a major revamp to change all sub dicts into sub domain objects, so PTAL @seanlip

seanlip · 2018-05-21T20:18:07Z

core/storage/statistics/gae_models.py

    # The customization args dict for the given issue_type.
-    issue_customization_args = ndb.JsonProperty(default=None)
+    issue_customization_args = ndb.JsonProperty(default=None, required=True)


Why do you need a default value at all here? None is never valid, right?

seanlip

Looks good on the whole, only one issue with schema versions I think.

seanlip · 2018-05-22T06:27:16Z

core/controllers/reader_test.py

@@ -1361,10 +1360,10 @@ def test_playthrough_gets_added_to_cyclic_issues(self):
            'is_valid': True
        }

-        self.exp_issue = {
+        self.exp_issue_dict = {
            'issue_type': 'CyclicStateTransitions',
            'schema_version': 1,


Here and below, I thought you were removing the schema_version key from the stuff that gets passed in?

seanlip · 2018-05-22T06:32:19Z

core/domain/stats_domain.py

        """
        self.issue_type = issue_type
-        self.schema_version = schema_version
-        self.customization_args = customization_args
+        self.schema_version = stats_models.CURRENT_ISSUE_SCHEMA_VERSION


This seems off. Shouldn't schema_version be passed in here? If you're loading an old issue from the database you can't just change its schema version like this, you need to actually do a migration like in the Exploration object.

seanlip · 2018-05-22T06:33:44Z

core/domain/stats_domain.py

-            'schema_version': self.schema_version,
-            'customization_args': exp_domain.get_full_customization_args(
-                self.customization_args,
+            'issue_customization_args': exp_domain.get_full_customization_args(


Technically this has nothing to do with exp_domain. Perhaps move get_full_customization_args to utils.py instead if multiple parts of the code from different places are using it.

OK to do this in a follow-up PR, but please make sure to track it with an issue and that it actually gets done.

Filed a followup issue.

(For record-keeping: please refer to the follow-up issue number in your comment.)

Note: field as #4968.

seanlip · 2018-05-22T06:34:11Z

core/domain/stats_domain.py

                single key, 'value', whose corresponding value is the value of
                the customization arg.
        """
        self.action_type = action_type
-        self.schema_version = schema_version
-        self.customization_args = customization_args
+        self.schema_version = stats_models.CURRENT_ACTION_SCHEMA_VERSION


seanlip

Almost there!

seanlip · 2018-05-23T07:54:07Z

core/controllers/reader.py

@@ -318,6 +318,7 @@ def _require_exp_issue_dict_is_valid(self, exp_issue_dict):

        dummy_exp_issue = stats_domain.ExplorationIssue(
            exp_issue_dict['issue_type'],
+            stats_models.CURRENT_ISSUE_SCHEMA_VERSION,


You probably need to pass the schema version in from the frontend and run this through the upgrade process as needed, rather than just assign CURRENT_ISSUE_SCHEMA_VERSION. This is because the server may update "under" the learner, who is using a frontend that sends a dict following the old schema version.

Or, perhaps simpler, you could just discard the data if the schema version sent in the data doesn't match the schema version in the backend.

Either way, I don't think you can just assume that the latest schema version applies.

Ditto below for actions.

@seanlip That may be true. But, even if the server does update while the front end is sending a dict, it doesn't matter right? After we store it, whenever we use get_from_model(), its going to update the dict from the stored version to the later version anyway. Thoughts?

Edit: Wait, my bad. I get it, lets pass the schema version in from the front end. Ignore the comment.

seanlip · 2018-05-23T07:54:42Z

core/controllers/reader.py

@@ -413,6 +423,7 @@ def post(self, exploration_id):
                playthrough_data['is_valid'])
            issue = stats_domain.ExplorationIssue(
                playthrough_data['issue_type'],
+                stats_models.CURRENT_ISSUE_SCHEMA_VERSION,


Ditto, don't just assume current schema version

seanlip · 2018-05-23T07:55:23Z

core/controllers/reader_test.py

@@ -1362,7 +1366,6 @@ def test_playthrough_gets_added_to_cyclic_issues(self):

        self.exp_issue_dict = {
            'issue_type': 'CyclicStateTransitions',
-            'schema_version': 1,


Looks like you've now deleted this here and added it everywhere else ...

Why isn't this getting caught as a problem?

@seanlip For both actions and issues passed from the front end, I was not initially passing the schema version in, but now that im doing that, Ill fix these tests.

seanlip · 2018-05-23T07:55:58Z

core/controllers/reader_test.py

@@ -1437,7 +1442,6 @@ def test_cyclic_issues_of_different_order_creates_new_issue(self):

        self.exp_issue_dict = {
            'issue_type': 'CyclicStateTransitions',
-            'schema_version': 1,


Ditto. Unless this is meant to be part of a test that checks that incoming dicts with no schema version are discarded (and if that test doesn't already exist, it should).

seanlip · 2018-05-23T07:56:31Z

core/domain/stats_domain.py

@@ -601,19 +601,21 @@ class ExplorationIssue(object):
    """Domain object representing an exploration issue."""

    def __init__(
-            self, issue_type, issue_customization_args, playthrough_ids):
+            self, issue_type, schema_version, issue_customization_args,


Odd to separate issue type and issue customization args by schema version. Maybe move schema version to the beginning or the end (since it describes all the other stuff). (Probably end.)

@seanlip Actually, my train of thought was this: The issue type kind of identifies this exploration issue first. Then, we have the schema version. The issue type and schema version together point to a unique customization_args . When I thought about it like that, this seemed to make more sense?

seanlip · 2018-05-23T07:59:51Z

core/domain/stats_domain_test.py

                'issue_customization_args': expected_customization_args,
                'playthrough_ids': []
            })

+    def test_update_exp_issue_from_model(self):


This is a really well-written test! Very nice.

seanlip · 2018-05-23T08:00:50Z

core/domain/stats_services.py

@@ -31,6 +32,68 @@
 VERSION_ALL = 'all'


+def _migrate_issue_schema(exp_issue_dict):
+    """Holds the responsibility of performing a step-by-step sequential update
+    of an exploration issue structure based on the schema version of the input.


exploration issue structure --> exploration issue dict

based on the schema version of the input --> based on its schema version

seanlip · 2018-05-23T08:02:39Z

core/domain/stats_services.py

+
+def _migrate_action_schema(learner_action_dict):
+    """Holds the responsibility of performing a step-by-step sequential update
+    of an learner action structure based on the schema version of the input.


See above, similar comments apply

seanlip · 2018-05-23T08:05:17Z

core/domain/stats_services.py

@@ -31,6 +32,68 @@
 VERSION_ALL = 'all'


+def _migrate_issue_schema(exp_issue_dict):


_migrate_to_latest_issue_schema

seanlip · 2018-05-23T08:05:30Z

core/domain/stats_services.py

+        issue_schema_version += 1
+
+
+def _migrate_action_schema(learner_action_dict):


ditto: naming (there should be a notion of "latest")

seanlip

Almost there!

seanlip · 2018-05-23T18:44:35Z

Hi Pranav -- the GitHub page is down, so I can't actually look at your PR, but I was able to review the individual commit <0f19145> and I left two suggestions. They're nits, so basically this PR LGTM. Except that I can't actually approve it, because the GitHub PR page is down. I have sent a support request and will let you know if this changes.

brianrodri · 2018-05-23T18:57:40Z

Hmm nope, I'm getting the Unicorn! page as well

…

On Wed, May 23, 2018 at 2:44 PM Sean Lip ***@***.***> wrote: Hi Pranav -- the GitHub page is down, so I can't actually look at your PR, but I was able to review the individual commit <0f19145> and I left two suggestions. They're nits, so basically this PR LGTM. Except that I can't actually approve it, because the GitHub PR page is down. I have sent a support request and will let you know if this changes. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4946 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE26rOlbGzpktFT1LJn_P01XBZMVVtL6ks5t1a4VgaJpZM4UBWX2> .

introduce controller and tests

7924d5b

pranavsid98 requested review from brianrodri and seanlip May 16, 2018 13:50

pranavsid98 added 2 commits May 16, 2018 19:24

remove print statement

1d1bb37

added decorator

52b5136

added more tests

f82e943

brianrodri suggested changes May 17, 2018

View reviewed changes

addressed review changes

5b8592b

brianrodri suggested changes May 17, 2018

View reviewed changes

seanlip reviewed May 17, 2018

View reviewed changes

pranavsid98 added 3 commits May 18, 2018 13:26

addressed changes

c47c596

Merge branch 'develop' into store_playthrough_controller

22cc37f

changed issue_id/action_id to issue_type/action_type

966e986

pranavsid98 assigned brianrodri and seanlip May 18, 2018

pranavsid98 mentioned this pull request May 18, 2018

Playthrough Visualisation: Milestone 1.2 (Part 3) #4952

Merged

brianrodri approved these changes May 18, 2018

View reviewed changes

brianrodri suggested changes May 18, 2018

View reviewed changes

addressed all review changes

fd9b53c

brianrodri approved these changes May 18, 2018

View reviewed changes

seanlip requested changes May 19, 2018

View reviewed changes

pranavsid98 added 2 commits May 20, 2018 13:52

addressed sean's review comments

ca4c0b1

fixed backend tests

89f63b3

seanlip requested changes May 20, 2018

View reviewed changes

seanlip reviewed May 20, 2018

View reviewed changes

pranavsid98 added 2 commits May 21, 2018 13:43

addressed all review comments

c9a19a1

lint fixes

66f05c7

seanlip reviewed May 21, 2018

View reviewed changes

sub dicts converted to domain objects

6bc06e4

seanlip requested changes May 22, 2018

View reviewed changes

pranavsid98 mentioned this pull request May 22, 2018

Move get_full_customization_args from exp_domain to utils #4968

Closed

schema version migrations

7dab178

seanlip reviewed May 23, 2018

View reviewed changes

addressed review comments

0f19145

seanlip approved these changes May 23, 2018

View reviewed changes

brianrodri merged commit f5426da into oppia:develop May 23, 2018

pranavsid98 deleted the store_playthrough_controller branch May 24, 2018 05:05

pranavsid98 mentioned this pull request Jun 2, 2018

Playthrough Visualisation: Milestone 2.1 #5015

Merged

		@@ -31,6 +32,68 @@
		VERSION_ALL = 'all'


		def _migrate_issue_schema(exp_issue_dict):

		issue_schema_version += 1


		def _migrate_action_schema(learner_action_dict):

Playthrough Visualisation: Milestone 1.2 (Part 2) #4946

Playthrough Visualisation: Milestone 1.2 (Part 2) #4946

Conversation

pranavsid98 commented May 16, 2018 • edited Loading

codecov-io commented May 16, 2018 • edited Loading

Codecov Report

brianrodri left a comment

Choose a reason for hiding this comment

brianrodri May 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianrodri May 17, 2018 • edited Loading

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

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianrodri May 17, 2018 • edited Loading

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

brianrodri May 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianrodri left a comment • edited Loading

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

brianrodri May 17, 2018 • edited Loading

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

pranavsid98 commented May 18, 2018

brianrodri 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

brianrodri left a comment

Choose a reason for hiding this comment

seanlip commented May 18, 2018

seanlip 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

pranavsid98 commented May 16, 2018 •

edited

Loading

codecov-io commented May 16, 2018 •

edited

Loading

brianrodri May 17, 2018 •

edited

Loading

brianrodri May 17, 2018 •

edited

Loading

brianrodri May 17, 2018 •

edited

Loading

brianrodri May 17, 2018 •

edited

Loading

brianrodri left a comment •

edited

Loading

brianrodri May 17, 2018 •

edited

Loading