-
Notifications
You must be signed in to change notification settings - Fork 4
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
Include New Citizenship Status Value #3428
Conversation
- Update test message
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3428 +/- ##
========================================
Coverage 91.43% 91.43%
========================================
Files 300 300
Lines 8647 8647
Branches 640 640
========================================
Hits 7906 7906
Misses 621 621
Partials 120 120
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
…m/raft-tech/TANF-app into 3386-citizenship-status-new-value
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.
LGTM!
@@ -364,7 +364,7 @@ | |||
required=False, | |||
validators=[ | |||
category3.orValidators([ | |||
category3.isBetween(0, 2, inclusive=True), | |||
category3.isBetween(0, 3, inclusive=True), |
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.
@elipe17 @klinkoberstar this change works as expected, but i am also noticing that 0 is allowed and seems inconsistent with the coding instructions for item 25 (see screenshot here)
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.
@elipe17 these changes look good to me. screenshots of before and after below.
S= staging (before)
Q = qasp (after)
please note my comment in the code re: item 25 for closed case files, which is also evident in the error message for closed case data files (staging) below. cc: @klinkoberstar
added |
@@ -415,7 +415,7 @@ | |||
startIndex=57, | |||
endIndex=58, | |||
required=False, | |||
validators=[category2.isOneOf([0, 1, 2, 9])], |
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.
@ADPennington We also treat 0 as a valid value for Item 42 in the T2 file as a Cat 2 check. I'm not sure if this is intentional or an oversight. Is there any reason why would have wanted to give STTs the flexibility to submit blank/0 in this field rather than 9 ?
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.
Good catch. I believe it was a relic of an earlier version of the updated coding instructions. I believe it was before we tested whether 0 would cause problems in FTANF. We can test it again to be sure and spin up a new ticket to remove it. 0 should only be possible if the field is "optional" for certain types of reported individuals.
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 is helpful! I'll make a note to investigate and work on a follow up ticket!
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.
Just want to confirm that this can merge then, or does zero need to be removed from possible values?
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.
yep @elipe17 -- let's proceed with merging. 🚀 cc: @klinkoberstar @lhuxraft
Flagged for dev to take a look at - we can also discuss at standup tomorrow if needed 🙂 |
Summary of Changes
Pull request closes Update Citizenship/Immigration Status Acceptable Values (TANF only) #3386
How to Test
List the steps to test the PR
These steps are generic, please adjust as necessary.
3
Deliverables
More details on how deliverables herein are assessed included here.
Deliverable 1: Accepted Features
Checklist of ACs:
Deliverable 2: Tested Code
CodeCov Report
comment in PR)CodeCov Report
comment in PR)Deliverable 3: Properly Styled Code
Deliverable 4: Accessible
iamjolly
andttran-hub
using Accessibility Insights reveal any errors introduced in this PR?Deliverable 5: Deployed
Deliverable 6: Documented
Deliverable 7: Secure
Deliverable 8: User Research
Research product(s) clearly articulate(s):