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

[feat] 서랍메뉴 - 나무 공개 설정 #72

Merged
merged 6 commits into from
Jul 17, 2022

Conversation

suubinkim
Copy link
Member

이슈

작업 내용

  • 나무 공개 설정 api
  • 나무 숲 조회 api 수정 - 조회 유저에 따라 전체 또는 공개 나무 조회

기타 사항

  • folderAcceptanceTest에 테스트 작성하려고 하는데 이거 넣으면 여기저기서 갑자기 에러나고 이거 빼면 전체 성공하더라구요.. 하나만 돌리면 성공하구요.. 그래서 일단 뺏습니다...

체크리스트

  • example

@suubinkim suubinkim added 💡feature API feature 📬메시지함 메시지함 labels Jul 12, 2022
@suubinkim suubinkim added this to the 5차 스프린트 milestone Jul 12, 2022
@suubinkim suubinkim added this to To do in BeTree via automation Jul 12, 2022
@suubinkim suubinkim self-assigned this Jul 12, 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.

수빈님 수고하셨습니다아~

지금 tokenInterceptor 부분에서 조건문분기가 잘 안되고있을거같아서 mock으로 컨트롤러 테스트하는거말고 folderAcceptanceTest 를 하긴 해야할거같아요! 하나만 돌렸을땐 그래도 성공하는거면 일단 테스트코드 다시 작성해서 @Disabled 처리해서 올려주실수있을까요?? 저도가끔 그런테스트들 있어서 ㅠ
몇가지 보고 코멘트 남겨둘게요~!! 감사합니당

@ResponseStatus(HttpStatus.NO_CONTENT)
@PutMapping("/api/forest/opening")
public ResponseEntity<Void> openTree(@ApiIgnore @LoginUser LoginUserDto loginUser,
@RequestParam Long treeId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

삭제랑 편집이랑 컨벤션을 맞추려면 treeId가 pathvariable인게 좋지 않을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

저는 메세지 컨트롤러에서 boolean 값 바꾸는건 다 RequestParam으로 해서 여기도 맞춘거였어요! 움 pathvariable도 저는 상관없는데 그럼 추가해서 컨벤션 맞출까용?!

Copy link
Contributor

Choose a reason for hiding this comment

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

아하 불리언값 바꾸는건 그렇게하신거군요 그럼 그대로해도 괜찮을거같아요~~

Comment on lines +149 to +152
@Transactional
public void updateTreeOpening(Long userId, Long treeId) {
Folder folder = validateAndGetFolder(userId, treeId);
folder.updateOpening();
Copy link
Contributor

Choose a reason for hiding this comment

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

기획상 기본폴더는 나무 비공개처리 하는거 추가해야되려나요 저희 ??

Copy link
Member Author

@suubinkim suubinkim Jul 14, 2022

Choose a reason for hiding this comment

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

그 boolean 값 초기에 설정 안 하면 false 로 되는거 아닌가용?! ? 예외처리 같은거 말씀하시는걸까요?!

Copy link
Contributor

Choose a reason for hiding this comment

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

아 초기에는 false돼있군용 넹 예외처리필요할거같아요 기본나무 공개로바꾸려는거 막는걸로요!

@@ -289,4 +289,35 @@ void deleteTreeTest() {
.isInstanceOf(BetreeException.class)
.hasMessageContaining("기본 나무를 삭제할 수 없습니다.");
}

@Test
@DisplayName("유저 나무 공개 - 공개 설정하려는 나무가 존재하지 않으면 에외가 발생한다.")
Copy link
Contributor

Choose a reason for hiding this comment

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

    /**
     * 나무(폴더) 처리시에 userId, treeId 검증 및 tree 주인과 user 일치여부 파악
     *
     * @param userId
     * @param treeId
     * @return Folder
     */
    private Folder validateAndGetFolder(Long userId, Long treeId) {

FolderService에 이 로직 쓰는 메서드들은 위에 유저나무 - 라고 해서 한번에 테스트 하고있어서 나무존재에러, 주인일치여부에러 테스트는 삭제해도될거같아요!

Comment on lines +90 to +98

// 나무숲 상세조회
if (request.getRequestURI().startsWith("/api/forest/") && request.getMethod().equals(String.valueOf(HttpMethod.GET))) {
return true;
}
// 물주기
if (request.getRequestURI().equals("/api/messages") && request.getMethod().equals(String.valueOf(HttpMethod.POST))) {

// 물주기 & 나무 숲 조회
if (request.getRequestURI().equals("/api/messages") && request.getMethod().equals(String.valueOf(HttpMethod.POST)) ||
request.getRequestURI().equals("/api/forest") && request.getMethod().equals(String.valueOf(HttpMethod.GET))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

나무숲 상세조회랑 나무숲 조회가 둘다 /api/forest/로 시작하고 get 메소드라서 지금 상태로는 나무상세조회 조건문에서 바로 true가 될거같은데 맞나요..? 나무숲조회일때 뒤에 treeId 요청하는거로 구분하거나 url을 좀 변경해야될거같아욥

Copy link
Member Author

Choose a reason for hiding this comment

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

아 제가 테스트 하면서 리턴 값 맞게 나오게 하려고 이렇게 저렇게 해봤는데 상세조회는 true나와야 하는게 맞는거 아닌가요..!? 그래서 상세랑 나무 숲 조회 조건문 안 겹치게 하려고 상세조회 조건문에 원래 /api/forest 였는데 마지막에 / 붙여서 나무 숲 조회랑 구별하고 나무숲 조회 조건문은 startsWith 말고 equals로 했어용

제가 이해 잘 못 한 부분 있을까요?!

Copy link
Contributor

Choose a reason for hiding this comment

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

아하 마지막에 / 이거붙이셨군요 제가 덜봤나봐요ㅜㅜ그럼 이대로하시면 될거같습니당!!

.collect(Collectors.toList());
}

return folderRepository.findAllByUserIdAndOpening(userId, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

이부분은 로직상 문제는 없긴 한데 나무숲에 나무 자체가 많지 않아서 그냥 findAllByUserId로만 조회한다음에 stream.filter로 opening 걸러도 되지 않을까요?! 또 나무숲에서 보여지는 조건이 달라질때마다 레파지토리에 메서드를 계속 추가하게 되는것보단 백엔드단에서는 적당히 전체를 가져와서 코드로 필터링해도 될거같아요!!

Copy link
Member Author

Choose a reason for hiding this comment

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

좋습니다!!

@kang-jisu
Copy link
Contributor

테스트도 여러개 있을때 하나 실패하는게 cascade설정이나 뭔가 해결해봐야할거같긴한데 잘 모르겠네요 ㅠ
일단 다른 문제는 없는거같으니 제가 머지하겠습니당!!

@kang-jisu kang-jisu merged commit d193d87 into YAPP-Github:develop Jul 17, 2022
BeTree automation moved this from To do to Done Jul 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡feature API feature 📬메시지함 메시지함
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants