-
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 mail reservation #731
base: develop
Are you sure you want to change the base?
Changes from 32 commits
f89b43c
559f00a
faa24f5
81f6188
ad730d6
742c8a6
1d6032b
d09c01b
b8168e6
f12edfa
f5777a4
9e29d51
501928e
e914ed2
f766d5d
3b6cd96
ba9d7b0
16e0a74
e2d7e91
0a673d4
ee17685
1d11f1b
2c3341f
f0257c5
fb64635
237ba8c
4bf23cb
19cee58
8bf6500
f2760b4
49f91f6
6f23fa7
7cdbeda
3e96c58
4ba0c6f
1c21188
56a5115
5c5c180
dd427ec
0fe7829
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
package apply.application.mail | ||
|
||
import apply.domain.mail.MailHistory | ||
import apply.domain.mail.MailMessage | ||
import apply.domain.mail.MailReservation | ||
import apply.domain.mail.MailReservationStatus | ||
import java.time.LocalDateTime | ||
|
||
data class MailMessageResponse( | ||
val id: Long, | ||
val subject: String, | ||
val body: String, | ||
val sender: String, | ||
val recipients: List<String>, | ||
val createdDateTime: LocalDateTime, | ||
val sentTime: LocalDateTime?, | ||
val reservation: MailReservationResponse?, | ||
val histories: List<MailHistoryResponse> | ||
) { | ||
constructor( | ||
mailMessage: MailMessage, | ||
mailReservation: MailReservation? = null, | ||
mailHistories: List<MailHistory> = emptyList() | ||
) : this( | ||
mailMessage.id, | ||
mailMessage.subject, | ||
mailMessage.body, | ||
mailMessage.sender, | ||
mailMessage.recipients, | ||
mailMessage.createdDateTime, | ||
mailHistories.firstOrNull()?.sentTime, | ||
mailReservation?.let { MailReservationResponse(it) }, | ||
mailHistories.map { MailHistoryResponse(it) } | ||
) | ||
} | ||
|
||
data class MailReservationResponse( | ||
val id: Long, | ||
val mailMessageId: Long, | ||
val status: MailReservationStatus, | ||
val reservationTime: LocalDateTime | ||
) { | ||
constructor(mailReservation: MailReservation) : this( | ||
mailReservation.id, | ||
mailReservation.mailMessageId, | ||
mailReservation.status, | ||
mailReservation.reservationTime | ||
) | ||
} | ||
|
||
data class MailHistoryResponse( | ||
val id: Long, | ||
val mailMessageId: Long, | ||
val recipients: List<String>, | ||
val success: Boolean, | ||
val sentTime: LocalDateTime | ||
) { | ||
constructor(mailHistory: MailHistory) : this( | ||
mailHistory.id, | ||
mailHistory.mailMessageId, | ||
mailHistory.recipients, | ||
mailHistory.success, | ||
mailHistory.sentTime | ||
) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
package apply.application.mail | ||
|
||
import apply.domain.mail.MailHistoryRepository | ||
import apply.domain.mail.MailMessageRepository | ||
import apply.domain.mail.getOrThrow | ||
import org.springframework.stereotype.Service | ||
import org.springframework.transaction.annotation.Transactional | ||
|
||
@Transactional | ||
@Service | ||
class MailHistoryService( | ||
private val mailHistoryRepository: MailHistoryRepository, | ||
private val mailMessageRepository: MailMessageRepository | ||
) { | ||
fun findAll(): List<MailData> { | ||
val histories = mailHistoryRepository.findAll() | ||
val messagesById = mailMessageRepository.findAllById(histories.map { it.mailMessageId }).associateBy { it.id } | ||
return histories.map { MailData(messagesById.getValue(it.mailMessageId), it) } | ||
} | ||
|
||
fun getById(mailHistoryId: Long): MailData { | ||
val mailHistory = mailHistoryRepository.getOrThrow(mailHistoryId) | ||
val mailMessage = mailMessageRepository.getOrThrow(mailHistory.mailMessageId) | ||
return MailData(mailMessage, mailHistory) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,83 @@ | ||||||
package apply.application.mail | ||||||
|
||||||
import apply.domain.mail.MailHistoryRepository | ||||||
import apply.domain.mail.MailMessage | ||||||
import apply.domain.mail.MailMessageRepository | ||||||
import apply.domain.mail.MailReservation | ||||||
import apply.domain.mail.MailReservationRepository | ||||||
import apply.domain.mail.MailReservationStatus | ||||||
import org.springframework.stereotype.Service | ||||||
import org.springframework.transaction.annotation.Transactional | ||||||
import java.time.LocalDateTime | ||||||
|
||||||
@Transactional | ||||||
@Service | ||||||
class MailMessageService( | ||||||
private val mailService: MailService, | ||||||
private val mailMessageRepository: MailMessageRepository, | ||||||
private val mailReservationRepository: MailReservationRepository, | ||||||
private val mailHistoryRepository: MailHistoryRepository | ||||||
) { | ||||||
fun findSentMails(): List<MailMessageResponse> { | ||||||
val histories = mailHistoryRepository.findAll() | ||||||
val messagesById = findMessageMapById(histories.map { it.mailMessageId }) | ||||||
return messagesById.map { (id, message) -> | ||||||
MailMessageResponse( | ||||||
mailMessage = message, | ||||||
mailHistories = histories.filter { it.mailMessageId == id } | ||||||
) | ||||||
} | ||||||
} | ||||||
|
||||||
fun findReservedMails(): List<MailMessageResponse> { | ||||||
val reservations = mailReservationRepository.findByStatus(MailReservationStatus.WAITING) | ||||||
val messagesById = findMessageMapById(reservations.map { it.mailMessageId }) | ||||||
|
||||||
return reservations | ||||||
.filter { messagesById.contains(it.mailMessageId) } | ||||||
.map { | ||||||
MailMessageResponse( | ||||||
mailMessage = messagesById.getValue(it.mailMessageId), | ||||||
mailReservation = it | ||||||
) | ||||||
} | ||||||
} | ||||||
|
||||||
fun reserve(request: MailData): MailMessageResponse { | ||||||
val mailMessage = mailMessageRepository.save(request.toMailMessage()) | ||||||
val mailReservation = mailReservationRepository.save( | ||||||
MailReservation(mailMessageId = mailMessage.id, reservationTime = request.sentTime) | ||||||
) | ||||||
return MailMessageResponse(mailMessage, mailReservation) | ||||||
} | ||||||
|
||||||
fun cancelReservation(mailMessageId: Long) { | ||||||
val mailReservation = mailReservationRepository.findByMailMessageId(mailMessageId) | ||||||
?: throw IllegalArgumentException("메일 예약이 존재하지 않습니다. email: $mailMessageId") | ||||||
check(mailReservation.canCancel()) { "예약 취소할 수 없는 메일입니다." } | ||||||
mailReservationRepository.deleteById(mailReservation.id) | ||||||
mailMessageRepository.deleteById(mailReservation.mailMessageId) | ||||||
} | ||||||
|
||||||
fun sendReservedMail(standardTime: LocalDateTime = LocalDateTime.now()) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. c: 예약된 메일들을 보내는거라 s 붙이는게 좋겠네요
Suggested change
|
||||||
val reservations = mailReservationRepository.findByReservationTimeBetweenAndStatus( | ||||||
standardTime.minusMinutes(1), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1분 범위에 예약된 메일을 모두 발송하기 위해 1분 추가, 빼기를 한게 맞을까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 네 맞습니다! 트리거가 정확하게 시작하는걸 보장하기 어렵기도 하고, 메일 예약이 15분 단위로 설정되니 다른 시간의 예약을 조회할일은 없어 앞뒤로 1분을 넣었어요. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. r: 1시 10분이 예약되어 있다고 했을 때 1시 9분, 1시 10분, 1시 11분에 요청이 안오면 해당 메일은 발송을 못하는 케이스가 존재할 것 같은데요. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 오 그렇네요. 아마 이전에 코드를 짤 때 예약시간 수정을 생각하다 보니 사이값을 검색하도록 만들었던 것 같아요. |
||||||
standardTime.plusMinutes(1), | ||||||
MailReservationStatus.WAITING | ||||||
) | ||||||
val messagesById = findMessageMapById(reservations.map { it.mailMessageId }) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MailReservation이랑 MailMessage를 한 번에 불러오면 쿼리 호출 횟수도 줄이고 코드 복잡도도 줄어들거 같아요 @Query("select m" +
" from MailMessage m" +
" join MailReservation ms on ms.mailMessageId = m.id" +
" where ms.reservationTime between :from and :to" +
" and ms.status = :status")
fun findByReservationTimeBetweenAndStatus(
from: LocalDateTime,
to: LocalDateTime,
status: MailReservationStatus
): List<MailMessage> There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 오 이렇게하면 바로 예약 MailMessage 를 가져올 수 있군요! 넘 좋습니다 ㅎㅎㅎ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 쿼리에서 select에 ms를 추가하면 될거에요. 그런데 MailMessage가 MailReservation를 간접참조 하고 있으면 좀 더 수정해야 해요. dto를 새로 만들거나 객체를 참조하도록 바꾸는 방법이 있겠네요 |
||||||
|
||||||
reservations.forEach { mailReservation -> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. c: 해당 메일 발송 작업이 하나의 트랜잭션으로 묶여야 할 이유가 있을까요? 개별적으로 성공 실패가 나뉘는게 더 자연스럽지 않을까 합니다. |
||||||
mailReservation.send() | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 질문) 지금 트랜잭션으로 묶여 있는데 MailReservation.status가 SENDING로 DB에 존재하는 케이스가 생길 수 있나요? |
||||||
mailReservationRepository.save(mailReservation) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. c: 메일 예약을 개별 저장하는 것보다 한 번에 처리하는건 어떨까요 mailReservationRepository.saveAll(reservations) |
||||||
mailService.sendMailsByBccSynchronous(MailData(messagesById.getValue(mailReservation.id)), emptyMap()) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. c: MailService를 인프라용 객체가 아닌 도메인용으로 본다면 MailMessage 객체를 전달하는게 나을 것 같아요. val mailMessage = messagesById.getValue(mailReservation.id)
mailService.sendMailsByBccSynchronous(mailMessage) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 어드민에서 직접 사용하는 비동기 방식의 메일 전송 메서드와 맞추긴 했는데, 저도 고민되긴 하더라구요 ㅋㅋ |
||||||
mailReservation.finish() | ||||||
} | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 회의 시간에 제이슨이 언급하셨던 예약 메일 전송 시 트랜잭션 처리와 관련된 코드입니다. 기존의 메일 전송은 Async로 동작하지만, 이번에는 문제를 단순화하여 동기 방식으로 처리하기로 했어요. (현재 설정 상 초당 300건의 메일 전송이 가능) 혹시 제가 잘못 생각하고 있거나 아니면 더 좋은 동작 아이디어가 있다면 의견 부탁드립니다! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 지금은 의미가 없어 보이긴 하네요.. 하지만 비동기로 전환할 것을 염두에 두고 남겨둬도 좋을 것 같아요. 다른 메일 발송 기능은 비동기로 처리하고 있기도 하구요. |
||||||
|
||||||
private fun findMessageMapById(mailMessageIds: List<Long>): Map<Long, MailMessage> { | ||||||
return mailMessageRepository | ||||||
.findAllById(mailMessageIds) | ||||||
.associateBy { it.id } | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -4,12 +4,14 @@ import apply.application.ApplicationProperties | |||||
import apply.domain.applicationform.ApplicationFormSubmittedEvent | ||||||
import apply.domain.mail.MailHistory | ||||||
import apply.domain.mail.MailHistoryRepository | ||||||
import apply.domain.mail.MailMessageRepository | ||||||
import apply.domain.recruitment.RecruitmentRepository | ||||||
import apply.domain.recruitment.getOrThrow | ||||||
import apply.domain.user.PasswordResetEvent | ||||||
import apply.domain.user.UserRepository | ||||||
import apply.domain.user.getOrThrow | ||||||
import org.springframework.boot.autoconfigure.mail.MailProperties | ||||||
import org.springframework.context.ApplicationEventPublisher | ||||||
import org.springframework.core.io.ByteArrayResource | ||||||
import org.springframework.scheduling.annotation.Async | ||||||
import org.springframework.stereotype.Service | ||||||
|
@@ -24,11 +26,13 @@ private const val MAIL_SENDING_UNIT: Int = 50 | |||||
class MailService( | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. c: MailMessageService가 있고 MailService가 있으니까 헷갈리네요;;; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저도 만들면서 메일 관련 서비스들이 헷갈렸습니다... ( |
||||||
private val userRepository: UserRepository, | ||||||
private val recruitmentRepository: RecruitmentRepository, | ||||||
private val mailMessageRepository: MailMessageRepository, | ||||||
private val mailHistoryRepository: MailHistoryRepository, | ||||||
private val applicationProperties: ApplicationProperties, | ||||||
private val templateEngine: ISpringTemplateEngine, | ||||||
private val mailSender: MailSender, | ||||||
private val mailProperties: MailProperties | ||||||
private val mailProperties: MailProperties, | ||||||
private val eventPublisher: ApplicationEventPublisher, | ||||||
) { | ||||||
@Async | ||||||
@TransactionalEventListener | ||||||
|
@@ -90,10 +94,10 @@ class MailService( | |||||
|
||||||
@Async | ||||||
fun sendMailsByBcc(request: MailData, files: Map<String, ByteArrayResource>) { | ||||||
val mailMessage = mailMessageRepository.save(request.toMailMessage()) | ||||||
val body = generateMailBody(request) | ||||||
val recipients = request.recipients + mailProperties.username | ||||||
|
||||||
// TODO: 성공과 실패를 분리하여 히스토리 관리 | ||||||
val succeeded = mutableListOf<String>() | ||||||
val failed = mutableListOf<String>() | ||||||
for (addresses in recipients.chunked(MAIL_SENDING_UNIT)) { | ||||||
|
@@ -102,15 +106,23 @@ class MailService( | |||||
.onFailure { failed.addAll(addresses) } | ||||||
} | ||||||
|
||||||
mailHistoryRepository.save( | ||||||
MailHistory( | ||||||
request.subject, | ||||||
request.body, | ||||||
request.sender, | ||||||
request.recipients, | ||||||
request.sentTime | ||||||
) | ||||||
) | ||||||
saveMailHistories(mailMessage.id, succeeded, failed) | ||||||
} | ||||||
|
||||||
fun sendMailsByBccSynchronous(request: MailData, files: Map<String, ByteArrayResource>) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이번에는 메일 예약에서 파일 첨부 기능을 지원하지 않기로 해서 크게 신경쓰지는 않았는데, 장기적인 관점으로는 구구 말씀처럼 합치는 방향으로 가는게 좋아보여요 ㅎㅎ |
||||||
val mailMessage = mailMessageRepository.save(request.toMailMessage()) | ||||||
val body = generateMailBody(request) | ||||||
val recipients = mailMessage.recipients + mailProperties.username | ||||||
|
||||||
val succeeded = mutableListOf<String>() | ||||||
val failed = mutableListOf<String>() | ||||||
for (addresses in recipients.chunked(MAIL_SENDING_UNIT)) { | ||||||
runCatching { mailSender.sendBcc(addresses, mailMessage.subject, body, files) } | ||||||
.onSuccess { succeeded.addAll(addresses) } | ||||||
.onFailure { failed.addAll(addresses) } | ||||||
} | ||||||
|
||||||
saveMailHistories(mailMessage.id, succeeded, failed) | ||||||
} | ||||||
|
||||||
fun generateMailBody(mailData: MailData): String { | ||||||
|
@@ -124,4 +136,22 @@ class MailService( | |||||
} | ||||||
return templateEngine.process("mail/common", context) | ||||||
} | ||||||
|
||||||
private fun saveMailHistories( | ||||||
mailMessageId: Long, | ||||||
succeeded: MutableList<String>, | ||||||
failed: MutableList<String> | ||||||
) { | ||||||
val mailHistories = mutableListOf<MailHistory>() | ||||||
|
||||||
if (succeeded.isNotEmpty()) { | ||||||
mailHistories.add(MailHistory(mailMessageId, succeeded, true)) | ||||||
} | ||||||
|
||||||
if (failed.isNotEmpty()) { | ||||||
mailHistories.add(MailHistory(mailMessageId, failed, false)) | ||||||
} | ||||||
|
||||||
mailHistoryRepository.saveAll(mailHistories) | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,8 @@ import apply.domain.judgmentitem.JudgmentItemRepository | |
import apply.domain.judgmentitem.ProgrammingLanguage | ||
import apply.domain.mail.MailHistory | ||
import apply.domain.mail.MailHistoryRepository | ||
import apply.domain.mail.MailMessage | ||
import apply.domain.mail.MailMessageRepository | ||
import apply.domain.mission.Mission | ||
import apply.domain.mission.MissionRepository | ||
import apply.domain.recruitment.Recruitment | ||
|
@@ -57,6 +59,7 @@ class DatabaseInitializer( | |
private val missionRepository: MissionRepository, | ||
private val judgmentItemRepository: JudgmentItemRepository, | ||
private val assignmentRepository: AssignmentRepository, | ||
private val mailMessageRepository: MailMessageRepository, | ||
private val mailHistoryRepository: MailHistoryRepository, | ||
private val database: Database | ||
) : CommandLineRunner { | ||
|
@@ -429,13 +432,25 @@ class DatabaseInitializer( | |
} | ||
|
||
private fun populateMailHistories() { | ||
val mailMessage = MailMessage( | ||
subject = "[우아한테크코스] 프리코스를 진행하는 목적과 사전 준비", | ||
body = "안녕하세요.", | ||
sender = "[email protected]", | ||
recipients = listOf("[email protected]", "[email protected]", "[email protected]", "[email protected]"), | ||
creatorId = 1L, | ||
) | ||
mailMessageRepository.save(mailMessage) | ||
|
||
val mailHistories = listOf( | ||
MailHistory( | ||
subject = "[우아한테크코스] 프리코스를 진행하는 목적과 사전 준비", | ||
body = "안녕하세요.", | ||
sender = "[email protected]", | ||
recipients = listOf("[email protected]", "[email protected]", "[email protected]", "[email protected]"), | ||
sentTime = createLocalDateTime(2020, 11, 5, 10) | ||
mailMessageId = mailMessage.id, | ||
recipients = mailMessage.recipients.subList(0, 2), | ||
success = true | ||
), | ||
MailHistory( | ||
mailMessageId = mailMessage.id, | ||
recipients = mailMessage.recipients.subList(3, 4), | ||
success = true | ||
) | ||
) | ||
mailHistoryRepository.saveAll(mailHistories) | ||
|
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.
r:
@ Transactional
을 추가하고 타임아웃을 정해두시죠!