[FE] [BE] FIX : 마이 프로필 조회 시 연장권 정보 추가 #1424 #1427
Conversation
…dev/fix_put_lentExtension_to_MyProfileResponseDto/#1424
' of https://github.com/innovationacademy-kr/42cabi into be/dev/fix_put_lentExtension_to_MyProfileResponseDto/#1424
' of https://github.com/innovationacademy-kr/42cabi into be/dev/fix_put_lentExtension_to_MyProfileResponseDto/#1424
' of https://github.com/innovationacademy-kr/42cabi into be/dev/fix_put_lentExtension_to_MyProfileResponseDto/#1424
' of github.com:innovationacademy-kr/42cabi into be/dev/fix_put_lentExtension_to_MyProfileResponseDto/#1424
| .collect(Collectors.toList()); | ||
| boolean isLentExtensionAvailable = !lentExtensionNotExpiredByUserId.isEmpty(); | ||
| return userMapper.toMyProfileResponseDto(user, cabinet, banHistory, isLentExtensionAvailable); | ||
| List<LentExtensionResponseDto> lentExtensionResponseDtos = getMyActiveLentExtension(user).getResult(); |
There was a problem hiding this comment.
getMyActiveLentExtension 메서드를 호출하여 LentExtensionPagiantionDto에서 getResult는 것보다,
해당 메서드 내에 result를 구하는 부분만 메서드 추출하여 처리하는 것이 좋을 것 같습니다.
There was a problem hiding this comment.
동의합니다. Facade 자체는 Controller와 직접적으로 연결되어서 사용되는 부분인데, 본인이 public으로 열어둔 메서드를 사용해서 getResult를 별도로하는 것은 해당하는 메서드에서 '불필요한 로직을 더 수행함'에 대한 부분도 있을 것 같고, 내부에서 재사용하는 메서드(getMyActiveLentExtension)가 어떻게 변경될지 모른다는 가정을 해보면 유지보수적으로 좋지 않은 방식일 것 같습니다.
별도로 Facade의 메서드에서 원하는 정보를 가져오는 방법을 하위 계층의 객체에게 명령하는 방식이 더 좋을 것 같습니다.
There was a problem hiding this comment.
반영했습니다
private 메소드로 수정했습니다.
There was a problem hiding this comment.
급해서 private 메소드로 수정 했다가, LentExtensionService에서 가져오도록 수정
| if (lentExtensionResponseDtos.isEmpty()) | ||
| return userMapper.toMyProfileResponseDto(user, cabinet, banHistory, null); | ||
| return userMapper.toMyProfileResponseDto(user, cabinet, banHistory, lentExtensionResponseDtos.get(0)); |
There was a problem hiding this comment.
추후 연장권의 종류가 다양해지고 동시에 여러 연장권을 가지는 상황을 고려해서 저 조회 로직을 List로 작성했는데,
마이 페이지에서 단 하나의 연장권만 보여주는 것이라면 Optional로 하나만 조회해오는 쿼리를 새로 작성하고
optionalFetcher에서 get을 통해 없다면 null, 있다면 값을 리턴하도록 하는건 어떻습니까
There was a problem hiding this comment.
하나만 보여주는 방식이라면 jpark2님의 말씀에 동의합니다.
Z1Park
requested review from
enaenen,
leedonggyu1848,
eunbi9n,
sichoi42 and
Ssuamje
Z1Park
added
COMMON
| @Query("SELECT c.cabinetId " + | ||
| "FROM Cabinet c " + | ||
| "WHERE c.status = org.ftclub.cabinet.cabinet.domain.CabinetStatus.PENDING AND c.cabinetPlace.location.floor = :floor") | ||
| Optional<List<Long>> findPendingCabinets(@Param("floor") Integer floor); | ||
|
|
||
| @Query("SELECT c.cabinetId " + | ||
| "FROM Cabinet c " + | ||
| "WHERE c.status = org.ftclub.cabinet.cabinet.domain.CabinetStatus.AVAILABLE AND c.cabinetPlace.location.floor = :floor") |
There was a problem hiding this comment.
DBMS 자체에서는 해당하는 컬럼을 varchar(문자열)로 두어서 사용하고 있을거여서, JPQL을 직접 이용한 내부 상수로 작성하는 것보다 <해당 컬럼> = '해당컬럼의 문자열'과 같은 방식으로 사용하는게 더 나을 것 같다는 생각이 드네요(현재 방식은 패키지에 변경이 생겨도 영향이 있습니다). 근데 이런식으로하면 해당하는 상수의 문자열 값이 변경될 때 터지게 되는데, 이 부분은 어플리케이션에서 변화가 생기면 DB와 정합성을 맞추기 위한 어쩔 수 없는 과정이라고 생각이 드네요.
한편, 별도의 상태를 정해서 find하는 메서드보다 해당하는 상수(사물함 상태)를 지정해서 좀 더 제너럴하게 받아올 수 있게 작성해주시면 좋을 것 같습니다.
| @Override | ||
| @Transactional | ||
| public CabinetPendingResponseDto getPendingCabinets() { | ||
| log.debug("getPendingCabinets"); | ||
| List<List<CabinetPreviewDto>> cabinetPreviewDtos = new ArrayList<>(); | ||
| for (int i = 2; i <= 5; i++) { | ||
| List<CabinetPreviewDto> cabinetPreviewDtoList = new ArrayList<>(); | ||
| // pending 상태인 사물함들의 cabinetId를 가져온다. | ||
| List<Long> pendingCabinetsIdByFloor = cabinetOptionalFetcher.findPendingCabinets(i); | ||
| // 순회를 돌면서 cabinetPreviewDto를 가져온다. | ||
| for (Long pendingCabinetId : pendingCabinetsIdByFloor) { | ||
| cabinetPreviewDtoList.add(cabinetMapper.toCabinetPreviewDto(cabinetOptionalFetcher.findCabinet(pendingCabinetId), | ||
| 0, "")); | ||
| } | ||
| // available 상태인 사물함들의 cabinetId를 가져온다. | ||
| List<Long> availableCabinetsIdByFloor = cabinetOptionalFetcher.findAvailableCabinets(i); | ||
| for (Long availableCabinetId : availableCabinetsIdByFloor) { | ||
| cabinetPreviewDtoList.add(cabinetMapper.toCabinetPreviewDto(cabinetOptionalFetcher.findCabinet(availableCabinetId), | ||
| 0, "")); | ||
| } | ||
| cabinetPreviewDtos.add(cabinetPreviewDtoList); | ||
| } | ||
|
|
||
| return new CabinetPendingResponseDto(cabinetPreviewDtos); | ||
| } |
There was a problem hiding this comment.
이 부분은 jpark2님이 올리신 getPendingCabinets PR과 별개인가요?
| lentExtensionOptionalFetcher.findLentExtensionByUserId(userId) | ||
| .stream() | ||
| .filter(lentExtension -> | ||
| lentExtension.getExpiredAt().isBefore(LocalDateTime.now()) | ||
| lentExtension.getExpiredAt().isAfter(LocalDateTime.now()) | ||
| && lentExtension.getUsedAt() == null) | ||
| .collect(Collectors.toList()); | ||
| if (findLentExtension.isEmpty()) { |
There was a problem hiding this comment.
위에서 작성된 LentExtensionResponseDto(연장권 자체에 대한 정보)를 조회하는 로직으로 변경되었는데,
useLentExtension 메서드 자체의 시그니처를 보면 프론트단에서 '어떠한 연장권을 사용하겠다'라는 식의 매개변수는 보이지 않고, 직접적으로 백엔드 단에서 임의적으로 가장 최근의 것을 사용하는 것 처럼 보이는데요, 이 부분 또한 로직이 반영되어야 할 것 같습니다.
| .collect(Collectors.toList()); | ||
| boolean isLentExtensionAvailable = !lentExtensionNotExpiredByUserId.isEmpty(); | ||
| return userMapper.toMyProfileResponseDto(user, cabinet, banHistory, isLentExtensionAvailable); | ||
| List<LentExtensionResponseDto> lentExtensionResponseDtos = getMyActiveLentExtension(user).getResult(); |
There was a problem hiding this comment.
동의합니다. Facade 자체는 Controller와 직접적으로 연결되어서 사용되는 부분인데, 본인이 public으로 열어둔 메서드를 사용해서 getResult를 별도로하는 것은 해당하는 메서드에서 '불필요한 로직을 더 수행함'에 대한 부분도 있을 것 같고, 내부에서 재사용하는 메서드(getMyActiveLentExtension)가 어떻게 변경될지 모른다는 가정을 해보면 유지보수적으로 좋지 않은 방식일 것 같습니다.
별도로 Facade의 메서드에서 원하는 정보를 가져오는 방법을 하위 계층의 객체에게 명령하는 방식이 더 좋을 것 같습니다.
| if (lentExtensionResponseDtos.isEmpty()) | ||
| return userMapper.toMyProfileResponseDto(user, cabinet, banHistory, null); | ||
| return userMapper.toMyProfileResponseDto(user, cabinet, banHistory, lentExtensionResponseDtos.get(0)); |
There was a problem hiding this comment.
하나만 보여주는 방식이라면 jpark2님의 말씀에 동의합니다.
…dev/fix_put_lentExtension_to_MyProfileResponseDto/#1424 # Conflicts: # backend/src/main/java/org/ftclub/cabinet/cabinet/service/CabinetFacadeServiceImpl.java
getActiveLentExtensionList 메소드, LentExtensionService 쪽으로 수정
getMyProfile 에서, LentExtension 하나만 가져오도록 메소드 수정
해당 사항 (중복 선택)
설명
마이 프로필 조회 시 연장권 정보를 추가적으로 전달
https://github.com/innovationacademy-kr/42cabi/issues/#1424