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

A5d2 nomensage enable #260

Merged
merged 8 commits into from
Jan 30, 2025
Merged

A5d2 nomensage enable #260

merged 8 commits into from
Jan 30, 2025

Conversation

mlan225
Copy link
Member

@mlan225 mlan225 commented Jan 24, 2025

Resolves #250

Review checklist

  • Inputting a valid value of 10 - 70 or 99 in NOMENSAGE (7b) enables checkboxes in 7c
  • Inputting an invalid value that IS NOT 10 - 70 or 99 will disable the checkboxes in 7c
  • Finalizing a packet with a value of 9988 in NOMENSAGE (7b) and no checkboxes selected in 7c will not produce an error message for no selected checkboxes
  • Finalizing a packet with a value of NOT 99 in NOMENSAGE (7b) and no checkboxes selected will produce an error message to select a checkbox

Allow for NOMENSAGE to be 99 and enable checkboxes in 7c of the A5D2

99 = enable checkboxes

image

88 = disable checkboxes

image

valid range error for NOMENSAGE

image

Validation error if no checkbox is selected when checkboxes are enabled (and NOT 99)

image

Complex range logic for client side checkbox disable lead to using javascript instead for the NOMENSAGE behavior
@mlan225 mlan225 marked this pull request as draft January 24, 2025 21:54
@mlan225 mlan225 marked this pull request as ready for review January 24, 2025 22:05
@mlan225
Copy link
Member Author

mlan225 commented Jan 28, 2025

hey @ashleybot , Unless im overlooking somethig, Im having a hard time replicating your result. Is it possible that the old javascript was still cached and not causing the new disable to occur in the checkboxes?

Here are my results on the branch
image
image

@ashleybot
Copy link
Member

ashleybot commented Jan 28, 2025

This is incorrect:

Finalizing a packet with a value of 99 in NOMENSAGE (7b) and no checkboxes selected in 7c will not produce an error message for no selected checkboxes

@mlan225 NOMENSAGE = 99 should require at least one checkbox.

@mlan225
Copy link
Member Author

mlan225 commented Jan 28, 2025

  • Added validation to require checkbox selection when NOMENSAGE == 99

image

@mlan225
Copy link
Member Author

mlan225 commented Jan 28, 2025

Adding a comment about it here @ashleybot , but currently NOMENSAGE has no requirement attributes and may be a worthy of an issue card.

If i am reading the documentation correctly, it appears that NOMENSAGE has a "conditional missingness", which i interpret as "will sometimes be required under certain conditions".
https://github.com/naccdata/uniform-data-set/blob/main/forms/uds/a5d2/form_a5d2_ivp_questions_and_vars.csv

image

@ashleybot
Copy link
Member

ashleybot commented Jan 28, 2025

Adding a comment about it here @ashleybot , but currently NOMENSAGE has no requirement attributes and may be a worthy of an issue card.

If i am reading the documentation correctly, it appears that NOMENSAGE has a "conditional missingness", which i interpret as "will sometimes be required under certain conditions". https://github.com/naccdata/uniform-data-set/blob/main/forms/uds/a5d2/form_a5d2_ivp_questions_and_vars.csv

@mlan225 Yep, this is correct and it would be a quick fix in this feature branch. So, the rules should follow

  • NOMENSAGE RequiredIfRange MENARCHE 5-25
  • NOMENSAGE RequiredIf MENARCHE = 99

References

If MENARCHE in (5-25, 99) then NOMENSAGE must be present

https://github.com/naccdata/uniform-data-set/blob/main/forms/uds/a5d2/form_a5d2_ivp_error_checks_mc.csv

@ashleybot
Copy link
Member

@mlan225 Realized this morning that the RequiredIf annotations are already being added in this PR #261

@ashleybot ashleybot self-requested a review January 29, 2025 20:32
Copy link
Member

@ashleybot ashleybot left a comment

Choose a reason for hiding this comment

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

@mlan225 NOMENSAGE changes look good! Good to bump to 5.0.2.

@ashleybot ashleybot mentioned this pull request Jan 29, 2025
3 tasks
@mlan225 mlan225 merged commit e9ed732 into main Jan 30, 2025
6 checks passed
@mlan225 mlan225 deleted the a5d2-nomensage-enable branch January 30, 2025 06:57
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.

A5D2 NOMENSAGE = 99 should enable checkboxes in 7c
2 participants