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

[Fix] 챌린지 상세 조회 API 참가 전, 참가 중 분리 #235

Merged
merged 5 commits into from
Mar 7, 2023

Conversation

heoboseong7
Copy link
Collaborator

@heoboseong7 heoboseong7 commented Mar 5, 2023

관련 Issue

작업 내용

  • 챌린지 상세 조회 API 필드 challengeCategory, createAt 추가
  • 변경 내용 문서화 테스트 반영
  • 챌린지 상세 조회 참가 전, 참가 중으로 분리
    • 참가 전 챌린지 상세 조회 API 필드 수정(불필요한 필드 제거)
    • 참가 중 챌린지 상세 조회 API 추가
  • 챌린지 상세 조회 참가 전, 참가 중 문서화용 테스트 추가

Figma 링크

@heoboseong7 heoboseong7 self-assigned this Mar 5, 2023
@github-actions
Copy link

github-actions bot commented Mar 5, 2023

테스트통과 🤖

Copy link
Collaborator

@Youhoseong Youhoseong left a comment

Choose a reason for hiding this comment

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

참여전이나 참여중이나 하나의 api를쓰는걸까요?

@heoboseong7
Copy link
Collaborator Author

참여전이나 참여중이나 하나의 api를쓰는걸까요?

네. 둘 다 챌린지의 상세 정보를 조회해서 하나로 묶었어요

@Youhoseong
Copy link
Collaborator

필요로 하는 정보가 좀 다른거같긴해서 ㅎㅎ넵

@heoboseong7
Copy link
Collaborator Author

필요로 하는 정보가 좀 다른거같긴해서 ㅎㅎ넵

지금까지 하던 대로면 나누는게 맞아 보이긴 하는데.. 나누는게 더 나을까요?

@Youhoseong
Copy link
Collaborator

네 저는 각자 상태에서 필요한 부분만 내려주도록 나누는 것도 좋다고 생각해요.

@heoboseong7
Copy link
Collaborator Author

이건 그럼 2개로 분리할게요 ㅎ

@github-actions
Copy link

github-actions bot commented Mar 5, 2023

테스트통과 🤖

@heoboseong7 heoboseong7 changed the title [Fix] 챌린지 상세 조회 API 필드 추가 [Fix] 챌린지 상세 조회 API 참가 전, 참가 중 분리 Mar 5, 2023
@heoboseong7
Copy link
Collaborator Author

확인 한번 부탁드립니다 ㅎ

Comment on lines 64 to 68
@GetMapping("/detail/participating")
@FLogging
public ApiResponse<ParticipatingChallengeDetailResponse> getMyChallengeDetail() {
return ApiResponse.ok(challengeApiService.getMyChallengeDetail());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

ChallengeParticipateController가 맞지않을까요?

어찌보면 참가중인것과 참가중이지 않은걸 나누고 있다고 볼 수도 있을거같네요

Copy link
Collaborator

Choose a reason for hiding this comment

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

image

participation/ participating 이 사용되는거같은데 통일해보는건 어떨까요? 지금 정하면될거같아요~

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

챌린지의 정보를 조회하는 API라서 ChallengeController에 넣었어요
participation은 챌린지 참가를 나타내고 participating은 참여중을 나타내서 지금은 다른 의미를 가지고 있어요.

Copy link
Collaborator

Choose a reason for hiding this comment

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

음음.. 저는

GET /api/v1/challenge/participation => 참여중인 것 조회.
POST /api/v1/challenge/participation => 참여하기.
통일하더라도 http method로 충분히 구분되는거같긴하는데요 어때요?

participation/check 도 말씀하신 거에 따르면 participating이 더 맞는거같기도하고요.

Copy link
Collaborator

Choose a reason for hiding this comment

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

참여중이다, 참여하다를 분리하기보단 그냥 도메인 개념으로 통일하는건 어떨까해요~

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

늦게 봤어요 ㅠ
participation으로 단일화 한다면 GET /api/v1/challenge/participation이 참가중인 챌린지를 조회 하는지, 참가중인 챌린지 참가의 정보를 조회하는지 구분하기 어렵지 않을까요?

지금 다시 보니 participating이 참여중 보다는 나의~ 뜻을 가지는 것 같아요. 그래서 participating을 제거하고 id의 자리에 my를 붙이는 방법은 어떨까요? 대신, 이대로 하려면 my를 추가해야 할 API들이 많이 보이네요..

Copy link
Collaborator

Choose a reason for hiding this comment

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

참가중인 챌린지를 조회하는지, 챌린지 참가 정보를 조회하는지는 좀 더 고민해서 다르게 표현하면 직관적으로 구분은 가능 할 것 같아요. 말씀하신 방법이 방안일 수도 있고요.

지금까지 작성된 path를 수정할 필욘 없겠지만 (나중에 한번 맞춰보는 시간을 가지는 것은 몰라도),
앞으로는 최종 목적이 챌린지를 조회하는 것인지, 말씀하신대로 참가 정보를 조회하는 것인지가 구분이 잘 되었으면 좋겠네요.

말하신 방법도 좋고, 더 좋은 것이 떠오르신다면 반영하셔서 머지하셔도 될듯요~!

Comment on lines 142 to 143

@Transactional(readOnly = true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

트랜잭션은 왜 필요하다고 판단하신거에요?

Copy link
Collaborator Author

@heoboseong7 heoboseong7 Mar 7, 2023

Choose a reason for hiding this comment

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

지금 코드에서는 필요 없겠네요. 버릇처럼 추가해놓았어요.
이런 경우에 빼는 것이 좋을까요? 성능적으론 문제가 되지 않을 것 같아서 정해두면 좋을 것 같아요.

Copy link
Collaborator

Choose a reason for hiding this comment

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

저는 이런건 관행이나 룰로 정하는 영역이라기보단
당연히 필요하면 쓰고 필요하지 않으면 안쓰는거라고 생각해요~

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 여기서는 제거할게요

@github-actions
Copy link

github-actions bot commented Mar 7, 2023

테스트통과 🤖

@github-actions
Copy link

github-actions bot commented Mar 7, 2023

테스트통과 🤖

@heoboseong7 heoboseong7 merged commit 54dc457 into main Mar 7, 2023
@heoboseong7 heoboseong7 deleted the issue-#234 branch March 7, 2023 12:37
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.

[Fix] 챌린지 상세 조회 API 필드 추가
2 participants