Develop#1253
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
There are no files to review. Please make sure that you have added files to the pull request. Some files and directories may be ignored from the review or failed to load.
[CODE: NFR]
Additional info:
Ignored files list:
pom.xmlsrc/main/resources/log4j2.xml
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
There are no files to review. Please make sure that you have added files to the pull request. Some files and directories may be ignored from the review or failed to load.
[CODE: NFR]
Additional info:
Ignored files list:
pom.xmlsrc/main/resources/log4j2.xml
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
There are no files to review. Please make sure that you have added files to the pull request. Some files and directories may be ignored from the review or failed to load.
[CODE: NFR]
Additional info:
Ignored files list:
pom.xmlsrc/main/resources/log4j2.xml
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
There are no files to review. Please make sure that you have added files to the pull request. Some files and directories may be ignored from the review or failed to load.
[CODE: NFR]
Additional info:
Ignored files list:
pom.xmlsrc/main/resources/log4j2.xml
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
slade13
left a comment
slade13
left a comment
There was a problem hiding this comment.
Good job with adding the logger configuration file and creating both Console and File appenders. However, the task is not fully completed yet.
Please fix the following points:
- replace
e.printStackTrace()with a proper logger method; - add logger usage in service classes instead of leaving TODO comments;
- remove all TODO comments before submitting the solution;
- add the generated log file or
logs/directory to.gitignore; - make sure your log messages are informative and include useful parameters, for example
loginoruserId; - make sure all files end with an empty line.
Also, remember not to log sensitive data such as passwords. After these changes, the solution will better match the task requirements.
mateuszwojtkowiak
left a comment
mateuszwojtkowiak
left a comment
There was a problem hiding this comment.
Good job! However your project is missing TODO statements in AuthenticationServiceImpl class:
@Override
public User login(String login, String password) throws AuthenticationException {
//TODO: add corresponding log message about method login was called
User user = findByLogin(login);
if (!user.getPassword().equals(password)) {
throw new AuthenticationException("Username or password are incorrect");
}
return user;
}
slade13
left a comment
slade13
left a comment
There was a problem hiding this comment.
Please review my comments and add necessary updates to your code.
mateuszwojtkowiak
left a comment
mateuszwojtkowiak
left a comment
There was a problem hiding this comment.
Great job! The task is accepted. There are just a few minor improvements needed. Please review my comments and apply the suggested changes.
| User user = findByLogin(login); | ||
|
|
||
| if (!user.getPassword().equals(password)) { | ||
| logger.warn("Authentication failed for user {}", login); |
There was a problem hiding this comment.
It would be better to write a message about incorrect credentials:
logger.warn("Authentication failed. Incorrect credentials for login={}", login);
| @Override | ||
| public Order completeOrder(Long userId) { | ||
| // TODO: add log message about method completeOrder was called | ||
| logger.info("Method completeOrder was called. Params: userId={}", userId); |
There was a problem hiding this comment.
It should be debug, not info:
logger.debug("Method completeOrder was called. Params: userId={}", userId);
| // NOTE: In production ready code this order identifier should be generated by DB | ||
| // For test purpose we simplify this and return dummy data | ||
| order.setOrderId(1L); | ||
| logger.info("Order {} was completed for user {}", order.getOrderId(), userId); |
There was a problem hiding this comment.
It would look better that way:
logger.info("Order was completed. Params: orderId={}, userId={}, productsCount={}",
order.getOrderId(), userId, products.size());
4 participants
No description provided.