Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 6 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
46787a8
Added schema for handler class argument SkillMasteryDataHandler
davisong Dec 9, 2021
66af425
Fixed linting check
davisong Dec 9, 2021
1053e4d
Added validator for mastery_change_per_skill
davisong Jan 3, 2022
8143b0a
Fixed linting issue
davisong Jan 3, 2022
c26c9ab
Fixed more linting issues
davisong Jan 3, 2022
6628376
Fixed even more linting issues
davisong Jan 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 20 additions & 0 deletions core/controllers/skill_mastery.py
View file
Open in desktop
Expand Up @@ -21,6 +21,7 @@
from core import utils
from core.controllers import acl_decorators
from core.controllers import base
from core.controllers import domain_objects_validator
from core.domain import skill_domain
from core.domain import skill_fetchers
from core.domain import skill_services
Expand All @@ -33,6 +34,25 @@ class SkillMasteryDataHandler(base.BaseHandler):
"""

GET_HANDLER_ERROR_RETURN_TYPE = feconf.HANDLER_TYPE_JSON
URL_PATH_ARGS_SCHEMAS = {}
HANDLER_ARGS_SCHEMAS = {
'GET': {
'comma_separated_exp_ids': {
'schema': {
'type': 'basestring'
}
}
Comment on lines +40 to +44
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
'PUT': {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'mastery_change_per_skill': {
'schema': {
'type': 'object_dict',
'validation_method': (
domain_objects_validator.validate_params_dict)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this schema used?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@davisong davisong Dec 16, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vojtechjelinek PTAL

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
}
}

@acl_decorators.can_access_learner_dashboard
def get(self):
Expand Down
1 change: 0 additions & 1 deletion core/handler_schema_constants.py
View file
Open in desktop
Expand Up @@ -80,7 +80,6 @@
'SiteLanguageHandler',
'SkillDataHandler',
'SkillDescriptionHandler',
'SkillMasteryDataHandler',
'SkillsDashboardPageDataHandler',
'StartedTranslationTutorialEventHandler',
'StateCompleteEventHandler',
Expand Down