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

refactor: 물주기, 메세지함 API 작업 #34

Merged
merged 9 commits into from
Jun 29, 2022
Merged

Conversation

suubinkim
Copy link
Member

@suubinkim suubinkim commented Jun 27, 2022

이슈

작업 내용

  • 물주기, 메세지 목록 조회, 열매 맺기 ApiRespinse 수정
  • 물주기 - folderId가 null일때 수신자 디폴트 폴더로 처리
  • MessageController, Service 테스트 케이스 추가 및 수정
  • 메세지 삭제
  • 메세지 폴더 이동
  • 메세지 즐겨찾기 설정
  • 즐겨찾기한 메세지 목록 조회
    ++
  • 메세지 dto에 작성일자/시간 추가
  • 메세지 읽음 여부 변경 API

기타 사항

  • controller에서 바로 pageable 받기
  • 예외처리로 optional 사용
  • stream 사용
  • 메세지 삭제 - 1차로 물리 삭제 처리 (나중에 삭제여부 필드 추가 필요)
  • 물주기 - 비로그인시 처리 필요

체크리스트

@suubinkim suubinkim added 🌲칭찬 칭찬 🔍test test 🧹refactor refactoring, chore labels Jun 27, 2022
@suubinkim suubinkim added this to the 2차 스프린트 milestone Jun 27, 2022
@suubinkim suubinkim added this to To do in BeTree via automation Jun 27, 2022
@suubinkim suubinkim self-assigned this Jun 27, 2022
Copy link
Contributor

@kang-jisu kang-jisu left a comment

Choose a reason for hiding this comment

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

기능 테스트, Optional Stream 적용 다 넘 잘해주셨네요!! 😆👍
몇가지 코멘트 남길테니 확인부탁드릴게요!

그리고 토요일에 그냥 머지해버리느라 못한 리뷰 이전 PR에 마저 남겨놨는데 그것도 확인해주시면 좋을 것 같아요 !!
링크 : #27
❤️

+)
아 그리고, 추가로 메시지 관련 수정,삭제하는 API들에 대해서 지금은 유저랑 메시지 Id를 따로 받고 있는데
메시지의 sender,receiver Id에 해당하지 않는 사용자가 메시지 삭제를 한다거나 (지금은 삭제 처리 간소화했으니 보낸쪽에서는 확인만 할 수 있고, 삭제는 받는쪽에서만 할수있다고 조건을 건다거나,, )
메시지 receiver id에 해당하지 않는 사용자가 메시지 익명여부, 즐겨찾기들을 수정한다거나 하는 요청이 지금은 가능한것같아서요
userId랑 message와 관련된 userId 일치여부에 따라서 변경 불가능한 처리를 해줘야할 것 같아요!!
나중에 이슈 만들어서 한번에 처리하거나요!!

아아아 그래서 findById말고 findByIdAndUserId 하신거군여!!맞나여!!
--> 네 맞습니당..!!! 근데 다시 생각해보니까
지금 제가 구현한거는 로그인한 유저가 받은 메세지 삭제인데,,
내가 보낸 메세지 삭제하려면 따로 api 추가해야겠네요 근데 이건 2차니까 일단 얘기하신게 맞아용 ㅎㅎ

.andExpect(status().isCreated());
.andExpect(status().isBadRequest())
.andExpect(jsonPath("$.code").value("C001"))
.andExpect(jsonPath("$.message").value("Invalid input value"));
Copy link
Contributor

Choose a reason for hiding this comment

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

andExpect(jsonPath("$.errors[0].message").value(""))
저도 몇개는 그냥 C001 에러로 확인했는데 앞으로 validation 예외처리 테스트할 때 저희 jsonPath값 이렇게 지정해서 확인하면 혹시 예외처리 메시지 달라졌을 때 관련 테스트코드 발견하기 좋을 것 같아요!

@Test
void getMessageListErrorTreeId() throws Exception {

given(messageService.getMessageList(anyLong(), any(), eq(19L))).willThrow(new BetreeException(ErrorCode.TREE_NOT_FOUND));
Copy link
Contributor

Choose a reason for hiding this comment

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

오 이건 그냥 질문인데 mock 처리할 때 eq(19L)처럼 쓰면 param으로 전달한 treeId 19L에 맞게 되는건가요!!
저 가끔 예외처리할 때 그냥 19L로 적었더니 안되서 treeId부분에도 anyLong()써서 대충 해결한 것 같거든요 ㅠ
참고하겠습니당 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

저도 안돼서 검색해봤더니 Mockito 쓸때 여러 인자 중에 하나라도 any()같은 임의값 사용하면 나머지도 그런 처리를 해줘야되서 특정값 쓰려면 eq쓰는거라고 하더라고용.. Argument Matcher 검색하면 나와요 !!

appleFolder = Folder.builder().id(1L).fruit(FruitType.APPLE).level(0L).user(user1).build();

}
private final Pageable pageable = Mockito.mock(Pageable.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

오호 Pagable mock 처리 알아갑니다 👍

})
@ResponseStatus(HttpStatus.OK)
@GetMapping("/api/messages")
public ResponseEntity<MessagePageResponseDto> getMessageList(@ApiIgnore @LoginUser LoginUserDto loginUser,
@RequestParam int page,
@PageableDefault(sort = "createdDate", direction = Sort.Direction.DESC) Pageable pageable,
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@@ -121,7 +120,7 @@ public ResponseEntity<Object> openingMessage(@ApiIgnore @LoginUser LoginUserDto
public ResponseEntity<Object> deleteMessages(@ApiIgnore @LoginUser LoginUserDto loginUser,
@RequestBody List<Long> messageIds) {

log.info("[messageIdList] : {}", messageIds + ", [loginUser Id] : {}" + loginUser.getId());
log.info("[messageIdList] : {}", messageIds + ", [loginUser Id] : " + loginUser.getId());
Copy link
Contributor

Choose a reason for hiding this comment

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

 log.info("[messageIdList] : {}, [loginUser Id] : {}", messageIds, loginUser.getId());

이렇게 처음 string에 {}로 구분해서 한번에 넣고 뒤에 인자 추가할 수 있어요!

public static MessagePageResponseDto of(List<MessageBoxResponseDto> responseDto, boolean hasNext) {
return MessagePageResponseDto.builder()
.responseDto(responseDto)
.hasNext(hasNext)
Copy link
Contributor

Choose a reason for hiding this comment

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

질문이 있는데 페이징 처리 들어갈 때 마다 hasNext 필드 추가하시던데 혹시 이게 어떤의미로 쓰이는건가요!!??
(이상하거나 빼자는건 아니고 아니고 따로 검색하거나 코드보고 이해하기 귀찮아서 쓸모가있어서 추가하신거겠지~~하고 자꾸 넘겨버려서 질문드려요 ㅋㅋㅋ 🙇‍♀️ )
hasNext는 있는데 hasPrev는 안하시는 이유라던가 ,, Slice 쓸 때 제공되는 기능중에 하나인가요 ??

Copy link
Member Author

Choose a reason for hiding this comment

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

hasNex가 다음 데이터가 있는지 여부인데 제가 찾아본거랑 이해한거 같이 말하자면,, 무한 스크롤은 더보기같은 거니까 다음 페이지가 있나만 확인 하면 될 것 같아서 hasNext만 추가한거예요 있으면 더 보여주고 없다면 그만 이런 식으로.. ? 근데 프론트쪽에서 어떨지 잘 모르겠어요 ㅋㅋ

Copy link
Contributor

Choose a reason for hiding this comment

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

아!무한스크롤경우를 생각하면 hasNext여부가 필요하겠네요!! 좋습니다아

Comment on lines 140 to 143
messageIds.forEach(id -> {
Message message = messageRepository.findByIdAndUserId(id, userId).orElseThrow(() -> new BetreeException(MESSAGE_NOT_FOUND, "messageId = " + id));
messageRepository.delete(message);
});
Copy link
Contributor

@kang-jisu kang-jisu Jun 27, 2022

Choose a reason for hiding this comment

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

👍

그냥 리뷰니까 쪼금 더 빡빡하게 남기자면
근처에서 사용하는 Id가 userId, messagesId로 구분이 필요하니까 forEach에서 사용하는 'id'가 messageId라고 명시해주는게 나중에 보기에 안헷갈릴것 같아요!
+) messageId, userId다 관련있는거니까 에러메시지에 userId도 추가해주면 좋겠네요!!

mockMvc.perform(post("/api/messages")
.header("Authorization", "Bearer " + JwtTokenTest.JWT_TOKEN_TEST)
.contentType(MediaType.APPLICATION_JSON)
.param("isLogin", String.valueOf(true))
Copy link
Contributor

Choose a reason for hiding this comment

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

isLogin 쓰이지 않는 param이 아직 남아있는 것 같네요! 지워야할 것 같아요

Copy link
Contributor

@kang-jisu kang-jisu left a comment

Choose a reason for hiding this comment

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

머지해도 좋을 것 같아요 ☺️

@@ -54,7 +54,7 @@ public Message(Long id, String content, Long senderId, boolean anonymous, boolea
* 읽음 여부 상태 변경 메서드
*/
public void updateAlreadyRead() {
this.alreadyRead = !this.alreadyRead;
this.alreadyRead = true; //false 될 수 없음
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

…m-1-BE into feature/message

� Conflicts:
�	src/main/java/com/yapp/betree/service/MessageService.java
…ssage

# Conflicts:
#	src/main/java/com/yapp/betree/service/MessageService.java
@suubinkim suubinkim merged commit cf014b9 into develop Jun 29, 2022
BeTree automation moved this from To do to Done Jun 29, 2022
@suubinkim suubinkim deleted the feature/message branch June 29, 2022 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌲칭찬 칭찬 🧹refactor refactoring, chore 🔍test test
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants