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

feat : 채팅방과 관련된 서비스 구현을 진행합니다. #91

Merged
merged 12 commits into from
Oct 1, 2024

Conversation

JoeCP17
Copy link
Contributor

@JoeCP17 JoeCP17 commented Sep 3, 2024

구현 진행항목

  • 채팅방 생성
  • 채팅방 참여 / 나가기
  • 유저가 참가중인 채팅방 조회 ( 전체 , 특정 Room 조회 )
  • 채팅방 내 메시지 조회 ( cursor 페이지네이션 적용 )

개선 진행항목

  1. Mock 항목을 대체하여 db와 연동한 Chat Room API를 구현합니다.
  2. 이전, Response 내 논의했던 응답값 내용을 추가합니다.
  3. 채팅방 메시지 조회 시, cursor 방식의 메시지 조회 기능을 추가합니다.

@JoeCP17 JoeCP17 changed the base branch from main to epic/chat September 3, 2024 06:08
@JoeCP17
Copy link
Contributor Author

JoeCP17 commented Sep 3, 2024

go lint 업데이트에 따른 G115 이슈항목 수정

references
moby/moby#48358

@JoeCP17 JoeCP17 changed the title feat : 채팅방 조회 Mock API와 채팅방 생성, 참여, 나가기 기능을 구현합니다. feat : 채팅방과 관련된 서비스 구현을 진행합니다. Sep 12, 2024
@JoeCP17 JoeCP17 self-assigned this Sep 12, 2024
@JoeCP17 JoeCP17 added the feat New feature label Sep 12, 2024
@JoeCP17 JoeCP17 requested a review from litsynp September 12, 2024 17:26
Copy link
Contributor

@litsynp litsynp left a comment

Choose a reason for hiding this comment

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

👍 전반적으로 괜찮아보이는데 코멘트 확인 부탁해요

Comment on lines +103 to +117
if prevQuery != "" {
var atoiError error
prev, atoiError = strconv.Atoi(prevQuery)
if atoiError != nil || prev <= 0 {
return 0, 0, 0, ErrInvalidPagination(errors.New("expected integer value bigger than 0 for query: prev"))
}
}

if nextQuery != "" {
var atoiError error
next, atoiError = strconv.Atoi(nextQuery)
if atoiError != nil || next <= 0 {
return 0, 0, 0, ErrInvalidPagination(errors.New("expected integer value bigger than 0 for query: next"))
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

prev, next는 optional string 이어야 해요. 지금은 optional integer 군요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

맞아요 id값 uuid으로 마이그레이션 진행될 경우 변경할 예정입니다.

현재는, integer 값으로 설정되어있어 임시로 처리한 상황입니다

Copy link
Contributor

Choose a reason for hiding this comment

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

chat 쪽은 마이그레이션 대상이 아니었긴 한데... 한번에 해도 괜찮을 것 같긴 하네요
그럼 한번에 epic 브랜치에서 리베이스해서 다시 마이그레이션 PR 업데이트해볼게요

@@ -72,7 +73,7 @@ func NewRouter(app *firebaseinfra.FirebaseApp) (*echo.Echo, error) {
// wsServer := chat.NewWebSocketServer(stateManager)
// go wsServer.Run()
// chat.InitializeWebSocketServer(ctx, wsServer, chatService)
// chatHandler := handler.NewChatController(wsServer, stateManager, authService, *chatService)
// chatHandler := handler.NewChatHandler(wsServer, stateManager, authService, *chatService)
Copy link
Contributor

Choose a reason for hiding this comment

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

여기 지워도 될 듯합니다

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NewChatHandler 말씀하시는거 맞을까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

넵 주석이기도 하고 이미 chatHandler 생성을 위에서 하셔서 여긴 필요 없을 것 같아요!

}

type Message struct {
ID int64 `field:"id" json:"id"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Room, Message 모두 ID(및 FK)에 uuidv7를 써볼까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

네넹 추후 한번 여쭤보려 하긴했는데, uuid PR 참고해서 바꿔두면 될까요??

Copy link
Contributor

Choose a reason for hiding this comment

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

마이그레이션 이후에 한번에 바꾸죠!

type CreateRoomRequest struct {
RoomName string `json:"roomName" validate:"required"`
RoomType string `json:"roomType" validate:"required"`
JoinUserIDs *[]int64 `json:"joinUserIds"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Create 할 땐 방장 한 명, Join할 때도 한 명(로그인한 유저) 일 것 같은데 아닐까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

제가 생각했던 유입될 수 있는 비지니스 플로우는 아래와 같은데요.

  1. 이벤트에 참여를 한 유저들이 채팅방 생성시 자동 참여가 될 수 있다.

Create 되는 시점엔 N명의 참여자가 발생할수도 있다고 생각해, Array 형태로 유저 아이디를 받아 최초 처리 할 수있도록 처리를 해뒀어요

그 외, 중도 참여자가 발생할 경우는 1명의 유저가 Join할 수 있도록 JoinChatRoom API를 따로 빼뒀습니당

Copy link
Contributor

Choose a reason for hiding this comment

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

(한명의 사용자에 의해) 이벤트 생성 직후 바로 채팅방이 만들어질 것 같아요! 그래서 한명만 있으면 될 것 같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

�이벤트에 참여하는 참석자는 별도로, 채팅방에 참여해야하기에 채팅방 생성 시점은 방장 한명만 참가하도록 생성된다.

위 제가 이해한 방향이 맞을까요!? 맞다면 다시 수정해둘게용

Copy link
Contributor

Choose a reason for hiding this comment

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

넵 맞습니다. 그리고 JoinRoom은 로그인한 유저 한명만 조인하도록 하고 있으니 현행 유지하면 될 것 같아요!

@JoeCP17 JoeCP17 requested a review from litsynp September 14, 2024 15:14
@JoeCP17 JoeCP17 merged commit bee2ca4 into epic/chat Oct 1, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants