zhongjiajie commented on code in PR #11255:
URL: https://github.com/apache/dolphinscheduler/pull/11255#discussion_r944056233
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -203,23 +198,23 @@ public User createUser(String userName,
String phone,
String queue,
int state) {
- User user = new User();
Date now = new Date();
+ checkUserParams(userName, userPassword, email, phone);
+
+ User tempUser = userMapper.queryByUserNameAccurately(userName);
+ if (tempUser != null) {
+ throw new ServiceException(Status.NAME_EXIST, userName);
+ }
Review Comment:
Why we should add this check, BTW?
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -203,23 +198,23 @@ public User createUser(String userName,
String phone,
String queue,
int state) {
- User user = new User();
Date now = new Date();
+ checkUserParams(userName, userPassword, email, phone);
+
+ User tempUser = userMapper.queryByUserNameAccurately(userName);
+ if (tempUser != null) {
+ throw new ServiceException(Status.NAME_EXIST, userName);
+ }
+
+ User user = new User(userName, userPassword, email, phone, tenantId,
state);
- user.setUserName(userName);
- user.setUserPassword(EncryptionUtils.getMd5(userPassword));
- user.setEmail(email);
- user.setTenantId(tenantId);
- user.setPhone(phone);
- user.setState(state);
// create general users, administrator users are currently built-in
user.setUserType(UserType.GENERAL_USER);
- user.setCreateTime(now);
- user.setUpdateTime(now);
if (StringUtils.isEmpty(queue)) {
queue = "";
}
user.setQueue(queue);
+ user.setCreateTime(now);
Review Comment:
can we add `createtime` to the class `User` constructor? we can add multiple
constructor to different behavior
```java
// for new user
public User(String userName, String userPassword, String email, String
phone, Integer tenantId, Integer state) {
Date now = new Date();
this.userName = userName;
this.userPassword = EncryptionUtils.getMd5(userPassword);
this.email = email;
this.phone = phone;
this.tenantId = tenantId;
this.state = state;
this.updateTime = now;
this.createTime = now;
}
// for update user, we keep the create
public User(int userId, String userName, String userPassword, String
email, String phone, Integer tenantId, Integer state, Date createTime) {
this.User(userName, userPassword, email, phone, tenantId, state)
this.id = userId;
this.createTime = createTime;
}
```
##########
dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/UsersServiceTest.java:
##########
@@ -622,37 +644,33 @@ public void testRegisterUser() {
String userPassword = "userTest";
String repeatPassword = "userTest";
String email = "[email protected]";
- try {
- //userName error
- Map<String, Object> result = usersService.registerUser(userName,
userPassword, repeatPassword, email);
- Assert.assertEquals(Status.REQUEST_PARAMS_NOT_VALID_ERROR,
result.get(Constants.STATUS));
- userName = "userTest0002";
- userPassword = "userTest000111111111111111";
- //password error
- result = usersService.registerUser(userName, userPassword,
repeatPassword, email);
- Assert.assertEquals(Status.REQUEST_PARAMS_NOT_VALID_ERROR,
result.get(Constants.STATUS));
+ //userName error
+ Map<String, Object> result = usersService.registerUser(userName,
userPassword, repeatPassword, email);
+ Assert.assertEquals(Status.REQUEST_PARAMS_NOT_VALID_ERROR,
result.get(Constants.STATUS));
Review Comment:
You should also make this tests work, I just change the comemnt yesterday
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -383,77 +377,36 @@ public Map<String, Object> updateUser(User loginUser, int
userId,
String phone,
String queue,
int state,
- String timeZone) throws IOException {
+ String timeZone) {
Map<String, Object> result = new HashMap<>();
result.put(Constants.STATUS, false);
if (resourcePermissionCheckService.functionDisabled()) {
- putMsg(result, Status.FUNCTION_DISABLED);
- return result;
+ throw new ServiceException(Status.FUNCTION_DISABLED.getMsg());
}
if (check(result, !canOperator(loginUser, userId),
Status.USER_NO_OPERATION_PERM)) {
- return result;
+ throw new ServiceException(Status.USER_NO_OPERATION_PERM.getMsg());
}
User user = userMapper.selectById(userId);
if (user == null) {
- putMsg(result, Status.USER_NOT_EXIST, userId);
- return result;
+ throw new ServiceException(Status.USER_NOT_EXIST.getMsg());
}
- if (StringUtils.isNotEmpty(userName)) {
-
- if (!CheckUtils.checkUserName(userName)) {
- putMsg(result, Status.REQUEST_PARAMS_NOT_VALID_ERROR,
userName);
- return result;
- }
- User tempUser = userMapper.queryByUserNameAccurately(userName);
- if (tempUser != null && tempUser.getId() != userId) {
- putMsg(result, Status.USER_NAME_EXIST);
- return result;
- }
- user.setUserName(userName);
- }
-
- if (StringUtils.isNotEmpty(userPassword)) {
- if (!CheckUtils.checkPasswordLength(userPassword)) {
- putMsg(result, Status.USER_PASSWORD_LENGTH_ERROR);
- return result;
- }
- user.setUserPassword(EncryptionUtils.getMd5(userPassword));
- }
-
- if (StringUtils.isNotEmpty(email)) {
- if (!CheckUtils.checkEmail(email)) {
- putMsg(result, Status.REQUEST_PARAMS_NOT_VALID_ERROR, email);
- return result;
+ checkUserParams(userName, userPassword, email, phone);
+ if (StringUtils.isNotEmpty(timeZone)) {
+ if (!CheckUtils.checkTimeZone(timeZone)) {
+ throw new
ServiceException(MessageFormat.format(Status.TIME_ZONE_ILLEGAL.getMsg(),
timeZone));
}
- user.setEmail(email);
- }
-
- if (StringUtils.isNotEmpty(phone) && !CheckUtils.checkPhone(phone)) {
- putMsg(result, Status.REQUEST_PARAMS_NOT_VALID_ERROR, phone);
- return result;
+ user.setTimeZone(timeZone);
}
+ user = new User(userName, userPassword, email, phone, tenantId, state);
Review Comment:
You have to pass exists user id to the user object you want to update,
otherwise will raise error
##########
dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/UsersServiceImpl.java:
##########
@@ -383,77 +377,36 @@ public Map<String, Object> updateUser(User loginUser, int
userId,
String phone,
String queue,
int state,
- String timeZone) throws IOException {
+ String timeZone) {
Map<String, Object> result = new HashMap<>();
result.put(Constants.STATUS, false);
if (resourcePermissionCheckService.functionDisabled()) {
- putMsg(result, Status.FUNCTION_DISABLED);
- return result;
+ throw new ServiceException(Status.FUNCTION_DISABLED.getMsg());
}
if (check(result, !canOperator(loginUser, userId),
Status.USER_NO_OPERATION_PERM)) {
- return result;
+ throw new ServiceException(Status.USER_NO_OPERATION_PERM.getMsg());
Review Comment:
I preview reviewed have a mistake, you can directly use `throw new
ServiceException(Status.USER_NO_OPERATION_PERM);`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
