-
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
#449-2 채팅 빈 문자열 검증 #502
#449-2 채팅 빈 문자열 검증 #502
Conversation
…449.2-chat-validation
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.
LGTM
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.
LGTM!
채팅 메세지 trim은 백이 아니라 프론트에서 처리하는 것 같은데, 혹시 모르니 백에서도 한 번 더 해주면 어떨까요?
app.js
Outdated
@@ -77,7 +77,7 @@ app.use(require("./src/middlewares/errorHandler")); | |||
const serverHttp = http | |||
.createServer(app) | |||
.listen(httpPort, () => | |||
logger.info(`Express 서버가 ${httpPort}번 포트에서 시작됨.`) | |||
logger.info(`Express server started from port ${httpPort}`) |
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.
엇 이거 no-show 관련 PR에서 작업하신 것 아니었나요..?
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 새로 파겠습니다
그리고 trim의 경우 의문인 게 예를들어 유저가 " 1"을 보내고 싶은 경우, 채팅을 보내도 "1"로 가게 되는데 저희가 이걸 trim을 통해 의도적으로 변환을 해줘도 되는 건지에 대한 생각이 있습니다
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.
그렇다면 trim 말고 python에서 rstrip처럼 오른쪽 공백만 제거하는 것은 어떨까요?
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.
유사 채팅 서비스인 카카오톡의 경우 양쪽 공백 모두 제거 안하는 것 같더라고요! 공백포함해서 50자 정도로 제한하면 괜찮지 않을까요?
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.
오, 글자 수 제한은 도배 방지를 위해 추가되면 좋을 것 같습니다! 저는 예전 트위터처럼 140자 제한으로 의견 드려봅니다. (50자는 경우에 따라 부족할 수도 있을 것 같아요)
trim을 제안드린 이유는 사용자가 맨 앞뒤의 공백을 의도하고 채팅을 보내는 경우가 드물 것 같아서 제안드린 것이긴 합니다! trim 관련해서는 다른 팀원 분들 의견도 같이 들어보고 결정해도 좋을 것 같습니다
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 Branch 사용하면 해결될 것 같은 느낌이네요
업데이트 사용해서 해당 문제는 해결하였습니다~! |
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.
LGTM!
Summary
It closes #449
채팅이 공백이 아닌 문자 하나 이상을 포함 해야만 validate 하도록 정규 표현식을 추가해주었습니다. (patterns.js 참고)
*현재대로라면 빈 문자열을 보낼 시 오류 페이지로 넘어가서 프론트 작업 확인 후 머지되어야 합니다!!
관련 front issue: sparcs-kaist/taxi-front#777
Extra info
Images or Screenshots
Further Work
[Front] 해당 패턴을 사용시 전송 버튼 disable로 변경 및 해당 validation 오류(400) 대처할 수 있어야 함.