-
Notifications
You must be signed in to change notification settings - Fork 102
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(mail): implement the mail history function #377
Conversation
# Conflicts: # src/main/kotlin/apply/application/UserService.kt # src/test/kotlin/apply/application/UserServiceTest.kt
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.
고생하셨습니다! 몇 가지 코멘트를 남겼으니 확인해주시면 감사하겠습니다.
|
||
@Transactional | ||
@Service | ||
class MailHistoryService( |
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.
MailHistoryService
와 MailService
를 따로 사용해주는군요!
개인적으로는 MailHistoryService 와 mailService 가 같이 사용되는 경우가 많을 것 같은데 분리하신 이유가 있을까요?
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.
저도 이 부분에 대해서 고민이 되었는데요. MailService
는 @Transactional
이 아닌 @Async
로직을 사용하는 메소드로 구성되어 있더라구요. 같은 서비스에 넣을까하다가 비동기로직과 아닌 로직을 구분하는게 좋을 것 같아서 분리했어요.
혹시 다른 분들도 의견도 어떤지 궁금해요~ㅎㅎㅎ
@Transactional | ||
@Service | ||
class MailHistoryService( | ||
private val emailHistoryRepository: EmailHistoryRepository |
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.
현재 Email
과 Mail
이 혼용되어서 사용되고 있네요
MailService, MailData, MailService 과 EmilahistoryRepository response 형태에서 사용하는 eamil 필드같은 경우인데
두 가지를 혼용해서 사용해도 괜찮은 부분일까요?
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.
저도 이 부분에 대해서 고민이 되었는데요. 기존 몇몇 구현되어 있는 코드가 MailSerive
MailData
등등 email 대신 mail이라는 키워드를 사용하더라구요. email이라는 내용에 특정하지 않고 좀 더 범용적인 의미로 mail을 사용한 걸까 싶은 생각이 들었어요. 기존 작성되어 있는 코드를 따라가기 위해서 Mail
이라는 단어를 사용하였어요.
EmailHistoryRepository
는 제가 수정하면서 놓친 부분이네요. 일단 MailHistoryRepository
로 수정해놓을게요.
이 또한 다른 분들의 의견도 받아서 용어를 mail이 아닌 email로 통일하고 싶다는 의견이 많다면 반영하겠습니다.
val user = userRepository.findByEmail(email) | ||
if (user != null) { | ||
return MailTargetResponse(user.name, user.email) | ||
} | ||
return MailTargetResponse(email = email) |
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.
val user = userRepository.findByEmail(email) | |
if (user != null) { | |
return MailTargetResponse(user.name, user.email) | |
} | |
return MailTargetResponse(email = email) | |
val user = userRepository.findByEmail(email) ?: return MailTargetResponse(email = email) | |
return MailTargetResponse(user.name, user.email) |
이렇게 사용하시는 건 어떨까요?
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.
고생하셨습니다. 👍
소소한 피드백을 남겼습니다.
@@ -150,7 +151,7 @@ data class EvaluationTargetData( | |||
) | |||
|
|||
data class MailTargetResponse( | |||
val name: String, | |||
val name: String = NO_NAME, |
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.
공백으로 채우고 View에서 변경하면 어떨까요? 응용 계층이 View에 의존하는 부분도 사라질 거예요.
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.
Cheater와 같은 로직으로 null 가능처리하고 null인 경우 NO_NAME 을 보여주도록 변경하였습니다 :)
val emailHistory = mailHistoryRepository.findAll() | ||
return emailHistory.map { | ||
MailData( | ||
it.subject, | ||
it.body, | ||
it.sender, | ||
it.recipients, | ||
it.sentTime, | ||
it.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.
재사용하지 않는 지역 변수의 수는 최소화하면 어떨까요? MailData
에 부 생성자를 추가할 수도 있어요.
val emailHistory = mailHistoryRepository.findAll() | |
return emailHistory.map { | |
MailData( | |
it.subject, | |
it.body, | |
it.sender, | |
it.recipients, | |
it.sentTime, | |
it.id | |
) | |
} | |
return mailHistoryRepository.findAll() | |
.map { MailData(it.subject, it.body, it.sender, it.recipients, it.sentTime, it.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.
이미 부 생성자를 만들어 놨었는데 활용하지 못하고 있었네요! 수정하였습니다~ㅎㅎㅎ
@@ -34,4 +34,9 @@ class UserService( | |||
fun editInformation(id: Long, request: EditInformationRequest) { | |||
userRepository.getById(id).changePhoneNumber(request.phoneNumber) | |||
} | |||
|
|||
fun findMailTargetByEmail(email: String): MailTargetResponse { |
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.
UserService
에 위치하는 것이 적절할까요?
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.
MailService
-> MailSenderService
MailHistoryService
-> MailService
로 변경하고, 해당 메소드는 MailService
로 이동하였습니다.
fun recipientsCount(): Int { | ||
return recipients.size | ||
} |
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.
오!! 👍 👍 상태는 프로퍼티 값을 참조하도록 변경하였어요!ㅎㅎㅎ 감사합니다~
@Column(nullable = false) | ||
@Lob | ||
val recipients: 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.
AttributeConverter
를 사용하면 어떨까요? 아래의 글이 도움 될 거예요.
https://www.baeldung.com/jpa-attribute-converters
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.
오~ 최고최고! 적용하였습니다! 매번 변환작업을 할 필요 없이 타입을 변환해줄 수 있군요!! 👍 👍
val sentTime: LocalDateTime = LocalDateTime.now(), | ||
|
||
id: Long = 0L |
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.
val sentTime: LocalDateTime = LocalDateTime.now(), | |
id: Long = 0L | |
val sentTime: LocalDateTime = LocalDateTime.now(), | |
id: Long = 0L |
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.
어노테이션이 없으면 붙이는 컨벤션이군요! 수정하였습니다~!!
setRowCount(mailData.recipientsCount()) | ||
this.fill(mailData) | ||
this.recipientFilter.isVisible = false | ||
this.mailTargetsGrid.getColumnByKey(DELETE_BUTTON).isVisible = false | ||
this.fileUpload.isVisible = false | ||
this.submitButton.isVisible = false |
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.
BindingFormLayout
을 상속한 'Form'과 페이지인 'View'를 분리하면 어떨까요?
#337 (comment)
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.
Form
과 View
를 분리하여 리팩토링 진행하였습니다 :)
data.recipients.forEach { | ||
mailTargets.add(userService.findMailTargetByEmail(it)) | ||
} |
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.
해당 쿼리 findAllByEmailIn
을 사용하여 1번의 쿼리로 실행하게 변경하였습니다. 하지만 이 경우, 보여줄 때 입력 순서가 아닌 user 에 해당하는 email 끼리 먼저 나오고, (이름없음)
이메일이 나오게 됩니다. 오히려 구분되어 있어서 좋을 것 같다는 생각이 들기도 한데 어떨지 모르겠네요ㅎㅎㅎ
@@ -0,0 +1,3 @@ | |||
package apply.utils | |||
|
|||
const val DELIMITER = "," |
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.
AttributeConverter
를 사용하면서 없어졌네요!!ㅎㅎㅎ
const val SEND_VALUE: String = "send" | ||
const val DETAIL_VALUE: String = "detail" |
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.
예약 발송 등 메일 수정 기능이 추가될 수 있으니 기존 NEW_VALUE
및 EDIT_VALUE
를 사용하면 어떨까요?
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.
넵넵! 기존 NEW_VALUE
와 EDIT_VALUE
를 사용하는 방식으로 변경하였습니다!
566c309
to
0a347b5
Compare
5971f6a
to
825d03b
Compare
sent_time datetime(0) not null, | ||
primary key (id) | ||
) engine = InnoDB | ||
default charset = utf8mb4; |
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.
확인 부탁드립니다 😄
부분 디테일 수정
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.
배럴 고생많으셨습니다 👍
남겼던 코멘트들은 구두로 즉시 처리해주셔서 바로 Approve 드렸습니다. 🙏
mailTargets.removeAndRefresh(response) | ||
private fun createEnterBox(): Component { | ||
return support.views.createEnterBox(labelText = "받는사람 추가") { | ||
if (it.isNotBlank()) { |
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.
이메일 형식이 아닌경우에도 입력이 안되게 예외처리(= 입력X + 에러창)하면 어떨까요? 🤔
이메일 정규식: [0-9a-zA-Z]([-_.]?[0-9a-zA-Z][(-_.]?[0-9a-zA-Z]{2,3}
🤗
(관리자는 실수하지 않으니까... 스킵하셔두 될지도 ...? 🙄)
|
||
@Entity | ||
class MailHistory( | ||
@Column(nullable = false) |
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.
제 의견은 Entity에도 제약조건을 두는 것이 좋다고 생각했는데 다른 Entity 중에서 따로 길이제한을 두는 Entity가 없어서 거기까지는 안하셔도 될 것 같아요 😃
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.
수고하셨습니다! 간단한 리뷰 하나 남겼어요~
@@ -44,35 +38,16 @@ class MailFormView( | |||
private val body: TextArea = createBody() | |||
private val mailTargets: MutableSet<MailTargetResponse> = mutableSetOf() | |||
private val mailTargetsGrid: Grid<MailTargetResponse> = createMailTargetsGrid(mailTargets) | |||
private val mailTargetGridTitle: Label = Label() |
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.
라벨이 footer로 바뀌면서 더이상 필요 없는것 같네요~
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.
확인했습니다! 고생많으셨어요
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.
고생하셨습니다.
마무리 댓글만 확인 부탁드릴게요. :)
val (id, value) = it.destructured | ||
if (value == EDIT_VALUE) { | ||
mailForm.fill(mailHistoryService.getById(id.toLong())) | ||
this.submitButton.isVisible = false |
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.
sumitButton을 가리킬 때 this를 사용하신 이유가 있을까요?
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.
명시적으로 나타내기 위해서 this를 넣었는데 제거했습니다! :)
} | ||
} | ||
|
||
private fun refreshRowCount() { |
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.
몇가지 코멘트 남겨봐요.
고생하시는게 눈에 보여서 반영은 안하셔도 됩니다 ㅋㅋㅋ
가볍게만 봐주세요. 화이팅!
src/main/kotlin/support/Dates.kt
Outdated
fun createLocalDateTime( | ||
now: LocalDateTime | ||
): LocalDateTime { | ||
return LocalDateTime.of(now.year, now.month, now.dayOfMonth, now.hour, now.minute, now.second) |
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.
정확히 어떤 의도로 만들었는지 파악은 안되지만, 초단위 아래를 버리려는 의도면 truncateTo(SECONDS)
라는 메서드가 있긴합니다!
파라미터도 now
보다는 time
정도가 좋지 않을까요?
add(element).also { | ||
mailTargetsGrid.dataProvider.refreshAll() | ||
refreshRowCount() | ||
} | ||
} | ||
|
||
private fun MutableSet<MailTargetResponse>.addAllAndRefresh(elements: Collection<MailTargetResponse>) { | ||
addAll(elements).also { mailTargetsGrid.dataProvider.refreshAll() } | ||
addAll(elements).also { | ||
mailTargetsGrid.dataProvider.refreshAll() | ||
refreshRowCount() | ||
} | ||
} | ||
|
||
private fun MutableSet<MailTargetResponse>.removeAndRefresh(element: MailTargetResponse) { | ||
remove(element).also { mailTargetsGrid.dataProvider.refreshAll() } | ||
remove(element).also { | ||
mailTargetsGrid.dataProvider.refreshAll() | ||
refreshRowCount() | ||
} |
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.
also가 없어도 돌아갈 거 같은데, 혹시 같은 컨텍스트로 묶으려는 이유가 있었을까요?
return Grid<MailTargetResponse>(10).apply { | ||
addSortableColumn("이름") { it.name ?: NO_NAME }.setFooter("받는사람: ${mailTargets.size}명") | ||
addSortableColumn("이메일", MailTargetResponse::email) | ||
addColumn(createRemoveButton()).key = DELETE_BUTTON |
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.
addColumn(createRemoveButton()).key = DELETE_BUTTON | |
addColumn(createRemoveButton()).apply { key = DELETE_BUTTON } |
동작은 같겠지만 여기는 apply
를 적용해서 column을 만드는 컨텍스트에 key 설정을 추가하는 느낌을 주면 어떨까요?
setItems(mailTargets) | ||
} |
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.
setItems(mailTargets) | |
} | |
setItems(mailTargets) | |
dataProvider.addDataProviderListener { | |
columns.first().setFooter("받는사람: ${mailTargets.size}명") | |
} | |
} |
이렇게 사용하면 refreshRowCount()
가 필요없긴 하네요.
전 직관적인 걸 좋아해서 지금 코드가 더 좋긴합니다!
override fun setParameter(event: BeforeEvent, @WildcardParameter parameter: String) { | ||
val result = FORM_URL_PATTERN.find(parameter) | ||
result?.let { | ||
val (id, value) = it.destructured | ||
if (value == EDIT_VALUE) { | ||
mailForm.fill(mailHistoryService.getById(id.toLong())) | ||
this.submitButton.isVisible = false | ||
} | ||
} ?: UI.getCurrent().page.history.back() // TODO: 에러 화면을 구현한다. | ||
} |
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.
override fun setParameter(event: BeforeEvent, @WildcardParameter parameter: String) { | |
val result = FORM_URL_PATTERN.find(parameter) | |
result?.let { | |
val (id, value) = it.destructured | |
if (value == EDIT_VALUE) { | |
mailForm.fill(mailHistoryService.getById(id.toLong())) | |
this.submitButton.isVisible = false | |
} | |
} ?: UI.getCurrent().page.history.back() // TODO: 에러 화면을 구현한다. | |
} | |
override fun setParameter(event: BeforeEvent, @WildcardParameter parameter: String) { | |
val result = FORM_URL_PATTERN.find(parameter) ?: return goBack() | |
val (id, value) = result.destructured | |
if (value == EDIT_VALUE) { | |
mailForm.fill(mailHistoryService.getById(id.toLong())) | |
this.submitButton.isVisible = false | |
} | |
} | |
private fun goBack() { | |
UI.getCurrent().page.history.back() // TODO: 에러 화면을 구현한다. | |
} |
depth가 적을수록 보기 좋다고 생각합니다 ㅎㅎ
private fun MutableSet<MailTargetResponse>.addAndRefresh(element: MailTargetResponse) { | ||
add(element).also { mailTargetsGrid.dataProvider.refreshAll() } | ||
add(element).also { | ||
mailTargetsGrid.dataProvider.refreshAll() |
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.
고생하셨습니다. 👍
Closes #306
메일관리
메일 전송
메일 이력 조회
참고
,
를 구분자로 저장하여 하나의 문자열로 저장