#651 도착 확인 기능 #652
#651 도착 확인 기능 #652
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds an arrival confirmation feature (#651) that allows participants to indicate they've arrived at the departure location. It also introduces optional room identifiers (emoji and numeric) to help distinguish between rooms departing around the same time. According to the PR description, the team will decide which identifier type to keep (emoji or numeric) after discussions.
Key Changes:
- Added
toggleArrivalHandlerendpoint to allow participants to mark their arrival status before departure - Implemented room identifier allocation system with emoji and numeric options
- Extended the room and participant schemas to support these new features
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/modules/roomIdentifier.ts |
New module for allocating unique emoji and numeric identifiers to rooms within a 30-minute time window |
src/services/rooms.ts |
Added toggleArrivalHandler for toggling arrival status; updated createHandler to allocate room identifiers |
src/routes/rooms.ts |
Added /arrival POST endpoint with validation for the new arrival toggle feature |
src/routes/docs/schemas/roomsSchema.ts |
Added Zod schema validation for toggleArrivalHandler with roomId and isArrived fields |
src/modules/stores/mongo.ts |
Extended participant schema with optional isArrived boolean; extended room schema with optional emojiIdentifier and numericIdentifier strings |
src/modules/populates/rooms.ts |
Updated room population and formatting logic to include new identifier and arrival fields in API responses |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
thxx132
left a comment
thxx132
left a comment
There was a problem hiding this comment.
LGTM 이제 numeric 이랑 비교해보고 괜찮은 것으로 가면 될 것 같아요
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const updateArrivalHandler: RequestHandler = async (req, res) => { | ||
| try { | ||
| const { roomId, isArrived } = req.body as UpdateArrivalBody; | ||
|
|
||
| const user = await userModel.findOne({ _id: req.userOid, withdraw: false }); | ||
| if (!user) { | ||
| return res | ||
| .status(400) | ||
| .json({ error: "Rooms/:id/updateArrival : User not found" }); | ||
| } | ||
|
|
||
| const roomObject = await roomModel | ||
| .findOneAndUpdate( | ||
| { | ||
| _id: roomId, | ||
| part: { | ||
| $elemMatch: { | ||
| user: user._id, | ||
| settlementStatus: { $nin: ["paid", "sent"] }, | ||
| }, | ||
| }, | ||
| }, | ||
| { $set: { "part.$.isArrived": isArrived } }, | ||
| { new: true } | ||
| ) | ||
| .lean() | ||
| .populate<RoomPopulatePath>(roomPopulateOption); | ||
|
|
||
| if (!roomObject) { | ||
| const participantExists = await roomModel.exists({ | ||
| _id: roomId, | ||
| "part.user": user._id, | ||
| }); | ||
| if (participantExists) { | ||
| return res.status(400).json({ | ||
| error: | ||
| "Rooms/:id/updateArrival : cannot update after settlement or payment", | ||
| }); | ||
| } | ||
| return res.status(404).json({ | ||
| error: "Rooms/:id/updateArrival : cannot find corresponding room", | ||
| }); | ||
| } | ||
|
|
||
| // 수정한 방 정보를 반환합니다. | ||
| const isOver = getIsOver(roomObject, user._id.toString()); | ||
| return res.send(formatSettlement(roomObject, { isOver })); | ||
| } catch (err) { | ||
| logger.error(err); | ||
| return res.status(500).json({ | ||
| error: "Rooms/:id/updateArrival : internal server error", | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The new updateArrivalHandler function lacks test coverage. Given that the codebase has comprehensive test coverage for room handlers (as seen in test/services/rooms.ts with tests for createHandler, joinHandler, commitSettlementHandler, etc.), this new handler should also have corresponding tests to verify:
- Successful arrival status updates
- Rejection when user is not found
- Rejection when user is not a participant
- Rejection when settlement/payment status prevents updates
- Correct response format
| export const allocateEmojiIdentifier = async (departureTime: Date) => { | ||
| const nearbyRooms = await roomModel | ||
| .find( | ||
| { | ||
| time: { | ||
| $gte: departureTime.getTime() - timeWindow, | ||
| $lte: departureTime.getTime() + timeWindow, | ||
| }, | ||
| }, | ||
| "emojiIdentifier" | ||
| ) | ||
| .lean(); | ||
|
|
||
| const usedEmojis = new Set( | ||
| nearbyRooms | ||
| .map((room) => room.emojiIdentifier) | ||
| .filter((identifier): identifier is string => Boolean(identifier)) | ||
| ); | ||
| const availableEmojis = emojis.filter((emoji) => !usedEmojis.has(emoji)); | ||
|
|
||
| return availableEmojis.length > 0 | ||
| ? availableEmojis[crypto.randomInt(availableEmojis.length)] | ||
| : undefined; | ||
| }; |
There was a problem hiding this comment.
The new allocateEmojiIdentifier function lacks test coverage. Given that the codebase has test coverage for other modules, this function should have tests to verify:
- Correct emoji allocation when emojis are available
- Returns undefined when all emojis are in use
- Proper filtering of already-used emojis
- Correct time window filtering of nearby rooms
|
|
||
| const createTime = new Date(time); | ||
| const departureTime = new Date(time); | ||
| const createTime = new Date(departureTime); |
There was a problem hiding this comment.
The variable departureTime is created from time, and then createTime is created from departureTime, but this is redundant. Line 52 should directly use time instead:
const departureTime = new Date(time);
const createTime = new Date(time);This is more efficient and clearer in intent since both are independent Date objects created from the same source.
| const createTime = new Date(departureTime); | |
| const createTime = new Date(time); |
thxx132
left a comment
thxx132
left a comment
There was a problem hiding this comment.
LGTM 입니다! 컨플릭트 해결하고 프론트랑 같이 머지하면 될 것 같아요
Summary
It closes #651
Extra info
회의를 통해 이모지와 숫자 식별자 중 무엇이 더 직관적일지 결정 후, 둘 중 하나만 유지할 예정입니다.
일단은 도착 여부를 정산 이후에는 변경하지 못하도록 해 두었는데, 구체적인 도착 여부 설정 가능한 조건과, 채팅 전송 여부를 회의를 통해 결정하면 좋을 것 같습니다.