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.

Sign up for GitHub

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

Refactor: 포인트관련 로직을 포인트 서비스계층으로 통합, 개별 트랜잭션 적용, 테스트 코드 개선 #132

Merged
merged 10 commits into from
Nov 4, 2024
Merged

Refactor: 포인트관련 로직을 포인트 서비스계층으로 통합, 개별 트랜잭션 적용, 테스트 코드 개선 #132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d77cff3
refactor: 불필요한 private method 통합
zzoe2346 Oct 28, 2024
5c15109
refactor: 개별 트랜잭션으로 변경
zzoe2346 Oct 29, 2024
e6a6982
Merge branch 'Weekly' of https://github.com/kakao-tech-campus-2nd-ste…
zzoe2346 Oct 31, 2024
3e865f0
refactor: 포인트 관련 로직 서비스계층으로 옮김
zzoe2346 Oct 31, 2024
7642b1f
test: 테스트 추가하고, 가독성 개선
zzoe2346 Oct 31, 2024
b2f8984
refactor: SQL 수정
zzoe2346 Nov 1, 2024
7b5aa1a
Merge branch 'Weekly' of https://github.com/kakao-tech-campus-2nd-ste…
zzoe2346 Nov 4, 2024
97cd2f2
test: 안부전화 서비스에서 포인트 로직 서비스층으로 통합으로 인한 수정
zzoe2346 Nov 4, 2024
ec6622b
merge: conflict solve
zzoe2346 Nov 4, 2024
0084704
remove: 깃헙 액션 테스트 통과를 위한 수정
zzoe2346 Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/com/example/sinitto/callback/repository/CallbackRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public interface CallbackRepository extends JpaRepository<Callback, Long> {

Page<Callback> findAllBySeniorIn(List<Senior> seniors, Pageable pageable);

List<Callback> findAllByStatusAndPendingCompleteTimeBefore(Callback.Status status, LocalDateTime dateTime);
List<Callback> findAllByStatusAndPendingCompleteTimeBetween(Callback.Status status, LocalDateTime startDateTime, LocalDateTime endDateTime);

boolean existsBySeniorAndStatusIn(Senior senior, List<Callback.Status> statuses);
}
87 changes: 31 additions & 56 deletions src/main/java/com/example/sinitto/callback/service/CallbackService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,8 @@
import com.example.sinitto.member.entity.Member;
import com.example.sinitto.member.entity.Senior;
import com.example.sinitto.member.repository.MemberRepository;
import com.example.sinitto.point.entity.Point;
import com.example.sinitto.point.entity.PointLog;
import com.example.sinitto.point.repository.PointLogRepository;
import com.example.sinitto.point.repository.PointRepository;
import com.example.sinitto.point.service.PointService;
import org.springframework.data.domain.Page;
import org.springframework.data.domain.Pageable;
import org.springframework.scheduling.annotation.Scheduled;
Expand All @@ -38,21 +36,19 @@ public class CallbackService {
private final CallbackRepository callbackRepository;
private final MemberRepository memberRepository;
private final SeniorRepository seniorRepository;
private final PointRepository pointRepository;
private final PointLogRepository pointLogRepository;
private final PointService pointService;

public CallbackService(CallbackRepository callbackRepository, MemberRepository memberRepository, SeniorRepository seniorRepository, PointRepository pointRepository, PointLogRepository pointLogRepository) {
public CallbackService(CallbackRepository callbackRepository, MemberRepository memberRepository, SeniorRepository seniorRepository, PointService pointService) {
this.callbackRepository = callbackRepository;
this.memberRepository = memberRepository;
this.seniorRepository = seniorRepository;
this.pointRepository = pointRepository;
this.pointLogRepository = pointLogRepository;
this.pointService = pointService;
}

@Transactional(readOnly = true)
public Page<CallbackResponse> getWaitingCallbacks(Long memberId, Pageable pageable) {

checkAuthorization(memberId);
checkIsSinitto(memberId);

return callbackRepository.findAllByStatus(Callback.Status.WAITING, pageable)
.map((callback) -> new CallbackResponse(callback.getId(), callback.getSeniorName(), callback.getPostTime(), callback.getStatus(), callback.getSeniorId()));
Expand All @@ -61,7 +57,7 @@ public Page<CallbackResponse> getWaitingCallbacks(Long memberId, Pageable pageab
@Transactional
public void acceptCallbackBySinitto(Long memberId, Long callbackId) {

checkAuthorization(memberId);
checkIsSinitto(memberId);

if (callbackRepository.existsByAssignedMemberIdAndStatus(memberId, Callback.Status.IN_PROGRESS)) {
throw new ConflictException("이 요청을 한 시니또는 이미 진행중인 콜백이 있습니다.");
Expand All @@ -76,7 +72,7 @@ public void acceptCallbackBySinitto(Long memberId, Long callbackId) {
@Transactional
public void changeCallbackStatusToPendingCompleteBySinitto(Long memberId, Long callbackId) {

checkAuthorization(memberId);
checkIsSinitto(memberId);

Callback callback = getCallbackOrThrow(callbackId);

Expand All @@ -95,42 +91,36 @@ public void changeCallbackStatusToCompleteByGuard(Long memberId, Long callbackId
Long guardId = senior.getMember().getId();

if (!guardId.equals(memberId)) {
throw new ForbiddenException("이 API를 요청한 보호자는 이 콜백을 요청 한 시니어의 보호자가 아닙니다.");
throw new ForbiddenException("이 API 를 요청한 보호자는 이 콜백을 요청 한 시니어의 보호자가 아닙니다.");
}

earnPointForSinitto(callback.getAssignedMemberId());
pointService.earnPoint(callback.getAssignedMemberId(), CALLBACK_PRICE, PointLog.Content.COMPLETE_CALLBACK_AND_EARN);
callback.changeStatusToComplete();
}

private void earnPointForSinitto(Long sinittoMemberId) {

Point sinittoPoint = pointRepository.findByMemberId(sinittoMemberId)
.orElseThrow(() -> new NotFoundException("포인트 적립 받을 시니또와 연관된 포인트가 없습니다"));

sinittoPoint.earn(CALLBACK_PRICE);

pointLogRepository.save(new PointLog(PointLog.Content.COMPLETE_CALLBACK_AND_EARN.getMessage(), sinittoPoint.getMember(), CALLBACK_PRICE, PointLog.Status.EARN));
}

@Scheduled(cron = "0 */10 * * * *")
@Transactional
public void changeOldPendingCompleteToCompleteByPolicy() {

LocalDateTime referenceDateTimeForComplete = LocalDateTime.now().minusDays(DAYS_FOR_AUTO_COMPLETE);

List<Callback> callbacks = callbackRepository.findAllByStatusAndPendingCompleteTimeBefore(Callback.Status.PENDING_COMPLETE, referenceDateTimeForComplete);
List<Callback> callbacks = callbackRepository.findAllByStatusAndPendingCompleteTimeBetween(Callback.Status.PENDING_COMPLETE, referenceDateTimeForComplete.minusMinutes(5), referenceDateTimeForComplete.plusMinutes(5));
Copy link
Member

Choose a reason for hiding this comment

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

👍


for (Callback callback : callbacks) {

earnPointForSinitto(callback.getAssignedMemberId());
callback.changeStatusToComplete();
completeCallbackIndividually(callback);
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

transaction의 크기를 줄일 수 있어서 좋은 것 같습니다!!

@Transactional
public void completeCallbackIndividually(Callback callback) {

pointService.earnPoint(callback.getAssignedMemberId(), CALLBACK_PRICE, PointLog.Content.COMPLETE_CALLBACK_AND_EARN);
callback.changeStatusToComplete();
}

Comment on lines +109 to +119
Copy link
Member

Choose a reason for hiding this comment

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

개인적으로는
공통 트랜잭션 상태일 때 여러 완료상태로 가야할 콜백 중 하나의 문제만으로도 모든 변화가 롤백되어 재수행해야 하는 상황을 고려했을 때를 생각해본다면
해당 개별 트랙잭션이 현재까지 진행된 콜백 상태변화는 저장되어 재수행하지 않아도 된다는 점에서 얻는 이점이 DB I/O가 잦아진다는 단점보다 더 크다는 생각이 들어 좋은 것 같습니다.
다만 개별 트랙잭션 로직 안에 엔티티 메서드 수행은 오버헤드가 크지 않을 것 같은데 pointService 메서드 호출은 오버헤드가 상대적으로 높지 않을까 걱정이네요..! 스프링이 해결해주려나요...??!

Copy link
Contributor Author

@zzoe2346 zzoe2346 Nov 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

의견 감사합니다! 지호님 말씀대로 개별 트랜잭션이 콜백 상태를 좀 신속히? 저장시켜 좋은거 같네요.

pointService 메서드 호출에 관한 말씀은 제가 잘 이해했는지 모르겠는데 나름 찾아본거 공유해드려보면, JVM 에서 최적화도 해주는것도 있고 메모리에 이미 올라가있는 객체의 메서드를 참조하는 방식이라 오버헤드가 발생하긴 하지만 엄청 크지는 않을걸로 예상이 됩니다. 또한 이 방식이 다소 리소스를 더 먹더라도 개발자 입장에서 유지보수 측면에서의 이득이 더 크다라는 관점에서 보면 좋은점도 있는거 같아요! 장단점이 다있어 보여요.

아니면 pointService 메서드 호출에서 포인트 DB 관련해서 오버헤드를 말씀하신 걸까요? 요 부분은 콜백 상태변화랑 포인트 적립/차감이 땔 수 없는 부분이라 어쩔수 없을거 같기는 합니다.🥲

Copy link
Member

Choose a reason for hiding this comment

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

JVM 에서 최적화도 해주는것도 있고 메모리에 이미 올라가있는 객체의 메서드를 참조하는 방식이라 오버헤드가 발생하긴 하지만 엄청 크지는 않을걸로 예상이 됩니다

아하! 하나 배워갑니다 감사합니다 ㅎㅎ 해당 의견 관련한 부분 남긴게 맞습니다 👍

@Transactional
public void cancelCallbackAssignmentBySinitto(Long memberId, Long callbackId) {

checkAuthorization(memberId);
checkIsSinitto(memberId);

Callback callback = getCallbackOrThrow(callbackId);

Expand All @@ -145,55 +135,40 @@ public String createCallbackByCall(String fromNumber) {

String phoneNumber = TwilioHelper.trimPhoneNumber(fromNumber);

Senior senior = findSeniorByPhoneNumber(phoneNumber);
Senior senior = seniorRepository.findByPhoneNumber(phoneNumber)
.orElse(null);

if (senior == null) {
return TwilioHelper.convertMessageToTwiML(FAIL_MESSAGE_NOT_ENROLLED);
}

Point point = findPointWithWriteLock(senior.getMember().getId());
if (point == null || !point.isSufficientForDeduction(CALLBACK_PRICE)) {
return TwilioHelper.convertMessageToTwiML(FAIL_MESSAGE_NOT_ENOUGH_POINT);
}

if (callbackRepository.existsBySeniorAndStatusIn(senior, List.of(Callback.Status.WAITING, Callback.Status.IN_PROGRESS))) {
return TwilioHelper.convertMessageToTwiML(FAIL_MESSAGE_ALREADY_HAS_CALLBACK_IN_PROGRESS_OR_WAITING);
}

point.deduct(CALLBACK_PRICE);
try {
pointService.deductPoint(senior.getMember().getId(), CALLBACK_PRICE, PointLog.Content.SPEND_COMPLETE_CALLBACK);
} catch (Exception e) {
return TwilioHelper.convertMessageToTwiML(FAIL_MESSAGE_NOT_ENOUGH_POINT);
}
Comment on lines +149 to +153
Copy link
Member

Choose a reason for hiding this comment

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

point관련 로직이라 포괄적인 try-catch문 사용 좋은 것 같습니다 👍


pointLogRepository.save(
new PointLog(
PointLog.Content.SPEND_COMPLETE_CALLBACK.getMessage(),
senior.getMember(),
CALLBACK_PRICE,
PointLog.Status.SPEND_COMPLETE)
);
callbackRepository.save(new Callback(Callback.Status.WAITING, senior));

return TwilioHelper.convertMessageToTwiML(SUCCESS_MESSAGE);
}

private Senior findSeniorByPhoneNumber(String phoneNumber) {
return seniorRepository.findByPhoneNumber(phoneNumber)
.orElse(null);
}

private Point findPointWithWriteLock(Long memberId) {
return pointRepository.findByMemberIdWithWriteLock(memberId)
.orElse(null);
}

@Transactional(readOnly = true)
public CallbackResponse getAcceptedCallback(Long memberId) {

checkAuthorization(memberId);
checkIsSinitto(memberId);

Callback callback = callbackRepository.findByAssignedMemberIdAndStatus(memberId, Callback.Status.IN_PROGRESS)
.orElseThrow(() -> new NotFoundException("요청한 시니또에 할당된 콜백이 없습니다"));

return new CallbackResponse(callback.getId(), callback.getSeniorName(), callback.getPostTime(), callback.getStatus(), callback.getSeniorId());
}

private void checkAuthorization(Long memberId) {
private void checkIsSinitto(Long memberId) {

Member member = memberRepository.findById(memberId)
.orElseThrow(() -> new NotFoundException("멤버가 아닙니다"));
Expand Down Expand Up @@ -224,11 +199,11 @@ public Page<CallbackUsageHistoryResponse> getCallbackHistoryOfGuard(Long memberI

List<Senior> seniors = seniorRepository.findAllByMember(member);


return callbackRepository.findAllBySeniorIn(seniors, pageable)
.map(callback -> new CallbackUsageHistoryResponse(callback.getId(), callback.getSeniorName(), callback.getPostTime(), callback.getStatus()));
}

@Transactional(readOnly = true)
public CallbackForSinittoResponse getCallbackForSinitto(Long memberId, Long callbackId) {

Callback callback = callbackRepository.findById(callbackId)
Expand Down
57 changes: 8 additions & 49 deletions src/main/java/com/example/sinitto/helloCall/service/HelloCallService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,8 @@
import com.example.sinitto.member.entity.Member;
import com.example.sinitto.member.entity.Senior;
import com.example.sinitto.member.repository.MemberRepository;
import com.example.sinitto.point.entity.Point;
import com.example.sinitto.point.entity.PointLog;
import com.example.sinitto.point.repository.PointLogRepository;
import com.example.sinitto.point.repository.PointRepository;
import com.example.sinitto.point.service.PointService;
import org.springframework.data.domain.Page;
import org.springframework.data.domain.PageImpl;
import org.springframework.data.domain.Pageable;
Expand All @@ -38,20 +36,18 @@ public class HelloCallService {
private final SeniorRepository seniorRepository;
private final MemberRepository memberRepository;
private final HelloCallTimeLogRepository helloCallTimeLogRepository;
private final PointRepository pointRepository;
private final PointLogRepository pointLogRepository;
private final PointService pointService;


public HelloCallService(HelloCallRepository helloCallRepository, TimeSlotRepository timeSlotRepository,
SeniorRepository seniorRepository, MemberRepository memberRepository, HelloCallTimeLogRepository helloCallTimeLogRepository,
PointRepository pointRepository, PointLogRepository pointLogRepository) {
SeniorRepository seniorRepository, MemberRepository memberRepository,
HelloCallTimeLogRepository helloCallTimeLogRepository, PointService pointService) {
this.helloCallRepository = helloCallRepository;
this.timeSlotRepository = timeSlotRepository;
this.seniorRepository = seniorRepository;
this.memberRepository = memberRepository;
this.helloCallTimeLogRepository = helloCallTimeLogRepository;
this.pointRepository = pointRepository;
this.pointLogRepository = pointLogRepository;
this.pointService = pointService;
}

@Transactional
Expand All @@ -73,22 +69,7 @@ public void createHelloCallByGuard(Long memberId, HelloCallRequest helloCallRequ
timeSlotRepository.save(timeSlot);
}

Point point = pointRepository.findByMemberIdWithWriteLock(memberId)
.orElseThrow(() -> new NotFoundException("멤버에 연관된 포인트가 없습니다."));

if (!point.isSufficientForDeduction(helloCall.getPrice())) {
throw new BadRequestException("포인트가 부족합니다.");
}

point.deduct(helloCall.getPrice());

pointLogRepository.save(
new PointLog(
PointLog.Content.SPEND_COMPLETE_HELLO_CALL.getMessage(),
senior.getMember(),
helloCall.getPrice(),
PointLog.Status.SPEND_COMPLETE
));
pointService.deductPoint(memberId, helloCall.getPrice(), PointLog.Content.SPEND_COMPLETE_HELLO_CALL);
}

@Transactional
Expand Down Expand Up @@ -178,18 +159,7 @@ public void deleteHellCallByGuard(Long memberId, Long helloCallId) {

helloCall.checkGuardIsCorrect(member);

Point point = pointRepository.findByMemberIdWithWriteLock(memberId)
.orElseThrow(() -> new NotFoundException("멤버에 연관된 포인트가 없습니다."));

point.earn(helloCall.getPrice());

pointLogRepository.save(
new PointLog(
PointLog.Content.SPEND_CANCEL_HELLO_CALL.getMessage(),
member,
helloCall.getPrice(),
PointLog.Status.SPEND_CANCEL)
);
pointService.refundPointByDelete(memberId, helloCall.getPrice(), PointLog.Content.SPEND_CANCEL_HELLO_CALL);

helloCall.checkStatusIsWaiting();
helloCallRepository.delete(helloCall);
Expand Down Expand Up @@ -248,18 +218,7 @@ public void makeCompleteHelloCallByGuard(Long memberId, Long helloCallId) {

helloCall.changeStatusToComplete();

Point sinittoPoint = pointRepository.findByMember(helloCall.getMember())
.orElseThrow(() -> new NotFoundException("포인트 적립 받을 시니또와 연관된 포인트가 없습니다"));

sinittoPoint.earn(helloCall.getPrice());

pointLogRepository.save(
new PointLog(
PointLog.Content.COMPLETE_HELLO_CALL_AND_EARN.getMessage(),
sinittoPoint.getMember(),
helloCall.getPrice(),
PointLog.Status.EARN)
);
pointService.earnPoint(helloCall.getMember().getId(), helloCall.getPrice(), PointLog.Content.COMPLETE_HELLO_CALL_AND_EARN);
}

@Transactional(readOnly = true)
Expand Down
60 changes: 60 additions & 0 deletions src/main/java/com/example/sinitto/point/service/PointService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,4 +96,64 @@ public void savePointWithdrawRequest(Long memberId, int price) {

pointLogRepository.save(new PointLog(PointLog.Content.WITHDRAW_REQUEST.getMessage(), member, price, PointLog.Status.WITHDRAW_REQUEST));
}

@Transactional
public void earnPoint(Long memberId, int price, PointLog.Content contentForPointLog) {

Point point = pointRepository.findByMemberId(memberId)
.orElseThrow(() -> new NotFoundException("멤버에 연관된 포인트가 없습니다."));

point.earn(price);

pointLogRepository.save(
new PointLog(
contentForPointLog.getMessage(),
point.getMember(),
price,
PointLog.Status.EARN)
);
}

@Transactional
public void deductPoint(Long memberId, int price, PointLog.Content contentForPointLog) {

Point point = pointRepository.findByMemberIdWithWriteLock(memberId)
.orElseThrow(() -> new NotFoundException("멤버에 연관된 포인트가 없습니다."));

if (!point.isSufficientForDeduction(price)) {
throw new BadRequestException("포인트가 부족합니다.");
}

point.deduct(price);

pointLogRepository.save(
new PointLog(
contentForPointLog.getMessage(),
point.getMember(),
price,
PointLog.Status.SPEND_COMPLETE
));
}

@Transactional
public void refundPointByDelete(Long memberId, int price, PointLog.Content contentForPointLog) {

Point point = pointRepository.findByMemberIdWithWriteLock(memberId)
.orElseThrow(() -> new NotFoundException("멤버에 연관된 포인트가 없습니다."));

if (!point.isSufficientForDeduction(price)) {
throw new BadRequestException("포인트가 부족합니다.");
}

point.deduct(price);

pointLogRepository.save(
new PointLog(
contentForPointLog.getMessage(),
point.getMember(),
price,
PointLog.Status.SPEND_CANCEL
));
}

}
Loading