2695 field spaces #2704

elipe17 · 2023-09-15T19:25:17Z

Summary of Changes

Updated required fields in T1 - T3 schemas to support blanks
Updated validators for required fields to allow blanks
Added new step to parsing to disable validators based on fields being blank to avoid erroneous ParserErrors
Added test for parsing records with allowed blank fields
Pull request closes As tech lead, I need some space-filled values to be allowed in TANF Section 1 data files #2695

How to Test

cd tdrs-frontend && docker-compose up
cd tdrs-backend && docker-compose up

Log in as user who can submit datafiles
Submit tanf_section1_blanks.txt and verify all records parse and there are no ParserErrors

Deliverables

More details on how deliverables herein are assessed included here.

Deliverable 1: Accepted Features

Checklist of ACs:

relevant T1 data elements with spaces as values are not flagged as cat 2 (out-of-range) errors
relevant T2 data elements with spaces as values are not flagged as cat 2 (out-of-range) errors
relevant T3 data elements with spaces as values are not flagged as cat 2 (out-of-range) errors
Testing Checklist has been run and all tests pass
lfrohlich and/or adpennington confirmed that ACs are met.

Deliverable 2: Tested Code

Are all areas of code introduced in this PR meaningfully tested?
- If this PR introduces backend code changes, are they meaningfully tested?
- If this PR introduces frontend code changes, are they meaningfully tested?
Are code coverage minimums met?
- Frontend coverage: [insert coverage %] (see CodeCov Report comment in PR)
- Backend coverage: [insert coverage %] (see CodeCov Report comment in PR)

Deliverable 3: Properly Styled Code

Are backend code style checks passing on CircleCI?
Are frontend code style checks passing on CircleCI?
Are code maintainability principles being followed?

Deliverable 4: Accessible

Does this PR complete the epic?
Are links included to any other gov-approved PRs associated with epic?
Does PR include documentation for Raft's a11y review?
Did automated and manual testing with iamjolly and ttran-hub using Accessibility Insights reveal any errors introduced in this PR?

Deliverable 5: Deployed

Was the code successfully deployed via automated CircleCI process to development on Cloud.gov?

Deliverable 6: Documented

Does this PR provide background for why coding decisions were made?
If this PR introduces backend code, is that code easy to understand and sufficiently documented, both inline and overall?
If this PR introduces frontend code, is that code easy to understand and sufficiently documented, both inline and overall?
If this PR introduces dependencies, are their licenses documented?
Can reviewer explain and take ownership of these elements presented in this code review?

Deliverable 7: Secure

Does the OWASP Scan pass on CircleCI?
Do manual code review and manual testing detect any new security issues?
If new issues detected, is investigation and/or remediation plan documented?

Deliverable 8: User Research

Research product(s) clearly articulate(s):

the purpose of the research
methods used to conduct the research
who participated in the research
what was tested and how
impact of research on TDP
(if applicable) final design mockups produced for TDP development

- Added new tests - Updated make validator to handle type errors

- Add datafile

elipe17 · 2023-09-15T19:37:26Z

Adding patch that implements disabling validators.
disable_validators.patch

…2695-field-spaces

…not reqruired

…ed to be - removed unnecessary validator - updated schemas -

- Updated incorrect fields and catch validator error

- Updating tests

…2695-field-spaces

codecov · 2023-09-25T16:57:28Z

Codecov Report

Merging #2704 (9a53a06) into develop (8ec31f9) will decrease coverage by 0.56%.
Report is 53 commits behind head on develop.
The diff coverage is 91.63%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2704      +/-   ##
===========================================
- Coverage    92.99%   92.43%   -0.56%     
===========================================
  Files          219      239      +20     
  Lines         4482     5354     +872     
  Branches       385      473      +88     
===========================================
+ Hits          4168     4949     +781     
- Misses         242      312      +70     
- Partials        72       93      +21

Flag	Coverage Δ
dev-backend	`92.32% <92.15%> (-0.52%)`	⬇️
dev-frontend	`92.95% <78.26%> (-0.65%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
tdrs-backend/tdpservice/core/logger.py	`100.00% <100.00%> (ø)`
tdrs-backend/tdpservice/data_files/serializers.py	`100.00% <100.00%> (ø)`
...rs-backend/tdpservice/data_files/test/factories.py	`100.00% <100.00%> (ø)`
tdrs-backend/tdpservice/data_files/validators.py	`96.15% <ø> (ø)`
tdrs-backend/tdpservice/data_files/views.py	`90.65% <100.00%> (+0.08%)`	⬆️
tdrs-backend/tdpservice/email/email.py	`76.78% <100.00%> (-1.55%)`	⬇️
...vice/email/helpers/account_deactivation_warning.py	`100.00% <100.00%> (ø)`
...backend/tdpservice/email/helpers/account_status.py	`84.84% <100.00%> (ø)`
tdrs-backend/tdpservice/email/helpers/data_file.py	`100.00% <100.00%> (ø)`
tdrs-backend/tdpservice/parsers/admin.py	`100.00% <100.00%> (ø)`
... and 53 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7115532...9a53a06. Read the comment docs.

tdrs-backend/tdpservice/parsers/schema_defs/tanf/t1.py

…2695-field-spaces

- Updated logic on when to run field validators - Updated schemas

George-Hudson

lgtm though I am mostly just trying to familiarize myself.

jtimpe

minor conversation topic, maybe some unused code. overall looks good, thank you!

tdrs-backend/tdpservice/parsers/validators.py

jtimpe · 2023-10-10T13:18:01Z

tdrs-backend/tdpservice/parsers/row_schema.py

@@ -114,7 +114,9 @@ def run_field_validators(self, instance, generate_error):
            else:
                value = getattr(instance, field.name, None)

-            if field.required and not value_is_empty(value, field.endIndex-field.startIndex):
+            is_empty = value_is_empty(value, field.endIndex-field.startIndex)


value_is_empty doesn't test for 0-filled strings, which i think is fine but worth calling out since we have other 'space'-fill operators (@ and #). the end result currently is

'000' -> 0
' ' -> None
'###' -> None
'@@@' -> None

maybe a possibility is to add 0-fills to value_is_empty so that 0 becomes None for non-required fields, but i think this could cause side effects with actual 0 values we want to preserve. moreover, i think you're currently meeting the requirements as i understand them - treat 0 as 0 but allow for space-filling as well - space filling just results in a slightly different result.

Ya I didn't think we wanted to add zero fill to the value_is_empty function since there are so many fields across the schemas that expect zero as a valid value. I will point Alex at this as well to make sure everything is good.

@ADPennington can you verify that this is the logic we want in these situations?

not sure i understand how we are using "#" and "@" but im seeing the expected behavior @elipe17 @jtimpe

…2695-field-spaces

andrew-jameson

LGTM

…2695-field-spaces

ADPennington

lgtm! @elipe17 🚀

@reitermb can you investigate during UX research if users understand that None means a space-filled value was provided? (see below)

elipe17 added 2 commits September 13, 2023 11:41

- Added support to fields that can allow blanks as valid fields

f94c1ff

- Added new tests - Updated make validator to handle type errors

- Updating blank fields to not be required

8a12d79

elipe17 added backend dev labels Sep 15, 2023

elipe17 self-assigned this Sep 15, 2023

- Fix lint errors

5ede638

- Add datafile

elipe17 added 11 commits September 18, 2023 10:59

Merge branch 'develop' of https://github.com/raft-tech/TANF-app into …

49b148d

…2695-field-spaces

- updating fields to be required until guidance comes out on what is …

780a6c7

…not reqruired

- ADding None check

0750677

- Error checking

c63ba74

- Updated to run field validators even if field is empty but is allow…

6f32a02

…ed to be - removed unnecessary validator - updated schemas -

- Switching back to numIsBlank

870c01d

-Adding back numIsBlank

c66fd8a

- adding isNone since empty string fields are also None

ca0b22c

- Added all missing fields to test file

45644e6

- Updated incorrect fields and catch validator error

- Updating test

ccb80fc

- Fix lint errors

265d498

elipe17 changed the title ~~Draft: 2695 field spaces~~ 2695 field spaces Sep 21, 2023

elipe17 requested review from andrew-jameson, raftmsohani and jtimpe September 21, 2023 21:13

elipe17 added 3 commits September 25, 2023 10:15

- Re-added missing fields

78f108b

- Updating tests

- Updated tests

2d96c9b

Merge branch 'develop' of https://github.com/raft-tech/TANF-app into …

dfefa46

…2695-field-spaces

andrew-jameson reviewed Sep 27, 2023

View reviewed changes

tdrs-backend/tdpservice/parsers/schema_defs/tanf/t1.py Outdated Show resolved Hide resolved

elipe17 added 3 commits October 2, 2023 09:36

Merge branch 'develop' of https://github.com/raft-tech/TANF-app into …

22d3fa5

…2695-field-spaces

- Fixing errors from merge

6817519

- Removed extra flag

18c987a

- Updated logic on when to run field validators - Updated schemas

elipe17 added 2 commits October 4, 2023 08:51

- Fixed failing tests

88cc777

- Fix lint

f444601

George-Hudson approved these changes Oct 4, 2023

View reviewed changes

elipe17 requested a review from andrew-jameson October 4, 2023 15:42

jtimpe approved these changes Oct 10, 2023

View reviewed changes

elipe17 added 2 commits October 10, 2023 08:33

Merge branch 'develop' of https://github.com/raft-tech/TANF-app into …

374ff3c

…2695-field-spaces

- Removing unnused validator

c6aba84

andrew-jameson approved these changes Oct 10, 2023

View reviewed changes

raftmsohani approved these changes Oct 10, 2023

View reviewed changes

elipe17 requested a review from ADPennington October 11, 2023 18:30

elipe17 added the QASP Review label Oct 11, 2023

Merge branch 'develop' of https://github.com/raft-tech/TANF-app into …

ce8521d

…2695-field-spaces

ADPennington added Deploy with CircleCI-qasp Deploy to https://tdp-frontend-qasp.app.cloud.gov through CircleCI and removed Deploy with CircleCI-qasp Deploy to https://tdp-frontend-qasp.app.cloud.gov through CircleCI labels Oct 18, 2023

ADPennington approved these changes Oct 20, 2023

View reviewed changes

ADPennington added Ready to Merge and removed QASP Review Deploy with CircleCI-qasp Deploy to https://tdp-frontend-qasp.app.cloud.gov through CircleCI labels Oct 20, 2023

Merge branch 'develop' into 2695-field-spaces

9a53a06

andrew-jameson merged commit 6785277 into develop Nov 6, 2023
12 checks passed

andrew-jameson deleted the 2695-field-spaces branch November 6, 2023 14:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2695 field spaces #2704

2695 field spaces #2704

elipe17 commented Sep 15, 2023 •

edited by ADPennington

Loading

elipe17 commented Sep 15, 2023 •

edited

Loading

codecov bot commented Sep 25, 2023 •

edited

Loading

George-Hudson left a comment

jtimpe left a comment

jtimpe Oct 10, 2023

elipe17 Oct 10, 2023

elipe17 Oct 11, 2023

ADPennington Oct 20, 2023

andrew-jameson left a comment

ADPennington left a comment

2695 field spaces #2704

2695 field spaces #2704

Conversation

elipe17 commented Sep 15, 2023 • edited by ADPennington Loading

Summary of Changes

How to Test

Deliverables

elipe17 commented Sep 15, 2023 • edited Loading

codecov bot commented Sep 25, 2023 • edited Loading

Codecov Report

George-Hudson left a comment

Choose a reason for hiding this comment

jtimpe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew-jameson left a comment

Choose a reason for hiding this comment

ADPennington left a comment

Choose a reason for hiding this comment

elipe17 commented Sep 15, 2023 •

edited by ADPennington

Loading

elipe17 commented Sep 15, 2023 •

edited

Loading

codecov bot commented Sep 25, 2023 •

edited

Loading