Fix part of #13162: added schemas for SkillMasteryDataHandler #14395

davisong · 2021-12-09T19:49:57Z

Overview

This PR fixes or fixes part of Write schemas for handler class arguments #13162.
This PR does the following:

Adds schema for handler class argument SkillMasteryDataHandler

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

2021-12-09.14-43-15.mp4

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-12-09T19:50:12Z

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

oppiabot · 2021-12-09T23:15:50Z

Hi @davisong. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

oppiabot · 2021-12-12T07:03:46Z

Hi @davisong, 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!

vojtechjelinek

Thanks! Left a few comments.

vojtechjelinek · 2021-12-12T12:02:43Z

core/controllers/skill_mastery.py

+                    'validation_method': (
+                        domain_objects_validator.validate_params_dict)


Why is this schema used?

mastery_change_per_skill is stored as a dictionary of multiple key value pairs, each one representing a different skill and the corresponding degree of mastery. I was unable to find a domain object for that schema so I resorted to using validate_params_dict as a general method of validation.

@vojtechjelinek PTAL

I don't think that makes sense, you should properly validate all the files of this dictionary. validate_params_dict is used in a case where we don't know the structure at all, but here we should be able to understand the structure

I understand the structure but I'm very confused on how to validate it under the current schema definition. dict requires static keys to define a name and associated schema value, but mastery_change_per_skill could potentially be any skill_id.

@vojtechjelinek PTAL

Okay, then you might need to write a validator that will take a dict where keys need to have skill ID format and values follow some given format.

oppiabot · 2021-12-12T12:06:04Z

Unassigning @vojtechjelinek since the review is done.

oppiabot · 2021-12-29T15:47:53Z

Hi @davisong, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 4 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

aks681 · 2022-01-04T23:33:39Z

core/controllers/skill_mastery.py

+                }
+            }
+        },
+        'PUT': {


@vojtechjelinek Shouldn't def put use self.normalized_payload? Also, is there a corresponding normalized_request for get?

Ah, yes, use self.normalized_payload instead of self.payload everywhere.

vojtechjelinek

Thanks! Left a few comments.

vojtechjelinek · 2022-01-05T14:00:06Z

core/controllers/domain_objects_validator.py

+    Returns:
+        dict(str, float). Returns mastery_change_per_skill after validation.
+    """
+    for (skill_id, mastery) in mastery_change_per_skill:


Suggested change

for (skill_id, mastery) in mastery_change_per_skill:

for skill_id, mastery in mastery_change_per_skill:

vojtechjelinek · 2022-01-05T14:01:59Z

core/controllers/skill_mastery.py

+                }
+            }
+        },
+        'PUT': {


Ah, yes, use self.normalized_payload instead of self.payload everywhere.

vojtechjelinek · 2022-01-05T14:06:14Z

core/controllers/skill_mastery.py

+            'comma_separated_exp_ids': {
+                'schema': {
+                    'type': 'basestring'
+                }
+            }


Please refactor this to use JsonEncodedInString see other places to understand how it works.

oppiabot · 2022-01-05T14:09:36Z

Unassigning @vojtechjelinek since the review is done.

oppiabot · 2022-01-06T19:56:32Z

Hi @davisong, 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 · 2022-01-13T20:13:55Z

Hi @davisong, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 4 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

davisong added 2 commits December 9, 2021 14:38

Added schema for handler class argument SkillMasteryDataHandler

46787a8

Fixed linting check

66af425

davisong requested review from aks681 and vojtechjelinek as code owners December 9, 2021 19:49

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

oppiabot bot assigned aks681 and vojtechjelinek Dec 9, 2021

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Dec 9, 2021

oppiabot bot assigned davisong Dec 9, 2021

vojtechjelinek added the PR CHANGELOG: Data handling -- @vojtechjelinek label Dec 10, 2021

oppiabot bot added the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label Dec 12, 2021

vojtechjelinek removed the REVIEWERS: Please add changelog label label Dec 12, 2021

vojtechjelinek requested changes Dec 12, 2021

View reviewed changes

oppiabot bot unassigned vojtechjelinek Dec 12, 2021

oppiabot bot added stale and removed stale labels Dec 29, 2021

davisong added 4 commits January 3, 2022 18:32

Added validator for mastery_change_per_skill

1053e4d

Fixed linting issue

8143b0a

Fixed more linting issues

c26c9ab

Fixed even more linting issues

6628376

oppiabot bot removed the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label Jan 3, 2022

aks681 reviewed Jan 4, 2022

View reviewed changes

aks681 assigned vojtechjelinek and unassigned aks681 Jan 4, 2022

vojtechjelinek requested changes Jan 5, 2022

View reviewed changes

oppiabot bot unassigned vojtechjelinek Jan 5, 2022

oppiabot bot added the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label Jan 6, 2022

oppiabot bot added the stale label Jan 13, 2022

oppiabot bot closed this Jan 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix part of #13162: added schemas for SkillMasteryDataHandler #14395

Fix part of #13162: added schemas for SkillMasteryDataHandler #14395

davisong commented Dec 9, 2021

oppiabot bot commented Dec 9, 2021

oppiabot bot commented Dec 9, 2021

oppiabot bot commented Dec 12, 2021

vojtechjelinek left a comment

vojtechjelinek Dec 12, 2021

davisong Dec 12, 2021

davisong Dec 16, 2021 •

edited

vojtechjelinek Dec 22, 2021

davisong Jan 1, 2022

vojtechjelinek Jan 3, 2022

oppiabot bot commented Dec 12, 2021

oppiabot bot commented Dec 29, 2021

aks681 Jan 4, 2022

vojtechjelinek Jan 5, 2022

vojtechjelinek left a comment

vojtechjelinek Jan 5, 2022

vojtechjelinek Jan 5, 2022

vojtechjelinek Jan 5, 2022

oppiabot bot commented Jan 5, 2022

oppiabot bot commented Jan 6, 2022

oppiabot bot commented Jan 13, 2022

		'validation_method': (
		domain_objects_validator.validate_params_dict)

	for (skill_id, mastery) in mastery_change_per_skill:
	for skill_id, mastery in mastery_change_per_skill:

Fix part of #13162: added schemas for SkillMasteryDataHandler #14395

Fix part of #13162: added schemas for SkillMasteryDataHandler #14395

Conversation

davisong commented Dec 9, 2021

Overview

Essential Checklist

Proof that changes are correct

PR Pointers

oppiabot bot commented Dec 9, 2021

oppiabot bot commented Dec 9, 2021

oppiabot bot commented Dec 12, 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

davisong Dec 16, 2021 • 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

oppiabot bot commented Dec 12, 2021

oppiabot bot commented Dec 29, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented Jan 5, 2022

oppiabot bot commented Jan 6, 2022

oppiabot bot commented Jan 13, 2022

davisong Dec 16, 2021 •

edited