Fix Part of #13162: Add argument schema for ExplorationCompleteEventHandler, ExplorationMaybeLeaveHandler, SolutionHitEventHandler classes #14208

SD-13 · 2021-11-09T14:29:04Z

Overview

This PR fixes or fixes part of Write schemas for handler class arguments #13162.
This PR does the following: This PR adds schema for ExplorationCompleteEventHandler, ExplorationMaybeLeaveHandler, SolutionHitEventHandler classes.

Essential Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The linter/Karma presubmit checks have passed locally on your machine.
"Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)
- This lets reviewers restart your CircleCI tests for you.
The PR is made from a branch that's not called "develop".

Proof that changes are correct

PR Pointers

Make sure to follow the instructions for making a code change.
Oppiabot will notify you when you don't add a PR_CHANGELOG label. If you are unable to do so, please @-mention a code owner (who will be in the Reviewers list), or ask on Gitter.
For what code owners will expect, see the Code Owner's wiki page.
Make sure your PR follows conventions in the style guide, otherwise this will lead to review delays.
Never force push. If you do, your PR will be closed.
Oppiabot can assign anyone for review/help if you leave a comment like the following: "{{Question/comment}} @{{reviewer_username}} PTAL"
Some of the e2e tests are flaky, and can fail for reasons unrelated to your PR. We are working on fixing this, but in the meantime, if you need to restart the tests, please check the "If your build fails" wiki page.

oppiabot · 2021-11-09T14:29:17Z

Hi @vojtechjelinek, @aks681 -- could one of you please add the appropriate changelog label to this pull request? Thanks!

SD-13 · 2021-11-09T18:06:36Z

@aks681 @vojtechjelinek Please review my PR and tell me what changes are needed. Thanks

core/controllers/reader.py

vojtechjelinek

Thanks! Left a few comments.

vojtechjelinek · 2021-11-10T14:31:47Z

core/controllers/reader.py

+            },
+            'state_name': {
+                'schema': {
+                    'type': schema_utils.SCHEMA_TYPE_BASESTRING


I think we usually do not use constants for these simple strings (we do not use them in schemas, in other parts of the codebase we use them).

I have replaced those constants. Please tell me If it needs any other changes.

vojtechjelinek · 2021-11-10T14:32:52Z

core/controllers/reader.py

+            exploration_id, self.normalized_payload.get('exploration_version'),
+            self.normalized_payload.get('state_name'),
+            self.normalized_payload.get('session_id'),
+            self.normalized_payload.get('time_spent_in_state_secs'))


Suggested change

exploration_id, self.normalized_payload.get('exploration_version'),

self.normalized_payload.get('state_name'),

self.normalized_payload.get('session_id'),

self.normalized_payload.get('time_spent_in_state_secs'))

exploration_id,

self.normalized_payload.get('exploration_version'),

self.normalized_payload.get('state_name'),

self.normalized_payload.get('session_id'),

self.normalized_payload.get('time_spent_in_state_secs'))

Please let me know if I missed any changes here.

vojtechjelinek · 2021-11-10T14:41:47Z

core/controllers/reader.py

+            'params': {
+                'schema': {
+                    'type': 'object_dict',
+                    'validation_method': (
+                        domain_objects_validator.validate_params),
+                }
+            }


Why are the params send like this?

Can some more structured manner be used?

I think here I picked the wrong type. Later I found params arg in the domain layer and I have updated the schema accordingly. Please have a look and let me know if any changes are needed.

SD-13 · 2021-11-12T15:25:42Z

@vojtechjelinek PTAL. Thanks : )

oppiabot · 2021-11-12T15:28:46Z

Unassigning @SD-13 since a re-review was requested. @SD-13, please make sure you have addressed all review comments. Thanks!

…handlers_4

oppiabot · 2021-11-14T22:54:20Z

Hi @SD-13, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks!

oppiabot · 2021-12-01T22:42:26Z

Hi @SD-13, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks!

…handlers_4

Nik-09

Thank you @SD-13, added one comment.

Nik-09 · 2021-12-02T08:12:01Z

core/controllers/domain_objects_validator.py

+        params: dict. Data that needs to be validated.
+    """
+    if not isinstance(params, dict):
+        raise Exception('Excepted dict, received %s' % params)


I was asking to add a validation inorder to expect the type of the params to be dict, as you have done here.
Also verify that the keys in params dict are of string type.

Also can you modify the type annotation here

Yes, I found in many places that it is taking string-type keys.
previously I tried to add rules for both keys and values individually and that was causing lots of e2e failures and it seemed like they were transformed into string types before passing into the validator.
Also, I did not find anything that violates them. then what did you ask me to modify? Can you please explain

I think you can ignore this and leave it for now

vojtechjelinek

LGTM!

aks681

Lgtm as codeowner

oppiabot · 2021-12-03T13:22:58Z

Unassigning @aks681 since they have already approved the PR.

oppiabot · 2021-12-03T13:23:02Z

Hi @vojtechjelinek, this PR is ready to be merged. Author of this PR does not have permissions to merge this PR. Before you merge it, please make sure that there are no pending comments that require action from the author's end. Thanks!

SD-13 · 2021-12-03T13:24:24Z

Thanks, @aks681 @vojtechjelinek : )

Nik-09

@SD-13 Update the PR with upstream.

Nik-09 · 2021-12-03T13:53:43Z

core/controllers/domain_objects_validator.py

+        params: dict. Data that needs to be validated.
+    """
+    if not isinstance(params, dict):
+        raise Exception('Excepted dict, received %s' % params)


I think you can ignore this and leave it for now

oppiabot · 2021-12-03T13:57:44Z

Hi @vojtechjelinek, this PR is ready to be merged. Author of this PR does not have permissions to merge this PR. Before you merge it, please make sure that there are no pending comments that require action from the author's end. Thanks!

…handlers_4

SD-13 · 2021-12-03T14:07:38Z

@SD-13 Update the PR with upstream.

Done.

SD-13 added 2 commits November 9, 2021 13:36

adds schema for few handlers

28b02cb

updated according to lint checks

09b7e9e

SD-13 requested review from aks681 and vojtechjelinek as code owners November 9, 2021 14:29

oppiabot bot added the REVIEWERS: Please add changelog label label Nov 9, 2021

oppiabot bot assigned aks681 and vojtechjelinek Nov 9, 2021

SD-13 commented Nov 9, 2021

View reviewed changes

core/controllers/reader.py Show resolved Hide resolved

vojtechjelinek added PR CHANGELOG: Data handling -- @vojtechjelinek and removed REVIEWERS: Please add changelog label labels Nov 10, 2021

vojtechjelinek suggested changes Nov 10, 2021

View reviewed changes

vojtechjelinek assigned SD-13 and unassigned vojtechjelinek and aks681 Nov 10, 2021

SD-13 added 4 commits November 11, 2021 00:11

replaced constants and added few suggested changes

b731977

removed an unused import

5866f88

updated schema for params arg

383ce1a

updated according to lint checks

09a042d

oppiabot bot assigned vojtechjelinek and unassigned SD-13 Nov 12, 2021

Merge remote-tracking branch 'upstream/develop' into Adds_schema_for_…

04cc509

…handlers_4

oppiabot bot added the PR: don't merge - STALE BUILD label Nov 14, 2021

SD-13 added 2 commits November 15, 2021 20:34

updated few schema and tests

388dee8

updated with lint checks

199e702

oppiabot bot removed the PR: don't merge - STALE BUILD label Nov 15, 2021

SD-13 assigned vojtechjelinek and Nik-09 and unassigned vojtechjelinek and Nik-09 Nov 29, 2021

oppiabot bot added the PR: don't merge - STALE BUILD label Dec 1, 2021

Merge remote-tracking branch 'upstream/develop' into Adds_schema_for_…

9193f6c

…handlers_4

oppiabot bot removed the PR: don't merge - STALE BUILD label Dec 2, 2021

Nik-09 reviewed Dec 2, 2021

View reviewed changes

Nik-09 assigned SD-13 and unassigned Nik-09 Dec 2, 2021

vojtechjelinek approved these changes Dec 2, 2021

View reviewed changes

vojtechjelinek assigned aks681 and unassigned vojtechjelinek Dec 2, 2021

SD-13 removed their assignment Dec 2, 2021

aks681 approved these changes Dec 3, 2021

View reviewed changes

oppiabot bot unassigned aks681 Dec 3, 2021

oppiabot bot added the PR: LGTM label Dec 3, 2021

oppiabot bot assigned vojtechjelinek Dec 3, 2021

Nik-09 approved these changes Dec 3, 2021

View reviewed changes

Nik-09 enabled auto-merge (squash) December 3, 2021 13:55

Merge remote-tracking branch 'upstream/develop' into Adds_schema_for_…

10bf152

…handlers_4

Nik-09 merged commit dd898b9 into oppia:develop Dec 3, 2021

SD-13 mentioned this pull request Jan 27, 2022

Write schemas for handler class arguments #13162

Open

99 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Part of #13162: Add argument schema for ExplorationCompleteEventHandler, ExplorationMaybeLeaveHandler, SolutionHitEventHandler classes #14208

Fix Part of #13162: Add argument schema for ExplorationCompleteEventHandler, ExplorationMaybeLeaveHandler, SolutionHitEventHandler classes #14208

SD-13 commented Nov 9, 2021

oppiabot bot commented Nov 9, 2021

SD-13 commented Nov 9, 2021

vojtechjelinek left a comment

vojtechjelinek Nov 10, 2021

SD-13 Nov 10, 2021

SD-13 Nov 16, 2021

vojtechjelinek Nov 10, 2021

SD-13 Nov 10, 2021

SD-13 Nov 16, 2021

vojtechjelinek Nov 10, 2021

vojtechjelinek Nov 10, 2021

SD-13 Nov 12, 2021

SD-13 commented Nov 12, 2021

oppiabot bot commented Nov 12, 2021

oppiabot bot commented Nov 14, 2021

oppiabot bot commented Dec 1, 2021

Nik-09 left a comment

Nik-09 Dec 2, 2021

SD-13 Dec 2, 2021

Nik-09 Dec 3, 2021

SD-13 Dec 3, 2021

vojtechjelinek left a comment

aks681 left a comment

oppiabot bot commented Dec 3, 2021

oppiabot bot commented Dec 3, 2021

SD-13 commented Dec 3, 2021

Nik-09 left a comment

Nik-09 Dec 3, 2021

oppiabot bot commented Dec 3, 2021

SD-13 commented Dec 3, 2021

Fix Part of #13162: Add argument schema for ExplorationCompleteEventHandler, ExplorationMaybeLeaveHandler, SolutionHitEventHandler classes #14208

Fix Part of #13162: Add argument schema for ExplorationCompleteEventHandler, ExplorationMaybeLeaveHandler, SolutionHitEventHandler classes #14208

Conversation

SD-13 commented Nov 9, 2021

Overview

Essential Checklist

Proof that changes are correct

PR Pointers

oppiabot bot commented Nov 9, 2021

SD-13 commented Nov 9, 2021

vojtechjelinek 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

SD-13 commented Nov 12, 2021

oppiabot bot commented Nov 12, 2021

oppiabot bot commented Nov 14, 2021

oppiabot bot commented Dec 1, 2021

Nik-09 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

vojtechjelinek left a comment

Choose a reason for hiding this comment

aks681 left a comment

Choose a reason for hiding this comment

oppiabot bot commented Dec 3, 2021

oppiabot bot commented Dec 3, 2021

SD-13 commented Dec 3, 2021

Nik-09 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented Dec 3, 2021

SD-13 commented Dec 3, 2021