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.

Sign up for GitHub

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

Jump to bottom

Lickem22 user management #455

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Lickem22 user management #455

wants to merge 9 commits into from

Conversation

lickem22
Copy link
Contributor

@lickem22 lickem22 commented Nov 13, 2024 •
edited by jira bot
Loading

Reviewer:@sidravi1
Estimate: 1h

Ticket

Fixes: AAQ-844

Description

Goal

The goal of this PR is to add a frontend for the user management apis.

Changes

The following changes have been made:

  • Adding a user management page only available to admin users. This page allows to:
    • Add new user
    • Edit an existing user
    • Reset a user password
  • Added an endpoint to list all users as well as tests

Future Tasks (optional)

How has this been tested?

To-do before merge (optional)

Checklist

Fill with x for completed.

  • My code follows the style guidelines of this project
  • I have reviewed my own code to ensure good quality
  • I have tested the functionality of my code to ensure it works as intended
  • I have resolved merge conflicts

(Delete any items below that are not relevant)

  • I have updated the automated tests
  • I have updated the scripts in scripts/
  • I have updated the requirements
  • I have updated the README file
  • I have updated affected documentation
  • I have added a blogpost in Latest Updates
  • I have updated the CI/CD scripts in .github/workflows/
  • I have updated the Terraform code

sidravi1
sidravi1 requested changes Nov 17, 2024
View reviewed changes
Copy link
Member

@sidravi1 sidravi1 left a comment

Choose a reason for hiding this comment

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

Thank @lickem22.

Left comments for a couple of medium things and a few more small things.
Mind taking a look? Happy to review it again this week.

core_backend/app/auth/schemas.py Outdated Show resolved Hide resolved
core_backend/app/user_tools/routers.py Outdated Show resolved Hide resolved
core_backend/app/user_tools/routers.py Outdated Show resolved Hide resolved
core_backend/app/users/models.py Show resolved Hide resolved
admin_app/src/app/integrations/page.tsx Show resolved Hide resolved
admin_app/src/app/login/page.tsx
@@ -433,12 +435,16 @@ const Login = () => {
open={showRegisterModal}
onClose={handleRegisterModalClose}
onContinue={handleRegisterModalContinue}
registerUser={apiCalls.registerUser}
registerUser={(user: UserBodyPassword | UserBody) => {
Copy link
Member

Choose a reason for hiding this comment

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

Why is UserBody type needed for user here? Is it for google login user and the password gets set as blank?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a "Parent" modal UserModal that can be done to register the first user, register all users and edit users. Since it can do both registering and editing it needs to support UserBodyPassword and UserBody, If I do :
(user: UserBodyPassword) I am going to get a syntax error.

admin_app/src/app/user-management/page.tsx Show resolved Hide resolved
admin_app/src/app/user-management/page.tsx Show resolved Hide resolved
admin_app/src/app/user-management/components/UserCard.tsx Outdated Show resolved Hide resolved
admin_app/src/app/user-management/components/UserCard.tsx
Copy link
Member

Choose a reason for hiding this comment

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

So we are using a List of Cards? Do we really need Cards? Can we just use List and ListItem?

Copy link
Contributor Author

@lickem22 lickem22 Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Put all of it in the page ? I personnally think it is cleaner this way, too much information was making it harder to navigate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sidravi1 sidravi1 sidravi1 requested changes

@suzinyou suzinyou Awaiting requested review from suzinyou suzinyou is a code owner

@amiraliemami amiraliemami Awaiting requested review from amiraliemami amiraliemami is a code owner

@Tanmay-97 Tanmay-97 Awaiting requested review from Tanmay-97 Tanmay-97 is a code owner

@markbotterill markbotterill Awaiting requested review from markbotterill markbotterill is a code owner

@tonyzhao6 tonyzhao6 Awaiting requested review from tonyzhao6 tonyzhao6 is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lickem22 @sidravi1