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

#55 [FEAT, FIX] 아바타 관련 API #68

Merged
merged 17 commits into from
Oct 28, 2023
Merged

#55 [FEAT, FIX] 아바타 관련 API #68

merged 17 commits into from
Oct 28, 2023

Conversation

05AM
Copy link
Member

@05AM 05AM commented Oct 20, 2023

☀️ 작업 사항

  • 유저 아바타 정보(현재 아바타, 해금한 아바타들) 불러오기 API
  • 아바타 해금 API
  • 유저 아바타 변경 API

☀️ 관련 이슈

☀️ 참고사항

ERD 변경이 있습니다! 확인 부탁드립니다.

  • avatar table 변경 후 작업하려고 하셨던 코드에 오류가 나서 임시로 수정해두었습니다! avatarTheme enum value 관련 에러였습니다. 편하신대로 수정 부탁드립니다.

@05AM 05AM added FEATURE 새로운 기능 FIX 수정 labels Oct 20, 2023
@05AM 05AM requested a review from hye-on October 20, 2023 19:50
@05AM 05AM self-assigned this Oct 20, 2023
@@ -125,7 +125,7 @@ public BaseResponse<MissionLogCreateRes> postMissionLog(
// 미션 수행 기록 생성
missionService.createMissionLog(userId, missionLogCreateReq);
// 경험치 정산
UserProgressionResult upr = userService.userProgression(userId);
UserProgressionResult upr = userService.userProgress(userId);
Copy link
Contributor

Choose a reason for hiding this comment

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

p4 : 컨트롤러말고 위의 함수에서 이 함수를 호출해도 좋을 것 같습니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

저의 개인적인 의견으로는 미션 로그를 생성하는 것과 유저의 데이터를 업데이트하는 것은 다른 기능이라고 생각해서 이렇게 작성했습니다!

Copy link
Contributor

@hye-on hye-on Oct 27, 2023

Choose a reason for hiding this comment

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

다른 기능일때는 두 서비스를 컨트롤러에서 호출하는군요.
혹시 두 서비스가 트랜잭션으로 묶일 필요가 있다고 생각하시는지 궁금합니다!
두 서비스의 함수는 각각 트랜잭션이라서요!
미션 기록이 실패해도 경험치는 정산되거나, 경험치 정산 실패해도 미션 수행기록은 성공할 수 있게 하는 것을 염두에 둔것인지 궁금합니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

아하 그런 점을 염두한다면 missionSerivce에 공통 메소드를 만들어서 진행하는게 좋을 것 같습니다! 수정해서 올리겠습니다

// UserAvatar userAvatar = userAvatarRepository.findByUser_Id(userId);
// roomUserMissionLogRes.setTheme(AvatarTheme.valueOf(userAvatar.getAvatar().getTheme()).getValue());
UserAvatar userAvatar = userAvatarRepository.findByUser_IdAndIsActiveIsTrue(userId)
.orElseThrow(() -> new BaseException(Status.INTERNAL_SERVER_ERROR));
Copy link
Contributor

Choose a reason for hiding this comment

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

INTERNAL_SERVER_ERROR로 하신 이유가 궁금합니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

isActive가 true인 것을 찾을 수 없다면 이건 현재 코드든 어딘가의 코드든 서버 로직에 문제가 있는 것이라고 판단하여 해당 상태코드를 반환하게 하였습니다.

Copy link
Contributor

@hye-on hye-on Oct 27, 2023

Choose a reason for hiding this comment

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

그렇군요. "예상치 못한 서버 에러입니다. 제보 부탁드립니다." 라는 메시지가 반환되면 에러가 어디서 났는지 찾기 어려우니까 유저아바타에서 에러가났다는 것을 알 수 있는 다른 상태코드를 내려주는 것이 어떨까요?

Copy link
Member Author

@05AM 05AM Oct 27, 2023

Choose a reason for hiding this comment

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

네 좋은 것 같습니다! 혹시 적합한 http 상태코드를 추천해주실 수 있을까요? 500 에러로 할지 다른걸로 할지 고민이 됩니다.

Copy link
Contributor

@hye-on hye-on Oct 27, 2023

Choose a reason for hiding this comment

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

400, 유저의 현재아바타를 찾지못했습니다.
500, 유저의 대표 아바타가 없습니다.

현재 상태코드에 메시지만 바꾸어도 좋을 것 같습니다!

private int unlockLevel;

@Schema(description = "생성일자")
private String createdAt;
Copy link
Contributor

Choose a reason for hiding this comment

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

생성일자 반환하는 이유가 궁금합니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

avatar 엔티티를 그냥 복붙해서 사용해서 그런것 같습니다! 수정하겠습니다

Copy link
Contributor

@hye-on hye-on left a comment

Choose a reason for hiding this comment

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

뛰어쓰기 수정은 별도로 pr 올려주시면 감사하겠습니다.
고생하셨습니다!

@05AM 05AM merged commit 597a6bc into main Oct 28, 2023
1 check passed
@05AM 05AM deleted the feature/avatar branch November 23, 2023 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FEATURE 새로운 기능 FIX 수정
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT] 아바타 관련 API
2 participants