-
Notifications
You must be signed in to change notification settings - Fork 1
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
Fix/#338 피드백, 탈퇴 변경 관련 수정 #348
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.
고생하셨습니다 변경점이 많았네요!
@ApiResponse(code = 400, | ||
message = "1. 사유를 선택 안한 경우, NO를 보내주세요. (feedbackType)\n" | ||
+ "2. 의견은 200 글자 이내로 입력해주세요. (comment)\n" | ||
+ "3. 의견이 없는 경우, 빈 스트링(\"\")을 보내주세요. (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.
이 부분 기존 내용에 해당 하는 부분이라서 지워야 할 것 같아요!
created_at datetime(6) null, | ||
updated_at datetime(6) null, | ||
comment varchar(300) null, | ||
feedback_type varchar(30) null |
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.
feedback_type 이미 배포 서버에 15개 데이터밖에 없지만 ... 살려두는 건 좀 그런가요 ? 낭비일까요
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.
90일 뒤면 사라지는데 ㅠ 배포 직전에 백업해두고 지우는걸로 하죵
public static final String SLACK_USER_DELETE = "SLACK_USER_DELETE"; | ||
public static final String SLACK_USER_DELETE_FEEDBACK = "SLACK_USER_DELETE_FEEDBACK"; |
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.
아예 지우던지(sqs 관련 로직이 삭제되었으니까) 위에 SLACK_USER_DELETE 여기에 삭제 주석 달아주면 좋을 것 같아요!
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.
FeedbackType enum 에는 deprecated 주석 있는데 여기는 다 삭제된 것 같아요 ..,.,. 저는 우선 살려두고 나중에 한번에 지우는게 좋지않을까 싶습니당 ..
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.
그 FeedbackType enum 을 아직 안 지운 이유는 v1 탈퇴를 살려두려면 필요해서였어요!
다른 것들은 탈퇴 알림 관련한 것들이라 지웠습니다!
@PreventDuplicateRequest | ||
@Version | ||
@Auth | ||
@PostMapping("/v1/user/delete/feedback") |
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.
이친구는 요번(v2)에 함께 생성된 친구니까 v2가 맞지 않을까 싶습니다 ?! 어떻게 생각하시나요
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.
아 저는 v1, v2 를 사용하는 이유가 기존 api 를 살려두고 같은 역할을 하는 api 를 새롭게 만들때 사용하는 걸로 이해했는데 아닌가욤..?!
해당 api 는 기존에 없던 아이라 v1 으로 했어요!
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.
흠 그렇군요 .,., 저는 이번 배포에 포함된 변경 및 추가 api 는 v2 로 명시하긴 했는데요 .. (ex. 대표룰 조회/수정)
컨벤션 정해둔게 없었던 것 같아서 이렇게 픽스하는 걸로 하면 좋을 듯 하네요. 제쪽은 v1 으로 맞춰서 배포 해두었습니다~~
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.
확인했습니다! 클라-서버 공지만 한번 확인 부탁드립니다!
그러면 해당 PR 은 머지해도 될까요?
67d3484
to
37ea16c
Compare
✒️ 관련 이슈번호
🔑 Key Changes
📢 To Reviewers