Frontend File Validation

[elipe17]

Summary of Changes

• Updated error message when submitting and invalid datafile to be more verbose.
  Pull request closes As a user who submits data files, I want to know if my file upload has the incorrect file extension #2664

How to Test

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

• Log in as a user who can submit datafiles
• Submit a datafile with either an invalid extension or an empty datafile. Verify error message accurately explains the issue with the file.

Deliverables

More details on how deliverables herein are assessed included here.

Deliverable 1: Accepted Features

Checklist of ACs:

• when a user uploads an a file with incorrect extension , an error message indicating such is displayed
• 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?
  passes accessibility but image not rendering for unrecognized file types (which is reasonable for this ticket) but also not rendering for recognized but unexpected file types, so needs a follow-on ticket.

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

Merging #2690 (4d0ae48) into develop (8ec31f9) will decrease coverage by 0.40%.
Report is 43 commits behind head on develop.
The diff coverage is 92.32%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2690      +/-   ##
===========================================
- Coverage    92.99%   92.59%   -0.40%     
===========================================
  Files          219      239      +20     
  Lines         4482     5307     +825     
  Branches       385      469      +84     
===========================================
+ Hits          4168     4914     +746     
- Misses         242      300      +58     
- Partials        72       93      +21     

Flag	Coverage Δ
dev-backend	92.51% <92.89%> (-0.33%)	⬇️
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 52 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 954edc7...4d0ae48. Read the comment docs.

[jtimpe]

Looks mostly good from my perspective but I did have one note. When a file of mismatched extension is dropped in or selected, the message is displayed and the file is immediately cleared out of the upload box. This is fine but maybe slightly better would be to leave the selected file in the upload box but disable the submission, so the user can compare the extension to the allowed extensions.

@reitermb i'm curious to get your thoughts so I requested your review

[reitermb]

Looks mostly good from my perspective but I did have one note. When a file of mismatched extension is dropped in or selected, the message is displayed and the file is immediately cleared out of the upload box. This is fine but maybe slightly better would be to leave the selected file in the upload box but disable the submission, so the user can compare the extension to the allowed extensions.

@reitermb i'm curious to get your thoughts so I requested your review

I like that as an enhancement—though I think we should make sure we're keeping behavior consistent in allowing subsets of sections to be submitted. e.g. this scenario:

Keeping the error, keeping the file the user attempted to attach visible, but allowing any passable files that were attached to submit.

If we go down this road it would also actually make things more consistent with the current (slightly bugged) screenreader experience on these file pickers (this relates to #1107). Current behavior is that when this extension error occurs the screenreader still reads the file name even though it's no longer visibly attached.

[raftmsohani]

Only small change but the rest looks good to me 👍

[elipe17]

Looks mostly good from my perspective but I did have one note. When a file of mismatched extension is dropped in or selected, the message is displayed and the file is immediately cleared out of the upload box. This is fine but maybe slightly better would be to leave the selected file in the upload box but disable the submission, so the user can compare the extension to the allowed extensions.
@reitermb i'm curious to get your thoughts so I requested your review

I like that as an enhancement—though I think we should make sure we're keeping behavior consistent in allowing subsets of sections to be submitted. e.g. this scenario:

Keeping the error, keeping the file the user attempted to attach visible, but allowing any passable files that were attached to submit.

If we go down this road it would also actually make things more consistent with the current (slightly bugged) screenreader experience on these file pickers (this relates to #1107). Current behavior is that when this extension error occurs the screenreader still reads the file name even though it's no longer visibly attached.

@reitermb, This would be what the frontend looks like if we didnt remove the file from the selector. How do you feel about this?

[reitermb]

Looks mostly good from my perspective but I did have one note. When a file of mismatched extension is dropped in or selected, the message is displayed and the file is immediately cleared out of the upload box. This is fine but maybe slightly better would be to leave the selected file in the upload box but disable the submission, so the user can compare the extension to the allowed extensions.
@reitermb i'm curious to get your thoughts so I requested your review

I like that as an enhancement—though I think we should make sure we're keeping behavior consistent in allowing subsets of sections to be submitted. e.g. this scenario:
Keeping the error, keeping the file the user attempted to attach visible, but allowing any passable files that were attached to submit.
If we go down this road it would also actually make things more consistent with the current (slightly bugged) screenreader experience on these file pickers (this relates to #1107). Current behavior is that when this extension error occurs the screenreader still reads the file name even though it's no longer visibly attached.

@reitermb, This would be what the frontend looks like if we didnt remove the file from the selector. How do you feel about this?

It's good with me! If it's easy to add one style override in there we could also make the background of the file picker our light red error color (#f4e3db) to give it a slightly more cohesive error state but definitely just a nice to have here vs an a11y or usability need. I wouldn't add any scope for that tweak.

[elipe17]

@jtimpe @reitermb , We could also do something like the screenshot below. Remove the file, but use it's name in the error message so the user can see the incorrect/missing extension. This also side steps the weird render issue with the icon in the file drop box when we leave the file present in the error state. Let me know your thoughts.

[reitermb]

@jtimpe @reitermb , We could also do something like the screenshot below. Remove the file, but use it's name in the error message so the user can see the incorrect/missing extension. This also side steps the weird render issue with the icon in the file drop box when we leave the file present in the error state. Let me know your thoughts.

I'd lean keeping the file name in the normal spot rather than in the error message so that we're not mixing expectations for where to look for it inside the component. It would also make a small a11y issue (file picker errors are only read by screenreaders when they occur rather than anytime the input gets focus) that exists currently slightly higher severity.

Re: the icon are there any lightweight options to provide one for the "if else" non-accepted extension case? If so we could throw an error icon of some flavor in there to emphasize the issue even more.

[ADPennington]

@elipe17 is this ready for qasp again?

im still seeing this:

[elipe17]

@elipe17 is this ready for qasp again?

im still seeing this:

@ADPennington it is not ready yet. I am hoping to work with Mo today to see if we can figure out why it doesnt work when running in docker. I will ping you to let you know when it is ready again.

[elipe17]

@ADPennington This is what it looks like when the file is removed from the input.

[ADPennington]

approving this because it passes a11y review and there is a meaningful message to alert the user that the file uploaded does not have a proper extension. @elipe17 @reitermb @ttran-hub @Smithh-Co

Below are screenshots of different file types being uploaded and results.

tdrs file names (expected)

non-txt or tdrs filename (unexpected)

.pdf (unexpected but no file image rendering)

txt file name (expected)

.png (image file rendering but unexpected file type)