Skip to content
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

09-implemented-shopping-cart-Cart-item #9

Merged
merged 23 commits into from
Oct 1, 2023
Merged

Conversation

RostyslavOnysh
Copy link
Owner

@RostyslavOnysh RostyslavOnysh commented Sep 25, 2023

No description provided.

Copy link

@okuzan okuzan left a comment

Choose a reason for hiding this comment

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

It seems like there is a lot of work that needs to be done in the services. If your application is not functioning properly, it is recommended to post a thread in the chat section discussing the issue and then provide a working solution for review. Service methods should not be overly complex. Try delegating some of the work to mappers and utility methods in entities that are responsible for updating relationships.


@PutMapping("/cart-items/{id}")
@PreAuthorize("hasRole('USER')")
@Operation(summary = "")
Copy link

Choose a reason for hiding this comment

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

add description


@DeleteMapping("/{cartItemId}")
@PreAuthorize("hasRole('USER')")
@Operation(summary = "")
Copy link

Choose a reason for hiding this comment

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

add description

@RestController
@RequestMapping("/cart")
public class ShoppingCartController {

Copy link

Choose a reason for hiding this comment

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

Suggested change

private final CartItemService cartItemService;

@PostMapping
@Operation(summary = "add new item")
Copy link

Choose a reason for hiding this comment

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

Suggested change
@Operation(summary = "add new item")
@Operation(summary = "Add new item to a shopping cart")

cartItemRepository.deleteById(id);
}
}

Copy link

Choose a reason for hiding this comment

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

remove line

Comment on lines 32 to 36
CartItem cartItem = new CartItem();
cartItem.setBook(bookRepository.getById(cartItemRequestDto.getBookId()));
cartItem.setQuantity(cartItemRequestDto.getQuantity());
Long id = userService.getAuthenticated().getId();
cartItem.setShoppingCart(shoppingCartRepository.getById(id));
Copy link

Choose a reason for hiding this comment

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

use mapper to transform request dto to CartItem model

import spring.boot.bookstore.model.ShoppingCart;

public interface ShoppingCartService {

Copy link

Choose a reason for hiding this comment

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

Suggested change

Comment on lines 59 to 64
} else {
return null;
}
} else {
return null;
}
Copy link

Choose a reason for hiding this comment

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

bad practice


@Repository
public interface ShoppingCartRepository extends JpaRepository<ShoppingCart, Long> {
Optional<ShoppingCart> getUserById(Long id);
Copy link

Choose a reason for hiding this comment

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

Suggested change
Optional<ShoppingCart> getUserById(Long id);
Optional<ShoppingCart> getById(Long id);

cartItem.setBook(bookRepository.getById(cartItemRequestDto.getBookId()));
cartItem.setQuantity(cartItemRequestDto.getQuantity());
Long id = userService.getAuthenticated().getId();
cartItem.setShoppingCart(shoppingCartRepository.getById(id));
Copy link

Choose a reason for hiding this comment

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

foreign key constraint error may be occuring here - what if getById returns null?

Copy link

@Ivan95kos Ivan95kos left a comment

Choose a reason for hiding this comment

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

Please, fix the previous comments

@Repository
public interface CartItemRepository extends JpaRepository<CartItem, Long> {
@Query("SELECT c FROM CartItem c WHERE c.shoppingCart.id =:shoppingCartId")
Set<CartItem> findCartItemByShoppingCartId(@Param("shoppingCartId") Long shoppingCart);

Choose a reason for hiding this comment

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

such simple queries should be handled by proper method naming, please recheck work without them and fix just naming method if required

@Service
@RequiredArgsConstructor
public class ShoppingCartServiceImpl implements ShoppingCartService {

Choose a reason for hiding this comment

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

not fixed

@Service
@RequiredArgsConstructor
public class CartItemServiceImpl implements CartItemService {

Choose a reason for hiding this comment

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

not fixed

@@ -13,9 +14,10 @@ public interface BookRepository extends JpaRepository<Book, Long>, JpaSpecificat
List<Book> findAllByTitleContainsIgnoreCase(String title);

@Query("FROM Book b JOIN b.categories c WHERE c.id =:categoryId")
List<Book> findByCategoryId(Long categoryId, Pageable pageable);
List<Book> findByCategoryId(@Param("categoryId") Long categoryId,

Choose a reason for hiding this comment

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

such simple queries should be handled by proper method naming, please recheck work without them and fix just the naming method if required

import spring.boot.bookstore.model.User;

public interface CartItemService {

Copy link

Choose a reason for hiding this comment

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

Suggested change

public User getAuthenticated() {
Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
return userRepository.findByEmail(authentication.getName()).orElseThrow(
() -> new RuntimeException("Can`t find user with according email"
Copy link

Choose a reason for hiding this comment

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

don't use generic runtime exception

return shoppingCartDto;
}

private ShoppingCart registerNewCart(User user) {
Copy link

Choose a reason for hiding this comment

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

duplicated method in two services

Comment on lines +61 to +62
cartItemRepository.findById(id)
.orElseThrow(() -> new EntityNotFoundException("Can`t find cart by ID : " + id));
Copy link

Choose a reason for hiding this comment

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

Suggested change
cartItemRepository.findById(id)
.orElseThrow(() -> new EntityNotFoundException("Can`t find cart by ID : " + id));

}

@Override
public CartItemResponseDto update(UpdateQuantityDto updateQuantityDto, Long id) {
Copy link

Choose a reason for hiding this comment

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

check if quantity is positive

@RostyslavOnysh RostyslavOnysh merged commit cb84fa4 into main Oct 1, 2023
2 checks passed
@RostyslavOnysh RostyslavOnysh deleted the shoppingCartBranch branch November 7, 2023 13:42
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.

4 participants