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

implement admin push notification controller #420

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bellatoris
Copy link
Contributor

Checklist

  • 충분한 양의 자동화 테스트를 작성했는가?

    • 계단정복지도 서비스는 사이드 프로젝트로 진행되는 만큼 충분한 QA 없이 배포되는 경우가 많습니다. 따라서 자동화 테스트를 꼼꼼하게 작성하는 것이 서비스 품질을 유지하는 데 매우 중요합니다.
  • 아직 테스트는 작성안하였습니다.

  • 너무 오랜만에 비즈니스 로직 짜는거라 감을 잃었어요... 회초리 부탁드립ㄴ디ㅏ.

  • admin 을 통해 호출할거지만 한동안 admin 페이지가 만들어지기 전까지는 서버내에서 호출할거라 인증을 붙여야 하는지 아직 모르겠네요

  • 애초에 이 controller가 user domain에 속하는지 좀 아리송합니다.

@bellatoris bellatoris requested a review from a team as a code owner November 7, 2024 13:10
Copy link

github-actions bot commented Nov 7, 2024

🔥🔥🔥 Backend CI Failed. github action link 🔥🔥🔥

Copy link
Contributor

@Zeniuus Zeniuus left a comment

Choose a reason for hiding this comment

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

빌드가 깨졌어요 :회초리:

)
}

transactionManager.doAfterCommit {
Copy link
Contributor

Choose a reason for hiding this comment

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

굳이 doAfterCommit에 넣을 필요가 있나요? 걍 트랜잭션 밖에서 해도 될 것 같아서용

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ㅇㅎ 저는 그냥 두개가 같다고 생각했는데 수환님은 doAfterCommit이 PersistentContext에 접근 가능한 것을 염두하고 "트랜잭션 밖" 이라고 말씀하신거죠?

Comment on lines +575 to +595
schema:
type: object
properties:
userIds:
type: array
items:
type: string
notification:
type: object
properties:
title:
type: string
body:
type: string
deepLink:
type: string
required:
- body
required:
- userIds
- notification
Copy link
Contributor

Choose a reason for hiding this comment

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

이거 약간 컨벤션이 없긴 한데, request dto를 따로 만드는 게 좋은 것 같습니다. 이렇게 하면 openapi generator가 지맘대로 dto class를 생성하는데, 이 dto class 이름이 openapi generator 버전 따라서 조금씩 달라지더라구요.

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.

2 participants