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

TTTA-HUB 22: Add User Permission to Unlock Approved Reports #478

Merged
merged 10 commits into from
Oct 28, 2021

Conversation

AdamAdHocTeam
Copy link

@AdamAdHocTeam AdamAdHocTeam commented Oct 27, 2021

Description of change

Added new global param to allow select users to UNLOCK approved reports.

How to test

Enable the new UNLOCK global user param on your user. Then create a new report and approve. When viewing the approved report you should now see the UNLOCK button. Clicking this button and agreeing to unlock should set the report and all approvers back to a NEEDS_ACTION state.

Issue(s)

Checklists

Every PR

  • Meets issue criteria
  • JIRA ticket status updated
  • Code is meaningfully tested
  • Meets accessibility standards (WCAG 2.1 Levels A, AA)
    - [ ] API Documentation updated
    - [ ] Boundary diagram updated
    - [ ] Logical Data Model updated
    - [ ] Architectural Decision Records written for major infrastructure decisions

Production Deploy

- [ ] Staging smoke test completed

After merge/deploy

  • Update JIRA ticket status

@AdamAdHocTeam AdamAdHocTeam changed the title Ttahub 22 unlock approved reports TTTA-HUB 22: Add Ability to Unlock Approved Reports Oct 27, 2021
@AdamAdHocTeam AdamAdHocTeam changed the title TTTA-HUB 22: Add Ability to Unlock Approved Reports TTTA-HUB 22: Add User Permission to Unlock Approved Reports Oct 27, 2021
@thewatermethod
Copy link
Collaborator

thewatermethod commented Oct 27, 2021

I understand it might be out of scope of this PR to handle all the accessibility issues with our modals, but I would at least like to see

  • "Copy URL Link" & "Print PDF" disabled when the modal is open. I will probably open a Jira for handling modal related issues keyboard navigation as far the sidebar links, etc
  • Probably some expectation setting via an aria-label that the button will reload the page if you do unlock an approved report
  • Focus jumps right to cancel when you open the window, which is good, and I think the page reloading when you click "Unlock approved report" will handle that focus management. However, when you pick cancel, ideally focus would be returned to that button.

Copy link
Collaborator

@thewatermethod thewatermethod left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few minor things

frontend/src/components/UnlockReportModal.css Outdated Show resolved Hide resolved
frontend/src/components/UnlockReportModal.css Outdated Show resolved Hide resolved
frontend/src/components/UnlockReportModal.js Outdated Show resolved Hide resolved
frontend/src/pages/ApprovedActivityReport/index.js Outdated Show resolved Hide resolved
@jasalisbury
Copy link

I understand it might be out of scope of this PR to handle all the accessibility issues with our modals, but I would at least like to see

  • "Copy URL Link" & "Print PDF" disabled when the modal is open. I will probably open a Jira for handling modal related issues keyboard navigation as far the sidebar links, etc
  • Probably some expectation setting via an aria-label that the button will reload the page if you do unlock an approved report
  • Focus jumps right to cancel when you open the window, which is good, and I think the page reloading when you click "Unlock approved report" will handle that focus management. However, when you pick cancel, ideally focus would be returned to that button.

Playing with USWDS modals here, the focus is trapped inside the modal and the modal closes when I click out. This makes me wonder, 1) Are we using the react-uswds modal correctly? 2) Do we need to update the react-uswds package so our modals act the same as their example?

Copy link

@jasalisbury jasalisbury left a comment

Choose a reason for hiding this comment

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

Looks good! Only have one comment. Also can you add the new route to the API documentation at docs/openapi?

frontend/src/pages/ApprovedActivityReport/index.js Outdated Show resolved Hide resolved
@AdamAdHocTeam
Copy link
Author

Looks good! Only have one comment. Also can you add the new route to the API documentation at docs/openapi?

This has been updated.

@AdamAdHocTeam AdamAdHocTeam merged commit bfd4f7e into main Oct 28, 2021
@AdamAdHocTeam AdamAdHocTeam deleted the ttahub-22-unlock-approved-reports branch October 28, 2021 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants