-
Notifications
You must be signed in to change notification settings - Fork 3
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.
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
LITE-27893 Add validation step for PPR upload #19
Conversation
1a54a8c
to
8ba3f9c
Compare
One general question - why are we not using CBC API for validation?
|
"enum": [ | ||
"OldName_1", | ||
"Name_EN", | ||
"Description_EN", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name and description both can be multilingual.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great, but only EN is required only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, will allow additional fields for "ServicePlans" that starts with "Name_" / "Description_"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll merge the PR, but please check that if any additional column appears we shouldn't fail because of it. The system should be treated as open - which means it retrieves all needed information from the input and ignore others
@rahulmondal Nope, on uploading the PPR we don't have the hub to validate against, the only thing that we can do here to check required sheets and fields inside |
8ba3f9c
to
a63e072
Compare
try: | ||
jsonschema.validate(dict_file, PPR_SCHEMA) | ||
except jsonschema.ValidationError as ex: | ||
error = ClientError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[SOL-C] Propose to make an error class to store all the errors in one place.
Otherwise we may get confused in the error codes. Do you agree?
"enum": [ | ||
"OldName_1", | ||
"Name_EN", | ||
"Description_EN", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great, but only EN is required only
"enum": [ | ||
"OldName_1", | ||
"Name_EN", | ||
"Description_EN", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll merge the PR, but please check that if any additional column appears we shouldn't fail because of it. The system should be treated as open - which means it retrieves all needed information from the input and ignore others
@d3rky |
No description provided.