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

Add New User Management View #2544

Merged
merged 19 commits into from
Apr 30, 2024
Merged

Add New User Management View #2544

merged 19 commits into from
Apr 30, 2024

Conversation

EMaksy
Copy link
Member

@EMaksy EMaksy commented Apr 23, 2024

Description

This pr adds a user management view. (Uses only crud)

Preview:

User View

Currently, the view fetches all existing users from the backend and renders them using our Table component.
An admin user is created when installing Trento and must not be deleted by the user, this means:

  • Delete button is disabled for the admin user. (a tooltip on hover will inform the user)
  • Delete button is enabled for all other users.

By clicking on the Delete Button, a modal is opened to ask the user if he is sure that he wants to delete the displayed user.
Modal open

Pressing the cancel button closes the modal and the user is not deleted.
By pressing the deleted button a second time, the user is soft deleted in the backend and not shown anymore in the user's view.
Pressing on the username will redirect to the user edit page with the users/:id/edit route
Pressing on Create User will redirect to the user creation page with /users/new route

What was added?

  • adds new user's management view
  • new css button classes to meet design requirements
  • added api calls for listing all and deleting users
  • added user Factory to create users and admin user data
  • adds an icon on the left side nav bar that navigates to the users view
  • adds new routes to trento index page
  • adds storybook story

How was this tested?

Added test for Users view:

  • Testing empty table
  • Testing table with admin user
  • Testing table with multiple users

What is missing?

  • E2E test will come in a separate pr.

@EMaksy EMaksy added enhancement New feature or request javascript Pull requests that update Javascript code env Create an ephimeral environment for the pr branch labels Apr 23, 2024
@EMaksy EMaksy self-assigned this Apr 23, 2024
@EMaksy EMaksy force-pushed the users_view branch 3 times, most recently from a957719 to d248360 Compare April 24, 2024 16:52
@EMaksy EMaksy marked this pull request as ready for review April 24, 2024 16:58
@EMaksy EMaksy requested a review from jagabomb April 24, 2024 16:59
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

A lot of comments 😅

Other generic comment.
I think we should have UT for the table itself, which are super easy to do, just passing users as props, and other simpler one for the page, where you mock fetch and delete, so you only have 2 tests here

assets/js/common/Button/Button.jsx Outdated Show resolved Hide resolved
assets/js/lib/api/users.js Outdated Show resolved Hide resolved
assets/js/lib/test-utils/factories/users.js Outdated Show resolved Hide resolved
assets/js/pages/Layout/Layout.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/Users.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/Users.stories.jsx Show resolved Hide resolved
assets/js/pages/Users/UsersPage.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/UsersPage.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/UsersPage.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/UsersPage.jsx Outdated Show resolved Hide resolved
@jagabomb
Copy link
Contributor

Thanks for this PR @EMaksy. This looks good to me so far. I have two minor pieces of feedback for now:

  1. For the Delete User pop-up/modal, can we change the warning banner word "action" to lowercase so that it reads as follows, "This action cannot be undone."
  2. For the loading state of the Users component, should the Create button also not be hidden (@arbulu89 what do you think, screenshot is supplied below)?
Screenshot 2024-04-25 at 09 31 46

@arbulu89
Copy link
Contributor

arbulu89 commented Apr 25, 2024

@jagabomb yes, i would hide it, or at least disable it.
Anyway, I would be still fine seeing the transition from Empty data in the table to a populated data..
Maybe we could add a new loading prop to the table components, so it says Loading.. in the table, so we don't need this spinner.
This would be beneficial for all the views in fact.
PD: Actually, we could do this right now! The Table component has a emptyStateText prop, we could put Loading... there if we are loading. What do you think?

Copy link
Member

@CDimonaco CDimonaco left a comment

Choose a reason for hiding this comment

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

Everything @arbulu89 said, plus some other comments,

Overall good job but please take attention to some details.

The updateTrigger in the use effect is the most important to tackle

assets/js/common/Button/Button.jsx Outdated Show resolved Hide resolved
assets/js/pages/Layout/Layout.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/Users.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/Users.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/Users.jsx Outdated Show resolved Hide resolved

function UsersPage() {
const [modalOpen, setModalOpen] = useState(false);
const [deleteUserId, setDeleteUserId] = useState(null);
Copy link
Member

Choose a reason for hiding this comment

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

Like said before, the visual thing of the inner component should be in the inner component.

We should follow a dummy/container components approach

This UsersPage is the container, that contains logic, external dependencies, api call, the inner component is dummy, but controlled in it's visual things and use the prop passed from outside just for side effects, like api calls

assets/js/pages/Users/UsersPage.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/UsersPage.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/UsersPage.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/UsersPage.jsx Outdated Show resolved Hide resolved
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Much better @EMaksy
The prod code is looking good

assets/js/pages/Users/Users.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/Users.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/Users.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/UsersPage.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/UsersPage.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/UsersPage.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/UsersPage.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/UsersPage.jsx Show resolved Hide resolved
assets/js/pages/Users/Users.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/Users.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/UsersPage.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/UsersPage.jsx Outdated Show resolved Hide resolved
@EMaksy EMaksy force-pushed the users_view branch 2 times, most recently from f91ac01 to 7b91f67 Compare April 26, 2024 11:25
Copy link
Contributor

@jagabomb jagabomb left a comment

Choose a reason for hiding this comment

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

LGTM!

@EMaksy EMaksy force-pushed the users_view branch 2 times, most recently from a14bf63 to 665df89 Compare April 26, 2024 14:47
Copy link
Contributor

@rtorrero rtorrero left a comment

Choose a reason for hiding this comment

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

Good stuff! few comments

assets/js/lib/test-utils/factories/users.js Outdated Show resolved Hide resolved
assets/js/pages/Users/Users.jsx Show resolved Hide resolved
assets/js/pages/Users/UsersPage.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/UsersPage.test.jsx Outdated Show resolved Hide resolved
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

@EMaksy
It looks good, but I don't really like the tests 😅
You did an effort to have a totally pure component, to not use it afterwards.
I think we need 2 test files, one to test the Users.jsx and other to test the UsersPage.jsx.
Testing the 1st one is super simple, because you don't need need to mock any axios call.

About the 2nd, you just need to tests.

  • Table is populated properly
  • Used deletion works (or doesn't work and you raise a toast).
    In fact, you don't test things that you could in the Page element, like what happens in error cases etc.

Having pure components is not for storybook only, it is to have easy testing

assets/js/pages/Users/Users.jsx Show resolved Hide resolved
assets/js/pages/Users/Users.stories.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/Users.stories.jsx Show resolved Hide resolved
assets/js/pages/Users/UsersPage.test.jsx Show resolved Hide resolved
assets/js/pages/Users/UsersPage.test.jsx Outdated Show resolved Hide resolved
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Thank you @EMaksy
More comments, but ready to merge so far

assets/js/pages/Users/Users.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/Users.jsx Show resolved Hide resolved
assets/js/pages/Users/UsersPage.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/UsersPage.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/UsersPage.test.jsx Show resolved Hide resolved
assets/js/pages/Users/UsersPage.test.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/UsersPage.test.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/Users.test.jsx Outdated Show resolved Hide resolved
assets/js/pages/Users/Users.test.jsx Outdated Show resolved Hide resolved
@arbulu89 arbulu89 dismissed CDimonaco’s stale review April 30, 2024 09:45

The commented issues already addressed

@EMaksy EMaksy force-pushed the users_view branch 3 times, most recently from 1308591 to 44496be Compare April 30, 2024 09:52
@EMaksy EMaksy merged commit 1fcf1eb into user_management Apr 30, 2024
26 checks passed
@EMaksy EMaksy deleted the users_view branch April 30, 2024 10:07
arbulu89 pushed a commit that referenced this pull request May 8, 2024
* Add users view

* Add users view test

* Add users story

* Refactor and rename code

* Refactor file structure and imports

* Refactor users component

* Refactor user component and delete unused views

* Enrich user's story

* Remove empty components

* Add banner and update test

* Refactor users page and users

* Disable Create button while loading users

* Address comments

* Fix storybook

* Refactor test and test for deletion api call

* Improve factory and texts

* order and clean up code

* Split up tests and clean up

* Address final pr comments
arbulu89 pushed a commit that referenced this pull request Jun 25, 2024
* Add users view

* Add users view test

* Add users story

* Refactor and rename code

* Refactor file structure and imports

* Refactor users component

* Refactor user component and delete unused views

* Enrich user's story

* Remove empty components

* Add banner and update test

* Refactor users page and users

* Disable Create button while loading users

* Address comments

* Fix storybook

* Refactor test and test for deletion api call

* Improve factory and texts

* order and clean up code

* Split up tests and clean up

* Address final pr comments
arbulu89 added a commit that referenced this pull request Jun 25, 2024
* Basic User management (#2529)

* Enforce password security un user schema

* Argon2 hash algorithm for user password

* Add fullname and email fields to user schema

* Mix phx gen json scaffolding for users

* Users context

* mix format

* User controller refactored for the new users context

* Pow custom context for getting non-deleted users

* Unique email for users

* Update release task and seed for users

* Users context prevents deletion of user with id 1

* Username cannot be updated

* Created and updated timestamps in users view

* Change of the username when users are deleted

* Add lock feature to Users

* Locked and deleted users are excluded from renew

* Session controller tests updated

* /me endpoint returns also user id

* User socket authenticatio

* User controller broadcast user actions on user topic

* Access token expiration to 3 minutes

* Fix could not delete admin user in context

* Trento default admin enabled

* User controller with spex

* Fix app jwt auth plug test typo

* User controller actions are dispatched to per user channels

* fix access token test

* fix alias in factory file

* mix credo

* fix mispell

* mix credo

* removed comments from user_controller_test

* Addressing review feedbacks

* Introduced users factory

* Addresing feedbacks

* Updated trento_dev sql regression dump

* Revert "Updated trento_dev sql regression dump"

This reverts commit ccce860.

* Ecto seed with user password update on conflict

* Run db seed on regression tests

* Listening to user related events on the frontend (#2542)

* Frontend socket authentication refactored

* Add logged user in the state when user performs login

* Subscription to user topic on user login

* Guard component put user in state after me call

We need to enrich the user in the store not only on login
but also when the session is valid, we already call the me
endpoint in the guard to get user details, we store these details in the
state to have always consistency between the logged users on the backend
and the user in the store (returned from me endpoint at each app load)

* Listening on user actions event in the frontend

* Addressing review feedbacks

* Create Abilities and UserAbilities schema (#2543)

* mix.gen.context Abilities

* Users abilities

* Addressing review feedbacks

* Mix format

* Install make and gcc in container and rpm (#2553)

* Install make in container and rpm

* Install gcc

* Add New User Management View (#2544)

* Add users view

* Add users view test

* Add users story

* Refactor and rename code

* Refactor file structure and imports

* Refactor users component

* Refactor user component and delete unused views

* Enrich user's story

* Remove empty components

* Add banner and update test

* Refactor users page and users

* Disable Create button while loading users

* Address comments

* Fix storybook

* Refactor test and test for deletion api call

* Improve factory and texts

* order and clean up code

* Split up tests and clean up

* Address final pr comments

* Create user form (#2548)

* Move api validation errors code to lib

* Add users api

* Add users factory

* Pass props to password component

* Add UserForm view

* Add CreateUser view

* Add CreateUser to the router

* Update password policy tooltip

* Use a more standard file naming

* Create dedicated function on save clicked

* Rewrite test descriptions

* Policy enforcing on users (#2552)

* users context return user abilities

* LoadUserPlug load the user from the database using stateless user

* Wip on user controller policy

* Refactor abilities

* User controller renamed to users controller

* Refactored user openapi schemas

* Users controller policy usage refactored

* Add basic permissions

* Users controller abilities refactored

* mix credo

* addressing review feedbacks

* mix credo

* Fix policy test

* Edit user form (#2566)

* Update UserForm to allow edition style

* Add EditUserPage

* Fix conflicts after rebase resolution

* Correct some typos and descriptions

* User profile routes (#2583)

* Update user profile in users context

* profile controller wip

* Prevent profile update for admin user

* Profile controller

* Add current password validation to the user schema when profile updates
password

* Current password field in profile update schema

* Addressing review feedbacks

* mix credo

* Add soft delete for email and update test (#2584)

* Enable user status update (#2591)

* Enable locking up user in creation

* Enable status select in frontend

* Disable edit button for admin user (#2592)

* Abilities controller and update (#2568)

* Add abilities controller and index endpoint

* Add and update abilities in user operations

* Adapt user controller to use abilities

* Load abilities in profile endpoint

* Delete user abilities on deletion

* Optimistic lock for users (#2595)

* Optimistic locking on user schema update

* wip on preconditions

* mix credo

* Add optimistic locking in users controller

* Add optimistic locking handling in EditUserPage

* fix typo in users test

* fix user schema

* Add top-right corner profile button & menu (#2590)

* Store email from /me endpoint in state

* Add and use new Profile Menu component

* Use profile endpoint instead of /me

* Fix users/profile controller tests (#2607)

* Fix users controller test adding if-match

* Add admin user in profile controller test to avoid 403

* Add a generate password button for the UserForm (#2576)

* Add generate password button for input form

* Disable password generation button if user is admin

* Multi select component (#2600)

* Add react-select dependency

* Create MultiSelect component

* Add className to component

* Update classNames usage

* Update value containers color

* Switch to use link in ProfileMenu (#2608)

* Users abilities frontend (#2611)

* Add abilities api

* Load abilities and user abilities

* Update tests and stories

* Forbidden guard for user entries (#2612)

* Update redux code to load user abilities

* Implement the ForbiddenGuard

* User ForbiddenGuard in users related items

* Add missing abilities field in the user state

* Add disabled prop to multi-select (#2620)

* Interpolate validation errors in error view (#2616)

* User Profile page (#2615)

* Moved common forms labels and components to @lib/forms

* Wip profile page

* User profile form stories

* Profile password change form

* ProfileForm test

* ProfilePasswordChangeForm test

* ProfilePage test

* Close password dialog when user click save in profile view

* fix tests

* User profile form disabled when the user is the default admin

* Profile form modal toggle lift up

* Addressing review feedbacks

* Multi select for permissions in user profile

* ProfilePage dispatch setUser command in redux when user is updated

* Updated usage of multiselect in profile form

* Address review feedbacks

* User edit create toast (#2613)

* Add success and error toast

* Add toast test for create user page

* Add error toast if user is not found

* Rescue StaleError in user update function when abilities are present (#2622)

* Password change request (#2624)

* Add password_change_requested_at field and logic

* Add new field to controller and api spec

* Add maybe_ prefix to conditional functions

* Change password change requested to boolean and add to admin view

* Change placeholders in UserForm and EditUserPage(#2626)

* Password change request frontend (#2625)

* Move the toast out of profile menu and change z-index

* Move toaster inside router so it can use router data

* Load password requested field in redux store

* Add ToastNotifications component

* Create new customNotify action

* Add password change request fork in sagan index

* Dismiss password change request notification

* Use password_change_requested as boolean

* Update notifications to allow receiving health icons

* Clean and improve e2e performance (#2630)

* Remove unused commands

* Load initial state only if needed

* e2e user management (#2631)

* Update login commands for multi user usage

* Remove actions from users factory

* Add users e2e tests

* Admin cannot be edited test scenarios added

* Remove empty describe

* Add validation when a password is generated (#2640)

* Refactor generatePassword function

* Refactor test, update tooltip and enrich backend

* Enrich password test list

* User totp fields (#2646)

* Add new totp field migrations  to the user

* Add new fields to user schema

* Validate totp_enabled_at can only be nil for admin update

* Fix credo error

* Topt enrollment backend procedure (#2651)

* reset totp for users

* topt enrollment context

* topt openapi payloads

* Renamed reset totp

* Changed error when totp is invalid in the totp enrollment procedure

* TOTP Procedure in profile controller

* fix mispelling

* Addressing review feedbacks

* Login with totp (#2647)

* Add update_user_totp function

* Add totp validation code

* Update session controller to authenticate with totp

* Fix session controller create schema code

* Filter out totp_code from phoenix logs

* Send totp_code in users saga login action

* Add totp input state in login page

* Remove required fields to avoid backward incompatible error

* Use common Input in login page

* Fix tests after conflict resolution

* Totp enrollment frontend (#2657)

* Totp enrollment box component

* totp enabled field in profile controller response

* Profile totp enrollment frontend procedure

* fix mispell

* Confirmation modal on totp disable

* Addressing review feedbacks

* Allow admin disable totp (#2658)

* Allow update of totp_enabled field to false

* Allow disabling totp feature from user mgmt

* Adjust formatting

* Remove unnecessary changes and improve UserForm changes

* Various cleanups & move code to controller

* Move the totp disabling to the context

* Improve test description

* Improve UserForm component

* Fix missed ===

* Remove default padding in the Select component

* Improve TOTP select disabled check

* Add update_user test for TOTP disable

* Address remaining feedback

* Policy support functions (#2670)

* TOTP authentication e2e tests (#2667)

* Move lock user tests to new describe

* Add TOTP tests

* Skip long lasting test by default

* Disabled guard (#2672)

* Move ForbiddenGuard to common

* Create the DisabledGuard component

* Update early return usage

* Improve cloneElement usage

* Make tooltip place a prop

* Refactor storybook functions

* Some small improvements in the user forms (#2673)

* Remove TOTP configuration in user creation form

* Add disabled state to Switch component

* Disable TOTP configuration for default admin

* Style TOTP enrollment box (#2674)

* Style totp enrollment box usage in profile form

* Use profileFactory in profile form tests

* Reword multi-factor text usage

* Update e2e tests

* Use proper command to login (#2712)

* Use proper command to login

* Add page reloads to get correct suma posted data

* Set python3 as ansible interpreter (#2713)

* Set python3 as ansible interpreter

* Pin ubuntu version for pr env ci stages

---------

Co-authored-by: Carmine Di Monaco <carmine.dimonaco@suse.com>
Co-authored-by: Eugen Maksymenko <emaksymenko@suse.com>
Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request env Create an ephimeral environment for the pr branch javascript Pull requests that update Javascript code
Development

Successfully merging this pull request may close these issues.

5 participants