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

Sai Preetham taking over for Clemar add a reminder to save changes modal to permissions management page #2309

Conversation

ClemarNunes
Copy link
Contributor

@ClemarNunes ClemarNunes commented May 31, 2024

Description

when a new permission is granted, a modal will appear reminding to save their changes.
will also open a modal informing you that you must log out and log back in for the permissions to take effect.

Additionally, if an admin receives a new permission, a modal will notify them that they need to log out and log back in for the changes to take effect.

Related PRS (if any):

none

Main changes explained:

-User reminder modal: Added a modal that appears when a new permission is granted, reminding to save their changes.
-permission modal: Created a modal that informs need to log out and log back in for new permissions to take effect.

How to test:

  1. check into current branch
  2. do npm install and ... to run this PR locally
  3. Clear site data/cache (Don't skip this or use incognito)
  4. log as Owner user
  5. go to dashboard→ Other Links→ Permissions Management→Manage User Permissions
  6. choose an admin or other users and assign some permissions
  7. make sure you open a modal to save when adding a permission
  8. Log in to the account that received the permission
  9. check if a modal informs you that you must log out and log in

Screenshots or videos of changes:

video.mp4

After fix:

Reminder.Modal.mov

Copy link
Contributor

@peter6866 peter6866 left a comment

Choose a reason for hiding this comment

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

Great work on implementing the permission reminder modals for users and admins! Here's the review based on the provided testing steps:

✅ User Reminder Modal:

  • Verified that when a new permission is granted to a user, a modal appears reminding them to save their changes.
  • Confirmed that the modal is displayed correctly and contains the appropriate message.

✅ Permission Modal:

  • Tested that after granting a new permission, a modal is displayed informing the user that they need to log out and log back in for the new permissions to take effect.
  • Verified that the modal is triggered correctly and provides clear instructions to the user.

✅ Admin Notification Modal:

  • Logged in as an Owner user and navigated to the Permissions Management section (Dashboard → Other Links → Permissions Management → Manage User Permissions).
  • Assigned new permissions to a user.
  • Confirmed that the user receives a modal notification informing them to log out and log back in for the changes to take effect.

✅ Modal Behavior:

  • Tested the modal behavior by granting permissions to different users and admins.
  • Verified that the modals are displayed consistently and contain the correct information.

Suggestions:

  • It was noticed that users may scroll within the reminder modal, so the permissions list will not scroll according causing a bad user experience.
  • There seems to be a typo in the "Close" button text on the modal that appears after logging in.

https://www.loom.com/share/30b9bac3ab744a71a2583c34411bacb1?sid=1b970a9d-f482-4699-9995-0ddee8a2eff2

@Vijeth-V
Copy link

Vijeth-V commented Jun 1, 2024

After the permissions are updated the user gets a popup of the permission updated. That works perfectly well. After logging out and logging back in the pop up is still popping everytime i logged back in.
I Think this is not the expected result. I am attaching the video took on my end do check if that happens on your end as well.
Thanks!

Screen.Recording.mp4

tianyangL11
tianyangL11 previously approved these changes Jun 2, 2024
Copy link

@tianyangL11 tianyangL11 left a comment

Choose a reason for hiding this comment

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

After the permissions changes the user gets a popup of the permission updated. But when I logged out and re-log in, it will still pop up, I think it may have some issue
977e8eaf4c1f7a6bb89ea5f3cad8100

@cecilia-uu
Copy link

The "Close" button text in the modal that appears after logging in has a typo.
There are some conflicts with other files.
截屏2024-06-08 下午10 03 19

@one-community one-community added do not review Do not review or look at code without full context Needs New Developer This is a PR that is partially developed but needs someone new to take it over and finish it. labels Jun 14, 2024
@akshit0211 akshit0211 force-pushed the clemar_Add_a_reminder_to_save_changes_modal_to_Permissions_Management_page branch from 1875f8a to 7bbec60 Compare June 21, 2024 15:06
@akshit0211 akshit0211 added medium priority and removed Needs New Developer This is a PR that is partially developed but needs someone new to take it over and finish it. do not review Do not review or look at code without full context labels Jun 21, 2024
Copy link
Contributor

@Sudershhh Sudershhh left a comment

Choose a reason for hiding this comment

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

The functionality is working as expected. The Modal is not responsive at all. Pplease make sure it is centered and the backdrop is grayed out.

PR 2309 - 2

PR 2309 - 3

PR 2309

@peter6866 peter6866 removed their request for review July 3, 2024 18:07
@peter6866
Copy link
Contributor

Hi, could you resolve the merge conflicts?

@akshit0211 akshit0211 force-pushed the clemar_Add_a_reminder_to_save_changes_modal_to_Permissions_Management_page branch from 8a0e088 to da64544 Compare July 4, 2024 23:52
@one-community one-community changed the title Clemar add a reminder to save changes modal to permissions management page Akshit taking over for Clemar add a reminder to save changes modal to permissions management page Jul 17, 2024
@souravwalke
Copy link

Hi, I have verified the changes in your PR. The functionality looks good.

Screenshot 2024-07-27 at 11 42 47 PM

Copy link

@AnkitLall AnkitLall left a comment

Choose a reason for hiding this comment

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

I've tested the feature according to the instructions in the PR, and it generally works as expected. However, I did notice a couple of issues:

  1. The "Manage User Permissions" and "Remember to save your changes!" popups are not responsive, and when they are open, you can still scroll the background. (Refer attached screen recording)
  2. The user reminder modal appears only when the user logs into their account, not when they are already logged in.

Additionally, I’m not sure, but shouldn’t this feature be designed to work only when the user is already logged into their account, rather than when they log in freshly?

Screen.Recording.2024-08-09.at.6.20.09.PM.mp4

@one-community one-community added Needs New Developer This is a PR that is partially developed but needs someone new to take it over and finish it. do not review Do not review or look at code without full context and removed High Priority - Please Review First This is an important PR we'd like to get merged as soon as possible labels Aug 18, 2024
Copy link

netlify bot commented Sep 6, 2024

Deploy Preview for highestgoodnetwork-dev ready!

Name Link
🔨 Latest commit 808a612
🔍 Latest deploy log https://app.netlify.com/sites/highestgoodnetwork-dev/deploys/66e36ec23c3af90007670ed8
😎 Deploy Preview https://deploy-preview-2309--highestgoodnetwork-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@one-community one-community changed the title Akshit taking over for Clemar add a reminder to save changes modal to permissions management page Sai Preetham taking over for Clemar add a reminder to save changes modal to permissions management page Sep 18, 2024
@one-community one-community added High Priority - Please Review First This is an important PR we'd like to get merged as soon as possible and removed Needs New Developer This is a PR that is partially developed but needs someone new to take it over and finish it. do not review Do not review or look at code without full context labels Sep 18, 2024
@nishitag5 nishitag5 self-requested a review September 21, 2024 21:53
Copy link

@nishitag5 nishitag5 left a comment

Choose a reason for hiding this comment

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

While editing user permissions, the system displays a dialog box prompting a logout and login to apply the changes. Upon logging in as the user with the updated permissions, I can confirm that the changes have been successfully applied. However, the prompt indicating 'Permissions have been updated, please log out and log back in to see the changes' does not appear.

Recording.2024-09-21.175929.mp4

Copy link

@lcohen730 lcohen730 left a comment

Choose a reason for hiding this comment

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

Followed instructions to test, but when logging into test account after setting new permission, there was no modal pop-up to inform me to log out then back in, and I waited a few minutes.

Also, small note: I noticed a grammatical error in the toaster notification that pops up after assigning permissions. It says "Be sure to tell them that you are changing these permissions and for that they need to log out and log back in..."

Screenshot 2024-09-28 at 15 28 28

The "for" should be removed. It should say "Be sure to tell them that you are changing these permissions and that they need to log out...".

Copy link
Contributor

@mrinalini-raghavendran19 mrinalini-raghavendran19 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 followed the instructions to test and a modal did not appear on the user account with the change in permissions informing them to log out. Please let me know if there's something else missing.

Besides that, I notice a few changes that could be made:

  1. When I minimize the screen, the save changes modal is hidden and does not appear to the user.
Screenshot 2024-10-01 at 2 20 18 PM Screenshot 2024-10-01 at 2 20 42 PM
  1. The green modal telling users the permissions have been updated appears too many times, once when you save changes and the next when you submit the changes. This may be a potential bug.
Screenshot 2024-10-01 at 2 21 31 PM

Do take a look!

@VMD281
Copy link

VMD281 commented Oct 26, 2024

I reviewed this PR and the functionality works perfectly!
PR#2309_1
PR#2309_2
PR#2309_3

Copy link

netlify bot commented Oct 28, 2024

Deploy Preview for highestgoodnetwork-dev ready!

Name Link
🔨 Latest commit d3d04c6
🔍 Latest deploy log https://app.netlify.com/sites/highestgoodnetwork-dev/deploys/67253bf8cc5d490008d19834
😎 Deploy Preview https://deploy-preview-2309--highestgoodnetwork-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@preethamdnr
Copy link
Contributor

Hello all,

I've made the following updates based on the feedback provided:

Reminder Modal Centering: Ensured that the "Remember to Save Your Changes" modal is properly centered on the screen, with equal spacing on both the left and right sides, especially for smaller screens.

Responsive Adjustments: Adjusted the modal's width and padding to make it responsive, so it remains within screen boundaries and looks balanced across different screen sizes.

Green Popup Notification Frequency: Updated the green popup notification to display only once per permission update action, avoiding multiple pop-ups and enhancing the user experience.

Please review the changes

Thank you!

@one-community
Copy link
Member

Thank you all, merging!

@one-community one-community merged commit 8696f57 into development Nov 5, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
High Priority - Please Review First This is an important PR we'd like to get merged as soon as possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.