2818 Update Indicator - 2

[atrimpe]

Summary of Changes

Altered header to only allow D, updated test data files, added test
Pull request closes #2818

How to Test

List the steps to test the PR
These steps are generic, please adjust as necessary.

cd tdrs-frontend && docker-compose -f docker-compose.yml -f docker-compose.local.yml up -d
cd tdrs-backend && docker-compose -f docker-compose.yml -f docker-compose.local.yml up -d 

1. Open http://localhost:3000/ and sign in.
2. Proceed with functional tests as described herein.
3. Test steps should be captured in the demo GIF(s) and/or screenshots below.

Demo GIF(s) and screenshots for testing procedure

Deliverables

More details on how deliverables herein are assessed included here.

Deliverable 1: Accepted Features

Checklist of ACs:

• If the update indicator != D, reject and do not parse the file.
• feedback report should indicate this as a cat 1 error.
  it shows as a cat 2 error but functions as a cat 1 error insofar as the data is not parsed

• 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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.48%. Comparing base (c76bc28) to head (f171f27).
Report is 3 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #2899   +/-   ##
========================================
  Coverage    93.48%   93.48%           
========================================
  Files          269      269           
  Lines         6234     6234           
  Branches       523      523           
========================================
  Hits          5828     5828           
  Misses         314      314           
  Partials        92       92           

Flag	Coverage Δ
dev-backend	93.64% <ø> (ø)
dev-frontend	92.62% <ø> (ø)

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

Files	Coverage Δ
...s-backend/tdpservice/parsers/schema_defs/header.py	100.00% <ø> (ø)
tdrs-backend/tdpservice/parsers/validators.py	89.60% <ø> (ø)

---

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 a8e288c...f171f27. Read the comment docs.

[atrimpe]

@ADPennington all good to go

[ADPennington]

@ADPennington all good to go

apologies for the delay in review. some conflicts here @atrimpe. I can review as soon as these are resolved.

[atrimpe]

@ADPennington all good to go

apologies for the delay in review. some conflicts here @atrimpe. I can review as soon as these are resolved.

@ADPennington all good

[ADPennington]

@atrimpe this is in good shape in terms of rejecting and not parsing files with the incorrect update indicator, but a couple of nits below.

• the error seems to be raised as a category 2 error instead of a category 1 error (which is one of the ACs). It appears that this is because of how the code is organized, where most of the elements in the header records are checked as postparsing_validators. If true, I think this is okay for now.
• the error message on this could be better. It is not clear what the problem is (e.g. N does not match D.). @reitermb what do you think? If we do not make a custom validator for this field, we'll need to be prepared with guidance in the TDP Knowledge Center, because this is a big change for STTs, one that will likely yield questions.

testing notes here

qasp approval on hold until @reitermb weighs in.

[reitermb]

@atrimpe this is in good shape in terms of rejecting and not parsing files with the incorrect update indicator, but a couple of nits below.

• the error seems to be raised as a category 2 error instead of a category 1 error (which is one of the ACs). It appears that this is because of how the code is organized, where most of the elements in the header records are checked as postparsing_validators. If true, I think this is okay for now.
• the error message on this could be better. It is not clear what the problem is (e.g. N does not match D.). @reitermb what do you think? If we do not make a custom validator for this field, we'll need to be prepared with guidance in the TDP Knowledge Center, because this is a big change for STTs, one that will likely yield questions.

testing notes here

qasp approval on hold until @reitermb weighs in.

I think one light tweak we could add here is Header Update Indicator: [x] does not match D so we're providing a "codeword" of sorts classifying what this is, nodding to layout docs, terminology in the knowledge center etc...

Re: The Knowledge Center I do think it's worth reframing this page around Submission Requirements more broadly—or adding a submission requirements page that will further link to that existing guide.

#2847 could also link to guidance around this in the section that breaks down what the header/trailer are.

Thoughts on this @ADPennington ?

[raftmsohani]

LGTM!

[ADPennington]

@atrimpe this is in good shape in terms of rejecting and not parsing files with the incorrect update indicator, but a couple of nits below.

• the error seems to be raised as a category 2 error instead of a category 1 error (which is one of the ACs). It appears that this is because of how the code is organized, where most of the elements in the header records are checked as postparsing_validators. If true, I think this is okay for now.
• the error message on this could be better. It is not clear what the problem is (e.g. N does not match D.). @reitermb what do you think? If we do not make a custom validator for this field, we'll need to be prepared with guidance in the TDP Knowledge Center, because this is a big change for STTs, one that will likely yield questions.

testing notes here
qasp approval on hold until @reitermb weighs in.

I think one light tweak we could add here is Header Update Indicator: [x] does not match D so we're providing a "codeword" of sorts classifying what this is, nodding to layout docs, terminology in the knowledge center etc...

Re: The Knowledge Center I do think it's worth reframing this page around Submission Requirements more broadly—or adding a submission requirements page that will further link to that existing guide.

#2847 could also link to guidance around this in the section that breaks down what the header/trailer are.

Thoughts on this @ADPennington ?

thanks @reitermb -- i think this would require a custom validator. we're currently using the matches validator which is used in several other places. @atrimpe @elipe17 is it feasible to make a custom validator for use here, per Miles' suggestion?

Also @reitermb i agree that we can plan for #2847 to handle guidance around this. I suspect that this PR will in the release for next week, will 2847 be ready by then? or will we have a draft hackmd ready?

[ADPennington]

currently blocked pending feedback from dev team. @atrimpe @elipe17

[reitermb]

Also @reitermb i agree that we can plan for #2847 to handle guidance around this. I suspect that this PR will in the release for next week, will 2847 be ready by then? or will we have a draft hackmd ready?

My goal is to have the content hack.md in review early next week before I'm partially OOO later that week/the next. My stretch goal will include the PR as well but depends on some in flight roadmap/backlog work with Rob & Andrew and tying up the submission history content PR (I'm shooting to have requested revisions on that in tomorrow).

[elipe17]

@atrimpe this is in good shape in terms of rejecting and not parsing files with the incorrect update indicator, but a couple of nits below.

• the error seems to be raised as a category 2 error instead of a category 1 error (which is one of the ACs). It appears that this is because of how the code is organized, where most of the elements in the header records are checked as postparsing_validators. If true, I think this is okay for now.
• the error message on this could be better. It is not clear what the problem is (e.g. N does not match D.). @reitermb what do you think? If we do not make a custom validator for this field, we'll need to be prepared with guidance in the TDP Knowledge Center, because this is a big change for STTs, one that will likely yield questions.

testing notes here
qasp approval on hold until @reitermb weighs in.

I think one light tweak we could add here is Header Update Indicator: [x] does not match D so we're providing a "codeword" of sorts classifying what this is, nodding to layout docs, terminology in the knowledge center etc...
Re: The Knowledge Center I do think it's worth reframing this page around Submission Requirements more broadly—or adding a submission requirements page that will further link to that existing guide.
#2847 could also link to guidance around this in the section that breaks down what the header/trailer are.
Thoughts on this @ADPennington ?

thanks @reitermb -- i think this would require a custom validator. we're currently using the matches validator which is used in several other places. @atrimpe @elipe17 is it feasible to make a custom validator for use here, per Miles' suggestion?

Also @reitermb i agree that we can plan for #2847 to handle guidance around this. I suspect that this PR will in the release for next week, will 2847 be ready by then? or will we have a draft hackmd ready?

@ADPennington, @reitermb, have a look at what I did wrt the matches validator. Since the cat1 PR merged I was able to leverage that to get the friendly name into the error message. I kept the friendly name generic for the moment since 2801 has a flushed out update to all friendly names.

[ADPennington]

@elipe17 @raftmsohani terraform pipeline issue during deployment.

[ADPennington]

@elipe17 thank you for this! HEADER update indicator: N does not match D.

[ADPennington]

@atrimpe @elipe17 @reitermb this is approved and ready to merge.

one note: the AC on this ticket was to raise a cat 1 (pre-parsing error) when update indicator isnt D. technically, this raised a cat 2 error, which i think is okay because: (1) error types will be removed from the error feedback report as part of #2897 and (2) the expected behavior of a cat 1 error (which is to not process files with incorrect header) is implemented . 🚀

test notes here