3398 - FRA Reports - search and upload #3445
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3445 +/- ##
===========================================
- Coverage 91.24% 91.20% -0.04%
===========================================
Files 303 311 +8
Lines 8734 9030 +296
Branches 650 693 +43
===========================================
+ Hits 7969 8236 +267
- Misses 645 663 +18
- Partials 120 131 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
jtimpe
requested review from
andrew-jameson,
raftmsohani,
elipe17,
reitermb and
victoriaatraft
| ref={errorsRef} | ||
| tabIndex="-1" | ||
| > | ||
| There {errorsCount === 1 ? 'is' : 'are'} {form.errors} error(s) in |
There was a problem hiding this comment.
I noticed the form error count doesn't update as errors are resolved which is different than the datafiles page.
| 'We can’t process that file format. Please provide a plain text file.' | ||
|
|
||
| const INVALID_EXT_ERROR = | ||
| 'Invalid extension. Accepted file types are: .txt, .ms##, .ts##, or .ts###.' |
There was a problem hiding this comment.
I think FRA is only supposed to be .csv and .xlsx. We should verify
There was a problem hiding this comment.
@victoriaatraft @reitermb @ADPennington could you confirm?
| return ( | ||
| <> | ||
| <div className={classNames({ 'border-bottom': isUploadReportToggled })}> | ||
| <SearchForm |
There was a problem hiding this comment.
If you cause a form error in the UploadForm it does not get reset if you re-search using the SearchForm.
| <fieldset className="usa-fieldset"> | ||
| <legend className="usa-label text-bold">File Type</legend> | ||
|
|
||
| {options.map(({ label, value }, index) => ( |
There was a problem hiding this comment.
We use this pattern for radio selecters in the request access page too. Should make the radio controls a generic "use everywhere" component?
There was a problem hiding this comment.
this is captured by another ticket - #3015 - but was a simple enough change (that we keep kicking around because it's low priority) so i went for it. I also made a DropdownSelect component that handles the year/quarter inputs, since those looked very similar. I tried to structure them in a way that we could go implement a Form higher-order component (or context component) and use these components within it.
one caveat is that each input still needs to be wrapped in a div to add the correct styling. e.g.,
<div
className={classNames('usa-form-group maxw-mobile margin-top-4', {
'usa-form-group--error': !valid,
})}
>
<DropdownSelect ... />
</div>This is partly necessary (form-group) partly stylistic (margin-top-4) and partly-logical (usa-form-group--error if invalid), so it didn't fit neatly into either the reusable input component or the form-specific component. To solve that, I added a classes prop where the form-specific (such as margins) styling can be added conditionally from the wrapping component, if desired. I think we use margin-top-4 everywhere, but this adds flexibility should we decide to change something in the future.
The other bit of weirdness is the default/disabled/hidden first option in DropdownSelect. I decided to key off value === '' to decide if it's the disabled option, rather than add a disabled: true key to the options array or passing in a separate disabledOption="xyz" prop to the component.
Open to suggestions on all of this! I realize both those design decisions may be confusing to read and implement
victoriaatraft
removed
the
Deploy with CircleCI-a11y
victoriaatraft
added
the
Deploy with CircleCI-a11y
jtimpe
added
Deploy with CircleCI-a11y
victoriaatraft
added
Deploy with CircleCI-a11y
victoriaatraft
removed
the
Deploy with CircleCI-a11y
|
Question and full disclosure I haven't re-gone through the code yet. But I was just testing and I signed in as a data analyst with FRA enabled and noticed that the non-default options on the file radio select are disabled. Are available selections being determined based off of my STT or is something amiss here? EDIT: This actually seems to be true no matter what group my used is assigned. I changed to OFA System Admin and saw the same behavior. I did clear browsing history and what not to be sure some weird state wasn't being cached. |
| description = | ||
| 'The Work Outcomes of TANF Exiters report contains the Social Security Numbers (SSNs) of all work-eligible individuals who exit TANF in a given quarter and the dates in YYYYMM format that each individual exited TANF.' | ||
| aboutLink = '' | ||
| templateLink = '' |
There was a problem hiding this comment.
waiting on the final links from UX!
The other options were disabled per the ticket AC - I had glossed over this line initially, so they were enabled the first time you saw it |
| handlePreview, | ||
| getTargetClassName, | ||
| tryGetUTF8EncodedFile, | ||
| } from './utils' | ||
|
|
||
| const INVALID_FILE_ERROR = |
There was a problem hiding this comment.
The file upload on FRA still expects standard datafiles.
There was a problem hiding this comment.
Found a form bug on this one too. Not sure if it was pre-existing or not. Upload a file that causes the file extension error seen above. Change search parameters, search, see that extension error is still rendered.
There was a problem hiding this comment.
Tried duplicating these on the TANF data files page and it seems to be allowing any file type to be uploaded.
There was a problem hiding this comment.
on the old data files page, we noticed that the extension errors are not being displayed. unsure exactly why, but it allows submission until the nginx file check catches it
There was a problem hiding this comment.
fixed allowed extensions and the persistent error bug. good catches, thank you!
There was a problem hiding this comment.
WRT the datafiles page, are we spinning off a new ticket for that?
| onError(error) | ||
| } | ||
|
|
||
| dispatch({ |
There was a problem hiding this comment.
Should we stick this in a finally block just for safety? Looks like onError just returns null for the moment so it might not be necessary.
| let errors = 0 | ||
| Object.keys(newFormState).forEach((key) => { | ||
| if (key !== 'errors') { | ||
| if (newFormState[key].touched && !newFormState[key].valid) { |
There was a problem hiding this comment.
nit: can we combine these conditions to one statement?
| return | ||
| } | ||
|
|
||
| const encodedFile = await tryGetUTF8EncodedFile( |
There was a problem hiding this comment.
This should only be used on FRA files iff it is a csv file. If we do this to a .xlsx it will be corrupted and unreadable on the backend.
Summary of Changes
Pull request closes #3398
TODO:implements search and upload backend endpoints -requires Add FRA report types to DataFile model and implement reparsing logic #3397TODO:implements FRA page permissions and feature flag -requires Implement feature flag for controlled access to FRA Data Files page #3399TODO:implement file encoding validation/conversion -requires Updated File Upload to Auto Encode to UTF-8 #3438How to Test
{"fra_reports": true}to user's feature flags to enable the page. Navigate to the FRA Reports page(TODO), uploads should succeedfra_reportsflag to various values. should work only when=== trueDeliverables
More details on how deliverables herein are assessed included here.
Deliverable 1: Accepted Features
Checklist of ACs:
lfrohlichand/oradpenningtonconfirmed that ACs are met.Deliverable 2: Tested Code
CodeCov Reportcomment in PR)CodeCov Reportcomment in PR)Deliverable 3: Properly Styled Code
Deliverable 4: Accessible
iamjollyandttran-hubusing 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):