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: 시간표 프레임 한번에 조회 구현 #1108

Merged
merged 5 commits into from
Dec 4, 2024

Conversation

seongjae6751
Copy link
Contributor

@seongjae6751 seongjae6751 commented Dec 2, 2024

🔥 연관 이슈

🚀 작업 내용

  1. 시간표 프레임을 한번에 조회 할 수 있게 구조를 변경했습니다.
    requestparam인 semester가 없을때는 아래와 같은 값을 반환하게 했습니다.
{
  "semesters": {
    "20242": [
      {
        "id": 1,
        "timetable_name": "시간표1",
        "is_main": false
      },
      {
        "id": 2,
        "timetable_name": "시간표2",
        "is_main": true
      },
      {
        "id": 3,
        "timetable_name": "시간표3",
        "is_main": false
      }
    ],
    "20241": [
      {
        "id": 4,
        "timetable_name": "시간표1",
        "is_main": false
      },
      {
        "id": 5,
        "timetable_name": "시간표2",
        "is_main": true
      },
      {
        "id": 6,
        "timetable_name": "시간표3",
        "is_main": false
      }
    ]
  }
}

💬 리뷰 중점사항

Copy link

github-actions bot commented Dec 2, 2024

Unit Test Results

342 tests   341 ✔️  1m 27s ⏱️
  41 suites      1 💤
  41 files        0

Results for commit 7f50bc9.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@Soundbar91 Soundbar91 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다.
하나의 API에서 값에 따라서 반환값이 달라져서, 스웨거에 추가 설명이 필요할 것 같아요..!

코멘트 확인 부탁드립니다.

@@ -77,7 +77,7 @@ ResponseEntity<TimetableFrameUpdateResponse> updateTimetableFrame(
@Operation(summary = "시간표 프레임 조회")
@SecurityRequirement(name = "Jwt Authentication")
@GetMapping("/v2/timetables/frame")
Copy link
Contributor

Choose a reason for hiding this comment

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

R

이거 왜 인지 모르겠는데, 컨트롤러랑 API Path가 다르네요..? 뭐지뭐지

Copy link
Contributor

Choose a reason for hiding this comment

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

이미 클라에서 적용이 됐을 것 같은데 /v2/timetables/frame은 기존 방식, /v2/timetables/frames는 새로운 방식으로 나눠도 좋을 것 같아요. 대응이 필요한,,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

스웨거에는 기존꺼도 /v2/timetables/frames로 올라가있네요!
저는 그래서 그냥 지금처럼 해도 되지 않을까 하는데 어떻게 생각하시나요

Copy link
Contributor

Choose a reason for hiding this comment

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

좋습니당

@@ -77,7 +77,7 @@ ResponseEntity<TimetableFrameUpdateResponse> updateTimetableFrame(
@Operation(summary = "시간표 프레임 조회")
@SecurityRequirement(name = "Jwt Authentication")
@GetMapping("/v2/timetables/frame")
ResponseEntity<List<TimetableFrameResponse>> getTimetablesFrame(
ResponseEntity<Object> getTimetablesFrame(
@RequestParam(name = "semester") String semester,
Copy link
Contributor

Choose a reason for hiding this comment

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

C

여기에도 required 옵션 줘야 하나용 ??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오 꼼꼼하시네요bb

Semester semester = semesterRepositoryV2.getBySemester(semesterRequest);
return timetableFrameRepositoryV2.findAllByUserIdAndSemesterId(userId, semester.getId()).stream()
.map(TimetableFrameResponse::from)
.toList();
}

@Transactional(readOnly = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

C

클래스에 readOnly 트랜잭션이 걸려있는데, 여기에 추가하신 이유가 궁금합니다 !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

지우겠습니다!

Collectors.mapping(TimetableFrameResponse::from, Collectors.toList())
));

return Map.of("semesters", groupedBySemester);
Copy link
Contributor

Choose a reason for hiding this comment

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

C

dto 클래스로 반환할 수 있을 것 같아요.
리펙토링 할 때 반영하면 좋을 것 같아요 !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

그냥 생각없이 만들어서 바로 올리고 씻으면서 dto로 변환해야겠다 바로 생각이 들었었습니다..ㅎㅎ
짚어주셔서 감사합니당 수정하겠습니다!

Copy link
Contributor

@kwoo28 kwoo28 left a comment

Choose a reason for hiding this comment

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

꼼꼼하게 잘 작성하신것같습니다. 수고 많으셨습니다!?

@seongjae6751 seongjae6751 merged commit 598ea50 into develop Dec 4, 2024
4 checks passed
@seongjae6751 seongjae6751 deleted the feat/1106-get-all-timetable-frame branch December 4, 2024 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

시간표 전체 프레임 한번에 조회 구현
3 participants