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

feat(mission): support markdown syntax for mission descriptions #762

Merged
merged 26 commits into from
Jul 24, 2024

Conversation

woowabrie
Copy link

Resolves #760

해결하려는 문제가 무엇인가요?

  • 과제 설명을 확인할 수 있다.

어떻게 해결했나요?

  • 과제 관리 어드민에서 설명 미리보기 추가
  • 참여하는 미션의 상세 보기 endpoint 추가

어떤 부분에 집중하여 리뷰해야 할까요?

  • 과제 상세 보기의 비즈니스 로직에서 빠진 부분이 없을지
  • 변환 로직의 위치

RCA 룰

r: 꼭 반영해 주세요. 적극적으로 고려해 주세요. (Request changes)
c: 웬만하면 반영해 주세요. (Comment)
a: 반영해도 좋고 넘어가도 좋습니다. 그냥 사소한 의견입니다. (Approve)

@woowabrie woowabrie self-assigned this Jul 11, 2024
Comment on lines 99 to 112
fun findByUserIdAndMissionId(memberId: Long, missionId: Long): MyMissionResponse {
val mission = missionRepository.getOrThrow(missionId)
val evaluationTarget = evaluationTargetRepository.findByEvaluationIdAndMemberId(mission.evaluationId, memberId)
?: throw NoSuchElementException("과제 참여 대상자가 아닙니다.")

check(!mission.hidden) { "비공개 상태의 과제입니다." }

val assignment = assignmentRepository.findByMemberIdAndMissionId(memberId, missionId)

return MyMissionResponse(
mission = mission,
submitted = assignment != null,
)
}
Copy link
Author

Choose a reason for hiding this comment

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

평가 대상자인 지원자에게만 과제 상세보기가 가능하도록 만들었습니다.
과제 상태와 참여 여부에 따른 로직에 이상이 없는지 체크 부탁드려요.

constructor(mission: Mission, submitted: Boolean) : this(
mission.id,
mission.title,
mission.formattedDescription,
Copy link
Author

Choose a reason for hiding this comment

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

파싱된 본문이 길다보니 이 값이 필요한 상황에서만 내려주도록 했는데 괜찮을까요?

Choose a reason for hiding this comment

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

a: 따봉~ MyMissionResponse.description의 이름도 같이 바꾸면 데이터 이름만 봤을 때도 의도가 잘 전달될 것 같아용~

Copy link
Author

Choose a reason for hiding this comment

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

네이밍은 (네오가 안계셨던 ㅋㅋ) 지지난 주간 회의에서 description을 그대로 사용한다라고 결정해서 이대로 갈게용

Comment on lines 46 to 47
val formattedDescription: String
get() = markdownToEmbeddedHtml(description)
Copy link
Author

Choose a reason for hiding this comment

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

도메인에 위치한게 왠지 찝찌입...

Choose a reason for hiding this comment

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

a: 저는 괜찮아 보이지만, 찝찝하시다면 Response로 옮기셔도..?ㅋㅋ

Copy link
Contributor

Choose a reason for hiding this comment

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

#762 (comment)#762 (comment) 가 반영되면 자연스럽게 해결될 것 같아요.

Copy link

@woowahan-neo woowahan-neo left a comment

Choose a reason for hiding this comment

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

고생하셨어요! r로 코멘트 남긴건 사소해서 approve 합니당~

Comment on lines 46 to 47
val formattedDescription: String
get() = markdownToEmbeddedHtml(description)

Choose a reason for hiding this comment

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

a: 저는 괜찮아 보이지만, 찝찝하시다면 Response로 옮기셔도..?ㅋㅋ

constructor(mission: Mission, submitted: Boolean) : this(
mission.id,
mission.title,
mission.formattedDescription,

Choose a reason for hiding this comment

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

a: 따봉~ MyMissionResponse.description의 이름도 같이 바꾸면 데이터 이름만 봤을 때도 의도가 잘 전달될 것 같아용~

Comment on lines 26 to 29
val style = element.style
style.set("margin-bottom", "10px")
style["margin-bottom"] = "10px"
element.style["margin-bottom"] = "10px"

Choose a reason for hiding this comment

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

Suggested change
val style = element.style
style.set("margin-bottom", "10px")
style["margin-bottom"] = "10px"
element.style["margin-bottom"] = "10px"
element.style["margin-bottom"] = "10px"

r: 중복!

Copy link
Contributor

@woowahan-pjs woowahan-pjs left a comment

Choose a reason for hiding this comment

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

고생하셨습니다. 👍 간단한 의견을 남겼습니다.
API 문서에 대한 테스트 코드도 추가해야 할 것 같아요!

@@ -95,7 +95,7 @@ data class MissionResponse(
)
}

data class MyMissionResponse(
data class MyMissionAndJudgementResponse(
Copy link
Contributor

Choose a reason for hiding this comment

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

c: ListMyMissionResponse라는 이름은 어떠세요?
https://en.dict.naver.com/#/entry/enko/d57a6e57bdc0458294a4f66108842840

Copy link
Author

Choose a reason for hiding this comment

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

제안주신 클래스명으로 바꿨을 경우 리턴 값이 List<ListMyMissionResponse>> 의 형태로 나오게 되어 일단 유지했습니다. ㅋㅋ
네이밍은 같은 미션 관련 DTO인 MissionAndEvaluationResponse 을 참고했어요.

@@ -95,7 +95,7 @@ data class MissionResponse(
)
}

data class MyMissionResponse(
data class MyMissionAndJudgementResponse(
val id: Long,
val title: String,
val description: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

r: description은 더 이상 불필요하지 않을까요? 과제의 내용을 미리 볼 수 있는 상황이 생길 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

삭제했어요.
다른 mission 관련 응답 살펴보고 description은 어드민, MyMissionResponse 에서만 사용하는 것 함께 확인했습니다.

Comment on lines 125 to 136
val mission = Mission(
missionData.title,
missionData.description,
missionData.evaluation.id,
missionData.startDateTime,
missionData.endDateTime,
missionData.submittable,
missionData.hidden,
missionData.id
)

return mission.formattedDescription
Copy link
Contributor

Choose a reason for hiding this comment

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

c: Mission의 인스턴스를 생성하고 변환하는 것은 번거로운데, 아래에서 리뷰하겠지만 description만 전달하면 어떨까요?

@@ -94,4 +95,19 @@ class MyMissionService(
judgmentRecord = judgment?.lastRecord
)
}

fun findByUserIdAndMissionId(memberId: Long, missionId: Long): MyMissionResponse {
Copy link
Contributor

Choose a reason for hiding this comment

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

r: 함수 이름에서 user를 빼야 하지 않을까요?

Comment on lines 100 to 104
val mission = missionRepository.getOrThrow(missionId)
val evaluationTarget = evaluationTargetRepository.findByEvaluationIdAndMemberId(mission.evaluationId, memberId)
?: throw NoSuchElementException("과제 참여 대상자가 아닙니다.")

check(!mission.hidden) { "비공개 상태의 과제입니다." }
Copy link
Contributor

Choose a reason for hiding this comment

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

r: 세 가지를 변경해야 할 것 같아요.

  • 과제 기간을 확인해야 할 것 같아요.
  • 과제 참여 대상자라거나 과제가 비공개라는 많은 정보 대신 과제가 존재하지 않는다는 메시지로 합치면 어떨까요?
  • evaluationTarget을 사용하지 않으면 컴파일러가 해당 줄을 최적화하고 삭제하는 걸까요?

Copy link
Author

Choose a reason for hiding this comment

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

  • 과제 기간, 과제 상태, 평가 대상자 여부 확인하도록 변경했어요.
  • 구체적 정보 없이 모두 404 응답으로 내려줍니다.
  • 변수 할당도 제거 완료!

Comment on lines 415 to 423
description = """
# 미션 - 숫자 야구 게임

## 🔍 진행 방식

- 미션은 **기능 요구 사항, 프로그래밍 요구 사항, 과제 진행 요구 사항** 세 가지로 구성되어 있다.
- 세 개의 요구 사항을 만족하기 위해 노력한다. 특히 기능을 구현하기 전에 기능 목록을 만들고, 기능 단위로 커밋 하는 방식으로 진행한다.
- 기능 요구 사항에 기재되지 않은 내용은 스스로 판단하여 구현한다.
""".trimIndent(),
Copy link
Contributor

Choose a reason for hiding this comment

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

c: 줄 시작 기준을 파이프 기호로 변경하면 줄바꿈이 훨씬 더 명확해지기 때문에 trimIndent()보다는 trimMargin()을 사용하는 것이 더 좋아요.

Suggested change
description = """
# 미션 - 숫자 야구 게임
## 🔍 진행 방식
- 미션은 **기능 요구 사항, 프로그래밍 요구 사항, 과제 진행 요구 사항** 세 가지로 구성되어 있다.
- 세 개의 요구 사항을 만족하기 위해 노력한다. 특히 기능을 구현하기 전에 기능 목록을 만들고, 기능 단위로 커밋 하는 방식으로 진행한다.
- 기능 요구 사항에 기재되지 않은 내용은 스스로 판단하여 구현한다.
""".trimIndent(),
description = """
|# 미션 - 숫자 야구 게임
|
|## 🔍 진행 방식
|
|- 미션은 **기능 요구 사항, 프로그래밍 요구 사항, 과제 진행 요구 사항** 세 가지로 구성되어 있다.
|- 세 개의 요구 사항을 만족하기 위해 노력한다. 특히 기능을 구현하기 전에 기능 목록을 만들고, 기능 단위로 커밋 하는 방식으로 진행한다.
|- 기능 요구 사항에 기재되지 않은 내용은 스스로 판단하여 구현한다.
""".trimMargin(),

import com.vaadin.flow.component.orderedlayout.VerticalLayout
import support.views.createContrastButton

class MissionPreviewDialog(
Copy link
Contributor

Choose a reason for hiding this comment

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

r: #733 (comment) 에도 글을 썼는데, 미리 보기 모달을 재사용할 수 있게 만들 수 있을까요?

}

private fun createHeader(): VerticalLayout {
return VerticalLayout(H2("과제 설명 미리보기")).apply {
Copy link
Contributor

Choose a reason for hiding this comment

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

c: "과제 미리 보기" 또는 메일 미리 보기와 합치면 "미리 보기"도 좋습니다.

Suggested change
return VerticalLayout(H2("과제 설명 미리보기")).apply {
return VerticalLayout(H2("미리 보기")).apply {

}
}

private fun getDataFromMissionForm(): MissionData {
Copy link
Contributor

Choose a reason for hiding this comment

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

c: MailsFormView 코드를 참고하면 좋을 것 같아요. 이와는 별개로 저도 생각해 본 것이 있는데, 미리 보기를 하려면 바인딩이 꼭 필요할까요?

Copy link
Author

Choose a reason for hiding this comment

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

일단 MailsFormView와 동일한 구조로 작성하였습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

c: 버전 4부터 시작해야 할 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

이거 고민하다가 V1~3과 달리 4는 하나로 합쳐지지 않은 것 같아서 4 버전대로 변경했어요.
배포 주기가 변경됨에 따라 버전 규칙을 어떻게 가져가실지는 따로 논의해보시죵!

Copy link
Author

@woowabrie woowabrie left a comment

Choose a reason for hiding this comment

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

@woowahan-pjs

피드백 주신 사항 반영했어요. 다시 한 번 검토 부탁드립니다 🙇‍♀️

@@ -95,7 +95,7 @@ data class MissionResponse(
)
}

data class MyMissionResponse(
data class MyMissionAndJudgementResponse(
Copy link
Author

Choose a reason for hiding this comment

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

제안주신 클래스명으로 바꿨을 경우 리턴 값이 List<ListMyMissionResponse>> 의 형태로 나오게 되어 일단 유지했습니다. ㅋㅋ
네이밍은 같은 미션 관련 DTO인 MissionAndEvaluationResponse 을 참고했어요.

constructor(mission: Mission, submitted: Boolean) : this(
mission.id,
mission.title,
mission.formattedDescription,
Copy link
Author

Choose a reason for hiding this comment

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

네이밍은 (네오가 안계셨던 ㅋㅋ) 지지난 주간 회의에서 description을 그대로 사용한다라고 결정해서 이대로 갈게용

@@ -95,7 +95,7 @@ data class MissionResponse(
)
}

data class MyMissionResponse(
data class MyMissionAndJudgementResponse(
val id: Long,
val title: String,
val description: String,
Copy link
Author

Choose a reason for hiding this comment

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

삭제했어요.
다른 mission 관련 응답 살펴보고 description은 어드민, MyMissionResponse 에서만 사용하는 것 함께 확인했습니다.

Comment on lines 100 to 104
val mission = missionRepository.getOrThrow(missionId)
val evaluationTarget = evaluationTargetRepository.findByEvaluationIdAndMemberId(mission.evaluationId, memberId)
?: throw NoSuchElementException("과제 참여 대상자가 아닙니다.")

check(!mission.hidden) { "비공개 상태의 과제입니다." }
Copy link
Author

Choose a reason for hiding this comment

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

  • 과제 기간, 과제 상태, 평가 대상자 여부 확인하도록 변경했어요.
  • 구체적 정보 없이 모두 404 응답으로 내려줍니다.
  • 변수 할당도 제거 완료!

}
}

private fun getDataFromMissionForm(): MissionData {
Copy link
Author

Choose a reason for hiding this comment

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

일단 MailsFormView와 동일한 구조로 작성하였습니다!

Copy link
Author

Choose a reason for hiding this comment

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

이거 고민하다가 V1~3과 달리 4는 하나로 합쳐지지 않은 것 같아서 4 버전대로 변경했어요.
배포 주기가 변경됨에 따라 버전 규칙을 어떻게 가져가실지는 따로 논의해보시죵!

}

@Test
fun `나의 과제를 상세 조회한다`() {
Copy link
Author

Choose a reason for hiding this comment

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

Mock 테스트로 보여서 넘겼었는데 문서화를 하고 있었군요 ㅋㅋ 추가했습니다.

@woowabrie woowabrie requested a review from woowahan-pjs July 18, 2024 01:24
Copy link
Contributor

@woowahan-pjs woowahan-pjs left a comment

Choose a reason for hiding this comment

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

이번에 승인하려고 했는데, 한 번 더 논의한 뒤 승인하도록 하겠습니다!

== 내 과제 조회
== 내 과제 목록 조회

operation::mission-list-me-get[snippets='http-request,http-response']
Copy link
Contributor

Choose a reason for hiding this comment

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

a: #553 (comment) 의 스니펫 디렉토리 명명 규칙에 따라 잘 작성하셨네요!


operation::mission-list-me-get[snippets='http-request,http-response']

== 내 과제 상세 조회
Copy link
Contributor

Choose a reason for hiding this comment

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

c: 내 과제 제출물 조회와 같이 내 과제 조회는 어떤가요? 경로에도 '상세'라는 단어가 드러나지 않으니깐요.

Suggested change
== 내 과제 상세 조회
== 내 과제 조회

val status: MissionStatus,
val submitted: Boolean,
) {
constructor(mission: Mission, submitted: Boolean, formattedDescription: String) : this(
Copy link
Contributor

Choose a reason for hiding this comment

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

a: 사소하지만 부 생성자의 매개 변수 순서를 주 생성자와 동일하게 설정하면 어떨까요?

Suggested change
constructor(mission: Mission, submitted: Boolean, formattedDescription: String) : this(
constructor(mission: Mission, description: String, submitted: Boolean) : this(

Comment on lines 101 to 116
val mission = missionRepository.getOrThrow(missionId)

evaluationTargetRepository.findByEvaluationIdAndMemberId(mission.evaluationId, memberId)
?: throw NoSuchElementException("과제가 존재하지 않습니다. id: $missionId")

if (!mission.isDescriptionViewable) {
throw NoSuchElementException("과제가 존재하지 않습니다. id: $missionId")
}

val assignment = assignmentRepository.findByMemberIdAndMissionId(memberId, missionId)

return MyMissionResponse(
mission = mission,
submitted = assignment != null,
markdownToEmbeddedHtml(mission.description),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

c: 아래 코드는 어떨까요? 조건문에 중괄호를 추가하거나, 생성자를 호출할 때 인자에 줄 바꿈을 추가할 수도 있습니다.

Suggested change
val mission = missionRepository.getOrThrow(missionId)
evaluationTargetRepository.findByEvaluationIdAndMemberId(mission.evaluationId, memberId)
?: throw NoSuchElementException("과제가 존재하지 않습니다. id: $missionId")
if (!mission.isDescriptionViewable) {
throw NoSuchElementException("과제가 존재하지 않습니다. id: $missionId")
}
val assignment = assignmentRepository.findByMemberIdAndMissionId(memberId, missionId)
return MyMissionResponse(
mission = mission,
submitted = assignment != null,
markdownToEmbeddedHtml(mission.description),
)
val mission = missionRepository.getOrThrow(memberId)
val evaluationTarget = evaluationTargetRepository.findByEvaluationIdAndMemberId(mission.evaluationId, memberId)
if (!mission.isDescriptionViewable || evaluationTarget == null) throw NoSuchElementException("과제가 존재하지 않습니다. id: $missionId")
val assignment = assignmentRepository.findByMemberIdAndMissionId(memberId, missionId)
return MyMissionResponse(mission, markdownToEmbeddedHtml(mission.description), submitted = assignment != null)

@@ -40,6 +42,9 @@ class Mission(
val isSubmitting: Boolean
get() = status == MissionStatus.SUBMITTING

val isDescriptionViewable: Boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

c: 개인 취향이에요! readable이나 readableDescription은 어떠세요?

src/main/kotlin/apply/domain/mission/Mission.kt Outdated Show resolved Hide resolved
)

Given("공개 상태의 과제가 있는 경우") {
val participatedUser = createMember(id = 1L)
Copy link
Contributor

Choose a reason for hiding this comment

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

r: '참여자'라는 새로운 용어가 등장하였는데요. 평가 대상자(evaluation target)라는 용어도 있어 대체할 수 있는지, 아니면 따로 불러야 하는지 논의해 보시죠!

@woowabrie
Copy link
Author

@woowahan-pjs
다시 한 번 검토 요청드립니다 🙇‍♀️

@woowahan-pjs woowahan-pjs changed the title feat(mission): implement function to retrieve description of mission feat(mission): support markdown syntax for mission descriptions Jul 24, 2024
@woowahan-pjs woowahan-pjs merged commit ec25e40 into develop Jul 24, 2024
@woowahan-pjs woowahan-pjs deleted the feature/mission-description-preview branch July 24, 2024 08:01
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.

3 participants