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

Tapan - Fixed UI for Create New Role Without Name and Permission #2031

Merged

Conversation

TapanPathak
Copy link

Description

Screenshot 2024-03-07 at 1 58 00 PM

Demo of Bug - https://www.dropbox.com/scl/fi/ei3ag16xb57isrx2ek7ak/Create-new-role-click-with-empty-fields-white-screen.mp4?rlkey=vb01psyvwwwmeyoigwmm1x1j7&e=1&dl=0

Related PRS (if any):

N/A

Main changes explained:

  • Fixed UI for handling new role request when rolename and permission are empty
  • Role Name needs to be given
  • Atleast one permission needs be to selected

How to test:

  1. check into current branch
  2. do npm install and ... to run this PR locally
  3. Clear site data/cache
  4. log as owner user
  5. Other Links -> Permissions Management -> Add New Role -> Create
  6. Try to create a role without giving rolename and without selecting any permission, it should thrown an error in UI.

Screenshots or videos of changes:

Screen.Recording.2024-03-05.at.3.21.07.PM.mov

@TapanPathak TapanPathak changed the title Tapan Tapan - Fixed UI for Create New Role Without Name and Permission Mar 7, 2024
@lacnoskillz lacnoskillz self-requested a review March 8, 2024 03:08
Copy link
Contributor

@lacnoskillz lacnoskillz left a comment

Choose a reason for hiding this comment

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

Hello @TapanPathak
I have pulled your changes and tested your code and it works as described. Nice work!
I attempted to make a role on dev without a name/role selected and the page goes blank

but with your changes the proper checks prevent that from happening nice catch!
See below video of successful role create.

2024-03-07.21-07-52.mp4

Copy link
Contributor

@Gugor99 Gugor99 left a comment

Choose a reason for hiding this comment

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

Report

Hi Tapan,
I tried to review your PR but compilation failed and I got an error message in the console. Please see screenshot for reference.

Screenshot

Cattura2

Copy link

@meetpadhiar4 meetpadhiar4 left a comment

Choose a reason for hiding this comment

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

Hi, I tested your PR and it works as intended. The role validation works fine and passed all the test cases. Please refer the video attached.

PR.2031.mov

@dhairya-mehta
Copy link

Hey Tapan, I tested and verified the PR, while checking the popup check
image

@nnamdixobi
Copy link

Screenshot 2024-03-08 165521
Everything compiled correctly but I was unable to see the button to add a new role

Copy link
Contributor

@yaow62023 yaow62023 left a comment

Choose a reason for hiding this comment

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

Hi, I have test your PR. The error message though out when no role name or no Permissions selected. Check the video below:

2031.mp4

Copy link
Contributor

@mohabbasd mohabbasd left a comment

Choose a reason for hiding this comment

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

Hello, I tested you PR and it works perfectly. Great Job! Below is a video for your reference.

Screen.Recording.2024-03-23.at.11.48.33.PM.mov

@MQChen211
Copy link

Hi, I tested your PR and it works as expected.
截屏2024-04-04 15 43 11
截屏2024-04-04 15 43 28
截屏2024-04-04 15 43 43

Copy link

@SushmithaPrathap SushmithaPrathap left a comment

Choose a reason for hiding this comment

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

Hey, I've gone through the PR and tested it according to the steps you provided. Tried out different combinations and everything works smoothly. Also, it correctly notifies if the role name already exists. Nicely done!

Here is the screen recording:

Screen.Recording.2024-04-04.at.10.17.52.PM.mov

Copy link

@Jaiwin-4998 Jaiwin-4998 left a comment

Choose a reason for hiding this comment

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

Hi, I just reviewed your PR and is working fine.

Screenshot 2024-04-04 at 10 26 05 PM Screenshot 2024-04-04 at 10 25 12 PM

Copy link

@SRamen1999 SRamen1999 left a comment

Choose a reason for hiding this comment

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

Everything works as expected. When create a role without giving role name and without selecting any permission, it throws an error. Also, it correctly notifies if the role name already exists.

Screen.Recording.2024-04-05.at.9.30.11.AM.mov
Screen.Recording.2024-04-05.at.9.32.09.AM.mov

@anandsesha
Copy link
Contributor

Hi, I tested your PR, and its working as expected. Role name and at least one permission needed, else shows appropriate UI error.
Screenshot (749)
Screenshot (750)
Screenshot (751)
Screenshot (752)

@MQChen211
Copy link

Hi, I tested your PR and all works as expected.

截屏2024-04-12 14 50 26 截屏2024-04-12 14 50 32 截屏2024-04-12 14 50 44 截屏2024-04-12 14 50 56

Copy link
Contributor

@KurtisIvey KurtisIvey left a comment

Choose a reason for hiding this comment

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

Approved. Everything functions as it should without any errors on the front or backend. Code changes are minimal while still accomplishing the desired fix. Photos attached for verification of clean error popup when criteria not met for creating new role.

Fantastic job!
Screenshot 2024-04-20 at 11 57 57 AM
Screenshot 2024-04-20 at 11 58 13 AM

@one-community
Copy link
Member

Thank you all, merging!

@one-community one-community merged commit c90ccb3 into development Apr 20, 2024
1 check passed
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.