-
-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
feature: A user-friendly user management page for apollo portal #4464
Conversation
Bumps [gson](https://github.com/google/gson) from 2.8.0 to 2.8.9. - [Release notes](https://github.com/google/gson/releases) - [Changelog](https://github.com/google/gson/blob/master/CHANGELOG.md) - [Commits](google/gson@gson-parent-2.8.0...gson-parent-2.8.9) --- updated-dependencies: - dependency-name: com.google.code.gson:gson dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
Codecov Report
@@ Coverage Diff @@
## master #4464 +/- ##
============================================
- Coverage 53.63% 53.52% -0.12%
+ Complexity 2702 2701 -1
============================================
Files 489 489
Lines 15294 15325 +31
Branches 1588 1596 +8
============================================
- Hits 8203 8202 -1
- Misses 6535 6565 +30
- Partials 556 558 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. |
@nobodyiam please help me review the code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are also several issues that need to be fixed.
-
When the user is created/updated, the user list needs to be updated to reflect the changes.
Now the administrator needs to refresh the page manually to see the newly added user or the updated user info. -
It's better to allow the administrator enable/disable the users without filling the passwords
How about we add enable/disable buttons in the operation column?
...ain/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserService.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/services/UserService.js
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/controller/UserController.js
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/controller/UserController.js
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/controller/UserController.js
Outdated
Show resolved
Hide resolved
If the enable or disable is put outside and becomes a separate button, the backend needs to write a new interface,like @PreAuthorize(value = "@permissionValidator.isSuperAdmin()")
@PostMapping("/user/enabled")
public void changeUserEnabled(@RequestBody UserPO user) {
} Because the current interface checks that the password cannot be empty. @PreAuthorize(value = "@permissionValidator.isSuperAdmin()")
@PostMapping("/users")
public void createOrUpdateUser(@RequestBody UserPO user) {
if (StringUtils.isContainEmpty(user.getUsername(), user.getPassword())) {
throw new BadRequestException("Username and password can not be empty.");
}
CheckResult pwdCheckRes = passwordChecker.checkWeakPassword(user.getPassword());
if (!pwdCheckRes.isSuccess()) {
throw new BadRequestException(pwdCheckRes.getMessage());
}
if (userService instanceof SpringSecurityUserService) {
((SpringSecurityUserService) userService).createOrUpdate(user);
} else {
throw new UnsupportedOperationException("Create or update user operation is unsupported");
}
} is it ok to add a new interface? |
...ain/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserService.java
Outdated
Show resolved
Hide resolved
...ortal/src/main/java/com/ctrip/framework/apollo/portal/spi/oidc/OidcLocalUserServiceImpl.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/services/UserService.js
Outdated
Show resolved
Hide resolved
...lo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/UserInfoController.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/services/UserService.js
Outdated
Show resolved
Hide resolved
BTW, I think it's ok to add a new interface to enable/disable users. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, it looks very good now! Please find some comments below.
BTW, please also update the CHANGES.md.
...lo-portal/src/main/java/com/ctrip/framework/apollo/portal/controller/UserInfoController.java
Outdated
Show resolved
Hide resolved
...ortal/src/main/java/com/ctrip/framework/apollo/portal/spi/oidc/OidcLocalUserServiceImpl.java
Outdated
Show resolved
Hide resolved
...ortal/src/main/java/com/ctrip/framework/apollo/portal/spi/oidc/OidcLocalUserServiceImpl.java
Outdated
Show resolved
Hide resolved
...ain/java/com/ctrip/framework/apollo/portal/spi/springsecurity/SpringSecurityUserService.java
Outdated
Show resolved
Hide resolved
apollo-portal/src/main/resources/static/scripts/controller/UserController.js
Outdated
Show resolved
Hide resolved
@nobodyiam do you have any other suggestions :D |
It looks great now! There is one minor issue though. I think we need to disable the user login name field in the edit user page: BTW, please also fix the conflict in CHANGES.md. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What's the purpose of this PR
A user-friendly user management page for apollo portal
Which issue(s) this PR fixes:
Fixes #4294
Brief changelog
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean test
to make sure this pull request doesn't break anything.CHANGES
log.