-
Notifications
You must be signed in to change notification settings - Fork 1
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
[DPMBE-58] 약속 생성, 삭제, 수정 비즈니스 로직을 작성한다 #72
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전체적으로 비지니스 로직은 usecase보다 service에 있는게 좋아보이는데 어떤가용?
service 레이어에 메서드 만들어져있는데 사용안되구있네용
@Transactional | ||
fun createPromise(promiseRequest: PromiseRequest): PromiseDto { | ||
val promise = promiseAdaptor.save( | ||
Promise( | ||
title = promiseRequest.title, | ||
mainUserId = promiseRequest.mainUserId, | ||
meetPlace = promiseRequest.meetPlace, | ||
endTime = promiseRequest.endTime, | ||
), | ||
) | ||
return PromiseDto.from(promise) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
request 요청으로 들어온 mainUserId가 존재하는지 검증이 필요없을까요??
@Transactional | ||
fun updatePromiseMeetPlace(promiseId: Long, meetPlace: PlaceVo): PromiseDto { | ||
val promise = promiseAdaptor.queryPromise(promiseId) | ||
promise.updateMeetPlace(meetPlace) | ||
return PromiseDto.from(promise) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
main user만 업데이트가 가능하다는 정책이 없었나용?
아무나 수정 가능한지가 궁금
@Component | ||
class PromiseUserRegisterHandler() | ||
class PromiseUserRegisterEventHandler() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
핸들러 구현 해주세용
// 1. 약속 장소 변경 | ||
@Operation(summary = "약속(promise) 장소 수정", description = "약속 장소를 변경합니다.") | ||
@PutMapping("/promises/{promise-id}/location") | ||
fun updatePromiseLocation(@RequestParam(value = "promise-id") promiseId: Long, @RequestBody meetPlace: PlaceVo): PromiseDto { | ||
return promiseRegisterUseCase.updatePromiseMeetPlace(promiseId, meetPlace) | ||
} | ||
|
||
@Operation(summary = "약속(promise)시간 수정", description = "약속을 수정합니다. (약속 제목, 약속 장소, 약속 시간)") | ||
@PutMapping("/promises/{promise-id}/end-times/{end-time}") | ||
fun updatePromiseEndTime(@RequestParam(value = "promise-id") promiseId: Long, @RequestParam(value = "end-time") endTime: LocalDateTime): PromiseDto { | ||
return promiseRegisterUseCase.updatePromiseEndTime(promiseId, endTime) | ||
} | ||
|
||
// 나의 약속 전부 조회 | ||
@Operation(summary = "나의 약속 전부 조회", description = "유저의 약속 전부 조회 (단, 예정된 약속과 지난 약속을 구분해서 조회") | ||
@GetMapping("/promises/users/{user-id}/") | ||
fun findByPromiseByUser(@RequestParam(value = "user-id") userId: Long): List<PromiseSplitedByPromiseTypeDto> { | ||
return promiseReadUseCase.findPromiseByUserIdSeparatedType(userId) | ||
} | ||
|
||
@Operation(summary = "월단위 약속 조회", description = "유저의 월간 약속 조회 (단, 예정된 약속과 지난 약속을 구분없이 조회)") | ||
@GetMapping("/promises/users/{user-id}/year-month/{year-month}") | ||
fun findByPromiseByUserAndYearMonth(@RequestParam(value = "user-id") userId: Long, @RequestParam(value = "year-month") yearMonth: String): List<PromiseFindDto> { | ||
return promiseReadUseCase.findPromiseByUserIdYearMonth(userId, yearMonth) | ||
} | ||
|
||
// 약속 취소 | ||
@Operation(summary = "약속 취소", description = "약속을 취소합니다.") | ||
@DeleteMapping("/promises/{promise-id}/user-id/{user-id}") | ||
fun deletePromise(@RequestParam(value = "promise-id") promiseId: Long, @RequestParam(value = "user-id") userId: Long) { | ||
promiseRegisterUseCase.deletePromise(promiseId, userId) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PathVariable로 변경!
if (userId == promise.mainUserId) { | ||
promise.deletePromise() | ||
} else { | ||
throw NotHostException() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw NotHostException() | |
throw NotHostException.EXCEPTION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UseCase에있는 커맨드 관련 로직은
도메인 레이어로 내릴 수 있음 좋을 것 같아요
약속 읽어드릴때
약속 - 약속 유저 - 유저
이렇게 nested 된 for문일경우
루프 돌련서 쿼리 나가면 거의 n+1급으로 느려지니
여유되실때 in 쿼리 활용하셔서 아이디 묶음 한방에 쿼리해서가져오시는거
추천드려요!
- 스트림쓰면 좀더 이뻐보이겠다 ㅎㅎ
} | ||
|
||
// 나의 약속 전부 조회 | ||
@Operation(summary = "나의 약속 전부 조회", description = "유저의 약속 전부 조회 (단, 예정된 약속과 지난 약속을 구분해서 조회") | ||
@GetMapping("/promises/users/{user-id}/") | ||
fun findByPromiseByUser(@RequestParam(value = "user-id") userId: Long): List<PromiseSplitedByPromiseTypeDto> { | ||
return promiseReadUseCase.findPromiseByUserIdSeparatedType(userId) | ||
} | ||
|
||
@Operation(summary = "월단위 약속 조회", description = "유저의 월간 약속 조회 (단, 예정된 약속과 지난 약속을 구분없이 조회)") | ||
@GetMapping("/promises/users/{user-id}/year-month/{year-month}") | ||
fun findByPromiseByUserAndYearMonth(@RequestParam(value = "user-id") userId: Long, @RequestParam(value = "year-month") yearMonth: String): List<PromiseFindDto> { | ||
return promiseReadUseCase.findPromiseByUserIdYearMonth(userId, yearMonth) | ||
} | ||
|
||
// 약속 취소 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
구현 요구에
다른 사람의 약속을 조회하라 라는게 있었나욤?
본인만 보는거라면 users/user-id 없애고
시큐리티 컨텍스트에서 꺼내오는게 좋아보입니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추가적으로
year-month 은 뭔가... 쿼리 파람으로 빠지는게 이쁘지않을까 싶긴하네요
} | ||
|
||
@Operation(summary = "약속(promise)시간 수정", description = "약속을 수정합니다. (약속 제목, 약속 장소, 약속 시간)") | ||
@PutMapping("/promises/{promise-id}/end-times/{end-time}") | ||
fun updatePromiseEndTime(@RequestParam(value = "promise-id") promiseId: Long, @RequestParam(value = "end-time") endTime: LocalDateTime): PromiseDto { | ||
return promiseRegisterUseCase.updatePromiseEndTime(promiseId, endTime) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이것도 같은이야기긴한데
end-times 가 패스 배리어블이 맞나? 싶음
put이니깐 바디로 가야하지않나
) { | ||
companion object { | ||
fun from(promise: Promise, users: List<UserInfoVo>): PromiseFindDto { | ||
val userValues = listOf<UserInfoVo>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
listOf 가 mutable 한가요?
val eachPromiseUsers = promiseUserAdaptor.findByPromiseId(promiseUser.promiseId) | ||
val participant = getParticipantUserInfo(eachPromiseUsers) | ||
val promiseType = if (promise.promiseType == PromiseType.BEFORE) "BEFORE" else "PAST" | ||
val promiseFindDto = PromiseFindDto.from(promise, participant) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
정적 팩터리메소드
from 이 하나일 때
of 가 여러개일 때 인걸로 알고 있습니다!
import com.depromeet.whatnow.domains.promise.domain.Promise | ||
|
||
data class PromiseSplitedByPromiseTypeDto( | ||
val promiseType: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기서 string 을 내려주지말고
PromiseType 이넘을 그대로 내려줬으면 어땠을 까 하네요
Delete 때문에 그러신거겠죠?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
찬진님 저도 이부분 보면서 공감되는데요,
Delete때문에 String으로 넣는다는건 어떤거죠??
val promise = promiseAdaptor.queryPromise(promiseUser.promiseId) | ||
val eachPromiseUsers = promiseUserAdaptor.findByPromiseId(promiseUser.promiseId) | ||
val participant = getParticipantUserInfo(eachPromiseUsers) | ||
val promiseType = if (promise.promiseType == PromiseType.BEFORE) "BEFORE" else "PAST" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete 된 상태의 promise도 노출될 가능성이 있지 않을 까요
126e0ff
to
fddcf98
Compare
val userValues = listOf<UserInfoVo>() | ||
for (user in users) { | ||
userValues.plus(user) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
listOf가 이뮤터블해서 plus 메소드 내부 동작이 ArrayList를 기존크기 + 1로 새로 할당해서 값을 추가하네용
mutableListOf가 좋아보임니다 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plus 연산은 mutableList 도 동일한 원리로 동작합니다 그래서 addAll 로 변환해서 작성했습니다 !
return PromiseDto.from(promise) | ||
} | ||
|
||
@RequiresMainUser |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RequiresMainUser
가 조금 더 보기 편하게 Controller에 붙어있으면 하는데 어떠신가요?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good
) { | ||
@Around("@annotation(requiresMainUser)") | ||
fun validateMainUserAccess(joinPoint: ProceedingJoinPoint, requiresMainUser: RequiresMainUser): Any? { | ||
val userId = SecurityUtils.currentUserId | ||
val promiseId = joinPoint.args[0] as Long | ||
val promise = promiseAdaptor.queryPromise(promiseId) | ||
if (userId == promise.mainUserId) { | ||
return joinPoint.proceed() | ||
} else { | ||
throw NotHostException() | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 안에서
트랜잭션 어노테이션을 활용한 클래스를 만들면
영속성 캐싱의 이점을 노려볼 순 있긴해요~!
근데이건 나중에 고민해 보셔도 좋을 듯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AOP 안에서 캐싱하는거 되게 좋은 전략인 것 같아요!
적어뒀다가 반영해보겠습니다!
val promiseId = joinPoint.args[0] as Long | ||
val promise = promiseAdaptor.queryPromise(promiseId) | ||
if (userId == promise.mainUserId) { | ||
return joinPoint.proceed() | ||
} else { | ||
throw NotHostException() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Promise.validMainUser() 이라는 메서드로 만들면 좋겠군요
NotHostException() 도 저희 입셉션 써주시궁
Kudos, SonarCloud Quality Gate passed! |
* feat : promise 생성 * feat : promise 생성 * feat : promise 위치 변경 * feat: PromiseUpdateEndTime * feat : 약속 시간 수정 * feat : UserInfoVo * feat : 내가 참여한 약속 리스트 조회(화면기반) * feat : 월단위 약속 조회 * feat : Promise 취소 및 월간 조회 로직 수정 * chore : 주석 제거 * chore : spotless * feat: spotless * refactor : AOP annotation pointCut 문법 수정 * refactor : 나의 약속 전부 조회 최적화 및 코드 개선 * refactor : 나의 약속 전부 조회 최적화 및 코드 개선 * refactor : 펙토리 메소드 패턴 및 code style 개선
개요
작업사항
이런식으로 제공할 예정입니다.
변경로직