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

History Service #69

Merged
merged 13 commits into from
Nov 3, 2024
Merged

History Service #69

merged 13 commits into from
Nov 3, 2024

Conversation

samuelim01
Copy link

Description

Create history service with API endpoints, consumers, and basic tests.

Checklist

  • I have updated documentation
  • All tests passing

Screenshots (if applicable)

@samuelim01 samuelim01 added this to the Milestone 6 milestone Oct 31, 2024
@samuelim01 samuelim01 self-assigned this Oct 31, 2024
Copy link

@LimZiJia LimZiJia left a comment

Choose a reason for hiding this comment

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

LGTM. Works on my end. Just a few questions because I am unfamiliar

The most confusing part is the naming of closeRoomController and updateUserStatusInRoomController in collaboration controller, because they don't say its close because of success and update because of forfeit. Maybe could make use of in-line comments?

Comment on lines +16 to +17
export async function updateHistory(roomId: IdType, userId: IdType, status: HistoryStatus) {
return await HistoryModel.findOneAndUpdate({ roomId, 'user._id': userId }, { $set: { status } }, { new: true });
Copy link

Choose a reason for hiding this comment

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

Is this because different users from the same room can have different status? If not why not update both users at once instead of sending two messages

Choose a reason for hiding this comment

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

User1 can forfeit while User2 can continue on with coding, so we had to separate it out with isForfeit status (according to the user) and roomStatus to indicate whether the room still has a remaining user. I guess the data representation is not so good now that I think about it😅😅as you'll have to check if room is close and check for forfeits at the same time to tell whether the room was closed due to forfeit or successful submission.
Eg; user1-> isForfeit = true, user2 -> isForfeit = true and room is closed would mean that the match ended because both users forfeited.

Copy link
Author

Choose a reason for hiding this comment

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

yep, one user could forfeit and the other could submit. A lil funny but I think it does make sense

Comment on lines +16 to +25
jwt.verify(token, config.JWT_SECRET, async (err, user) => {
const result = userSchema.safeParse(user);
if (err || result.error) {
handleUnauthorized(res, 'Authentication failed');
return;
}

req.user = result.data;
next();
});
Copy link

Choose a reason for hiding this comment

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

I am not too sure, but is (err, user) the decoded jwt token? user is the normal schema from user service something like {username: , email: , hashedpasswrd, id: , isAdmin: , create_at,}?

The parsing with zod seems cool.

Copy link
Author

Choose a reason for hiding this comment

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

yep so (err, user) would probably be written better as (err, payload), where payload is the decoded jwt token payload and err would be present if there were any issues parsing the jwt token such as invalid format or past the expiry time. Frankly I'm not too sure if userSchema is the full schema of what's in the payload object, but the nice thing is zod does parsing (validate + filter to keep only the fields you specify) instead of just validation.

Here's how the accessToken is generated in the user service:

const accessToken = jwt.sign(
            {
                id: user.id,
                username: user.username,
                role,
            },
            config.JWT_SECRET,
            {
                expiresIn: '1d',
            },
        );

services/history/README.md Outdated Show resolved Hide resolved
services/history/README.md Outdated Show resolved Hide resolved
@samuelim01
Copy link
Author

LGTM. Works on my end. Just a few questions because I am unfamiliar

The most confusing part is the naming of closeRoomController and updateUserStatusInRoomController in collaboration controller, because they don't say its close because of success and update because of forfeit. Maybe could make use of in-line comments?

@KhoonSun47 could you add some inline comments for closeRoomController and updateUserStatusInRoomController to give the reader an idea of when the functions are used?

@samuelim01 samuelim01 merged commit dea4987 into main Nov 3, 2024
6 checks passed
KhoonSun47 added a commit that referenced this pull request Nov 3, 2024
* main:
  Create GitHub Actions for auto deployment to AWS (#66)
  History Service (#69)
  Minor Fix To Handle isForfeit Status (#78)

# Conflicts:
#	services/collaboration/src/controllers/roomController.ts
@samuelim01 samuelim01 mentioned this pull request Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants