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.

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

Implement single expression QA checks #715

Open
wants to merge 21 commits into
base: develop
Choose a base branch
from

Conversation

dodumosu
Copy link
Collaborator

This pull request adds some enhancements to QA expressions in order to fully resolve nditech/apollo-issues#39

@dodumosu dodumosu requested a review from takinbo May 20, 2020 08:23
@dodumosu dodumosu force-pushed the complex-qa-logic branch from 738363c to a406db2 Compare May 21, 2020 02:56
Copy link
Collaborator

@takinbo takinbo left a comment

Choose a reason for hiding this comment

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

We should have test cases for this change.

def upgrade():
# ### commands auto generated by Alembic - please adjust! ###
fetch_query = sa.sql.text(
"SELECT id, quality_checks FROM form WHERE quality_checks IS NULL OR quality_checks = 'null'::jsonb;") # noqa
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you review this SQL query? Shouldn't it be IS NOT NULL?

for form_id, quality_checks in op.execute(fetch_query).fetchall():
for quality_check in quality_checks:
if 'expression' not in quality_check:
quality_check['expression'] = qb.build_expression(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we are practically doing away with criteria for quality_check shouldn't we also clean up the code and not have portions of the code that will never be used going forward? That should be the purpose of the data migration.

@@ -39,9 +39,13 @@
<td class="text-monospace align-middle"><a href="{{ url_for('formsview.quality_control_edit', form_id=form.id, qa=control.name) }}" class="text-decoration-none">{{ control.name }}</a></td>
<td class="text-monospace align-middle">{{ control.description }}</td>
<td class="text-monospace align-middle">
{%- if 'expression' in control -%}
<h5><span class="badge badge-pill badge-primary">{{ control.expression }}</span></h5>
{%- else -%}
{%- for criterion in control.criteria %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We would not be needing criteria anymore so the code should be cleaned up some.

if 'criteria' in logical_check:
if 'expression' in logical_check:
control_expression = logical_check.get('expression')
elif 'criteria' in logical_check:
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for criteria

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the migration working depends on this staying unchanged.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What if this was just put in the migration itself?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i'm also using it in the form import, which i have since updated

if 'criteria' in quality_check:
if 'expression' in quality_check:
quality_control['expression'] = quality_check['expression']
elif 'criteria' in quality_check:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove check for criteria

@@ -27,8 +29,10 @@
name = r'[a-zA-Z_][a-zA-Z0-9_]*'
lookup = "$" ("location" / "participant" / "submission") ("." / "@") name
null = "NULL"
true = "TRUE"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we have comparisons for TRUE or FALSE, it was put there as a hack when we were using the three-part criterion for the quality assurance and so isn't needed.

@dodumosu dodumosu force-pushed the complex-qa-logic branch from 80439ad to 28a6a53 Compare May 25, 2020 16:57
@dodumosu dodumosu force-pushed the complex-qa-logic branch 2 times, most recently from 2cea70a to 2997c7d Compare June 18, 2020 23:29
@dodumosu dodumosu force-pushed the complex-qa-logic branch 2 times, most recently from 0965183 to ae27e08 Compare July 27, 2020 12:37
@dodumosu dodumosu force-pushed the complex-qa-logic branch 3 times, most recently from 4ea9414 to 876be3c Compare September 24, 2020 08:47
@dodumosu dodumosu force-pushed the complex-qa-logic branch 2 times, most recently from bc1e20b to f490f61 Compare December 7, 2020 06:29
@dodumosu dodumosu force-pushed the complex-qa-logic branch 3 times, most recently from 4b6c9de to 4649e16 Compare January 9, 2021 22:21
@dodumosu dodumosu force-pushed the complex-qa-logic branch 2 times, most recently from b077c58 to c7e81ba Compare February 9, 2021 18:50
@dodumosu dodumosu force-pushed the complex-qa-logic branch from 9211c65 to 17dda8c Compare May 11, 2021 13:57
@dodumosu dodumosu force-pushed the complex-qa-logic branch 2 times, most recently from 64794a6 to 0fb54e7 Compare June 6, 2021 05:22
@dodumosu dodumosu force-pushed the complex-qa-logic branch 3 times, most recently from 5911047 to 0a6315f Compare August 20, 2021 17:51
@dodumosu
Copy link
Collaborator Author

At this point, a rewrite would be better than a rebase.

@dodumosu dodumosu closed this Dec 19, 2023
@dodumosu
Copy link
Collaborator Author

reopening to note, but still needs a lot of work

@dodumosu dodumosu reopened this Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants