-
Notifications
You must be signed in to change notification settings - Fork 2
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
Impl MEE 006 008 010 api #1217
base: dev
Are you sure you want to change the base?
Impl MEE 006 008 010 api #1217
Conversation
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.
👍 궁금한 사항들 남겨두었고, 안건 모듈을 만들어서 다시 확인해보면 좋을 것 같아요!
Update 11/25
주요 변경 사항은 |
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.
👍 👍 주석도 너무 잘 달아줘서 고맙고 지울 필요는 없어요! 본인이 고민했다는 것은 분명 나중에 이 코드를 보는분들도 고민할 것 같으니까요! 코멘트 확인하고 머지해주세요:)
const getEveryMappingDeletedAt = await tx | ||
.select({ isDeleted: MeetingMapping.deletedAt }) // CHACHA: 만약 모든 Meeting과 Agenda mapping이 deleted -> 그 Meeting은 공고 게시 상태로! | ||
.from(MeetingMapping) | ||
.where(and(eq(MeetingMapping.meetingId, meetingId))); |
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.
💊 아마 하나의 미팅에 포함된 아젠다 개수가 그렇게 많지는 않을거라(많아야 20개 정도?) 크게 문제는 되지 않지만, soft delete 된 row 개수가 궁금한 거라면 count 이용하면 응답을 더 깔끔하게 받을 수 있을것 같아요!
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.
❓ 원래는 서비스 + 컨트롤러 + 레포지토리가 전부 모듈 안으로 들어오는게 좋다고 생각했는데, 이번에는 한 쿼리가 agenda가 아닌 테이블도 건드리다 보니까 그 범위가 애매하다고 느껴집니다. 가능한 한 agendarepository 안으로 가져오고, 한 트랜잭션으로 묶이지 않아도 괜찮을 것 같은 쿼리를 분리하는 것에 대해 어떻게 생각해요?
요약 *
It closes #1215
스크린샷
이후 Task *