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

상품 등록 기능 구현 #26

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

상품 등록 기능 구현 #26

wants to merge 13 commits into from

Conversation

100gammmit
Copy link
Collaborator

@100gammmit 100gammmit commented Jan 16, 2025

📌 과제 설명

상품 등록 기능 구현 및 테스트

👩‍💻 요구 사항과 구현 내용

  • 상품 등록 및 조회 시 발생할 수 있는 예외 GlobalControllerAdvise에 추가
  • 서비스와, 컨트롤러에 상품 등록에 필요한 메서드 추가
  • 등록, 조회에 대한 컨트롤러 테스트 추가
  • 컨틀로러 테스트용 jwt인증필터 제외한 testSecurityConfig추가
  • 서비스 테스트 단위 테스트로 변환 및 조회 실패 테스트 케이스 추가

✅ 피드백 반영사항

  • 영규님 피드백대로 서비스에 @Transactional(readOnly = true)전역화 했습니당

✅ PR 포인트 & 궁금한 점

close #11

Copy link
Collaborator

@kdo1999 kdo1999 left a comment

Choose a reason for hiding this comment

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

지난번 PR때 제가 확인을 못한거 같은데 혹시 ProductRepository에 findById랑 findProductResponseById를 따로 두신 이유가 있는지 궁금합니다.

제 개인적인 생각에는 상품 조회시 응답 객체에 재고 필드가 필요 없지 않을까 하는데 이 부분도 같이 얘기 나눠보면 좋을 것 같아요
수고하셨습니다!


ProductResponse productResponse = productService.findProductResponseById(id);

return ResponseEntity.ok().body(GenericResponse.of(productResponse));
}

@PostMapping
@PreAuthorize("hasRole('ADMIN')")
Copy link
Collaborator

Choose a reason for hiding this comment

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

SecurityConfig에도 권한 설정이 되어 있던데 Config에서 할지 컨트롤러에서 따로 붙힐지 의견 나눠보고 통일시키는게 좋을 것 같습니당

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이것도 맞는 말씀이네요 컨트롤러에서 따로 붙이기엔 이미 securityConfig에 다 설정이 되어있으니 이부분도 지우고 다시 push하겠습니다.

그리고 find entity랑 응답 객체를 따로 둔건 아직 조회 시 어떤 필드를 필요로 할지 안정한 것 같아서 나중에 정한 다음 필요없는 필드 제외할 때 편하게 하려고 따로 뒀습니당

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아 조회쿼리를 따로 둔걸 말씀하신거면 그냥 필요없는 필드는 쿼리부터 없이 조회하는게 취향이라 그렇습니다ㅎㅎ

Copy link
Collaborator

Choose a reason for hiding this comment

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

내일은 상품에 들어갈 필드 한 번 의논해보면 될 거 같네요 ㅎㅎ


ProductResponse productResponse = productService.findProductResponseById(id);

return ResponseEntity.ok().body(GenericResponse.of(productResponse));
}

@PostMapping
@PreAuthorize("hasRole('ADMIN')")
public ResponseEntity<GenericResponse<Void>> create(@RequestBody @Validated(ValidationSequence.class) ProductForm productForm) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

GericResponse.of(String message)로 되어있어서 반한 타입은 Void대신 String으로 수정해야 될 것 같습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

그렇네요 수정하고 push하겠습니다

Copy link
Collaborator

Choose a reason for hiding this comment

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

수정하신거 확인했습니다.
내일 오전에 다른 분들 코멘트 없으면 머지하도록 하겠습니다.

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.

P-03
2 participants