Skip to content

Frontend/staff user invites#330

Merged
isabeleliassen merged 8 commits intocsg-org:developmentfrom
InspiringApps:frontend/staff-user-invites
Nov 21, 2024
Merged

Frontend/staff user invites#330
isabeleliassen merged 8 commits intocsg-org:developmentfrom
InspiringApps:frontend/staff-user-invites

Conversation

@jsandoval81
Copy link
Collaborator

@jsandoval81 jsandoval81 commented Nov 14, 2024

Requirements List

  • None

Description List

  • Small backend fix to avoid 502s when inviting users
  • Updated the form mixin to make disabled inputs configurable
  • Enable User list in the main nav for admins
  • Lay some groundwork for upcoming read-permission changes
  • Added a new UserInvite component to the UserList
  • Minor updates to the UserRowEdit component based on business reqs learned during user invite
  • Update the StaffUser serializer to include email / firstName / lastName attributes for invites

Testing List

  • yarn test:unit:all should run without errors or warnings
  • yarn serve should run without errors or warnings
  • yarn build should run without errors or warnings
  • Code review
  • Licensee-only users
    • Shouldn't see the Users link in the main nav
  • Staff users
    • Staff users without any type of admin privilege (compact or state) should not see the Users link in the main nav
    • Staff users with any type of admin privilege (compact or state) should see the Users link in the main nav
      • Should be able to click the new Invite button and successfully invite users
      • Users without compact-admin privileges should not be able to select the compact permission
      • Users with state-only-admin privileges should only be able to select states for which they are an admin

Closes #189

@ChiefStief
Copy link
Collaborator

ChiefStief commented Nov 18, 2024

There are a couple ways to mess up the alignment / formatting of the jurisdiction row

  1. In mobile:
Screenshot 2024-11-18 at 12 02 39 PM 2) With validation message: Screenshot 2024-11-18 at 12 01 09 PM

@ChiefStief
Copy link
Collaborator

ChiefStief commented Nov 18, 2024

Hard to show but the modal being up does not stop being able to scroll the background

@ChiefStief
Copy link
Collaborator

ChiefStief commented Nov 18, 2024

Not a huge deal but I might add a margin so that the modal doesn't go right to the top and bottom of the window:
Screenshot 2024-11-18 at 12 14 52 PM

@ChiefStief
Copy link
Collaborator

ChiefStief commented Nov 18, 2024 •
edited
Loading

I got a 502 when actually trying to do the invite. I thought the backend code added in the was a guard and if I included those params would still work. Is that not correct / do we need that backend update to test the whole system?
Screenshot 2024-11-18 at 12 29 51 PM

@ChiefStief
Copy link
Collaborator

ChiefStief commented Nov 18, 2024

I also dont have a good means to test the presence of the users link for many of the staff variations as I only have my one account. I suppose I could invite myself to the requisite permission levels and maintain those accounts once we get through the 502 issue. (I did get the email from the "failed" sign up")

@jsandoval81
Copy link
Collaborator Author

jsandoval81 commented Nov 18, 2024 •
edited
Loading

@ChiefStief

  1. There are a couple ways to mess up the alignment / formatting of the jurisdiction row

    • In mobile:

      • I added some styling to ellipsis the text on mobile. This is a pretty custom form UI so I suspect this can soak here and we can see how it performs.

    • With validation message:

      • I added some styling to keep the preserve the alignment better with this custom form UI

  2. Hard to show but the modal being up does not stop being able to scroll the background

    • I don't know of that's a hard requirement. I feel like that's acceptable for now, especially given the risk of manipulating the <body> element as the likely fix. If we don't undo that correctly then different browsers & form factors can easily get stuck

  3. Not a huge deal but I might add a margin so that the modal doesn't go right to the top and bottom of the window:

    • Another place where I'm not sure there's a hard requirement. I figured admins that were working with a lot of states would want as much real estate as possible.

  4. I got a 502 when actually trying to do the invite

    • This was the fix that the backend team committed to this branch in 82e202b

  5. I also dont have a good means to test the presence of the users link for many of the staff variations

    • I had to use our local mock data to test some of these scenarios. For instance, temporarily manipulating the permissions of staffAccount in mock.data.ts - changing some / all of the true to false.

@jsandoval81
Copy link
Collaborator Author

jsandoval81 commented Nov 18, 2024

@jlkravitz This is ready for your review.

@jlkravitz
Copy link
Collaborator

jlkravitz commented Nov 19, 2024 •
edited
Loading

  • review the pull request to get oriented
    • read the description of the pull request, which should summarize the changes made
    • read through every task on the Scrum board that's encompassed by this pull request
    • read the description of the commits that comprise the pull request
  • skim all new code, in the context of existing code, looking for problems (knowing that the vast majority of new code will be covered by tests)
  • review all tests
    • methodically review all new tests for correctness, quality of naming
    • look at code coverage of tests
    • determine what code isn’t tested, review that rigorously
  • review documentation to ensure that it matches changes
  • provide comments on the pull request on GitHub, as necessary
    • for comments that are specific to a particular line of code, comment on those specific lines
    • for comments that are more general, attach the comment to a random line in README.md (as opposed to commenting on the pull request itself), to be able to use GitHub's ability to thread discussions on those comments
  • run a security audit of dependencies (e.g. npm audit and pip audit) to ensure that there are no vulnerabilities that will be deployed to production (as opposed to vulnerabilities that only have an impact on the development environment)

jlkravitz
jlkravitz reviewed Nov 19, 2024
View reviewed changes
Copy link
Collaborator

@jlkravitz jlkravitz left a comment

Choose a reason for hiding this comment

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

Looks great! A few comments/questions.

jlkravitz
jlkravitz approved these changes Nov 20, 2024
View reviewed changes
Copy link
Collaborator

@jlkravitz jlkravitz left a comment

Choose a reason for hiding this comment

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

@isabeleliassen good to merge!

@isabeleliassen isabeleliassen merged commit 6ca79d9 into csg-org:development Nov 21, 2024
@jsandoval81 jsandoval81 deleted the frontend/staff-user-invites branch January 29, 2025 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jlkravitz jlkravitz jlkravitz approved these changes

@isabeleliassen isabeleliassen isabeleliassen approved these changes

@carlsims carlsims Awaiting requested review from carlsims

+1 more reviewer

@ChiefStief ChiefStief ChiefStief approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

user management invite UI

5 participants

@jsandoval81 @ChiefStief @jlkravitz @isabeleliassen @jusdino