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

#449-2 채팅 빈 문자열 검증 #502

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

Conversation

chlehdwon
Copy link
Contributor

@chlehdwon chlehdwon commented Mar 21, 2024

Summary

It closes #449

채팅이 공백이 아닌 문자 하나 이상을 포함 해야만 validate 하도록 정규 표현식을 추가해주었습니다. (patterns.js 참고)
*현재대로라면 빈 문자열을 보낼 시 오류 페이지로 넘어가서 프론트 작업 확인 후 머지되어야 합니다!!
관련 front issue: sparcs-kaist/taxi-front#777

Extra info

Images or Screenshots

image image

Further Work

[Front] 해당 패턴을 사용시 전송 버튼 disable로 변경 및 해당 validation 오류(400) 대처할 수 있어야 함.

@chlehdwon chlehdwon changed the title #449.2 채팅 빈 문자열 검증 #449-2 채팅 빈 문자열 검증 Mar 21, 2024
@chlehdwon chlehdwon self-assigned this Mar 21, 2024
Copy link

@xMHW xMHW left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@kmc7468 kmc7468 left a 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}`)
Copy link
Member

Choose a reason for hiding this comment

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

엇 이거 no-show 관련 PR에서 작업하신 것 아니었나요..?

Copy link
Contributor Author

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을 통해 의도적으로 변환을 해줘도 되는 건지에 대한 생각이 있습니다

Copy link

Choose a reason for hiding this comment

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

그렇다면 trim 말고 python에서 rstrip처럼 오른쪽 공백만 제거하는 것은 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

유사 채팅 서비스인 카카오톡의 경우 양쪽 공백 모두 제거 안하는 것 같더라고요! 공백포함해서 50자 정도로 제한하면 괜찮지 않을까요?

Copy link
Member

Choose a reason for hiding this comment

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

오, 글자 수 제한은 도배 방지를 위해 추가되면 좋을 것 같습니다! 저는 예전 트위터처럼 140자 제한으로 의견 드려봅니다. (50자는 경우에 따라 부족할 수도 있을 것 같아요)

trim을 제안드린 이유는 사용자가 맨 앞뒤의 공백을 의도하고 채팅을 보내는 경우가 드물 것 같아서 제안드린 것이긴 합니다! trim 관련해서는 다른 팀원 분들 의견도 같이 들어보고 결정해도 좋을 것 같습니다

Copy link
Member

Choose a reason for hiding this comment

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

앗 그리고 커밋 꼬인건 아래에 있는 Update Branch 사용하면 해결될 것 같은 느낌이네요

@chlehdwon
Copy link
Contributor Author

업데이트 사용해서 해당 문제는 해결하였습니다~!
해당 PR 머지는 trim 관련 논의 및 front 작업이 이뤄져야 하기 때문에 draft로 변경하겠습니다

@chlehdwon chlehdwon marked this pull request as draft March 22, 2024 14:14
@xMHW xMHW marked this pull request as ready for review March 26, 2024 14:50
Copy link
Member

@kmc7468 kmc7468 left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

채팅 검증 개선 및 오류 수정
3 participants