-
Notifications
You must be signed in to change notification settings - Fork 9
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
quality assurance updates #772
Conversation
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.
This PR should be simplified. Rather than display a modal with an error, we should rather not return any question codes that are multiple choice, image, location or comment fields. If a QA expression for some reason contains any of these, then it should return the no data
state and not attempt to compute the QA status.
apollo/formsframework/views_forms.py
Outdated
# sanity check | ||
expression = build_expression(quality_control) | ||
if expression == '': | ||
errors.add(FlagCause.EMPTY_EXPRESSION) | ||
else: | ||
try: | ||
parse_tree = parser.parse(expression) | ||
invalid_tags, multiselect_tags = verify_expression( | ||
form, parse_tree) | ||
|
||
if invalid_tags: | ||
errors.add(FlagCause.MISSING_VARIABLE) | ||
|
||
if multiselect_tags: | ||
errors.add(FlagCause.MULTISELECT_VARIABLE) | ||
except NoMatch: | ||
errors.add(FlagCause.MALFORMED_EXPRESSION) | ||
|
||
# the invalid tags and multi-select tags are returned | ||
# as sets | ||
invalid_tags = list(invalid_tags) | ||
multiselect_tags = list(multiselect_tags) | ||
|
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.
This will probably also need clean up considering we're not using those errors anymore?
apollo/submissions/qa/messages.py
Outdated
class FlagCause(enum.IntEnum): | ||
EMPTY_EXPRESSION = 1 | ||
MALFORMED_EXPRESSION = 2 | ||
MISSING_VARIABLE = 3 | ||
MULTISELECT_VARIABLE = 4 |
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.
If we're not returning any errors in the QA builder, do we still need this?
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.
A few extraneous unused lines and this PR should be good for merging
@@ -155,6 +158,12 @@ def __init__(self, defaults=True, **kwargs): | |||
def visit_variable(self, node, children): | |||
var_name = node.value | |||
if var_name not in self.form.tags: | |||
self.invalid_variables.add(var_name) |
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.
Is there a current usecase for these?
|
||
field = self.form.get_field_by_tag(var_name) | ||
if field['type'] == 'multiselect': | ||
self.multiple_choice_variables.add(var_name) |
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.
Same.
@@ -212,12 +221,19 @@ def visit_variable(self, node, children): | |||
self.variables.add(var_name) | |||
if var_name not in self.form.tags: | |||
self.lock_null = True | |||
self.invalid_variables.add(var_name) |
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.
Same.
|
||
if field['type'] == 'multiselect': | ||
self.lock_null = True | ||
self.multiple_choice_variables.add(var_name) |
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.
Same.
0e956f4
to
a2729c0
Compare
this commit restricts QA to `integer`, `select` and `string` question types
a2729c0
to
4c56a1d
Compare
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.
Could you please test this and confirm it's working? I applied the PR to a working system and all the questions are no longer visible in the QA builder.
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.
tACK
this PR contains the following changes to how Apollo works:
Missing