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

Add eligibility to response model - fixes #1292 #1300

Merged
merged 13 commits into from Nov 20, 2023

Conversation

becky-gilbert
Copy link
Contributor

@becky-gilbert becky-gilbert commented Nov 3, 2023

This PR adds a new 'eligibility' field to the Response model. The value is determined when the Response object is first created, and it is a list of one or more strings:

  • Eligible
  • Ineligible_TooYoung
  • Ineligible_TooOld
  • Ineligible_Criteria (based on the study's criteria expression)
  • Ineligible_Participation (based on the study's must have/not have participated requirements)

The eligibility value will only be set for newly-created Response objects. Existing responses will have an eligibility field that contains an empty list.

The 'eligibility' field value roughly corresponds to the 'red text' warnings displayed under the "Participate/Preview Now" button, which let the participant know that the selected child is too young/old or does not meet the study criteria. However, there are some minor differences between these:

  • If a child is too old for a study, then they are only shown the 'too old' warning text (even if they are also ineligible based on criteria/participation).
  • A child is shown the same warning when they are ineligible based on the study criteria expression and/or the prior participation requirements.

Examples

Eligible

Participation page:

eligible_warning

Data:

eligible_data

Study Responses -> Individual Responses -> Response details:

eligible_details

Ineligible: too young

Participation page:

ineligible_young_warning

Data:

ineligible_young_data

Study Responses -> Individual Responses -> Response details:

ineligible_young_details

Ineligible: too old

Ineligible: criteria

Participation page:

ineligible_criteria_warning

Data:

ineligible_criteria_data

Study Responses -> Individual Responses -> Response details:

ineligible_criteria_details

Ineligible: multiple reasons

Participation page:

ineligible_mulitple_warning

Data:

ineligible_multiple_data

Study Responses -> Individual Responses -> Response details:

ineligible_multiple_details

Existing responses

The 'eligibility' column will be present but will contain an empty list.

eligibility_existing

@becky-gilbert becky-gilbert linked an issue Nov 3, 2023 that may be closed by this pull request
4 tasks
@becky-gilbert becky-gilbert marked this pull request as draft November 3, 2023 20:13
@becky-gilbert
Copy link
Contributor Author

becky-gilbert commented Nov 7, 2023

Questions about this issue: what should the value of the eligibility field be in these cases?:

  1. Child's birthday is missing, which means we can't calculate age range eligibility. At the moment, the eligibility value is blank for existing responses in the database. Does it also make sense to use a blank value when the child's birthday is missing? Or do we need another 'Ineligible'-type category?

  2. Study's criteria expression is invalid (parsing error). Should we default to always treating the criteria check as eligible (since otherwise there will be no eligible children for the study)? Or always code this as 'Ineligible_Criteria'?

BTW, these issues revealed themselves in testing because (a) we don't require the birthday field in the Child account model, and (b) the criteria_expression in the Study model doesn't default to an empty string. So if it so happens that we did want to change those things, that might remove the need for handling these cases (except that maybe the researcher can still enter an invalid criteria expression - I'm not sure if there's any validation on that before it's saved).

@okaycj
Copy link
Contributor

okaycj commented Nov 7, 2023

  • Child's birthday is missing, which means we can't calculate age range eligibility. At the moment, the eligibility value is blank for existing responses in the database. Does it also make sense to use a blank value when the child's birthday is missing? Or do we need another 'Ineligible'-type category?

There's an issue on this #1285. If you look through the production database, all children have a birthday. This require for a birthday was enforced at the form level and probably should be at the database level. Assuming, we want to require a child to have a birthday.

  • Study's criteria expression is invalid (parsing error). Should we default to always treating the criteria check as eligible (since otherwise there will be no eligible children for the study)? Or always code this as 'Ineligible_Criteria'?

It my preference to not have an invalid criteria. Can we do a better job of validating the criteria on form submission? For existing invalid criteria, could we signal the researcher in someway? Maybe on the preview child selection view?

Copy link
Contributor

@okaycj okaycj left a comment

Choose a reason for hiding this comment

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

This is just a draft, but it seems to make sense. I'll do another review when you're ready.

Comment on lines +141 to +151

class ResponseEligibility(models.TextChoices):
"""Participant eligibility categories for Response model"""

# This must be determined at the point of the study participation/response, because child eligibility can change over time for a given study
ELIGIBLE = "Eligible"
INELIGIBLE_YOUNG = "Ineligible_TooYoung"
INELIGIBLE_OLD = "Ineligible_TooOld"
INELIGIBLE_CRITERIA = "Ineligible_CriteriaExpression"
INELIGIBLE_PARTICIPATION = "Ineligible_Participation"

Copy link
Contributor

Choose a reason for hiding this comment

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

TextChoices! I should've known about this, very nice.

@becky-gilbert
Copy link
Contributor Author

It turns out that we do require a birthday on the Child create form, and we validate the criteria expression on the study create/edit form.

I've updated the Child model so that blank is false and null is false. This ensures that a Child instance cannot be saved to the database without a birthday, which means that we don't need to validate the data that's coming out of the database (i.e. checking that a Child has a birthday before trying to access that field).

Similarly, I've added a default blank ('') value to the Study model's criteria_expression field. This prevents our testing tools from generating random text strings in this field when we create a new Study instance where the criteria_expression is not specified.

Copy link

sonarcloud bot commented Nov 15, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mekline
Copy link
Contributor

mekline commented Nov 15, 2023

Couple of comments from Ian on typos, but I think they are in the PR text rather than the codebase:

I found a few quick typos:
-"Eligibile" string should be "Eligible". Looking at the code examples in the comments, the actual code probably doesn't have this error and it's just in this documentation.

  • "If a child is shown the same warning when they are ineligible based on the study criteria expression and/or the prior participation requirements." should remove the "If" at the beginning I believe.

@becky-gilbert
Copy link
Contributor Author

Thanks Melissa and Ian! I checked the codebase and these errors are just in the PR text. Fixed! 👍

Copy link
Contributor

@okaycj okaycj left a comment

Choose a reason for hiding this comment

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

LGTM! That was a lot of work.

@becky-gilbert becky-gilbert merged commit 5d170da into develop Nov 20, 2023
5 checks passed
@becky-gilbert becky-gilbert deleted the add-eligibility-to-response-model branch November 20, 2023 17:41
rhodricusack pushed a commit to manybabies/mbah-lookit-api that referenced this pull request Jan 23, 2024
* add eligibility field to Response model/serializer, set value on object creation, add/refactor queries, add helper
* add eligibility to the Response fields displayed in admin pages
* add eligibility to response columns available for researcher download/display
* display eligibility value in details table on individual responses page
* alter child birthday field to not allow blank or null
* add a default blank value for Study criteria expression, modify response eligibility migration to include change to Study model
* fix failing tests: add required birthday to Child instances
* add tests for child_in_age_range_for_study_days_difference
* add tests for eligibility field, get_eligibility_for_response returns sorted list of eligibility strings
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.

Add redtext warning status to researcher session data
4 participants