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.

Sign up for GitHub

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

♻️ user block logic #437

Open
wants to merge 3 commits into
base: be/dev
Choose a base branch
from
Open

♻️ user block logic #437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
64bc33c
Create PR for #436
github-actions[bot] Dec 11, 2024
95017c4
:twisted_rightwards_arrows: merge conflict
HaiSeong Jan 6, 2025
78d6328
:recycle: blocking logic
HaiSeong Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 32 additions & 14 deletions backend/src/main/java/net/pengcook/user/aop/BlockedUserFilterAspect.java
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

단건조회 관련 커버리지가 낮은데 단건조회를 했을 때 차단 상태면 보이지 않는 것을 확인하는 테스트를 추가하면 어떨까요???
현재는 목록만 존재하네요~~~

Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@
import java.util.Optional;
import lombok.RequiredArgsConstructor;
import net.pengcook.authentication.domain.UserInfo;
import net.pengcook.user.domain.BlockeeGroup;
import net.pengcook.user.domain.BlockerGroup;
import net.pengcook.user.domain.Ownable;
import net.pengcook.user.domain.BlockedUserGroup;
import net.pengcook.user.exception.ForbiddenException;
import net.pengcook.user.service.UserService;
import org.aspectj.lang.ProceedingJoinPoint;
import org.aspectj.lang.annotation.Around;
Expand Down Expand Up @@ -35,9 +37,10 @@ public Object filterBlockedAuthorsFromList(ProceedingJoinPoint joinPoint) throws
return ownables;
}

BlockedUserGroup blockedUserGroup = userService.getBlockedUserGroup(userInfo.getId());
BlockeeGroup blockeeGroup = userService.getBlockeeGroup(userInfo.getId());
BlockerGroup blockerGroup = userService.getBlockerGroup(userInfo.getId());

return filterBlockedUsers(ownables, blockedUserGroup);
return filterBlockedUsers(ownables, blockeeGroup, blockerGroup);
}

@Pointcut("execution(java.util.Optional<net.pengcook.user.domain.Ownable+> net.pengcook..repository..*(..))")
Expand All @@ -53,19 +56,24 @@ public Object filterBlockedAuthorFromOptional(ProceedingJoinPoint joinPoint) thr
return ownableOptional;
}

BlockedUserGroup blockedUserGroup = userService.getBlockedUserGroup(userInfo.getId());
if (blockedUserGroup.isBlocked(ownableOptional.get().getOwnerId())) {
return Optional.empty();
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("게시글을 이용할 수 없습니다.");
}
Comment on lines +59 to 67
Copy link
Contributor

Choose a reason for hiding this comment

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

readRecipeDescription에서 해당 레시피가 없을 때 (아마도 홈 화면에서 레시피를 보고, 상세 정보를 누르기 전에 레시피가 지워졌을 경우에 생기겠죠.) NotFoundException을 던지고, 존재하지 않는 레시피입니다.라는 예외 메시지를 보냅니다.
사용자가 정보를 못 볼 때 이유 불문, 받는 메시지가 동일해야 하지 않을까 싶어요.


return ownableOptional;
}

@Pointcut("execution(net.pengcook.user.domain.Ownable+ net.pengcook..repository..*(..))")
public void repositoryMethodsReturningOwnable() {
public void repositoryMethodsReturningAuthorAble() {
}

@Around("repositoryMethodsReturningOwnable()")
@Around("repositoryMethodsReturningAuthorAble()")
Comment on lines -65 to +76
Copy link
Contributor

Choose a reason for hiding this comment

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

컨플릭트 해결할 때 잘못 고른 거 같아요...!!

public Object filterBlockedAuthor(ProceedingJoinPoint joinPoint) throws Throwable {
Ownable ownable = (Ownable) joinPoint.proceed();
Copy link
Contributor

Choose a reason for hiding this comment

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

바로 타입캐스팅을 하지 말고 instanceof를 사용하면 공통 로직이 많은 두 개의 메서드를 합칠 수 있지 않을까요...?!
이번 수정사항으로 두 메서드를 똑같이 수정하게 되었는데 한 번만 수정할 수 있게 되면 좋잖아요.


Expand All @@ -74,9 +82,14 @@ public Object filterBlockedAuthor(ProceedingJoinPoint joinPoint) throws Throwabl
return ownable;
}

BlockedUserGroup blockedUserGroup = userService.getBlockedUserGroup(userInfo.getId());
if (blockedUserGroup.isBlocked(ownable.getOwnerId())) {
return null;
BlockeeGroup blockeeGroup = userService.getBlockeeGroup(userInfo.getId());
if (blockeeGroup.contains(ownable.getOwnerId())) {
throw new ForbiddenException("차단한 사용자입니다.");
}

BlockerGroup blockerGroup = userService.getBlockerGroup(userInfo.getId());
if (blockerGroup.contains(ownable.getOwnerId())) {
throw new ForbiddenException("게시글을 이용할 수 없습니다.");
}

return ownable;
Expand All @@ -91,9 +104,14 @@ private UserInfo getCurrentUserInfo() {
}
}

private List<Ownable> filterBlockedUsers(List<Ownable> ownables, BlockedUserGroup blockedUserGroup) {
return ownables.stream()
.filter(item -> !blockedUserGroup.isBlocked(item.getOwnerId()))
private List<Ownable> filterBlockedUsers(
List<Ownable> authorAbles,
BlockeeGroup blockeeGroup,
BlockerGroup blockerGroup
) {
return authorAbles.stream()
.filter(item -> !blockeeGroup.contains(item.getOwnerId()))
.filter(item -> !blockerGroup.contains(item.getOwnerId()))
.toList();
}
}
4 changes: 2 additions & 2 deletions ...engcook/user/domain/BlockedUserGroup.java → ...et/pengcook/user/domain/BlockeeGroup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@
import lombok.AllArgsConstructor;

@AllArgsConstructor
public class BlockedUserGroup {
public class BlockeeGroup {

private final Set<User> users;

public boolean isBlocked(long userId) {
public boolean contains(long userId) {
Copy link

Choose a reason for hiding this comment

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

contains 라는 네이밍으로 포함을 확인하는 것 보단, 조금 더 동작이 명확했으면 좋겠어요.
Blockee는 차단 당한 사람이니 isBlocked(), 또는 blockedBy()를, Blocker은 차단 한 사람이니 isBlocking()을 사용해 보는 건 어떨까요?
조금 더 가독성이 좋아질 것 같아용

return users.stream()
.anyMatch(user -> user.isSameUser(userId));
}
Expand Down
15 changes: 15 additions & 0 deletions backend/src/main/java/net/pengcook/user/domain/BlockerGroup.java
View file
Open in desktop
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
Contributor

Choose a reason for hiding this comment

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

저는 사실 이 클래스의 존재 자체에 약간 회의적인데요...
오로지 일급컬렉션으로 묶어서 불변을 보장하기 위해 사용하는 건가요???
꼭 필요한 클래스인지, 그냥 set으로 돌아다니면 안 되는 것인지... 로키의 의견 궁금합니다!

그리고 위에 단 리뷰

사용자가 정보를 못 볼 때 이유 불문, 받는 메시지가 동일해야 하지 않을까

의 연장선에서, blocker와 blockee를 따로 볼 필요가 있을까?
접근하지 못하는 사용자들로 합치면 안되나?
이 부분에 대해서도 의견이 궁금해요~~~

Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package net.pengcook.user.domain;

import java.util.Set;
import lombok.AllArgsConstructor;

@AllArgsConstructor
public class BlockerGroup {

private final Set<User> users;

public boolean contains(long userId) {
return users.stream()
.anyMatch(user -> user.isSameUser(userId));
}
}
9 changes: 9 additions & 0 deletions backend/src/main/java/net/pengcook/user/exception/ForbiddenException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package net.pengcook.user.exception;

import org.springframework.http.HttpStatus;

public class ForbiddenException extends UserException {
public ForbiddenException(String message) {
super(HttpStatus.FORBIDDEN, message);
}
}
2 changes: 2 additions & 0 deletions backend/src/main/java/net/pengcook/user/repository/UserBlockRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
public interface UserBlockRepository extends JpaRepository<UserBlock, Long> {
List<UserBlock> findAllByBlockerId(long blockerId);

List<UserBlock> findAllByBlockeeId(long blockeeId);

void deleteByBlockeeId(long blockeeId);

void deleteByBlockerId(long blockerId);
Expand Down
15 changes: 12 additions & 3 deletions backend/src/main/java/net/pengcook/user/service/UserService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
import net.pengcook.like.repository.RecipeLikeRepository;
import net.pengcook.recipe.repository.RecipeRepository;
import net.pengcook.recipe.service.RecipeService;
import net.pengcook.user.domain.BlockedUserGroup;
import net.pengcook.user.domain.BlockeeGroup;
import net.pengcook.user.domain.BlockerGroup;
import net.pengcook.user.domain.User;
import net.pengcook.user.domain.UserBlock;
import net.pengcook.user.domain.UserReport;
Expand Down Expand Up @@ -109,11 +110,19 @@ public ReportResponse report(long reporterId, ReportRequest reportRequest) {
}

@Transactional(readOnly = true)
public BlockedUserGroup getBlockedUserGroup(long blockerId) {
public BlockeeGroup getBlockeeGroup(long blockerId) {

return userBlockRepository.findAllByBlockerId(blockerId).stream()
.map(UserBlock::getBlockee)
.collect(Collectors.collectingAndThen(Collectors.toSet(), BlockedUserGroup::new));
.collect(Collectors.collectingAndThen(Collectors.toSet(), BlockeeGroup::new));
}

@Transactional(readOnly = true)
public BlockerGroup getBlockerGroup(long blockeeId) {

return userBlockRepository.findAllByBlockeeId(blockeeId).stream()
.map(UserBlock::getBlocker)
.collect(Collectors.collectingAndThen(Collectors.toSet(), BlockerGroup::new));
}

@Transactional
Expand Down
8 changes: 4 additions & 4 deletions ...ook/user/domain/BlockedUserGroupTest.java → ...engcook/user/domain/BlockeeGroupTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,18 @@
import java.util.Set;
import org.junit.jupiter.api.Test;

class BlockedUserGroupTest {
class BlockeeGroupTest {

@Test
void isBlocked() {
User loki = new User(1L, "[email protected]", "loki", "로키", "loki.jpg", "KOREA");
User pond = new User(2L, "[email protected]", "pond", "폰드", "pond.jpg", "KOREA");

BlockedUserGroup blockedUserGroup = new BlockedUserGroup(Set.of(pond));
BlockeeGroup blockeeGroup = new BlockeeGroup(Set.of(pond));

assertAll(
() -> assertThat(blockedUserGroup.isBlocked(loki.getId())).isFalse(),
() -> assertThat(blockedUserGroup.isBlocked(pond.getId())).isTrue()
() -> assertThat(blockeeGroup.contains(loki.getId())).isFalse(),
() -> assertThat(blockeeGroup.contains(pond.getId())).isTrue()
);
}
}
12 changes: 6 additions & 6 deletions backend/src/test/java/net/pengcook/user/service/UserServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import net.pengcook.image.service.ImageClientService;
import net.pengcook.like.repository.RecipeLikeRepository;
import net.pengcook.recipe.repository.RecipeRepository;
import net.pengcook.user.domain.BlockedUserGroup;
import net.pengcook.user.domain.BlockeeGroup;
import net.pengcook.user.domain.Reason;
import net.pengcook.user.domain.Type;
import net.pengcook.user.domain.UserReport;
Expand Down Expand Up @@ -192,15 +192,15 @@ void blockUserWhenNotExistBlockee() {

@Test
@DisplayName("차단한 사용자들의 목록을 불러올 수 있다.")
void getBlockedUserGroup() {
void getBlockeeGroup() {
long blockerId = 1L;

BlockedUserGroup blockedUserGroup = userService.getBlockedUserGroup(blockerId);
BlockeeGroup blockeeGroup = userService.getBlockeeGroup(blockerId);

assertAll(
() -> assertThat(blockedUserGroup.isBlocked(2L)).isTrue(),
() -> assertThat(blockedUserGroup.isBlocked(3L)).isTrue(),
() -> assertThat(blockedUserGroup.isBlocked(4L)).isFalse()
() -> assertThat(blockeeGroup.contains(2L)).isTrue(),
() -> assertThat(blockeeGroup.contains(3L)).isTrue(),
() -> assertThat(blockeeGroup.contains(4L)).isFalse()
);
}

Expand Down
Loading