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(mail): change to send mail to members only #753

Merged
merged 10 commits into from
Jul 9, 2024

Conversation

woowabrie
Copy link

Resolves #745

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

  • 메일 이력에서 메일 주소 제거

어떻게 해결했나요?

  • MailHistory, MailData 는 mail 주소 대신 memberId를 갖도록 변경
  • memberId 기반으로 MailTarget을 조회하는 메서드 추가

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

  • MailTargetService를 통해 수신자 정보를 조회하는 기존 로직을 보고 MailData는 수신자 memberId만 내려주고 MailTargetService 를 사용해 상세 정보(이름, 메일주소)를 조회하는 형태로 구현했습니다. 구현 방식에 문제가 없을지 확인해 주세요.
  • 어드민에서 보낸 메일을 확인할 때, 탈퇴한 사용자는 노출되지 않도록 했습니다. 사용성에 이상 없을지 검토해 주세요.

RCA 룰

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

@woowabrie woowabrie self-assigned this May 21, 2024
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.

고생하셨습니다. 👍
기능에는 문제가 없으며, 빠르게 반영할 수 있는 간략한 리뷰만 남겨 보았어요.

@@ -91,7 +91,7 @@ class MailService(
@Async
fun sendMailsByBcc(request: MailData, files: Map<String, ByteArrayResource>) {
val body = generateMailBody(request)
val recipients = request.recipients + mailProperties.username
val recipients = memberRepository.findAllById(request.recipients).map { it.email } + mailProperties.username
Copy link
Contributor

Choose a reason for hiding this comment

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

a: IN 절과 관련하여 재밌는 내용이 있네요.

얼마전에 Hibernate 를 사용하는 시스템에서 메모리 부족으로 Full GC 연속 발생 -> API 응답이 극단적으로 저하되는 현상이 발생한 적이 있습니다. Memory Leak 은 아니지만, 그래도 메모리를 매우 많이 사용하고, 그게 아니더라도 성능샹 이득이 있으므로, 아래 Hibernate Property를 무조건 켜주는게 좋지 않을까 생각이 됩니다. Hibernate 5.2.18 부터 지원합니다.

대신에 sql에 다음과 같이 들어갈 수 있는데, where in (1,2,3,1,1,1) DBA 분들께도 이런 옵션 때문에 중복되어서 뒤에 같은 ID가 나타난다는 것을 사전에 공유하면 좋겠습니다.

Comment on lines 154 to 156
mailData?.let {
mailTargetsGrid.columns.first().setFooter("받는사람: ${mailTargets.size}명 (탈퇴 회원 포함 총 ${it.recipients.size}명)")
} ?: mailTargetsGrid.columns.first().setFooter("받는사람: ${mailTargets.size}명")
Copy link
Contributor

Choose a reason for hiding this comment

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

r: 탈퇴한 회원을 표시할 방법을 결정하였으므로 더 이상 mailData가 필요하지 않아 코드를 이전 상태로 되돌릴 수 있겠네요. 병합 후 #754 에서 다시 한 번 확인하도록 하겠습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

c: 앞으로 다양한 AttributeConverter가 나올 수 있고, 이를 추상화할 생각도 하셨을 것 같아서 아래 두 글을 바탕으로 간단히 만들어 보았어요. 또한, 코틀린이기 때문에 StringToListConverter 파일에 두 클래스를 모두 모아 두어도 좋을 것 같아요!

abstract class StringToListConverter<T : Any>(
    private val type: KClass<T>,
) : AttributeConverter<List<T>, String> {
    override fun convertToDatabaseColumn(attribute: List<T>): String {
        return attribute.joinToString(COMMA)
    }

    override fun convertToEntityAttribute(dbData: String): List<T> {
        return dbData.split(COMMA).mapNotNull { type.safeCast(it) }
    }

    companion object {
        private const val COMMA: String = ","
    }
}

@Converter
class StringToLongListConverter : StringToListConverter<Long>(Long::class)

Copy link
Author

Choose a reason for hiding this comment

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

아하 사실은 앞으로 사용될 것일지 감이 안잡혀서 일단 기존 컨버터를 삭제하고 새로 만들었었어요.

공유주신 코드 기반으로 작성하면서 String -> 제너릭 타입으로의 변환을 위해 변환 함수를 받도록 변경 했습니다!
KClass의 확장 함수 중에 변환과 관련된 함수가 있다면 알려주세요 ㅎㅎ

@@ -141,16 +140,20 @@ class MailTargetServiceTest : BehaviorSpec({
}
}

Given("특정 이메일을 가진 회원이 없는 경우") {
val email = "[email protected]"
Given("메일 이력을 통해 회원 id 목록을 확인할 수 있는 경우") {
Copy link
Contributor

Choose a reason for hiding this comment

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

a: 탈퇴한 회원이 구분되지 않아 테스트 코드 수정이 필요해 보이네요. 병합 후 #754 에서 수정하면 될 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

이후 작업을 놓치지 않게 하기위해 아래 주석을 추가해뒀습니다.

// TODO[#754]: 탈퇴한 회원의 경우 default 정보가 노출된다.

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.

고생하셨습니다~

@woowabrie
Copy link
Author

@woowahan-pjs
조금 늦었지만🥲 피드백 주신 내용 반영했습니다.
병합 후에 #754 에서 작업할 내용들이 있는데 함께 체크할게요!

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.

고생하셨습니다. 👍

@woowahan-pjs woowahan-pjs changed the title feat: Send emails to members only feat(mail): change to send mail to members only Jul 9, 2024
@woowahan-pjs woowahan-pjs merged commit 1775d74 into develop Jul 9, 2024
@woowahan-pjs woowahan-pjs deleted the feature/mail-to-member branch July 9, 2024 05:06
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