User module docs and logs #267

lyannne · 2026-01-23T21:06:47Z

ℹ️ Issue

Closes #257

📝 Description

Added logging and purpose statements to appropriate service methods.
Documented routes in swagger.
Made request body types in user.types.ts (used a json format so structure of data is clear to anyone testing).
Changed change-roles route from POST to PATCH (since nothing is getting created; it's more like a user being moved).

Later changed:

Changed delete-user to delete route, removed request body and made it a parameter.
Added guard to check if user is Admin or Employee since Inactive users should change anything.
Edited admin guard to contain user information.
Added 403 responses.
Fixed small frontend issue with register page saying "Don't have an account? Sign in" to "Have an account? Sign in".

✔️ Verification

N/A (just looked to see everything was there in swagger)

Test Changes

N/A

🏕️ (Optional) Future Work / Notes

N/A

…tch instead of post since that made more sense

added in

prooflesben

Just make sure the routes work in swagger but everything lgtm(lets get this money).

…tch instead of post since that made more sense

yumi520

lgtm

…nt be able to make any API calls, changed verifyuserguard instances to that

…ation, changed request bodies to use that information to get the user requested

Copilot

Pull request overview

This PR adds comprehensive logging, purpose statements, and Swagger documentation to the user module, following the requirements outlined in issue #257. The changes include updating HTTP methods to better align with RESTful conventions (POST to PATCH for change-role, POST to DELETE for delete-user), creating typed request bodies, adding a new guard for admin/employee authorization, and fixing a text error in the registration page.

Changes:

Added logging statements and purpose comments to all service methods
Documented all user controller routes with Swagger decorators (@apiresponse, @ApiParam, @ApiBearerAuth)
Changed change-role from POST to PATCH and delete-user from POST to DELETE to follow REST conventions
Created ChangeRoleBody type and modified endpoints to extract requestedBy from authenticated session
Added VerifyAdminOrEmployeeRoleGuard for endpoints requiring admin or employee access
Fixed registration page text from "Don't have an account?" to "Have an account?"

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
backend/src/user/user.service.ts	Added purpose statements, logging, error handling improvements, and email validation helper method
backend/src/user/user.controller.ts	Added comprehensive Swagger documentation, changed HTTP methods, updated to use typed request bodies and session-based authentication
backend/src/user/types/user.types.ts	Created ChangeRoleBody type for structured request data
backend/src/guards/auth.guard.ts	Added VerifyAdminOrEmployeeRoleGuard and enhanced VerifyAdminRoleGuard to attach user info to request
backend/src/auth/auth.controller.ts	Updated to use LoginBody type instead of individual body parameters
frontend/src/Register.tsx	Fixed text from "Don't have an account?" to "Have an account?"

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-24T21:42:49Z

backend/src/user/user.controller.ts

 import { User } from "../../../middle-layer/types/User";
 import { UserStatus } from "../../../middle-layer/types/UserStatus";
-import { VerifyAdminRoleGuard, VerifyUserGuard } from "../guards/auth.guard";
+import { VerifyAdminRoleGuard, VerifyUserGuard, VerifyAdminOrEmployeeRoleGuard } from "../guards/auth.guard";


Unused import: VerifyUserGuard is imported but never used in this file. All endpoints now use either VerifyAdminRoleGuard or VerifyAdminOrEmployeeRoleGuard.

Suggested change

import { VerifyAdminRoleGuard, VerifyUserGuard, VerifyAdminOrEmployeeRoleGuard } from "../guards/auth.guard";

import { VerifyAdminRoleGuard, VerifyAdminOrEmployeeRoleGuard } from "../guards/auth.guard";

Copilot · 2026-01-24T21:42:50Z

backend/src/user/user.controller.ts

+  @Patch("change-role")
+  @ApiResponse({
+    status : 200,
+    description : "User role changed successfully"
+  })
+  @ApiResponse({
+    status : 400,
+    description : "{Error encountered}"
+  })
+  @ApiResponse({
+    status : 401,
+    description : "Unauthorized"
+  })
+  @ApiResponse({
+    status : 403,
+    description : "Forbidden"
+  })
+  @ApiResponse({
+    status : 404,
+    description : "Not Found"
+  })
+  @ApiResponse({
+    status : 500,
+    description : "Internal Server Error"
+  })
  @UseGuards(VerifyAdminRoleGuard)
+  @ApiBearerAuth()
  async addToGroup(
-    @Body("user") user: User,
-    @Body("groupName") groupName: UserStatus,
-    @Body("requestedBy") requestedBy: User
+    @Body() changeRoleBody: ChangeRoleBody,
+    @Req() req: any
  ): Promise<User> {
+    // Get the requesting admin from the authenticated session (attached by guard)
+    const requestedBy: User = req.user;
+
    let newUser: User = await this.userService.addUserToGroup(
-      user,
-      groupName,
+      changeRoleBody.user,
+      changeRoleBody.groupName,
      requestedBy
    );
    return newUser as User;
  }


Breaking API change: Changed from POST to PATCH and modified request signature. The frontend code still uses POST method with the old signature that includes requestedBy in the body (see ApprovedUserCard.tsx:38-53 and PendingUserCard.tsx:33-44). This will break existing functionality until the frontend is updated to use PATCH and remove requestedBy from the request body.

Copilot · 2026-01-24T21:42:50Z

backend/src/user/user.controller.ts

+  @Delete("delete-user/:userId")
+  @ApiParam({
+    name: 'userId',
+    description: 'ID of the user to delete',
+    required: true,
+    type: String
+  })
+  @ApiResponse({
+    status : 200,
+    description : "User deleted successfully"
+  })
+  @ApiResponse({
+    status : 400,
+    description : "{Error encountered}"
+  })
+  @ApiResponse({
+    status : 401,
+    description : "Unauthorized"
+  })
+  @ApiResponse({
+    status : 403,
+    description : "Forbidden"
+  })
+  @ApiResponse({
+    status : 404,
+    description : "Not Found"
+  })
+  @ApiResponse({
+    status : 500,
+    description : "Internal Server Error"
+  })
  @UseGuards(VerifyAdminRoleGuard)
+  @ApiBearerAuth()
  async deleteUser(
-    @Body("user") user: User,
-    @Body("requestedBy") requestedBy: User
+    @Param('userId') userId: string,
+    @Req() req: any
  ): Promise<User> {
-    let deletedUser = await this.userService.deleteUser(user, requestedBy);
-    return user as User;
+    // Get the requesting admin from the authenticated session (attached by guard)
+    const requestedBy: User = req.user;
+
+    // Fetch the user to delete from the database
+    const userToDelete: User = await this.userService.getUserById(userId);
+
+    return await this.userService.deleteUser(userToDelete, requestedBy);
  }


Breaking API change: Changed from POST to DELETE and modified from body parameters to URL parameter. The frontend code still uses POST method with a request body containing user and requestedBy (see ApprovedUserCard.tsx:81-92 and PendingUserCard.tsx:64-75). This will break existing functionality until the frontend is updated to use DELETE with the userId as a URL parameter.

Copilot · 2026-01-24T21:42:50Z

backend/src/user/user.service.ts

+      this.logger.log(`Fetching user ${userId} from DynamoDB...`);
      const data = await this.dynamoDb.get(params).promise();
-      return data.Item;
+


The getUserById method doesn't handle the case when a user is not found in the database. DynamoDB's get operation returns an empty Item when the key doesn't exist, so data.Item will be undefined and will be cast to User. This should be validated and throw a NotFoundException when the user doesn't exist, especially since this method is now used by the deleteUser endpoint.

Suggested change

if (!data.Item) {

this.logger.warn(`User ${userId} not found in DynamoDB`);

throw new NotFoundException(`User with id ${userId} not found`);

}

Copilot · 2026-01-24T21:42:50Z

backend/src/user/types/user.types.ts

+  user!: {   
+    userId: string,
+    position: UserStatus,
+    email: string 
+  };


Missing @ApiProperty decorators on the class properties. While the ApiProperty import is present, the properties lack decorators, which means they won't appear in the Swagger documentation with proper descriptions and types. Each property should have an @ApiProperty decorator to document the expected structure in Swagger UI.

Suggested change

user!: {

userId: string,

position: UserStatus,

email: string

};

@ApiProperty({

type: 'object',

properties: {

userId: { type: 'string' },

position: { enum: Object.values(UserStatus) },

email: { type: 'string', format: 'email' },

},

})

user!: {

userId: string,

position: UserStatus,

email: string

};

@ApiProperty({ enum: UserStatus })

Copilot · 2026-01-24T21:42:51Z

backend/src/guards/auth.guard.ts

+        this.logger.warn("Access denied: User is not an Admin or Employee");
+        return false;
+      }
+


The new VerifyAdminOrEmployeeRoleGuard doesn't attach user information to the request object like VerifyAdminRoleGuard does (lines 77-82). While this guard is not currently used in endpoints that need req.user, it's inconsistent with the other guard and could lead to bugs if it's used in endpoints that expect req.user to be populated.

Suggested change

// Attach verified user information to the request, consistent with VerifyAdminRoleGuard

request.user = result;

Copilot · 2026-01-24T21:42:51Z

backend/src/user/user.controller.ts

 import { UserStatus } from "../../../middle-layer/types/UserStatus";
-import { VerifyAdminRoleGuard, VerifyUserGuard } from "../guards/auth.guard";
+import { VerifyAdminRoleGuard, VerifyUserGuard, VerifyAdminOrEmployeeRoleGuard } from "../guards/auth.guard";
+import { ApiResponse, ApiParam , ApiBearerAuth} from "@nestjs/swagger";


Inconsistent spacing in ApiParam decorator. There's a space after the comma between properties (e.g., "ApiParam , ApiBearerAuth"), which is inconsistent with the rest of the codebase and standard formatting conventions.

Suggested change

import { ApiResponse, ApiParam , ApiBearerAuth} from "@nestjs/swagger";

import { ApiResponse, ApiParam, ApiBearerAuth } from "@nestjs/swagger";

Copilot · 2026-01-24T21:42:51Z

backend/src/user/user.service.ts


-  async getUserById(userId: string): Promise<any> {
+  // purpose statement: retrieves user by their userId
+  // use case: not actually sure right now, maybe is there is an option for admin to click on a specific user to see details?


The purpose statement says "not actually sure right now, maybe is there is an option for admin to click on a specific user to see details?" This shows uncertainty about the use case. The method is now being used in the deleteUser endpoint (line 170 of user.controller.ts), so the purpose statement should be updated to reflect this concrete use case.

Suggested change

// use case: not actually sure right now, maybe is there is an option for admin to click on a specific user to see details?

// use case: used by the deleteUser endpoint (and similar admin operations) to load a specific user's record from DynamoDB before performing actions such as deletion or viewing details

Copilot · 2026-01-24T21:42:52Z

backend/src/user/types/user.types.ts

@@ -0,0 +1,11 @@
+import { ApiProperty } from '@nestjs/swagger';


Unused import ApiProperty.

Suggested change

import { ApiProperty } from '@nestjs/swagger';

prooflesben

addUserToGroup is missing some error loggin when errors are thrown. Missing some error logging around line 110 in user.service as well as 168

lyannne · 2026-01-25T21:29:56Z

addUserToGroup is missing some error loggin when errors are thrown. Missing some error logging around line 110 in user.service as well as 168

For line 110, I couldn't find a missing log but i updated everything else!

lyannne added 3 commits January 22, 2026 21:15

new types file

76955e2

logging

0d37649

swagger docs and types made, also switched change-roles route to a pa…

3932439

…tch instead of post since that made more sense

Copilot AI review requested due to automatic review settings January 23, 2026 21:06

lyannne linked an issue Jan 23, 2026 that may be closed by this pull request

DEV - Generate docs and log code for the user module #257

Open

Copilot started reviewing on behalf of lyannne January 23, 2026 21:07 View session

lyannne requested review from prooflesben and removed request for Copilot January 23, 2026 21:09

prooflesben and others added 2 commits January 23, 2026 16:44

added in

11b5bdc

Merge pull request #268 from Code-4-Community/Ben-Swagger-Auth-QuickFIx

aecad73

added in

prooflesben approved these changes Jan 23, 2026

View reviewed changes

prooflesben requested a review from yumi520 January 23, 2026 21:56

lyannne added 3 commits January 23, 2026 17:15

new types file

34e75b6

logging

a1d6804

swagger docs and types made, also switched change-roles route to a pa…

d93b28b

…tch instead of post since that made more sense

yumi520 approved these changes Jan 24, 2026

View reviewed changes

lyannne added 3 commits January 24, 2026 15:39

created VerifyAdminOrEmployeeRoleGuard since inactive accounts should…

29c297a

…nt be able to make any API calls, changed verifyuserguard instances to that

made delete route a delete, changed adminguard to contain user inform…

20f1fc0

…ation, changed request bodies to use that information to get the user requested

made delete route a delete, changed adminguard to contain user inform…

c0cc8e6

…ation, changed request bodies to use that information to get the user requested

Copilot AI review requested due to automatic review settings January 24, 2026 21:37

Copilot started reviewing on behalf of lyannne January 24, 2026 21:37 View session

fixed test

657accb

Copilot AI reviewed Jan 24, 2026

View reviewed changes

lyannne requested review from prooflesben and yumi520 January 25, 2026 03:49

prooflesben requested changes Jan 25, 2026

View reviewed changes

added more logging

e689fe9

lyannne requested a review from prooflesben January 25, 2026 21:34

	import { VerifyAdminRoleGuard, VerifyUserGuard, VerifyAdminOrEmployeeRoleGuard } from "../guards/auth.guard";
	import { VerifyAdminRoleGuard, VerifyAdminOrEmployeeRoleGuard } from "../guards/auth.guard";

+      if (!data.Item) {
+        this.logger.warn(`User ${userId} not found in DynamoDB`);
+        throw new NotFoundException(`User with id ${userId} not found`);
+      }



	// Attach verified user information to the request, consistent with VerifyAdminRoleGuard
	request.user = result;

	import { ApiResponse, ApiParam , ApiBearerAuth} from "@nestjs/swagger";
	import { ApiResponse, ApiParam, ApiBearerAuth } from "@nestjs/swagger";

	// use case: not actually sure right now, maybe is there is an option for admin to click on a specific user to see details?
	// use case: used by the deleteUser endpoint (and similar admin operations) to load a specific user's record from DynamoDB before performing actions such as deletion or viewing details

		@@ -0,0 +1,11 @@
		import { ApiProperty } from '@nestjs/swagger';

User module docs and logs #267

Are you sure you want to change the base?

User module docs and logs #267

Uh oh!

Conversation

lyannne commented Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ℹ️ Issue

📝 Description

✔️ Verification

Test Changes

🏕️ (Optional) Future Work / Notes

Uh oh!

prooflesben left a comment

Choose a reason for hiding this comment

Uh oh!

yumi520 left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

prooflesben left a comment

Choose a reason for hiding this comment

Uh oh!

lyannne commented Jan 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lyannne commented Jan 23, 2026 •

edited

Loading