Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 858: Upload functionality improvements #916

Merged
merged 17 commits into from
Jun 2, 2021

Conversation

jorgegonzalez
Copy link

@jorgegonzalez jorgegonzalez commented May 14, 2021

Summary of Changes

As a followup to some of the upload feature work, this PR addresses various issues to enhance clarity and reduce distractions in the upload flow, and is particularly impactful for screen reader users.

_Addresses issue #858
closes #858
Acceptance criteria as stated in the issue

How to Test

TODO

  • Login to the app
  • Navigate to the "Data Files" Tab
  • If the user is not an OFA Admin, ensure there is an error suggesting an STT is not selected
    • If this is the case, navigate to the "Profile" tab, select an STT, and click "Request Access"
    • Navigate back to "Data Files"
    • The missing STT error should be gone.
  • Click "Search" without filling any of the required fields.
  • Ensure the focus shifts to the new error message.
  • Fill out all required fields and hit "Search".
  • The file input section should appear with the correct header.
  • Make changes to the form.
    • Ensure that the file input section doesn not de-render.

Deliverable 1: Accepted Features

Performance Standard(s): At the beginning of each sprint, the Product Owner and development team will collaborate to define a set of user stories to be completed during the sprint. Acceptance criteria for each story will also be defined. The development team will deliver code and functionality to satisfy these user stories.

Acceptable Quality Level: Delivered code meets the acceptance criteria for each user story. Incomplete stories will be assessed and considered for inclusion in the next sprint.

  • Look up the acceptance criteria in the related issue; paste ACs below in checklist format.
  • Check against the criteria (as a hotfix, this PR addresses only one criterion):
    • File input "results" don't change or disappear unless "Search" is clicked
    • When (STT/Fiscal Year/Quarter) form has errors, focus shifts to error summary whenever "Search" is clicked
    • Changes to error summary continues to be announced as an alert as individual errors are resolved

As facilitator/product manager, @kniz-raft will decide if ACs are met from Raft's perspective.

Deliverable 2: Tested Code

Performance Standard(s): Code delivered under the order must have substantial test code coverage. Version-controlled HHS GitHub repository of code that comprises products that will remain in the government domain.

Acceptable Quality Level: Minimum of 90% test coverage of all code. All areas of code are meaningfully tested.

  • Are all areas of code introduced in this PR meaningfully tested?
    • If this PR introduces frontend code changes, are they meaningfully tested?
  • Are code coverage minimums met?

Deliverable 3: Properly Styled Code

Performance Standard(s): GSA 18F Front- End Guide

Acceptable Quality Level: 0 linting errors and 0 warnings

  • Are frontend code style checks passing on CircleCI?

Deliverable 4: Accessible

Performance Standard(s): Web Content Accessibility Guidelines 2.1 AA standards

Acceptable Quality Level: 0 errors reported using an automated scanner and 0 errors reported in manual testing

Deliverable 5: Deployed

Performance Standard(s): Code must successfully build and deploy into the staging environment.

Acceptable Quality Level: Successful build with a single command

NOTE: until we have a proper staging environment this may not be satisfiable prior to merging

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

Deliverable 6: Documented

Performance Standard(s): Summary of user stories completed every two weeks. All dependencies are listed and the licenses are documented. Major functionality in the software/source code is documented, including system diagram. Individual methods are documented inline in a format that permits the use of tools such as JSDoc. All non-inherited 800-53 system security controls are documented in the Open Control or OSCAL format and HHS Section 508 Product Assessment Template (PAT) are updated as appropriate.

Acceptable Quality Level: Combination of manual review and automated testing, if available

  • If this PR introduces frontend code, is that code documented both inline and overall?

Deliverable 7: Secure

Performance Standard(s): Open Web Application Security Project (OWASP) Application Security Verification Standard 3.0

Acceptable Quality Level: Code submitted must be free of medium- and high-level static and dynamic security vulnerabilities

  • Does the OWASP Scan pass on CircleCI?
  • Do manual code review and manual testing detect any security issues?
    • None detected

@jorgegonzalez jorgegonzalez self-assigned this May 14, 2021
@jorgegonzalez jorgegonzalez added the a11y Accessibility/Section 508 label May 14, 2021
@jorgegonzalez jorgegonzalez changed the base branch from raft-tdp-main to fix/858 May 14, 2021 20:36
Base automatically changed from fix/858 to raft-tdp-main May 17, 2021 21:45
@@ -191,7 +191,7 @@ describe('Reports', () => {
expect(getByText('Section 4 - Stratum Data')).toBeInTheDocument()
})

it('should de-render the UploadReports form after it has been toggled but the year is changed', () => {
it('should not de-render the UploadReports form after it has been toggled but the year is changed', () => {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AC describes the opposite of this test, and is updated below appropriately.

Comment on lines +141 to +145
<div
className="margin-top-4 usa-error-message"
role="alert"
ref={errorsRef}
tabIndex="-1"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure the error message is focused on submission, if there are errors.

}
if (form.length === 3) {
setIsToggled(true)
setSubmittedHeader(reportHeader)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Save a reference to the previous "submitted" header, so that the header that is currently rendered is static, and does not change if the user changes form selections.

@@ -194,7 +190,7 @@ function EditProfile() {
<STTComboBox
selectStt={setStt}
error={Boolean(errors.stt)}
selectedStt={profileInfo?.stt?.toLowerCase()}
selectedStt={profileInfo?.stt}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the .toLowerCase() call from all of these above for consistency.

const form = wrapper.find('.usa-form').hostNodes()
form.simulate('submit', {
preventDefault: () => {},
})

expect(store.dispatch).toHaveBeenCalledTimes(6)
expect(store.dispatch).toHaveBeenCalledTimes(1)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd eventually like to update all these (useless) toHaveBeenCalledTimes tests that count dispatch calls.

@jorgegonzalez jorgegonzalez marked this pull request as ready for review May 18, 2021 13:42
@jorgegonzalez jorgegonzalez added raft review This issue is ready for raft review and removed WIP labels May 18, 2021
@jorgegonzalez jorgegonzalez requested a review from jtwillis92 May 18, 2021 13:46
@jtwillis92
Copy link

I noticed that when the search form is changed and submitted after a file has been uploaded it follows to the new upload sections - is that intended?

@jorgegonzalez
Copy link
Author

I noticed that when the search form is changed and submitted after a file has been uploaded it follows to the new upload sections - is that intended?

Hmmm this sounds like a bug, fixing now

@jorgegonzalez jorgegonzalez requested review from jtwillis92 and removed request for jtwillis92 May 24, 2021 21:55
Copy link

@jtwillis92 jtwillis92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, tested locally and ran into no issues with the listed "How to Test" steps!

Here are some screenshots I took at various points throughout the testing flow:
Screenshot from 2021-06-01 15-48-13
Screenshot from 2021-06-01 15-48-32
Screenshot from 2021-06-01 15-49-15
Screenshot from 2021-06-01 15-48-41

@jtwillis92 jtwillis92 added QASP Review and removed raft review This issue is ready for raft review labels Jun 1, 2021
@jtwillis92 jtwillis92 requested a review from ADPennington June 1, 2021 19:52
@jorgegonzalez
Copy link
Author

jorgegonzalez commented Jun 1, 2021

Note that the An STT is not set for this user message is a temporary warning until we lock the Data Files page behind some role check.

@ADPennington
Copy link
Collaborator

@jorgegonzalez @jtwillis92 can the latest changes be merged into this branch?

@jorgegonzalez
Copy link
Author

2021-06-02 16 20 22

Copy link
Collaborator

@ADPennington ADPennington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deliverable 1: Accepted Features

Performance Standard(s): At the beginning of each sprint, the Product Owner and development team will collaborate to define a set of user stories to be completed during the sprint. Acceptance criteria for each story will also be defined. The development team will deliver code and functionality to satisfy these user stories.

Acceptable Quality Level: Delivered code meets the acceptance criteria for each user story. Incomplete stories will be assessed and considered for inclusion in the next sprint.

  • Look up the acceptance criteria in the related issue; paste ACs below in checklist format.
    • Check against the criteria:
    • File input "results" don't change or disappear unless "Search" is clicked
      during 6/2 dev sync, we tested the route where an STT user uploads a file but--before submitting-- makes selection changes. Currently that uploaded file will be lost without warning, so we agreed that it would be helpful if the user was notified of this before changes are applied. Captured this as issue As an OFA admin/STT user, I want to be notified if I change search selections after a file has been uploaded but not yet submitted #992.
    • When (STT/Fiscal Year/Quarter) form has errors, focus shifts to error summary whenever "Search" is clicked
    • Changes to error summary continues to be announced as an alert as individual errors are resolved

As Product Owner, @lfrohlich will decide if ACs are met.

Deliverable 2: Tested Code

Performance Standard(s): Code delivered under the order must have substantial test code coverage. Version-controlled HHS GitHub repository of code that comprises products that will remain in the government domain.

Acceptable Quality Level: Minimum of 90% test coverage of all code. All areas of code are meaningfully tested.

  • Are all areas of code introduced in this PR meaningfully tested?
    • If this PR introduces frontend code changes, are they meaningfully tested?
  • Are code coverage minimums met?

Deliverable 3: Properly Styled Code

Performance Standard(s): GSA 18F Front- End Guide

Acceptable Quality Level: 0 linting errors and 0 warnings

  • Are backend code style checks passing on CircleCI?
  • Are frontend code style checks passing on CircleCI?

Deliverable 4: Accessible

Performance Standard(s): Web Content Accessibility Guidelines 2.1 AA standards

Acceptable Quality Level: 0 errors reported using an automated scanner and 0 errors reported in manual testing

Deliverable 5: Deployed

Performance Standard(s): Code must successfully build and deploy into the staging environment.

Acceptable Quality Level: Successful build with a single command

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

Deliverable 6: Documented

Performance Standard(s): Summary of user stories completed every two weeks. All dependencies are listed and the licenses are documented. Major functionality in the software/source code is documented, including system diagram. Individual methods are documented inline in a format that permits the use of tools such as JSDoc. All non-inherited 800-53 system security controls are documented in the Open Control or OSCAL format and HHS Section 508 Product Assessment Template (PAT) are updated as appropriate.

Acceptable Quality Level: Combination of manual review and automated testing, if available

  • If this PR introduces frontend code, is that code documented both inline and overall?

Deliverable 7: Secure

Performance Standard(s): Open Web Application Security Project (OWASP) Application Security Verification Standard 3.0

Acceptable Quality Level: Code submitted must be free of medium- and high-level static and dynamic security vulnerabilities

  • Does the OWASP Scan pass on CircleCI?
  • Do manual code review and manual testing detect any security issues?
    none detected

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[A11y] Upload functionality improvements
3 participants