♻️ user block logic #437
♻️ user block logic #437
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
차단로직을 수정했습니다. 기존 단방향 차단만 존재했다면 이제 양방향으로 차단되도록 했습니다.
|
There was a problem hiding this comment.
두 클래스가 하는 일이 완전히 똑같은데, 나눠져 있을 필요가 있을까요??
현재 상태라면 하나의 클래스로 두고, 객체 두 개의 이름을 나눠야 하지 않을까요?????
There was a problem hiding this comment.
저는 사실 이 클래스의 존재 자체에 약간 회의적인데요...
오로지 일급컬렉션으로 묶어서 불변을 보장하기 위해 사용하는 건가요???
꼭 필요한 클래스인지, 그냥 set으로 돌아다니면 안 되는 것인지... 로키의 의견 궁금합니다!
그리고 위에 단 리뷰
사용자가 정보를 못 볼 때 이유 불문, 받는 메시지가 동일해야 하지 않을까
의 연장선에서, blocker와 blockee를 따로 볼 필요가 있을까?
접근하지 못하는 사용자들로 합치면 안되나?
이 부분에 대해서도 의견이 궁금해요~~~
There was a problem hiding this comment.
두 클래스가 하는 일이 완전히 똑같은데, 나눠져 있을 필요가 있을까요??
동의합니다. 객체 설계를 다시 고민해보겠습니다.
| public void repositoryMethodsReturningOwnable() { | ||
| public void repositoryMethodsReturningAuthorAble() { | ||
| } | ||
|
|
||
| @Around("repositoryMethodsReturningOwnable()") | ||
| @Around("repositoryMethodsReturningAuthorAble()") |
There was a problem hiding this comment.
컨플릭트 해결할 때 잘못 고른 거 같아요...!!
| BlockeeGroup blockeeGroup = userService.getBlockeeGroup(userInfo.getId()); | ||
| if (blockeeGroup.contains(ownableOptional.get().getOwnerId())) { | ||
| throw new ForbiddenException("차단한 사용자입니다."); | ||
| } | ||
|
|
||
| BlockerGroup blockerGroup = userService.getBlockerGroup(userInfo.getId()); | ||
| if (blockerGroup.contains(ownableOptional.get().getOwnerId())) { | ||
| throw new ForbiddenException("게시글을 이용할 수 없습니다."); | ||
| } |
There was a problem hiding this comment.
readRecipeDescription에서 해당 레시피가 없을 때 (아마도 홈 화면에서 레시피를 보고, 상세 정보를 누르기 전에 레시피가 지워졌을 경우에 생기겠죠.) NotFoundException을 던지고, 존재하지 않는 레시피입니다.라는 예외 메시지를 보냅니다.
사용자가 정보를 못 볼 때 이유 불문, 받는 메시지가 동일해야 하지 않을까 싶어요.
There was a problem hiding this comment.
메시지를 읽을 주체를 누구로 할것인가를 명확하게 해보는것이 필요할것 같아요.
개인적으로는 사용자가 정보를 못 볼 때 이유 불문, 받는 메시지가 동일해야 하지 않을까 싶어요. 라는 의견에는 조금 반대되는 생각을 가지는데요. 이유는 메시지를 읽는 사람이 구체적으로 어떤 행동을 취해야 할지 정보가 부족해질 확률이 있기 때문이에요. 따라서 저는 사실 메시지를 읽는 사람에게 자세한 메시지를 주는게 좋다고 생각합니다.
우선 제 의도는 이 메시지들은 사용자가 읽는 메시지라고 생각했어요. 클라이언트에서 결과로 403 코드를 받으면 에러 메시지를 사용자에게 전달하는 방식으로 처리 되는 식으로 만들거라 생각했어요. 실제로 어떤식으로 처리할지 악어에게 물어보고 결정해봐도 좋을것 같아요.
| } | ||
|
|
||
| @Around("repositoryMethodsReturningOwnable()") | ||
| @Around("repositoryMethodsReturningAuthorAble()") | ||
| public Object filterBlockedAuthor(ProceedingJoinPoint joinPoint) throws Throwable { | ||
| Ownable ownable = (Ownable) joinPoint.proceed(); |
There was a problem hiding this comment.
바로 타입캐스팅을 하지 말고 instanceof를 사용하면 공통 로직이 많은 두 개의 메서드를 합칠 수 있지 않을까요...?!
이번 수정사항으로 두 메서드를 똑같이 수정하게 되었는데 한 번만 수정할 수 있게 되면 좋잖아요.
There was a problem hiding this comment.
단건조회 관련 커버리지가 낮은데 단건조회를 했을 때 차단 상태면 보이지 않는 것을 확인하는 테스트를 추가하면 어떨까요???
현재는 목록만 존재하네요~~~
|
|
||
| private final Set<User> users; | ||
|
|
||
| public boolean isBlocked(long userId) { | ||
| public boolean contains(long userId) { |
There was a problem hiding this comment.
contains 라는 네이밍으로 포함을 확인하는 것 보단, 조금 더 동작이 명확했으면 좋겠어요.
Blockee는 차단 당한 사람이니 isBlocked(), 또는 blockedBy()를, Blocker은 차단 한 사람이니 isBlocking()을 사용해 보는 건 어떨까요?
조금 더 가독성이 좋아질 것 같아용
There was a problem hiding this comment.
안녕하세요. 짱수
좋은 네이밍이네요. 의견 감사합니다.
고민해보겠습니다!
차단로직을 수정했습니다. 기존 단방향 차단만 존재했다면 이제 양방향으로 차단되도록 했습니다.
closes #436