-
Notifications
You must be signed in to change notification settings - Fork 8
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
Bug #4984 fixed-method getAllAuthorities in UBSClientServiceImpl #947
base: dev
Are you sure you want to change the base?
Conversation
in method updateEmployeesAuthorities due to SonarLint
90239e1
to
33fa371
Compare
} | ||
|
||
@Override | ||
public void updateEmployeesAuthorities(UserEmployeeAuthorityDto dto, String email) { | ||
Employee employee = employeeRepository.findByEmail(dto.getEmployeeEmail()) | ||
.orElseThrow(() -> new NotFoundException(EMPLOYEE_DOESNT_EXIST)); |
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.
Why did you remove this?
Is it possible to update authorities for a non-existent employee?
@@ -1907,13 +1907,15 @@ public TariffsForLocationDto getTariffForOrder(Long id) { | |||
public Set<String> getAllAuthorities(String email) { | |||
Employee employee = employeeRepository.findByEmail(email) | |||
.orElseThrow(() -> new NotFoundException(EMPLOYEE_DOESNT_EXIST)); | |||
return userRemoteClient.getAllAuthorities(employee.getEmail()); | |||
Set<String> authorities = userRemoteClient.getAllAuthorities(employee.getEmail()); | |||
if (authorities.isEmpty()) { |
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.
please add test for this case
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.
Is it not a valid case when an employee does not have any authorities?
Do we set some authorities when we create employees?
And according to your changes, we will have empty authorities list if some error happens when we call /user/get-all-authorities
endpoint
In this case, the employee could have some authorities but we could not get them and your message COULD_NOT_RETRIEVE_EMPLOYEE_AUTHORITY
will not be valid
@@ -58,7 +59,7 @@ public void sendEmailNotification(NotificationDto notification, String email) { | |||
@Override | |||
public Set<String> getAllAuthorities(String email) { | |||
log.error(ErrorMessage.COULD_NOT_RETRIEVE_EMPLOYEE_AUTHORITY, throwable); | |||
return Collections.singleton(throwable.getMessage()); | |||
return Collections.emptySet(); |
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.
please add test for this case
@@ -259,7 +259,8 @@ public ResponseEntity<HttpStatus> deleteEmployeeImage(@PathVariable Long id) { | |||
@ApiResponse(code = 200, message = HttpStatuses.OK), | |||
@ApiResponse(code = 400, message = HttpStatuses.BAD_REQUEST), | |||
@ApiResponse(code = 401, message = HttpStatuses.UNAUTHORIZED), | |||
@ApiResponse(code = 403, message = HttpStatuses.FORBIDDEN) | |||
@ApiResponse(code = 403, message = HttpStatuses.FORBIDDEN), | |||
@ApiResponse(code = 404, message = HttpStatuses.NOT_FOUND) |
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.
please add test
@@ -31,6 +31,7 @@ public String findUuidByEmail(String email) { | |||
@Override | |||
public Optional<UserVO> findNotDeactivatedByEmail(String email) { | |||
log.error(ErrorMessage.USER_WITH_THIS_EMAIL_DOES_NOT_EXIST + email, throwable); | |||
|
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.
please remove this blank line
Kudos, SonarCloud Quality Gate passed! |
changed method getAllAuthorities in UBSClientServiceImpl and in UserRemoteClientFallbackFactory