-
Notifications
You must be signed in to change notification settings - Fork 0
add car sharing project #1
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
base: main
Are you sure you want to change the base?
Conversation
Elena-Bruyako
left a comment
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.
Good job! Let's first fix comments in main code
Add readme file
pom.xml
Outdated
| <dependency> | ||
| <groupId>org.mapstruct</groupId> | ||
| <artifactId>mapstruct</artifactId> | ||
| <version>1.6.3</version> |
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.
replace to properties
pom.xml
Outdated
| <dependency> | ||
| <groupId>org.mapstruct</groupId> | ||
| <artifactId>mapstruct-processor</artifactId> | ||
| <version>1.6.3</version> |
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.
same here
pom.xml
Outdated
| <path> | ||
| <groupId>org.projectlombok</groupId> | ||
| <artifactId>lombok</artifactId> | ||
| <version>1.18.30</version> |
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.
same here (for all places)
| description = "Register a new user with email, password and other details") | ||
| @PostMapping("/registration") | ||
| public UserResponseDto registration( | ||
| @RequestBody UserRegistrationRequestDto userRegistrationRequestDto) |
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.
| @RequestBody UserRegistrationRequestDto userRegistrationRequestDto) | |
| @RequestBody @Valid UserRegistrationRequestDto userRegistrationRequestDto) |
| @Operation(summary = "Login user", | ||
| description = "Authenticate user and return JWT token") | ||
| @PostMapping("/login") | ||
| public UserLoginResponseDto login(@RequestBody UserLoginRequestDto userLoginRequestDto) { |
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.
| public UserLoginResponseDto login(@RequestBody UserLoginRequestDto userLoginRequestDto) { | |
| public UserLoginResponseDto login(@RequestBody @Valid UserLoginRequestDto userLoginRequestDto) { |
| @Column(name = "first_name", nullable = false) | ||
| private String firstName; | ||
|
|
||
| @Column(name = "last_name", nullable = false) |
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.
| @Column(name = "first_name", nullable = false) | |
| private String firstName; | |
| @Column(name = "last_name", nullable = false) | |
| @Column(nullable = false) | |
| private String firstName; | |
| @Column(nullable = false) |
| public Page<PaymentResponseDto> getWithUserId(Pageable pageable, Long userId) { | ||
| return paymentRepository.findAllByRental_UserId(pageable, userId) | ||
| .map(paymentMapper::toDto); | ||
|
|
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.
remove redundant empty line
| public Page<PaymentResponseDto> getWithUserId(Pageable pageable, Long userId) { | |
| return paymentRepository.findAllByRental_UserId(pageable, userId) | |
| .map(paymentMapper::toDto); | |
| public Page<PaymentResponseDto> getWithUserId(Pageable pageable, Long userId) { | |
| return paymentRepository.findAllByRental_UserId(pageable, userId) | |
| .map(paymentMapper::toDto); |
| payment.setSessionId("none"); | ||
| payment.setSessionUrl("http://none.none"); | ||
|
|
||
| payment = paymentRepository.save(payment); |
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.
| payment = paymentRepository.save(payment); | |
| paymentRepository.save(payment); |
| "Can't find payment by id: " + paymentId)); | ||
|
|
||
| if (payment.getStatus() == Payment.Status.PAID) { | ||
| return; |
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 expected behaviour?
| user.setPassword(passwordEncoder.encode(userRegistrationRequestDto.getPassword())); | ||
|
|
||
| Role defaultRole = roleRepository.findByName(Role.RoleName.CUSTOMER) | ||
| .orElseThrow(() -> new EntityNotFoundException("Default role 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.
use CUSTOMER for message
liliia-ponomarenko
left a comment
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.
Good job! A few comments ;) Also, you can improve test coverage
| import org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler; | ||
|
|
||
| @ControllerAdvice | ||
| public class CustomGlobalExceptionHandler extends ResponseEntityExceptionHandler { |
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.
Let’s handle all your custom exception
|
|
||
| private int inventory; | ||
|
|
||
| @Column(name = "daily_fee", nullable = false) |
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.
| @Column(name = "daily_fee", nullable = false) | |
| @Column(nullable = false) |
| payment.setSessionId("none"); | ||
| payment.setSessionUrl("http://none.none"); |
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 do you hardcode these values?
| Payment payment = new Payment(); | ||
| payment.setRental(rental); | ||
| payment.setType(type); | ||
| payment.setStatus(Payment.Status.PENDING); | ||
| payment.setAmountToPay(amountToPay); | ||
| payment.setSessionId("none"); | ||
| payment.setSessionUrl("http://none.none"); |
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.
Move this logic to the private method and simplify it. a good method should have less than 20 lines
| } | ||
|
|
||
| @Override | ||
| @Transactional |
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.
move 'transaction above class
| } | ||
| } | ||
|
|
||
| private Long calculateExpirationTime() { |
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.
move private method after public
Elena-Bruyako
left a comment
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.
Almost perfect)
|
|
||
| @ExceptionHandler(RegistrationException.class) | ||
| public ResponseEntity<Object> handleRegistrationException(RegistrationException ex) { | ||
| return buildResponse(HttpStatus.BAD_REQUEST, ex.getMessage()); |
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.
| return buildResponse(HttpStatus.BAD_REQUEST, ex.getMessage()); | |
| return buildResponse(HttpStatus.CONFLICT, ex.getMessage()); |
| import org.springframework.data.jpa.repository.JpaRepository; | ||
|
|
||
| public interface UserRepository extends JpaRepository<User, Long> { | ||
| Boolean existsByEmail(String email); |
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.
| Boolean existsByEmail(String email); | |
| boolean existsByEmail(String email); |
| public UserResponseDto register(UserRegistrationRequestDto userRegistrationRequestDto) | ||
| throws RegistrationException { | ||
| if (userRepository.existsByEmail(userRegistrationRequestDto.getEmail())) { | ||
| throw new RegistrationException("Can't registration user " |
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.
| throw new RegistrationException("Can't registration user " | |
| throw new RegistrationException("Can't register user " |
No description provided.