[COT-17]Feature: 활동중인 부원 출결 정보 엑셀로 추출 #179

yunhacandy · 2024-10-13T13:35:07Z

✅ 작업 내용

세션 id를 넣으면 세션에 맞는 출결 정보를 엑셀로 추출
출결 정보를 입력하지 않은 경우에 대해서는 일괄 결석 처리

주석은 마지막에 지우겠습니다

Youthhing

리뷰가 조금 많은데 의견 남기거나 반영 부탁드립니다!

개인적으로 결과를 미리 세팅해놓고 이를 분리한 메서드의 파라미터로 넘기는 구조보단, 여러 곳에서 호출이 되지 않는다면

public <반환형> get ~~ , create ~~ (){
//..
}

이런 구조가 조금 메서드 이름과 어울릴 것 같습니다

Youthhing · 2024-10-14T09:28:59Z

src/main/java/org/cotato/csquiz/domain/attendance/service/AttendanceAdminService.java

@@ -95,4 +118,165 @@ public List<AttendanceRecordResponse> findAttendanceRecordsByAttendance(Long att

        return attendanceRecordService.generateAttendanceResponses(List.of(attendance));
    }
+
+    public ResponseEntity<byte[]> exportAttendanceRecordsToExcelBySessions(List<Long> sessionIds) {


Service 계층에도 ResponseEntity를 반환하는 이유가 있을까요?

엑셀 파일 다운로드 로직이 service 계층에서 처리되기 위해 코드를 작성했었습니다.
그러나 코드 리뷰를 바탕으로 고민한 결과 ResponseEntity는 service에서 반환하는 것이 적절하지 않다고 결론을 내리게 되어서 수정했습니다.

네네, 계층간 DTO 의존성을 고려하면 서비스 계층에 웹 계층 Dto를 필요 이상으로 끌고 올 필요는 없는 것 같네요~

Youthhing · 2024-10-14T09:29:39Z

src/main/java/org/cotato/csquiz/domain/attendance/service/AttendanceAdminService.java

+
+            // 파일 다운로드 설정
+            HttpHeaders headers = new HttpHeaders();
+            headers.add(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=attendance_summary.xlsx");


파일명은 항상 고정되는건가요?

피그마에 있는 디자인에서 사용자가 원하는대로 파일명을 정하는 화면이 없어서 일단 고정되게 했었습니다.
확장성을 생각해서 사용자가 원하는 파일명을 입력할 수 있게 하는게 좋을거 같아서 같이 수정하겠습니다 (다른 파트에게 이 부분에 대해 요청해야 할거 같습니다)

화면을 추가하기보단 서버에서 존재하는 데이터를 기반으로 나름의 기준을 가지고 이름을 정하면 좋을 것 같아요.

기수와 포함된 출석 날짜 또는 세션을 기준으로 잡으면 좋을 것 같아요
가령, 출결기록_9기_1,2,3주차 출석.xlsx와 같은 형식으로요

Youthhing · 2024-10-14T09:30:22Z

src/main/java/org/cotato/csquiz/domain/attendance/service/AttendanceAdminService.java

+
+            // 세션별 출석 데이터를 저장할 구조체
+            Map<String, Map<String, String>> memberStatisticsMap = new HashMap<>();
+            LinkedHashMap<String, String> sessionColumnNames = new LinkedHashMap<>();


이 부분만 LinkedHashMap으로 구현한 이유가 궁금합니다.

Youthhing · 2024-10-14T10:04:31Z

src/main/java/org/cotato/csquiz/api/attendance/controller/AttendanceController.java

+    @Operation(summary = "세션별 출석 기록 엑셀 다운로드 API")
+    @GetMapping("/excel")
+    public ResponseEntity<byte[]> downloadAttendanceRecordsAsExcelBySessions(
+            @RequestParam(name = "sessionIds") List<Long> sessionIds) {
+        return attendanceAdminService.exportAttendanceRecordsToExcelBySessions(sessionIds);
+    }


추가로, '출결'에 대한 '기록'들을 엑셀로 가져오는 것이니 클라이언트에서 넘어오는 값을 출결id로 정하고 API를 구상하는게 더 합리적인 것 같습니다!

오,, 근데 RequestParam으로 List가 넘어 오는군요 .. ?

Youthhing · 2024-10-14T10:00:57Z

src/main/java/org/cotato/csquiz/common/error/ErrorCode.java

@@ -81,6 +81,7 @@ public enum ErrorCode {
    //출석 관련 AT
    OFFLINE_ATTEND_FAIL(HttpStatus.BAD_REQUEST, "AT-101", "거리 부적합으로 인한 대면 출석 실패"),
    INVALID_ATTEND_TIME(HttpStatus.BAD_REQUEST, "AT-102", "시간 입력 범위가 잘못되었습니다."),
+    FILE_GENERATION_FAIL(HttpStatus.INTERNAL_SERVER_ERROR, "AT-201", "엑셀 파일 생성에 실패했습니다."),


Internal Server Error는 아래 S 태그 관련 에러로 옮겨주세요~

Youthhing · 2024-10-14T10:06:02Z

src/main/java/org/cotato/csquiz/domain/attendance/service/AttendanceAdminService.java

+    private void initializeMemberAttendance(Map<String, Map<String, String>> memberStatisticsMap, String columnName) {
+        List<String> allMemberNames = memberService.findActiveMember().stream()
+                .map(Member::getName)
+                .toList();
+
+        for (String memberName : allMemberNames) {
+            memberStatisticsMap
+                    .computeIfAbsent(memberName, k -> new HashMap<>())
+                    .put(columnName, "결석");
+        }
+    }


'결석'으로 초기화하는 이유가 있을까요?

추가로 Map의 Key를 Member객체 또는 멤버를 구분할 수 있는 id로 한게 아닌 이름으로 짠 이유가 있을까요?

map의 키는 중복이 안되는데, 이 경우에 동명이인 이슈가 고려되지 않은 것 같습니다.

코드를 짜는 과정에서 '결석'으로 전체 초기화를 하지 않았을때 출석 기록이 없어서 행의 일부가 땡겨지는 경우가 있었습니다.
이에 전체 초기화를 하게 되었는데 현재 방식 대신 출석 기록이 없는 경우에만 ‘결석’ 상태로 처리하는 방식이 더 효율적이라고 계산되어서 수정했습니다.

두번째 리뷰에 대해서는 동명이인 이슈를 고려하지 않아서 수정했습니다.

Youthhing · 2024-10-14T10:10:15Z

src/main/java/org/cotato/csquiz/domain/attendance/service/AttendanceAdminService.java

+        int colNum = 1;
+
+        for (String columnName : sessionColumnNames.keySet()) {
+            headerRow.createCell(colNum++).setCellValue(columnName);
+        }
+
+        headerRow.createCell(colNum++).setCellValue("출석");
+        headerRow.createCell(colNum++).setCellValue("대면");
+        headerRow.createCell(colNum++).setCellValue("비대면");
+        headerRow.createCell(colNum++).setCellValue("지각");
+        headerRow.createCell(colNum++).setCellValue("결석");


"출석", "대면"같은 값은 매직넘버 사용하지 말고 상수화 또는 이미 정해져있는 ENUM 필드 활용 권장드립니다~

@Youthhing 데이터가 아닌 header row 인데도 상수를 사용하면 안되나요?

지금 사용하고 있는게 매직넘버입니다! "출석" 과 같은 문자열이 파라미터에 그대로 들어가는 경우
매직넘버 사용을 지양하는 이유는 String이나 int와 같은 값은 컴파일 단계에서 걸리지 않는데,
해당 Value를 변경해야할 때 사용 위치를 다 찾아서 변경하기가 힘들고 컴파일 단계에서 알 수 없다는 단점이 있기 때문에 매직 넘버 대신 상수를 사용하는 것을 권장한다는 이야기 입니다.

매직넘버에 대한 이해가 부족했었습니다.
리뷰를 바탕으로 수정했습니다.
부원 이름에 대해서도 우선은 MemberRoleGroup.ACTIVE_MEMBERS.getDescription()을 사용했습니다.

Youthhing · 2024-10-14T10:13:11Z

src/main/java/org/cotato/csquiz/domain/attendance/service/AttendanceAdminService.java

+    // 실제 출석 기록을 업데이트하는 메소드
+    private void updateAttendanceRecords(Long sessionId, Map<String, Map<String, String>> memberStatisticsMap, String columnName) {
+        List<Attendance> attendances = attendanceRepository.findAllBySessionIdsInQuery(List.of(sessionId));
+        List<AttendanceRecordResponse> attendanceRecords = attendanceRecordService.generateAttendanceResponses(attendances);
+
+        for (AttendanceRecordResponse record : attendanceRecords) {
+            String memberName = record.memberInfo().name();
+            String attendanceStatus = determineAttendanceStatus(record.statistic());
+            memberStatisticsMap.get(memberName).put(columnName, attendanceStatus);
+        }
+    }


우선, 위에 initialize~메서드에서 activeMembers를 호출하는 쿼리를 날리는데 generateAttendanceResponses()를 호출해 내부적으로 Member 목록을 2번 쿼리하는것이 어색합니다.

그리고 약간? 희박한 경우지만 아래와 같은 경우에 동시성 이슈가 발생해 map.get()을 했을 때 NPE 발생할 수 있습니다.

Map을 초기화하는 당시에 활동 부원

generateAttendanceResponse에서 호출된 활동 중인 부원

두 시점에서의 활동 부원이 다를 가능성이 존재합니다.

Youthhing · 2024-10-14T14:39:12Z

src/main/java/org/cotato/csquiz/domain/attendance/service/AttendanceAdminService.java

+    private void collectAttendanceRecords(List<Long> sessionIds, Map<String, Map<String, String>> memberStatisticsMap,
+                                          LinkedHashMap<String, String> sessionColumnNames) {
+        for (Long sessionId : sessionIds) {
+            Session session = sessionRepository.findById(sessionId)
+                    .orElseThrow(() -> new EntityNotFoundException("세션을 찾을 수 없습니다: " + sessionId));
+
+            // 세션 이름을 생성하고 열 이름에 추가
+            String sessionDate = session.getSessionDateTime().format(DateTimeFormatter.ofPattern("yyyy-MM-dd"));
+            String columnName = session.getNumber() + "주차 세션 (" + sessionDate + ")";
+            sessionColumnNames.put(columnName, sessionDate);
+
+            // 회원들의 출석 상태를 '결석'으로 초기화하고, 출석 기록을 업데이트
+            initializeMemberAttendance(memberStatisticsMap, columnName);
+            updateAttendanceRecords(sessionId, memberStatisticsMap, columnName);
+        }
+    }


약간 개인적인 생각인데 분리한 메서드안의 작업이 for문만 있다면, for문 내부에 대한 메서드 분리가 조금 더 의미 있어 보입니다!

추가로, 이렇게 세션을 호출하면 선택된 모든 세션에 대해서 N개의 쿼리가 나가게 되지 않나요?

gikhoon

1차 리뷰 완료했습니다!

gikhoon · 2024-10-14T15:42:42Z

src/main/java/org/cotato/csquiz/domain/attendance/service/AttendanceAdminService.java

+            LinkedHashMap<String, String> sessionColumnNames = new LinkedHashMap<>();
+
+            // 모든 세션에 대한 출석 정보 수집
+            collectAttendanceRecords(sessionIds, memberStatisticsMap, sessionColumnNames);


한 메소드에 너무 많은 내용이 들어 있는 것 같습니다.

sessionId를 기반으로 session 목록 가져오기

session 목록를 하나씩 돌면서 column 명 셋팅 하는 메소드

결석으로 초기화 메소드

출결 결과를 기반으로 수정하는 메소드

로 나눠서 작성하면 더 단일 책임 원칙에 맞는 코드가 될 것 같습니다

gikhoon · 2024-10-14T15:44:13Z

src/main/java/org/cotato/csquiz/domain/attendance/service/AttendanceAdminService.java

+
+    // 회원의 출석 상태를 초기화하는 메소드
+    private void initializeMemberAttendance(Map<String, Map<String, String>> memberStatisticsMap, String columnName) {
+        List<String> allMemberNames = memberService.findActiveMember().stream()


한 엑셀에서 활동 중인 멤버는 모든 세션에서 동일하지 않을까요?

해당 쿼리는 각 세션마다 나갈 필요 없이 최초에 한번만 하면 된다고 생각합니다.

gikhoon · 2024-10-14T15:54:10Z

src/main/java/org/cotato/csquiz/domain/attendance/service/AttendanceAdminService.java

+
+    // 실제 출석 기록을 업데이트하는 메소드
+    private void updateAttendanceRecords(Long sessionId, Map<String, Map<String, String>> memberStatisticsMap, String columnName) {
+        List<Attendance> attendances = attendanceRepository.findAllBySessionIdsInQuery(List.of(sessionId));


sessionId가 하나인데 InQuery로 한 이유가 있을까요?

gikhoon · 2024-10-14T16:01:16Z

src/main/java/org/cotato/csquiz/domain/attendance/service/AttendanceAdminService.java

+        headerRow.createCell(colNum++).setCellValue("대면");
+        headerRow.createCell(colNum++).setCellValue("비대면");
+        headerRow.createCell(colNum++).setCellValue("지각");
+        headerRow.createCell(colNum++).setCellValue("결석");


마지막 줄은 colNum 값을 1 증가 할 필요 없을 것 같습니다.

gikhoon · 2024-10-14T16:02:19Z

src/main/java/org/cotato/csquiz/domain/attendance/service/AttendanceAdminService.java

+        row.createCell(colNum++).setCellValue(totalOffline);
+        row.createCell(colNum++).setCellValue(totalOnline);
+        row.createCell(colNum++).setCellValue(totalLate);
+        row.createCell(colNum++).setCellValue(totalAbsent);


마지막 줄은 colNum 값을 1 증가 할 필요 없을 것 같습니다.

…er 수정

- Excel 파일 다운로드 API에서 기본 파일명을 동적으로 설정할 수 있도록 fileName 파라미터 추가 - 한국어 파일명 지원을 위해 파일명 인코딩 처리 로직 추가

…e-excel' into feature/COT-17-extract-attendance-excel # Conflicts: # src/main/java/org/cotato/csquiz/domain/attendance/service/AttendanceAdminService.java

yunhacandy · 2024-10-16T09:15:06Z

시간이 없어서 1차로 리팩토링 했고 내일 더 하겠습니다..!!
리뷰는 지금 말고 내일 이후로 해주시면 감사하겠습니다:)

Youthhing · 2024-10-16T15:02:04Z

src/main/java/org/cotato/csquiz/domain/attendance/service/AttendanceAdminService.java

+    public String getEncodedFileName(String fileName) {
+        try {
+            String encodedFileName = URLEncoder.encode(fileName, StandardCharsets.UTF_8).replaceAll("\\+", "%20");
+            return encodedFileName + ".xlsx";
+        } catch (Exception e) {
+            throw new AppException(ErrorCode.FILE_NAME_ENCODING_FAIL);
+        }
+    }


파일과 관련된 내용은 별도의 유틸 클래스에 작성하는게 나을 것 같습니다.
이와 관련해서 엑셀 파일 제작과 관련된 내용을 별도로 분리하는걸 고려해보는건 어떨까요?

추가로 AbstaractXlsxView와 관련된 개념을 한번 공부해보면 좋을 것 같습니다

그런데 Encode하는 과정에서 Throw할 수 있는 에러가 뭐죠?

yunhacandy added 10 commits October 13, 2024 18:23

chore: 엑셀 추출을 위한 의존성 추가

bcffe18

feat: 출석 정보 엑셀 추출 구현

47774f8

style: 불필요한 세미콜 삭제 및 코드 컨벤션에 맞춰서 스타일 수정

2160432

refactor: 코드 가독성 향상을 위한 메서드 분리

f640b92

refactor: 기존에 정의된 enum (AttendanceType, AttendanceResult)을 활용하여 출석 상태 처리

c894c90

chore: api path 수정 및 securityConfig에 추가

a672cc8

style: 불필요한 세미콜론 삭제 및 코드 컨벤션에 맞춰서 스타일 수정

ce4e3ca

refactor: 코드 가독성 향상을 위한 메서드 분리

c78a67c

refactor: 기존에 정의된 enum (AttendanceType, AttendanceResult)을 활용하여 출석 상태 처리

61c482e

chore: api path 수정 및 securityConfig에 추가

6777ec5

yunhacandy requested review from Youthhing and gikhoon and removed request for Youthhing October 13, 2024 13:37

gikhoon assigned yunhacandy Oct 13, 2024

Youthhing reviewed Oct 14, 2024

View reviewed changes

gikhoon reviewed Oct 14, 2024

View reviewed changes

yunhacandy added 13 commits October 16, 2024 13:40

refactor: Service 단의 ResponseEntity(byte[]) 반환을 byte[]로 변경 및 Controll…

b12b00a

…er 수정

feat: 파일 다운로드 시 동적 파일명 지정 기능 추가

e086873

- Excel 파일 다운로드 API에서 기본 파일명을 동적으로 설정할 수 있도록 fileName 파라미터 추가 - 한국어 파일명 지원을 위해 파일명 인코딩 처리 로직 추가

refactor: URLEncoder 인코딩 방식 StandardCharsets.UTF_8로 수정

973cae6

refactor: 불필요한 증가 방지

febc601

refactor: 최초에 한번만 활동중인 멤버를 가져오게 수정

aadeb6b

refactor: 매직넘버 대신 기존 enum 필드 활용

5d26b91

style: 코드 컨벤션에 형식 맞추기 및 기본 파일 이름 변경

059f06d

refactor: '부원 이름' 매직넘버 대신 기존 enum 필드 활용

e2b143c

refactor: 확장성을 위해 sheet 이름 따로 설정하지 않게 수정

9545275

style: controller 반환 형식 수정

26f7e35

refactor: 출석 기록을 먼저 가져온 이후에 '결석'으로 채우는 걸로 로직 변경

f66d364

refactor: 동명이인 이슈를 고려해서 member id를 가지고 매핑

39d69bf

Merge remote-tracking branch 'origin/feature/COT-17-extract-attendanc…

dede143

…e-excel' into feature/COT-17-extract-attendance-excel # Conflicts: # src/main/java/org/cotato/csquiz/domain/attendance/service/AttendanceAdminService.java

Youthhing reviewed Oct 16, 2024

View reviewed changes

refactor: 세션에 맞게 자동으로 엑셀 이름 추출되어서 다운되도록 수정

58871c7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[COT-17]Feature: 활동중인 부원 출결 정보 엑셀로 추출 #179

[COT-17]Feature: 활동중인 부원 출결 정보 엑셀로 추출 #179

yunhacandy commented Oct 13, 2024

Youthhing left a comment

Youthhing Oct 14, 2024

yunhacandy Oct 16, 2024

Youthhing Oct 17, 2024

Youthhing Oct 14, 2024

yunhacandy Oct 16, 2024

Youthhing Oct 16, 2024

Youthhing Oct 14, 2024

Youthhing Oct 14, 2024

Youthhing Oct 14, 2024

Youthhing Oct 14, 2024

Youthhing Oct 14, 2024

Youthhing Oct 14, 2024

yunhacandy Oct 16, 2024

Youthhing Oct 14, 2024

yunhacandy Oct 14, 2024 •

edited

Loading

Youthhing Oct 14, 2024

yunhacandy Oct 16, 2024

Youthhing Oct 14, 2024

Youthhing Oct 14, 2024

Youthhing Oct 14, 2024

Youthhing Oct 14, 2024

gikhoon left a comment

gikhoon Oct 14, 2024

gikhoon Oct 14, 2024

gikhoon Oct 14, 2024

gikhoon Oct 14, 2024

gikhoon Oct 14, 2024

yunhacandy commented Oct 16, 2024

Youthhing Oct 16, 2024

Youthhing Oct 16, 2024

[COT-17]Feature: 활동중인 부원 출결 정보 엑셀로 추출 #179

Are you sure you want to change the base?

[COT-17]Feature: 활동중인 부원 출결 정보 엑셀로 추출 #179

Conversation

yunhacandy commented Oct 13, 2024

✅ 작업 내용

주석은 마지막에 지우겠습니다

Youthhing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yunhacandy Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gikhoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yunhacandy commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yunhacandy Oct 14, 2024 •

edited

Loading