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

알림 기능 구현 & 키워드 검색 기능 구현 #14

Closed
wants to merge 2 commits into from

Conversation

chanyoungit
Copy link
Member

구매 완료 버튼 클릭
물품 상태 변경
물품에 사물함을 할당하고 사물함 상태 변경
판매자에게 알림을 보내주는 서비스 구현
거래 내역 생성


판매자는 물건을 넣고 확인 버튼 누름
구매자에게 사물함에 물건이 보관되었다고 알림


구매자 수령 완료
물품 상태 변경
사물함 상태 변경

@chanyoungit chanyoungit self-assigned this Nov 9, 2024
Copy link
Collaborator

@KimGyeongLock KimGyeongLock left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!

private final NotificationService notificationService;

@GetMapping()
public ApiResponse<List<NotificationEntity>> allNotification(@AuthenticationPrincipal CustomOAuth2User oAuth2User) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

메서드 네이밍은 보통 동사나 전치사가 앞에 오도록 하면 좋을 것 같아요!
저도 자바 메서드 명명 규칙을 참고하였습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

메서드 네이밍은 보통 동사나 전치사가 앞에 오도록 하면 좋을 것 같아요! 저도 자바 메서드 명명 규칙을 참고하였습니다.

넵 앞으로는 메서드를 작성할 때 메서드 네이밍을 생각하면서 코드를 작성해야겠네요!

Comment on lines +27 to +31
@Column(name = "seller_account")
private String sellerAccount;

@Column(name = "seller_realname")
private String sellerRealname;
Copy link
Collaborator

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.

이거에 맞게 다른 부분도 수정되면 좋을 것 같네요!

Copy link
Member Author

Choose a reason for hiding this comment

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

이거에 맞게 다른 부분도 수정되면 좋을 것 같네요!

참고하겠습니다!

Copy link
Member

Choose a reason for hiding this comment

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

맞습니다~

Comment on lines +19 to +24
public List<NotificationEntity> allNotification(Long userId) {
List<NotificationEntity> notifications = notificationRepository.findByUserIdAndIsReadTrue(userId);

for(NotificationEntity notificationEntity : notifications) {
notificationEntity.setRead(false);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

알림을 읽지 않은 것(false)을 읽었다(true)로 바꾸는 게 좋을 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

알림을 읽지 않은 것(false)을 읽었다(true)로 바꾸는 게 좋을 것 같아요!

넵!

Comment on lines 24 to 29
ProductEntity productEntity = productService.findProductById(productId);

// 상태가 SELL이 아니라면 예외 발생
if (!productEntity.getStatus().equals(ProductStatus.SELL)) {
throw new AccessDeniedException(ErrorCode.ACCESS_DENIED);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

비지니스 로직 부분은 service단으로 넘겨도 좋을 거 같아요!

Copy link
Collaborator

Choose a reason for hiding this comment

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

findProductById의 경우 purchaseProduct 에서 비관적 락으로 Id를 찾더라구요 없어도 될 것 같아요

Copy link
Member Author

Choose a reason for hiding this comment

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

findProductById의 경우 purchaseProduct 에서 비관적 락으로 Id를 찾더라구요 없어도 될 것 같아요

코드가 중복되는군요 좋은 코멘트 감사합니다!

@@ -76,6 +92,14 @@ public TradeEntity completePurchase(Long productId, Long buyerId) {
productEntity.setLockerEntity(availableLockerEntity);
productRepository.save(productEntity);

Copy link
Collaborator

Choose a reason for hiding this comment

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

함수 코드가 길어지면 가독성이 떨어져서 메서드 분리가 좋을 거 같아요!
우테코에서 배운건데 메서드당 10줄 정도로 맞추더라구요!

Copy link
Member Author

Choose a reason for hiding this comment

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

꿀팁 감사합니다!

Copy link
Member

@alswp006 alswp006 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!

Comment on lines +27 to +31
@Column(name = "seller_account")
private String sellerAccount;

@Column(name = "seller_realname")
private String sellerRealname;
Copy link
Member

Choose a reason for hiding this comment

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

맞습니다~

this.sellerRealname = NotificationBuyerDTO.getSellerRealname();
this.isRead = NotificationBuyerDTO.isRead();
this.userId = NotificationBuyerDTO.getUserId();
}
Copy link
Member

Choose a reason for hiding this comment

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

판매자 전용 알림, 구매자 전용 알림보다 더 좋은 구성이 있을 것 같습니다!

@@ -10,10 +11,11 @@
@Builder
@NoArgsConstructor
@AllArgsConstructor
public class ProductEntity {
public class ProductEntity extends BaseEntity {
Copy link
Member

Choose a reason for hiding this comment

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

굿굿

// 비밀번호 생성
public String generateLockerPassword() {
Random random = new Random();
int password = 1000 + random.nextInt(9000);
Copy link
Member

Choose a reason for hiding this comment

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

꼭 1000이상으로 둬야할 필요는 없을 것 같아요.
0001이 나와도 비밀번호로 쓸 수 있으니까요!
String format으로 네자리만 맞춰주면 될 것 같습니다!

@alswp006 alswp006 closed this Dec 20, 2024
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.

3 participants