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

Save user to backend #1027

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Save user to backend #1027

wants to merge 7 commits into from

Conversation

oliver3
Copy link
Contributor

@oliver3 oliver3 commented Feb 12, 2025

This PR will resolve #983

  • A new useQueryParam hook to more easily retrieve and clear a message sent through the url query string

  • Save the user from the UserCreateDetailsPage, show an error if it occurs

  • Show success message on the user list page

  • NavBar management links for all /users* pages

  • Add UserCreate.test.tsx that tests the flow through the pages using the context and the api call at the end

NB

@oliver3 oliver3 linked an issue Feb 12, 2025 that may be closed by this pull request
@oliver3 oliver3 self-assigned this Feb 12, 2025
Copy link

github-actions bot commented Feb 12, 2025

Sigrid maintainability feedback

✅ You wrote maintainable code and achieved your objective of 3.5 stars

Show details

Sigrid compared your code against the baseline of 2025-02-13.

👍 What went well?

You fixed or improved 0 refactoring candidates.

👎 What could be better?

Unfortunately, 4 refactoring candidates were introduced or got worse.

Risk System property Location
🟡 Unit Size
(Introduced)
frontend/app/module/users/create/UserCreateDetailsPage.tsx
handleSubmit(FormEvent<HTMLFormElement>)
🟡 Unit Size
(Introduced)
frontend/app/module/users/UserListPage.tsx
UserListPage.tsx.UserListPage()
🟡 Unit Size
(Worsened)
frontend/app/module/users/create/UserCreateRolePage.tsx
handleSubmit(FormEvent<HTMLFormElement>)
🟡 Unit Complexity
(Introduced)
frontend/app/module/users/create/UserCreateDetailsPage.tsx
UserCreateDetailsPage.tsx.UserCreateDetailsPage()

📚 Remaining technical debt

2 refactoring candidates didn't get better or worse, but are still present in the code you touched.

View this system in Sigrid** to explore your technical debt

⭐️ Sigrid ratings

System property System on 2025-02-13 Before changes New/changed code
Volume 5.2 N/A N/A
Duplication 4.4 5.5 5.5
Unit Size 2.4 4.0 2.3
Unit Complexity 3.4 3.8 3.3
Unit Interfacing 3.0 5.5 5.5
Module Coupling 4.0 5.5 5.5
Component Independence 2.8 N/A N/A
Component Entanglement 3.7 N/A N/A
Maintainability 3.6 4.7 4.2

💬 Did you find this feedback helpful?

We would like to know your thoughts to make Sigrid better.
Your username will remain confidential throughout the process.


View this system in Sigrid

Copy link

codecov bot commented Feb 12, 2025

Codecov Report

Attention: Patch coverage is 98.21429% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.30%. Comparing base (35cbdd9) to head (48dc1a2).
Report is 3 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
.../module/users/create/UserCreateContextProvider.tsx 86.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1027      +/-   ##
==========================================
+ Coverage   89.85%   90.30%   +0.44%     
==========================================
  Files         249      250       +1     
  Lines       13073    13145      +72     
  Branches     1328     1352      +24     
==========================================
+ Hits        11747    11870     +123     
+ Misses       1232     1181      -51     
  Partials       94       94              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@oliver3 oliver3 added the frontend Issues or pull requests that relate to the frontend label Feb 12, 2025
@oliver3 oliver3 marked this pull request as ready for review February 12, 2025 16:47
@oliver3 oliver3 requested a review from a team as a code owner February 12, 2025 16:47
@jschuurk-kr
Copy link
Contributor

@oliver3 Played around for a few minutes and noticed two things:
1. We don't seem to have any restrictions on username. If you want spaces or emoji in your username, you can! This is arguably a feature, since I don't think there are any technical reasons for such restrictions.

2. For password length, six Abacus emoji (🧮🧮🧮🧮🧮🧮) is accepted as being 12 characters long. It's probably worth fixing to avoid the discussion "But what is a character?" for the minimum length requirement of passwords.

@oliver3
Copy link
Contributor Author

oliver3 commented Feb 13, 2025

@oliver3 Played around for a few minutes and noticed two things:

  1. We don't seem to have any restrictions on username. If you want spaces or emoji in your username, you can! This is arguably a feature, since I don't think there are any technical reasons for such restrictions.

  2. For password length, six Abacus emoji (🧮🧮🧮🧮🧮🧮) is accepted as being 12 characters long. It's probably worth fixing to avoid the discussion "But what is a character?" for the minimum length requirement of passwords.

We'll revisit the password in #1021 (comment) and I've added a comment there that we'll go for printable ASCII and space characters.

@oliver3 oliver3 marked this pull request as ready for review February 13, 2025 12:20
Copy link
Contributor

@praseodym praseodym left a comment

Choose a reason for hiding this comment

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

lgtm, two questions:

  1. Do we also want a Playwright e2e test for this functionality? (cc @jschuurk-kr)
  2. To be considered for a new issue in this epic: it looks like the user list page is currently (implicitly) sorted by user id, maybe sorting by role and then by username makes more sense? It looks like that is the sorting order in the Figma design.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed that the font size for the error message is too big (bigger than the rest of the text and bigger than in the design) which also causes it to wrap to two lines:
Screenshot 2025-02-13 at 16 59 41
(Screenshot from Safari)

Copy link
Contributor

@jorisleker jorisleker 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, great start!

  • Agree with @praseodym on the sorting order on the overview page
  • When saving/creating a user (on a slow connection), it takes quite some time for the form to be submitted. This is a bit confusing, it seems as if nothing is happening. Can we blank out the entire page or disable the submit button?

@jschuurk-kr
Copy link
Contributor

1. Do we also want a Playwright e2e test for this functionality? (cc @jschuurk-kr)

I think adding one happy path e2e test makes sense. All the rest I'd expect us to cover with component tests and backend integration tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Issues or pull requests that relate to the frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Save user to backend
4 participants