Skip to content

Conversation

@trokhim03
Copy link
Owner

No description provided.

Copy link

@Elena-Bruyako Elena-Bruyako left a 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>

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>

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>

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@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) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public UserLoginResponseDto login(@RequestBody UserLoginRequestDto userLoginRequestDto) {
public UserLoginResponseDto login(@RequestBody @Valid UserLoginRequestDto userLoginRequestDto) {

Comment on lines 36 to 39
@Column(name = "first_name", nullable = false)
private String firstName;

@Column(name = "last_name", nullable = false)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@Column(name = "first_name", nullable = false)
private String firstName;
@Column(name = "last_name", nullable = false)
@Column(nullable = false)
private String firstName;
@Column(nullable = false)

Comment on lines 36 to 39
public Page<PaymentResponseDto> getWithUserId(Pageable pageable, Long userId) {
return paymentRepository.findAllByRental_UserId(pageable, userId)
.map(paymentMapper::toDto);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove redundant empty line

Suggested change
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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
payment = paymentRepository.save(payment);
paymentRepository.save(payment);

"Can't find payment by id: " + paymentId));

if (payment.getStatus() == Payment.Status.PAID) {
return;

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"));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use CUSTOMER for message

Copy link

@liliia-ponomarenko liliia-ponomarenko left a 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 {

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@Column(name = "daily_fee", nullable = false)
@Column(nullable = false)

Comment on lines 61 to 62
payment.setSessionId("none");
payment.setSessionUrl("http://none.none");

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?

Comment on lines 56 to 62
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");

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

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() {

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

Copy link

@Elena-Bruyako Elena-Bruyako left a 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());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 "

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new RegistrationException("Can't registration user "
throw new RegistrationException("Can't register user "

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants