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

[BE] Feat/#378 Admin API 구현 #405

Merged
merged 25 commits into from
Sep 15, 2023
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
1000bed
feat: 전체 회원 조회 기능 구현
cpot5620 Sep 12, 2023
0a7f1cb
feat: 회원 삭제(탈퇴) 기능 구현
cpot5620 Sep 12, 2023
540a644
feat: 회원 삭제(탈퇴)시 Pin/Topic Soft-deleting 구현
cpot5620 Sep 12, 2023
1dd3c73
refactor: Admin DTO 분리
cpot5620 Sep 12, 2023
1c50017
feat: Member 상세 정보 조회 기능 구현
cpot5620 Sep 12, 2023
309b827
feat: Topic 삭제 및 이미지 삭제 기능 구현
cpot5620 Sep 12, 2023
1535ada
feat: Pin 삭제 및 이미지 삭제 기능 구현
cpot5620 Sep 12, 2023
d688eee
feat: Admin API 구현
cpot5620 Sep 13, 2023
06ef99b
refactor: Member 상태(차단, 탈퇴 등) 필드에 따른 로그인 로직 수정
cpot5620 Sep 13, 2023
c7c8ee8
refactor: @SqlDelete 삭제 및 JPQL 대체
cpot5620 Sep 13, 2023
7f2c47b
feat: AdminInterceptor 구현
cpot5620 Sep 13, 2023
523ce2c
test: Repository soft-deleting 테스트 구현
cpot5620 Sep 13, 2023
0e887ea
test: AdminQueryService 테스트 구현
cpot5620 Sep 13, 2023
190f5d3
test: AdminCommandService 테스트 구현
cpot5620 Sep 13, 2023
1932e58
test: AdminController Restdocs 테스트 구현
cpot5620 Sep 13, 2023
bb6fb8d
test: AdminInterceptor Mocking
cpot5620 Sep 14, 2023
0f54571
test: 통합 테스트 구현
cpot5620 Sep 14, 2023
a1b2b54
refactor: 오탈자 수정
cpot5620 Sep 14, 2023
1e23aee
refactor: Auth 관련 예외 클래스 추가
cpot5620 Sep 14, 2023
605a2e3
refactor: 불필요한 메서드 제거
cpot5620 Sep 14, 2023
e5a898d
refactor: findMemberById 예외 수정
cpot5620 Sep 14, 2023
53f9c42
test: GithubActions 실패 테스트 수정
cpot5620 Sep 14, 2023
f7afa50
refactor: isAdmin() 메서드 추가
cpot5620 Sep 15, 2023
9084afd
refactor: 회원 삭제(탈퇴)시, 추가 정보(즐겨찾기 등) 삭제
cpot5620 Sep 15, 2023
b30c821
refactor: Member status 기본값 설정
cpot5620 Sep 15, 2023
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
29 changes: 29 additions & 0 deletions backend/src/docs/asciidoc/admin.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
== 관리자 기능

=== 전체 회원 조회

operation::admin-controller-test/find-all-member-details[snippets='http-request,http-response']

=== 회원 상세 조회

operation::admin-controller-test/find-member[snippets='http-request,http-response']

=== 회원 차단(삭제)

operation::admin-controller-test/delete-member[snippets='http-request,http-response']

=== 토픽 삭제

operation::admin-controller-test/delete-topic[snippets='http-request,http-response']

=== 토픽 이미지 삭제

operation::admin-controller-test/delete-topic-image[snippets='http-request,http-response']

=== 핀 삭제

operation::admin-controller-test/delete-pin[snippets='http-request,http-response']

=== 핀 이미지 삭제

operation::admin-controller-test/delete-pin-image[snippets='http-request,http-response']
1 change: 1 addition & 0 deletions backend/src/docs/asciidoc/index.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,4 @@ include::member.adoc[]
include::permission.adoc[]
include::oauth.adoc[]
include::bookmark.adoc[]
include::admin.adoc[]
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
package com.mapbefine.mapbefine.admin.application;

import static com.mapbefine.mapbefine.permission.exception.PermissionErrorCode.PERMISSION_FORBIDDEN_BY_NOT_ADMIN;
import static com.mapbefine.mapbefine.topic.exception.TopicErrorCode.TOPIC_NOT_FOUND;

import com.mapbefine.mapbefine.auth.domain.AuthMember;
import com.mapbefine.mapbefine.member.domain.Member;
import com.mapbefine.mapbefine.member.domain.MemberRepository;
import com.mapbefine.mapbefine.member.domain.Status;
import com.mapbefine.mapbefine.member.exception.MemberErrorCode;
import com.mapbefine.mapbefine.member.exception.MemberException.MemberNotFoundException;
import com.mapbefine.mapbefine.permission.exception.PermissionException.PermissionForbiddenException;
import com.mapbefine.mapbefine.pin.domain.Pin;
import com.mapbefine.mapbefine.pin.domain.PinImageRepository;
import com.mapbefine.mapbefine.pin.domain.PinRepository;
import com.mapbefine.mapbefine.topic.domain.Topic;
import com.mapbefine.mapbefine.topic.domain.TopicRepository;
import com.mapbefine.mapbefine.topic.exception.TopicException;
import java.util.List;
import org.springframework.stereotype.Service;

@Service
public class AdminCommandService {

private final MemberRepository memberRepository;
private final TopicRepository topicRepository;
private final PinRepository pinRepository;
private final PinImageRepository pinImageRepository;

public AdminCommandService(
MemberRepository memberRepository,
TopicRepository topicRepository,
PinRepository pinRepository,
PinImageRepository pinImageRepository
) {
this.memberRepository = memberRepository;
this.topicRepository = topicRepository;
this.pinRepository = pinRepository;
this.pinImageRepository = pinImageRepository;
}

public void blockMember(AuthMember authMember, Long memberId) {
Member admin = findMemberById(authMember.getMemberId());

validateAdminPermission(admin);

Member member = findMemberById(memberId);
member.updateStatus(Status.BLOCKED);
List<Long> pinIds = extractPinIdsByMember(member);

topicRepository.deleteAllByMemberId(memberId);
pinRepository.deleteAllByMemberId(memberId);
pinImageRepository.deleteAllByPinIds(pinIds);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

S3 연동되면 또 생각해야 하는게 많아지려나요.. 후후..

Copy link
Collaborator

Choose a reason for hiding this comment

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

그리고 Permission 과 Bookmark, Atlas 등등은 안지워도 될까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

사실 Permission, Bookmark 해야되는거 알고 있는데, 귀찮아서요 ...
너무 많은 Repository를 알고 있어요......

Copy link
Collaborator

Choose a reason for hiding this comment

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

그쵸 사실 굉장히 귀찮은 것 같아요.. 이 부분은 추후에 의논을 통해서 해결해볼까요?


private Member findMemberById(Long id) {
return memberRepository.findById(id)
.orElseThrow(() -> new MemberNotFoundException(MemberErrorCode.MEMBER_NOT_FOUND, id));
}

private void validateAdminPermission(Member member) {
if (member.isAdmin()) {
return;
}

throw new PermissionForbiddenException(PERMISSION_FORBIDDEN_BY_NOT_ADMIN);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

요 부분도 AdminQueryService 에서 달았단 코멘트와 의견 동일합니다!

private List<Long> extractPinIdsByMember(Member member) {
return member.getCreatedPins()
.stream()
.map(Pin::getId)
.toList();
}

public void deleteTopic(AuthMember authMember, Long topicId) {
Member member = findMemberById(authMember.getMemberId());

validateAdminPermission(member);

topicRepository.deleteById(topicId);
}

public void deleteTopicImage(AuthMember authMember, Long topicId) {
Member member = findMemberById(authMember.getMemberId());

validateAdminPermission(member);

Topic topic = findTopicById(topicId);
topic.removeImage();
}

private Topic findTopicById(Long topicId) {
return topicRepository.findById(topicId)
.orElseThrow(() -> new TopicException.TopicNotFoundException(TOPIC_NOT_FOUND, List.of(topicId)));
}

public void deletePin(AuthMember authMember, Long pinId) {
Member member = findMemberById(authMember.getMemberId());

validateAdminPermission(member);

pinRepository.deleteById(pinId);
}

public void deletePinImage(AuthMember authMember, Long pinImageId) {
Member member = findMemberById(authMember.getMemberId());

validateAdminPermission(member);

pinImageRepository.deleteById(pinImageId);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
package com.mapbefine.mapbefine.admin.application;

import com.mapbefine.mapbefine.admin.dto.AdminMemberDetailResponse;
import com.mapbefine.mapbefine.admin.dto.AdminMemberResponse;
import com.mapbefine.mapbefine.auth.domain.AuthMember;
import com.mapbefine.mapbefine.member.domain.Member;
import com.mapbefine.mapbefine.member.domain.MemberRepository;
import com.mapbefine.mapbefine.member.domain.Role;
import com.mapbefine.mapbefine.member.exception.MemberErrorCode;
import com.mapbefine.mapbefine.member.exception.MemberException.MemberNotFoundException;
import com.mapbefine.mapbefine.permission.exception.PermissionErrorCode;
import com.mapbefine.mapbefine.permission.exception.PermissionException.PermissionForbiddenException;
import com.mapbefine.mapbefine.pin.domain.Pin;
import com.mapbefine.mapbefine.topic.domain.Topic;
import java.util.List;
import org.springframework.stereotype.Service;

@Service
public class AdminQueryService {

private final MemberRepository memberRepository;

public AdminQueryService(MemberRepository memberRepository) {
this.memberRepository = memberRepository;
}

public List<AdminMemberResponse> findAllMemberDetails(AuthMember authMember) {
Member admin = findMemberById(authMember.getMemberId());
validateAdminPermission(admin);
Copy link
Collaborator

Choose a reason for hiding this comment

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

P4. AuthMember 안에 isRole(Role role) 메서드를 두는 건 어떨까요?
Interceptor에서 이미 데이터를 가져오고 있는데 검증을 위해서 한번 더 가야하는 것 같아서용 ㅋㅋ

만약 해당 메서드 도입한다면 여기선 authMember.isRole(Role.ADMIN)로 변경 가능할 것 같네요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#405 (comment)

위 코멘트와 관련이 있는 것 같아요.

다른 분들의 의견처럼 Interceptor에서 수행하기 때문에, Service에서 검증이 필요 없다고 하시면 기존 코드 그대로 가져가도 될 것 같아요.
만약, 지금처럼 이중 검증을 수행한다면 해당 사항 반영하는 것이 더 좋을 것 같긴 하네요 ㅎㅎ..

내일 이야기 나누어보고 결정하도록 하겠습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

해당 사항 반영했습니당 ~!


List<Member> members = memberRepository.findAllByMemberInfoRole(Role.USER);
Copy link
Collaborator

Choose a reason for hiding this comment

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

P5. 전체 사용자가 아닌 USER만 찾는 것 같은데 ADMIN은 어떤 이유에서 제외하셨는지 궁금합니당ㅋㅋ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ADMIN이 ADMIN을 삭제할까봐...?
관리자들 사이에서의 개인정보는 지켜주고 싶어서 ..?
별 다른 이유는 없습니다 ㅎㅎ

그냥 관리자는 일반 유저들을 관리한다고 생각해서 위와 같이 구현했던 것 같아요.

모든 회원을 조회하도록 수정하는 것이 자연스럽다면, blockMember 메서드에서 삭제하려는 Member가 Admin인지 검증하는 로직이 필요하겠네요.

어떻게 생각하시나용


return members.stream()
.map(AdminMemberResponse::from)
.toList();
}

private Member findMemberById(Long id) {
return memberRepository.findById(id)
.orElseThrow(() -> new MemberNotFoundException(MemberErrorCode.MEMBER_NOT_FOUND, id));
}
Copy link
Collaborator

@yoondgu yoondgu Sep 14, 2023

Choose a reason for hiding this comment

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

P2.

이 코드 흐름에서 Member를 찾지 못한다면 400번대(사용자 잘못의 오류)보다는 코드나 데이터가 잘못되어서 발생하는 것 아닐까요 ??
Interceptor에서 이미 검증을 하니까요..!!
그래서 다른 서비스 클래스들에서는 같은 상황에 대해서 아래와 같이 internal server error로 처리되는 예외를 던지고 있습니당

(동일한 흐름의 코드들 모두 해당하는 코멘트입니다!)

    private Member findMemberById(Long id) {
        return memberRepository.findById(id)
                .orElseThrow(() -> new NoSuchElementException("findCreatorByAuthMember; member not found; id=" + id));
    }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이전 PR들에서 해당 부분을 전혀 인지 못했었던 것 같네요.
우선, 코드 통일성을 위해 해당 사항은 반영하도록 하겠습니다.

다만, Interceptor에서 검증을 수행하기 때문에, Server ERROR로 본다는 것은 Service 계층이 Interceptor에 의존적인 것 같다는 생각이 듭니다.

Copy link
Collaborator

@yoondgu yoondgu Sep 14, 2023

Choose a reason for hiding this comment

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

Service 계층이 Interceptor에 의존적인 것 같다는 생각이 듭니다.

이 부분 확실히 논의해야 할 부분인 것 같네요! 내일 얘기해보아요~


private void validateAdminPermission(Member member) {
if (member.isAdmin()) {
return;
}

throw new PermissionForbiddenException(PermissionErrorCode.PERMISSION_FORBIDDEN_BY_NOT_ADMIN);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

다른 분들이 이미 코멘트 달았나 모르긴 하겠지만, 현재 코드에서 Admin 관련 API 에 요청이 오는 경우 AdminAuthInterceptor 가 동작하고 거기에서 validateAdmin 을 진행해주는 걸로 알고 있습니다!

그렇기 때문에 실제 Service 로직에서는 admin 인지 검증하는 것이 불필요하지 않을까용?

제가 잘못생각한 부분이 있는지 쥬니짱이 맘껏 지적해주세용.

Copy link
Collaborator Author

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.

어쩔 tv!

public AdminMemberDetailResponse findMemberDetail(AuthMember authMember, Long memberId) {
Member admin = findMemberById(authMember.getMemberId());

validateAdminPermission(admin);

Member findMember = findMemberById(memberId);
List<Topic> topics = findMember.getCreatedTopics();
List<Pin> pins = findMember.getCreatedPins();

return AdminMemberDetailResponse.of(findMember, topics, pins);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package com.mapbefine.mapbefine.admin.dto;

import com.mapbefine.mapbefine.member.domain.Member;
import com.mapbefine.mapbefine.member.domain.MemberInfo;
import com.mapbefine.mapbefine.pin.domain.Pin;
import com.mapbefine.mapbefine.pin.dto.response.PinResponse;
import com.mapbefine.mapbefine.topic.domain.Topic;
import com.mapbefine.mapbefine.topic.dto.response.TopicResponse;
import java.time.LocalDateTime;
import java.util.List;

public record AdminMemberDetailResponse(
Long id,
String nickName,
String email,
String imageUrl,
List<TopicResponse> topics,
List<PinResponse> pins,
LocalDateTime updatedAt
) {

// TODO: 2023/09/12 topics, pins 모두 member를 통해 얻어올 수 있다. Service에서 꺼내서 넘겨줄 것인가 ? 아니면 DTO에서 꺼내올 것인가 ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

제가 최근들어 N + 1 문제에 대해서 알게되서 그런지, Member 에서 꺼내오는 것이 추후 Join 을 통해서 성능 개선하기에 쉬울 것 같아요.

Copy link
Collaborator

Choose a reason for hiding this comment

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

근데 옆에서 석진씨가 OneToMany 컬럼이 2개 이상일 때 Fetch Join 하면 에러날 수도 있다고 하네요! 다음에 같이 한번 실험해봐요 홍홍ㅎ옿옿옿옿오

public static AdminMemberDetailResponse of(
Member member,
List<Topic> topics,
List<Pin> pins
) {
MemberInfo memberInfo = member.getMemberInfo();
List<TopicResponse> topicResponses = topics.stream()
.map(TopicResponse::fromGuestQuery)
.toList();
List<PinResponse> pinResponses = pins.stream()
.map(PinResponse::from)
.toList();

return new AdminMemberDetailResponse(
member.getId(),
memberInfo.getNickName(),
memberInfo.getEmail(),
memberInfo.getImageUrl(),
topicResponses,
pinResponses,
member.getUpdatedAt()
);
}


}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package com.mapbefine.mapbefine.admin.dto;

import com.mapbefine.mapbefine.member.domain.Member;
import com.mapbefine.mapbefine.member.domain.MemberInfo;
import java.time.LocalDateTime;

public record AdminMemberResponse(
Long id,
String nickName,
String email,
String imageUrl,
LocalDateTime updatedAt
) {

public static AdminMemberResponse from(Member member) {
MemberInfo memberInfo = member.getMemberInfo();

return new AdminMemberResponse(
member.getId(),
memberInfo.getNickName(),
memberInfo.getEmail(),
memberInfo.getImageUrl(),
member.getUpdatedAt()
);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
package com.mapbefine.mapbefine.admin.presentation;

import com.mapbefine.mapbefine.admin.application.AdminCommandService;
import com.mapbefine.mapbefine.admin.application.AdminQueryService;
import com.mapbefine.mapbefine.admin.dto.AdminMemberDetailResponse;
import com.mapbefine.mapbefine.admin.dto.AdminMemberResponse;
import com.mapbefine.mapbefine.auth.domain.AuthMember;
import java.util.List;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.DeleteMapping;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RestController;

@RestController
@RequestMapping("/admin")
public class AdminController {

private final AdminQueryService adminQueryService;
private final AdminCommandService adminCommandService;


public AdminController(AdminQueryService adminQueryService, AdminCommandService adminCommandService) {
this.adminQueryService = adminQueryService;
this.adminCommandService = adminCommandService;
}

@GetMapping("/members")
public ResponseEntity<List<AdminMemberResponse>> findAllMembers(AuthMember authMember) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3.
AdminController에서도 AuthMember를 받아야 할까요?
AdminAuthInterceptor에서 해당 사용자가 관리자임을 확인하는 것 같은데,

이미 관리자가 호출한 게 확인이 되었다면 AuthMember를 아예 안받아도 되지는 않나요?!
서비스에서 이중검증 해주기 위함일까용?
아니면 Admin 객체를 받아도 되지 않을까요?
쥬니가 더 오래 잘 고민해주셨을 것이라 혹시 제가 생각 못한 이유가 있는지 궁금해요!!

Copy link
Collaborator Author

@cpot5620 cpot5620 Sep 14, 2023

Choose a reason for hiding this comment

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

Service에서 이중으로 검증하기 위함이었어요 !
사실 Admin 객체를 받는 것이 조금 더 자연스러울 것 같긴 하네요.
아니면, ArgumentResolver 사용해서 Member 객체를 받아도 될 것 같아요.

어떤 방법이든 Service에서 검증해줄 수는 있으니까요.

어떤 방법이 좋을까요 ?


도이뿐만 아니라, 준팍과 매튜도 동일한 내용에 대해서 코멘트 남겨주셔서 의견 덧붙입니다.

서비스가 핵심 비즈니스 로직을 담고 있는 만큼, 서비스에서의 검증은 필수적이라고 생각했어요.
Admin 역할을 가진 Member만 해당 메서드를 정상적으로 처리해야 하는데, Interceptor를 믿고 별도의 검증 로직이 없다는 건 너무 불안할 것 같아요. (물론 스프링 프레임워크를 사용하는 만큼, 이 부분은 믿고 가도 될 것 같기도 해요.)
또한, 이 상황은 Service가 Controller(Interceptor) 계층에 의존적인 상황인 것 같아요.

혹여나, 다른 Controller에서 해당 Service를 호출하게 된다면, 권한과 상관없이 관리자 기능을 수행할수도 있지 않을까요 ?

관리 포인트가 늘어나게 된다는 점에서 동일한 리뷰를 주신 것 같은데요.
매튜의 의견은 Service가 꼭 검증을 수행해야 한다면, AdminAuthInterceptor를 삭제해도 되지 않을까 ? 였어요.

저는 Interceptor가 권한 검증의 기능도 있겠지만, 잘못된 요청이 어플리케이션 자체로 넘어오지 않도록 하기 위함도 있다고 생각해요. 요청 자체를 거부해버리는 역할을 수행하는거죠.

중복 코드를 없애기 위해서, AOP를 통해 Admin관련 서비스 메서드들이 호출되기 이전에 매번 검증 메서드를 호출하도록 하는 방법이 있을 것 같아요. 다만, Admin만을 위해 AOP를 도입하는 것이 맞을까 라는 의문이 들기도 하네요.

내일 오전 강의 시작 전에(혹은 오후에) 잠깐 모여서 이야기 나누어봤으면 좋겠습니다.

List<AdminMemberResponse> responses = adminQueryService.findAllMemberDetails(authMember);

return ResponseEntity.ok(responses);
}

@DeleteMapping("/members/{memberId}")
public ResponseEntity<Void> deleteMember(AuthMember authMember, @PathVariable Long memberId) {
adminCommandService.blockMember(authMember, memberId);

return ResponseEntity.noContent().build();
}

@GetMapping("/members/{memberId}")
public ResponseEntity<AdminMemberDetailResponse> findMember(AuthMember authMember, @PathVariable Long memberId) {
AdminMemberDetailResponse response = adminQueryService.findMemberDetail(authMember, memberId);

return ResponseEntity.ok(response);
}

@DeleteMapping("/topics/{topicId}")
public ResponseEntity<Void> deleteTopic(AuthMember authMember, @PathVariable Long topicId) {
adminCommandService.deleteTopic(authMember, topicId);

return ResponseEntity.noContent().build();
}

@DeleteMapping("/topics/{topicId}/images")
public ResponseEntity<Void> deleteTopicImage(AuthMember authMember, @PathVariable Long topicId) {
adminCommandService.deleteTopicImage(authMember, topicId);

return ResponseEntity.noContent().build();
}

@DeleteMapping("/pins/{pinId}")
public ResponseEntity<Void> deletePin(AuthMember authMember, @PathVariable Long pinId) {
adminCommandService.deletePin(authMember, pinId);

return ResponseEntity.noContent().build();
}

@DeleteMapping("/pins/images/{imageId}")
Copy link
Collaborator

@yoondgu yoondgu Sep 14, 2023

Choose a reason for hiding this comment

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

P2.

deleteTopicImage와 같이 pins/{imageId}/images로 통일하는 것은 어떨까요 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pins/{imageId}/images 라는 URL은 조금 부자연스럽게 느껴지는 것 같아요.
imageId는 1:1로 매핑되는 값인데, 뒤에 /images가 추가된다는게 어색하게 느껴지네요 ㅠ_ㅠ

차라리 pinImages/{imageId}는 어떨까요 !?

Copy link
Collaborator

Choose a reason for hiding this comment

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

전 이 부분은 쥬니가 한 게 더 자연스럽다구 생각합니다.
Topics/ 구체적인 Topic/ 해당 토픽의 이미지
핀 / 핀의 이미지들 / 구체적인 이미지
이런 흐름이라고 생각해서요!

Pins 다음에 PinId가 들어가면 의미가 더 명확해지긴 하겠지만
불필요한 정보를 받을 필요도 없다고 생각하기 때문에 지금도 괜찮은 것 같네요

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.

아 이부분은 제가 생각을 잘못한 것 같아요!!! 쥬니가 하신대로 진행하는 것 동의합니다 ~

public ResponseEntity<Void> deletePinImage(AuthMember authMember, @PathVariable Long imageId) {
adminCommandService.deletePinImage(authMember, imageId);

return ResponseEntity.noContent().build();
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
import com.mapbefine.mapbefine.auth.domain.member.User;
import com.mapbefine.mapbefine.member.domain.Member;
import com.mapbefine.mapbefine.member.domain.MemberRepository;
import com.mapbefine.mapbefine.member.exception.MemberErrorCode;
import com.mapbefine.mapbefine.member.exception.MemberException.MemberNotFoundException;
import com.mapbefine.mapbefine.topic.domain.Topic;
import java.util.List;
import java.util.Objects;
Expand Down Expand Up @@ -60,4 +62,18 @@ private List<Long> getCreatedTopics(Member member) {
.toList();
}

public boolean isAdmin(Long memberId) {
if (Objects.isNull(memberId)) {
return false;
}

Member member = findMember(memberId);

return member.isAdmin();
}

private Member findMember(Long memberId) {
return memberRepository.findById(memberId)
.orElseThrow(() -> new MemberNotFoundException(MemberErrorCode.MEMBER_NOT_FOUND, memberId));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

P2.
findMember에서 예외를 던져서 404를 주는 대신 false를 반환하고,
AdminAuthInterceptor의 validateAdmin 메서드에서 던지는 예외 하나로 통일하는 게 어떨까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

차라리 validateRole 자체를 AuthService에 두는 건 어떨가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아래와 같이 수정하였습니다 !

    public boolean isAdmin(Long memberId) {
        if (Objects.isNull(memberId)) {
            return false;
        }

        Optional<Member> member = memberRepository.findById(memberId);

        return member.map(Member::isAdmin)
                .orElse(false);
    }

}
Loading