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

Yongjian - Update message for inactive user account after login #438

Conversation

ypan710
Copy link
Contributor

@ypan710 ypan710 commented Jul 7, 2023

Description

Screen Shot 2023-07-07 at 11 55 04 AM

Main changes explained:

Display the message "Sorry, this account is no longer active. If you feel this is in error, please contact your Manager and/or Administrator." if the user exists in the database but his or her profile is not active.

How to test:

  1. check into current branch from the backend
  2. do npm run build and npm start from the backend
  3. check into development branch from the frontend
  4. do npm run start:local to run the frontend
  5. log in as admin user
  6. go to dashboard→ User Management -> search for a non-admin role of under your account -> change "Active" status to "Inactive" by clicking on the green dot
  7. log out of your admin account
  8. attempt to login with the non-admin role with status already set to "Inactive" from step 6
  9. verify that the message "Sorry, this account is no longer active. If you feel this is in error, please contact your Manager and/or Administrator." is displayed after clicking on submit button after entering credentials

Screenshots

Screen Shot 2023-07-07 at 11 49 27 AM

Screen Shot 2023-07-07 at 11 47 18 AM

Screen Shot 2023-07-07 at 11 47 25 AM

Copy link

@HectorAgudelo HectorAgudelo left a comment

Choose a reason for hiding this comment

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

The changes to the code perform as expected. nice!
PR#438

2-PR#438

@AaronPersaud AaronPersaud self-requested a review July 8, 2023 02:52
Copy link
Contributor

@AaronPersaud AaronPersaud left a comment

Choose a reason for hiding this comment

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

Everything looks good!

Changed user to inactive

438_inactive_user

Inactive message

438_inactive_message

  • user gets inactive user message instead of invalid email/password

I have approved the PR.

@luisarevalo21
Copy link
Contributor

Hello ypan710,
I have reviewed your code and works as intended great job!

However, I did receive one error when attempting to login as both an admin and a volunteer. See screenshot.

Error Received

  • I get "Cannot set headers after are sent to the client" on the the backend server.
Screenshot 2023-07-07 at 8 06 53 PM Screenshot 2023-07-07 at 8 07 05 PM Screenshot 2023-07-07 at 8 07 24 PM Screenshot 2023-07-07 at 8 09 10 PM Screenshot 2023-07-07 at 8 09 29 PM Screenshot 2023-07-07 at 8 09 36 PM

Copy link

@siddharthmohite siddharthmohite left a comment

Choose a reason for hiding this comment

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

This works as expected. Displays the message if user is inactive.
Screenshot 2023-07-07 at 11 05 17 PM
Screenshot 2023-07-07 at 11 03 11 PM

@beblicarl beblicarl self-requested a review July 8, 2023 10:32
Copy link

@igorrochadasilva igorrochadasilva left a comment

Choose a reason for hiding this comment

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

Hello @ypan710 !
Great work = D

The Pr's works as intended.

Inactive user on dashboard user manager
inactive-user

If I try to access with the user, I receive the message.
message-error

The code looks well 🚀

@beblicarl
Copy link
Contributor

This functionality works as intended. However, we have some errors in the backend console log.

SCREENSHOT

issue #438

ERROR MESSAGE

error-438

@one-community
Copy link
Member

This functionality works as intended. However, we have some errors in the backend console log.

Please request a change rather than approve something like this.

Copy link
Contributor

@tdkent tdkent left a comment

Choose a reason for hiding this comment

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

Local testing of this branch confirms

  • the updated error message is displayed to a user attempting to login with an "inactive" account.
  • the error message is displayed regardless of the value entered in the Password input (unless no value is entered, in which case a validation error is displayed).
  • an error with code 403 is returned from the backend and logged to the frontend console.
438-frontend-error-display

Errors
The following error is logged to the backend console each time a login request for an inactive user is sent: Cannot set headers after they are sent to the client

  • the error does not occur when invoking the "Invalid password" error for an active user in the same if/else block.
438-backend-error

Testing Video:

438-be-demo.mov

@ypan710 ypan710 requested a review from tdkent July 13, 2023 21:35
Copy link
Contributor

@tdkent tdkent left a comment

Choose a reason for hiding this comment

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

Confirmed that the Cannot set headers after they are sent to the client error has been fixed. Nice job!

Copy link

@bienzguoa bienzguoa left a comment

Choose a reason for hiding this comment

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

It works perfect! The code is clear and helps understand.
Screenshot 2023-07-15 at 2 58 57 AM
Screenshot 2023-07-15 at 2 59 19 AM

Copy link

@iTvX iTvX left a comment

Choose a reason for hiding this comment

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

I have reviewed your code and test it, it looks good.

253656739-46e301de-a751-4f8c-bd3b-48cbacfeb305.-.01.mp4

Copy link
Contributor

@ramyaram2092 ramyaram2092 left a comment

Choose a reason for hiding this comment

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

Hello @ypan710 the functionality works as expected.

PR-438.mp4

No errors on the console as well .
Approving the PR on the same note

@one-community one-community merged commit 1d53c98 into development Jul 17, 2023
3 checks passed
@one-community
Copy link
Member

Thank you all, this one is low risk, merged! Thanks for all your excellent reviews!

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.