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: 운영진 관리 기능 구현 완료 #648

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

SongJaeHoonn
Copy link
Contributor

Summary

#642

효율적인 운영진 정보 관리를 위해, 운영진 CRUD를 구현했습니다.

Tasks

  • 운영진 등록 기능
  • 운영진 조회 기능
  • 운영진 수정 기능
  • 운영직 삭제 기능

ETC

  • MemberManagement 카테고리의 Executive 도메인을 새로 만들었습니다.

  • 현재 Member entity처럼, PK가 학번인 경우 그 값은 고유하고 변경되지 않기 때문에 Executive의 PK도 학번(id)으로 설정했으며, 이 값은 변경할 수 없도록 했습니다.

  • 운영진 직급을 회장,부회장과 같은 문자열로 처리하려고 했으나, 유지보수의 용이성을 위해 Enum으로 관리하도록 했습니다.

  • 정렬 순서는 PRESIDENT -> VICE_PRESIDENT -> GENERAL(학번 순) 입니다.

Screenshot

  • 등록
스크린샷 2024-12-27 오후 3 04 07
  • 조회
스크린샷 2024-12-27 오후 4 43 56
  • 수정 후 조회
스크린샷 2024-12-28 오후 4 00 04
  • 삭제 후 조회
스크린샷 2024-12-28 오후 4 26 55
  • 사진 등록
스크린샷 2024-12-27 오후 3 06 33

@SongJaeHoonn SongJaeHoonn added the ✨ Feature 새로운 기능 명세 및 개발 label Dec 28, 2024
@SongJaeHoonn SongJaeHoonn self-assigned this Dec 28, 2024
@SongJaeHoonn SongJaeHoonn linked an issue Dec 28, 2024 that may be closed by this pull request
4 tasks
Copy link
Collaborator

@limehee limehee left a comment

Choose a reason for hiding this comment

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

회장, 부회장, 운영진 등의 정보를 관리하기 위해 이전에 position 테이블을 생성한 적이 있어요. 이번에 작성된 부분과 일부 겹치는 내용이 있어, 두 도메인 중 하나를 삭제하고 통합하는 것이 더 효율적일 것 같아요.

"position은 다음과 같이 등록 가능<br>" +
"- PRESIDENT : 회장<br>" +
"- VICE_PRESIDENT : 부회장<br>" +
"- GENERAL : 일반 운영진<br>")
Copy link
Collaborator

Choose a reason for hiding this comment

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

API 문서에서 DTO의 속성을 통해 Enum 값을 추적할 수 있어, 별도로 position 정보를 기재할 필요는 없을 것 같아요.

image


@Operation(summary = "[A] 운영진 정보 삭제", description = "ROLE_ADMIN 이상의 권한이 필요함")
@PreAuthorize("hasRole('ADMIN')")
@DeleteMapping("/{id}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

해당 API와 '운영진 정보 수정' API 모두 Path 변수의 명칭을 id로 설정할 경우, 어떤 id를 의미하는지 명확하지 않을 수 있어요. 다른 API와 일관성을 유지하면서 의미를 명확히 전달하기 위해 executiveId로 변경할 것을 권장드려요.

@Override
public Executive getById(String id) {
ExecutiveJpaEntity jpaEntity = executiveRepository.findById(id)
.orElseThrow(() -> new NotFoundException("학번이 " + id + "인 운영진이 존재하지 않습니다."));
Copy link
Collaborator

Choose a reason for hiding this comment

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

로깅 메시지의 통일성을 위해 [Executive] id: " + executiveId + "에 해당하는 운영진이 존재하지 않습니다.로 수정 부탁드려요. 매개변수 명칭도 id에서 executiveId로 변경해주세요.

private String id;
private String name;
private String email;
private String field;
Copy link
Collaborator

Choose a reason for hiding this comment

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

현재 memberapplication 테이블에서 '관심 분야'를 나타내는 명칭으로 interests를 사용하고 있어요. field또는 interests 중 하나로 통일할 필요가 있어보여요.

Role.SUPER,
Set.of("boards", "profiles", "activity-photos", "membership-fees", "notices", "weekly-activities", "members",
"assignments", "submits")
"assignments", "submits", "executives")
Copy link
Collaborator

Choose a reason for hiding this comment

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

isUserAccessibleByCategory() 메서드에서 비로그인 사용자도 executives 경로에 접근할 수 있도록 허용하고 있지만, 현재 roleCategoryMap에는 운영진 이상의 권한만 허용되어 있어요. GUEST, USER 권한을 가진 사용자도 운영진 사진을 열람할 수 있도록 수정이 필요해요.

@limehee limehee requested a review from Copilot December 28, 2024 10:19
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 15 out of 30 changed files in this pull request and generated 1 comment.

Files not reviewed (15)
  • src/main/java/page/clab/api/domain/memberManagement/executive/application/service/ExecutiveRetrievalService.java: Evaluated as low risk
  • src/main/java/page/clab/api/domain/memberManagement/executive/application/service/ExecutiveRegisterService.java: Evaluated as low risk
  • src/main/java/page/clab/api/domain/memberManagement/executive/application/service/ExecutiveUpdateService.java: Evaluated as low risk
  • src/main/java/page/clab/api/domain/memberManagement/executive/adapter/in/web/ExecutiveRegisterController.java: Evaluated as low risk
  • src/main/java/page/clab/api/domain/memberManagement/executive/application/service/ExecutiveRemoveService.java: Evaluated as low risk
  • src/main/java/page/clab/api/domain/memberManagement/executive/adapter/in/web/ExecutiveRemoveController.java: Evaluated as low risk
  • src/main/java/page/clab/api/domain/memberManagement/executive/application/port/in/UpdateExecutiveUseCase.java: Evaluated as low risk
  • src/main/java/page/clab/api/domain/memberManagement/executive/application/port/in/RemoveExecutiveUseCase.java: Evaluated as low risk
  • src/main/java/page/clab/api/domain/memberManagement/executive/application/port/in/RetrieveExecutiveUseCase.java: Evaluated as low risk
  • src/main/java/page/clab/api/domain/memberManagement/executive/domain/Executive.java: Evaluated as low risk
  • src/main/java/page/clab/api/global/common/file/api/FileController.java: Evaluated as low risk
  • src/main/java/page/clab/api/domain/memberManagement/executive/application/exception/ExecutiveRegistrationException.java: Evaluated as low risk
  • src/main/java/page/clab/api/domain/memberManagement/executive/application/port/in/RegisterExecutiveUseCase.java: Evaluated as low risk
  • src/main/java/page/clab/api/domain/memberManagement/executive/application/dto/mapper/ExecutiveDtoMapper.java: Evaluated as low risk
  • src/main/java/page/clab/api/domain/memberManagement/executive/application/dto/request/ExecutiveUpdateRequestDto.java: Evaluated as low risk

private String name;

@NotNull(message = "{notNull.executive.email}")
@Schema(description = "이메일", example = "[email protected]")
Copy link
Preview

Copilot AI Dec 28, 2024

Choose a reason for hiding this comment

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

There is a typo in the email example: "[email protected]" should be "[email protected]".

Suggested change
@Schema(description = "이메일", example = "clab.coreteam@gamil.com")
@Schema(description = "이메일", example = "clab.coreteam@gmail.com")

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@SongJaeHoonn
Copy link
Contributor Author

@limehee

회장, 부회장, 운영진 등의 정보를 관리하기 위해 이전에 position 테이블을 생성한 적이 있어요. 이번에 작성된 부분과 일부 겹치는 내용이 있어, 두 도메인 중 하나를 삭제하고 통합하는 것이 더 효율적일 것 같아요.

Position 테이블이 멤버의 직책을 담당하는 것이라고 알고 있었는데, 직책 중 일반 회원, 코어팀도 포함되어 있어 운영진 정보만(회장, 부회장, 운영진) 관리하는 테이블을 하나 더 만들도록 했었습니다.

그런데 다시 보니 겹치는 부분이 상당히 많고, 직책 등록이 더욱 포괄적인 것 같아 Executive도메인을 삭제하고, Position도메인으로 운영진을 관리할 수 있는 로직을 옮기는 방식이 더 좋을 것 같습니다! 감사합니다. 작업 후 다시 리뷰 요청 드리겠습니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Feature 새로운 기능 명세 및 개발
Projects
None yet
Development

Successfully merging this pull request may close these issues.

운영진 관리 기능 구현
2 participants