3398 - FRA Reports - search and upload #3445
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3445 +/- ##
===========================================
- Coverage 91.15% 90.70% -0.46%
===========================================
Files 308 318 +10
Lines 8832 9189 +357
Branches 670 715 +45
===========================================
+ Hits 8051 8335 +284
- Misses 654 717 +63
- Partials 127 137 +10
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
|
I am getting circular import error. |
It is fixed now |
|
options for FRA file types are grayed out |
This is correct per the ticket AC. covered here - #3445 (comment) |
| @@ -14,7 +13,11 @@ import { | |||
| } from '../../actions/reports' | |||
| import Button from '../Button' | |||
| import createFileInputErrorState from '../../utils/createFileInputErrorState' | |||
| import { handlePreview, getTargetClassName } from './utils' | |||
There was a problem hiding this comment.
'Cancel' button in the file submit form: makes the form to disappear. I think it should only clear the form and not delete it.
There was a problem hiding this comment.
@reitermb @victoriaatraft hiding the form on Cancel is consistent with the other data files page, but Mo brings up a good point. thoughts?
There was a problem hiding this comment.
I'm good to move to clearing the upload portion of the form instead of hiding it. Open to doing that here or in a follow-on as long as we make sure to ticket/implement for both the TANF/SSP and FRA sides.
There was a problem hiding this comment.
I started to change the form cancel behaviors, but I forgot about a bug that prevented it from actually working
the uswds file input doesn't automatically clear the preview state when the file is set to null. it doesn't have a clear function exposed at all, so the only way to get the preview to disappear (even after you've cleared the file itself) is to un-render and re-render the file input. We could hide and re-show the form, but i thought the jumpiness would be poor user experience. i'll leave it the way it is for now
tdrs-backend/tdpservice/conftest.py
Outdated
| 'file': create_temporary_file(fake_file, 'report.csv'), | ||
| "original_filename": 'report.csv', | ||
| "slug": str(uuid.uuid4()), | ||
| "extension": "txt", |
There was a problem hiding this comment.
Also, should the section and group be different?
| } from '../../selectors/auth' | ||
|
|
||
| const isAllowed = ( | ||
| { permissions, isApproved }, | ||
| { permissions, isApproved, featureFlags }, |
There was a problem hiding this comment.
Should this have an override to all Sys admin and maybe developer groups to see everything? I was a little confused when I logged in as a Sys admin and could not see the FRA tab by default.
There was a problem hiding this comment.
This could certainly be follow on work. Or we can just specifiy that feature flags can be mutually exclusive from group assignments.
There was a problem hiding this comment.
good point. i personally prefer the explicit/mutually-exclusive approach but i'll let @ADPennington make the final call
|
@reitermb looking into header focus, then will move to QASP |
jtimpe
added
QASP Review
and removed
raft review
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):