-
Notifications
You must be signed in to change notification settings - Fork 2
Teams leave #136
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
base: dev
Are you sure you want to change the base?
Teams leave #136
Conversation
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.
Pull Request Overview
This PR implements a new "teams leave" endpoint that allows team members to leave their team, with special handling for team leaders who trigger team disbandment instead. The implementation includes comprehensive unit tests covering various scenarios and error cases.
- Adds a new
/teams/leavePOST endpoint with authentication and validation - Implements logic to handle member removal vs team disbandment for leaders
- Provides comprehensive test coverage for all edge cases and success scenarios
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/functions/teams/leave/handler.ts | Main handler implementing team leave logic with member removal and leader disbandment |
| src/functions/teams/leave/schema.ts | JSON schema validation for the endpoint requiring auth_token, auth_email, and team_id |
| src/functions/teams/leave/index.ts | Serverless function configuration for the POST /teams/leave endpoint |
| tests/teams-leave.test.ts | Comprehensive unit tests covering authentication, edge cases, and success scenarios |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
tests/teams-leave.test.ts
Outdated
| // mock validToken fail | ||
| (validateToken as jest.Mock).mockReturnValue(true); | ||
|
|
||
| (teamsDisband as unknown as jest.Mock) = jest.fn(); |
Copilot
AI
Aug 18, 2025
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.
This syntax is incorrect for TypeScript mock assignment. It should be (teamsDisband as jest.Mock) = jest.fn(); without the unknown cast.
| (teamsDisband as unknown as jest.Mock) = jest.fn(); | |
| (teamsDisband as jest.Mock) = jest.fn(); |
tests/teams-leave.test.ts
Outdated
| { | ||
| team_id: "team1234", | ||
| leader_email: "leader@gmail.com", | ||
| members: ["member1@gmail.com, member2@gmail.com"], |
Copilot
AI
Aug 18, 2025
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.
The members array contains a single string with comma-separated emails instead of separate string elements. It should be ["member1@gmail.com", "member2@gmail.com"] to properly test the logic.
| members: ["member1@gmail.com, member2@gmail.com"], | |
| members: ["member1@gmail.com", "member2@gmail.com"], |
src/functions/teams/leave/handler.ts
Outdated
| import type { ValidatedEventAPIGatewayProxyEvent } from '@libs/api-gateway'; | ||
| import { middyfy } from '@libs/lambda'; | ||
| import schema from './schema'; | ||
| import { MongoDB, validateToken, UserDoc, TeamDoc, teamsDisband } from '../../../util'; //change to actual disband |
Copilot
AI
Aug 18, 2025
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.
[nitpick] The comment indicates this is temporary code that needs to be replaced. Consider using a proper TODO comment format like // TODO: Replace teamsDisband with actual implementation.
| import { MongoDB, validateToken, UserDoc, TeamDoc, teamsDisband } from '../../../util'; //change to actual disband | |
| import { MongoDB, validateToken, UserDoc, TeamDoc, teamsDisband } from '../../../util'; // TODO: Replace teamsDisband with actual implementation |
src/functions/teams/leave/handler.ts
Outdated
|
|
||
| // 8. Check if user is team lead | ||
| if(teamInfo.role == "leader"){ | ||
| return await teamsDisband(auth_token, auth_email, team_id); //mocked function replace with right funtion name |
Copilot
AI
Aug 18, 2025
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.
There's a typo in the comment: 'funtion' should be 'function'.
| return await teamsDisband(auth_token, auth_email, team_id); //mocked function replace with right funtion name | |
| return await teamsDisband(auth_token, auth_email, team_id); //mocked function replace with right function name |
src/functions/teams/leave/handler.ts
Outdated
| pending_invites: [] | ||
| }; | ||
|
|
||
| // 11. update the mondoDB user |
Copilot
AI
Aug 18, 2025
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.
There's a typo in the comment: 'mondoDB' should be 'MongoDB'.
| // 11. update the mondoDB user | |
| // 11. update the MongoDB user |
avsomers25
left a comment
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.
Overall nice work! Just a few small things. to start make sure to run the linter and prettier before submitting a pr, and then take a look at the copilot comments there's a couple small things to fix. Lastly the teamDisband exists so you should update your code to use it. But most of this is great stuff!
|
Fixed all issues. Failing eslint because of the naming convention of auth_email, auth_token, team_id, which is how it is written in my issue. Can change if needed. |
|
@SivamshIndukuri It's weird because we use camelCase for variables but snake_case for database properties and API request body. You can re-name the variables in destructuring |
|
fixed all issues now |
Ayoobf
left a comment
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.
Excellent work, I've added my suggestions!
src/functions/teams/leave/handler.ts
Outdated
| // 7. Check if user is in team | ||
| if (!team.members.includes(authEmail)) { | ||
| return { | ||
| statusCode: 400, | ||
| body: JSON.stringify({ statusCode: 400, message: 'User not in team' }), | ||
| }; | ||
| } |
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.
What if leader leaves the team? At the moment, attempting this operation will result in a "user not in team" error, when in reality, the team should be disbanded. As stated earlier, team.members does NOT include the team leader so you will have to explicitly check for that case.
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.
Crude alternative but you get the point.
| // 7. Check if user is in team | |
| if (!team.members.includes(authEmail)) { | |
| return { | |
| statusCode: 400, | |
| body: JSON.stringify({ statusCode: 400, message: 'User not in team' }), | |
| }; | |
| } | |
| // 7. Check if user is in team | |
| if (!team.members.includes(authEmail) && team.leader_email !== (authEmail)) { | |
| return { | |
| statusCode: 400, | |
| body: JSON.stringify({ statusCode: 400, message: 'User not in team' }), | |
| }; | |
| } |
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.
export the teamsLeave endpoint HackRU-Backend\src\functions\index.ts as well.
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.
Where do you want me to export the index.ts
src/functions/teams/leave/handler.ts
Outdated
| // 6. No team members | ||
| if (team.members.length == 0) { | ||
| return { | ||
| statusCode: 400, | ||
| body: JSON.stringify({ statusCode: 400, message: 'Empty team member list' }), | ||
| }; | ||
| } |
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.
What if a team leader wants to leave a team that has no members? as of right now, the team leader is not included in that team.members.length query. When taking into account a team's capacity, you should always do team.members.length + 1 to get all members including the leader.
| body: JSON.stringify({ statusCode: 400, message: 'Team already disbanded' }), | ||
| }; | ||
| } | ||
|
|
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.
Do a validation check on leader email. Simple if exists check will be enough. The reason why we want to do this is because we may do a leader_email check later on. To be fair, a team's state should never be in this spot (where a team has no leader) but just to be safe.
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.
Do you want this check to be done if the leader wants to leave the team or always when running this endpoint
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.
Either or is fine. I think they are functionally equal so I'll leave it to your best judgement.
|
Fixed team lead logic issues |
I made an endpoint for members who leave a team and unit tests for it. I made a mock-up teamsDisband endpoint that needs to be replaced by the real function