-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add user list page #965
Add user list page #965
Conversation
Sigrid maintainability feedback✅ You wrote maintainable code and achieved your objective of 3.5 stars Show detailsSigrid compared your code against the baseline of 2025-02-06. 👍 What went well?
👎 What could be better?
📚 Remaining technical debt
View this system in Sigrid** to explore your technical debt ⭐️ Sigrid ratings
💬 Did you find this feedback helpful?We would like to know your thoughts to make Sigrid better. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List page looks good to me, with following review remarks:
- Have not been able to test date formatting in the 'last activity' column
- Have not been able to verify all user roles are displayed correctly (only 1 user in database)
- The 'admin' user in the database is an anonymous account, which should be impossible
- The button to add a user is lacking, but will probably be added when the 'add' feature is implemented?
Thanks @jorisleker ! The frontend preview is also deployed and has more users. The button will indeed be added later, and fair point about the 'admin', I'll add a fullname to make it more representable in a separate PR. |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #965 +/- ##
==========================================
+ Coverage 89.80% 89.94% +0.13%
==========================================
Files 238 241 +3
Lines 12561 12645 +84
Branches 1281 1289 +8
==========================================
+ Hits 11281 11373 +92
+ Misses 1191 1183 -8
Partials 89 89 ☔ View full report in Codecov by Sentry. |
@oliver3 approved changed test, but now thinking: we should probably say 'yesterday 13:37' if last activity was yesterday, and not the day name. Day name for anything 2-6 days ago is fine. Before that, it should be a full date. Earlier assumption was that Abacus would only be used during a couple of days, but now we have more insights into the NPVV we know that even for GR2026 it will be used during at least 1 week (GSB and CSB seatings are about 1 week apart) |
Something like this @jorisleker ? |
Resolves #915
list
touser_list
Page can be reached from the
/dev
page.