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

feat(oh2-287): admin user group crud #637

Open
wants to merge 50 commits into
base: develop
Choose a base branch
from

Conversation

gasp
Copy link
Contributor

@gasp gasp commented Jul 24, 2024

See oh2-287.

  • better handling of user&group tabs (allowing direct access via location state)
  • create a new usergroup
  • edit usergroups
  • manage usergroups permissions
  • delete a usergroup

create a new usergroup

group-create

edit a usergroup

group-update

manage usergroup permissions

groups-permissions

delete a usergroup

group-delete

gasp and others added 30 commits June 24, 2024 15:22
…rmatici#622)

* tests:OH2-326 | Tests | Add cypress e2e tests to cover admin/diseases

* chore:Improve cypress indexing
* fix:OH2-325 | design issues in wards admin

* chore:fix design issues in admin components

* fix:fix diseases types tests
…n/types/admissions) (informatici#627)

* tests:Tests | Add cypress e2e tests to cover admission types (admin/types/admissions)

* fix:check mode before setting it
…/discharges) (informatici#630)

* Add cypress e2e tests to cover discharge types (admin/types/discharges)

* Correction des reviews

* File organisation
…edicals) (informatici#632)

* Add cypress e2e tests to cover medical types (admin/types/medicals)

* Correction de typo

* Correction des attributs mal renseignes

* Remane medical file

* Files organisation
…e… (informatici#629)

* Tests | Add cypress e2e tests to cover delivery types (admin/types/deliveries)

* Ajout de la suppression

* Review corrections

* Files organisation
…formatici#623)

* tests:OH2-326 | Tests | Add cypress e2e tests to cover admin/diseases

* tests:OH2-328 | Tests | Add cypress e2e tests to cover admin/operations

* chore:improve cypress indexing
…n/types/diseases) (informatici#628)

* tests:OH2-355 | Tests / Add cypress e2e tests to cover diseases types (admin/types/diseases)

* chore: code quality improvement
* feature(OH2-335): Manager enabled/disabled diseases

* fix: Fix e2e tests

* update: Update diseases e2e tests

* fix: Fix disabled diseases e2e test

* styles(ADMIN/Diseases): Update responsiveness

---------

Co-authored-by: SteveGT96 <steve.tsala@intesys.it>
* feat(oh2-299): new user form

* feat: use UserGroupDTO as the form type rule

* feat: reset form

* feat: add user to backend and redirect

* feat: wait for the user to be saved before changing page

* feat: add translations to validations

* fix: customize username validation message

* fix: validation strings & touched

* feat: add success modal

* fix: added user description

* feat: confirm password

* fix: error message + icon

* fix: cancel instead of reset + error goes back to form
@gasp
Copy link
Contributor Author

gasp commented Jul 29, 2024

@mwithi This is using the soon-to-be-deprecated permission api.
Should I disable usergroups' permissions management from this PR?

@gasp
Copy link
Contributor Author

gasp commented Aug 29, 2024

@mwithi
Copy link
Member

mwithi commented Sep 4, 2024

@mwithi This is using the soon-to-be-deprecated permission api. Should I disable usergroups' permissions management from this PR?

I think you can wait for the API and then adjust this PR, is it ok for you? Thanks!

@mwithi
Copy link
Member

mwithi commented Sep 9, 2024

depends on informatici/openhospital-api#464 and informatici/openhospital-core#1391

both merged!

@gasp gasp marked this pull request as ready for review September 17, 2024 13:41
export const BASE_PATH = "https://oh2.open-hospital.org".replace(/\/+$/, "");
export const BASE_PATH = 'http://localhost:8080'.replace(/\/+$/, '');
Copy link
Member

Choose a reason for hiding this comment

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

This must remain unchanged @SilverD3 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. See this comment,

@mwithi
Copy link
Member

mwithi commented Sep 18, 2024

Integration tests seem good, but I would add a "select all" for columns and rows in the permission page.

image

Sorry if I don't remember: the user creation with its password encrypted is part of another issue/PR?

image

@gasp
Copy link
Contributor Author

gasp commented Sep 18, 2024

Integration tests seem good, but I would add a "select all" for columns and rows in the permission page.

image

Can we add this as a future iteration? this pull request is already humongous!

@gasp
Copy link
Contributor Author

gasp commented Sep 18, 2024

Sorry if I don't remember: the user creation with its password encrypted is part of another issue/PR?

image

Password is related to user, this is usergroup. AFAIK, password encryption is handled backend and does not (should not) affect frontend.

@SilverD3
Copy link
Contributor

@gasp I shared a video on slack, showing the result on my machine.

  1. You can see that the sidebar width grows based on the content of page. You could fix it by controlling the width of the "content display area", I mean the area where we show the content of the selected menu. I remember I've reported that issue in the pass and the fix I applied just worked for the component I was working on.
  2. The changes diff you show doesn't work properly. While editing a user group, if check and uncheck the same permission, you'll log it in the changes list (as a permission to revoke)
    Screenshot 2024-09-20 at 08-23-29 Open Hospital
  3. The reset button doesn't work
  4. Maybe you should wait (show a loader) until all the data has been loaded before displaying the edition form. You can see in the video that when I navigate to the user group edit page, some labels show and it's after about 2 or 3 seconds that form fields show.
  5. In the user group edit page, I suggest to use a scrollable component (div) or a collapsible one to display the permission list, so we don't have to scroll too much to reach form actions
  6. Until the endpoint for updating a group by passing the list of permissions is improved, you could use what I showed you at the meeting rather than making as many API calls as there are permissions.
PUT /users/groups/

// Request body
{
  "code": "labo",
  "desc": "Staff members working in the laboratory",
  "permissions": [
    {
      "name": "string",
      "id": 0,
      "description": "string"
    }
  ]
}

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.

5 participants