[SSF-211] promote volunteer to admin by jxuistrying · Pull Request #187 · Code-4-Community/ssf

jxuistrying · 2026-05-31T19:30:23Z

ℹ️ Issue

Closes [SSF-211]

📝 Description

added PATCH /api/users/:id/role endpoint for promotion
promotion updates role in DB, clears pantry assignments, and syncs Cognito groups
wrote service and controller tests
implemented column ui changes to promote to admin with confirmation modal
add alerts for success/error after promotion

✔️ Verification

used frontend to test promoting a volunteer to admin and checked the network tab to see user role updated to admin

🏕️ (Optional) Future Work / Notes

my yarn tests are failing currently but im not sure how to fix it yet.

jiang-h-y

Mostly lgtm, but could you move the promote button into a new column instead of putting it within "Actions" so it better matches the Figma design?

Like this:

jiang-h-y · 2026-06-01T15:57:52Z

Also backend tests are passing for me so not sure why you're having issues.

dburkhart07

few questions pending yurika's clarification, but still some good things to work on. make sure all these updates you make you make to the tests too!

dburkhart07 · 2026-06-03T08:01:15Z

+  @Roles(Role.ADMIN)
+  async promoteToAdmin(
+    @Param('id', ParseIntPipe) id: number,
+    @Body() dto: UpdateUserRoleDto,


This may be a very rare case where we do not need a body. We know we are already promoting the volunteer to admin, so we shouldn't need to verify any other data. Can we delete this and the dto, and the check in the controller?

dburkhart07 · 2026-06-03T08:01:36Z

    return this.usersService.update(id, dto);
  }

+  @Patch('/:id/role')


nit: Can we rename this to something more descriptive, perhaps @Patch('/:id/promote-volunteer)

renamed to promote-volunteer

dburkhart07 · 2026-06-03T08:02:35Z

+  async promoteToAdmin(
+    @Param('id', ParseIntPipe) id: number,
+    @Body() dto: UpdateUserRoleDto,
+  ): Promise<User> {


Most of our patch api calls will return void you'll see, let's do the same here and update the service function and all tests.

dburkhart07 · 2026-06-03T08:03:33Z

+        [userId],
+      );
+
+      if (user.userCognitoSub) {


This is a required field, it will aways exist

dburkhart07 · 2026-06-03T08:10:08Z

+      );
+    }
+
+    const queryRunner = this.dataSource.createQueryRunner();


I don't think we need any of this queryRunner, and we should be able to just set the volunteers to [] and save that. I think that may have been missed in the ticket, so can you do that? Should just be able to do this.repo.save instead (like all our other service functions).

dburkhart07 · 2026-06-03T08:12:00Z

+      if (user.userCognitoSub) {
+        await this.authService.addUserToGroup(user.email, 'admin');
+        await this.authService.removeUserFromGroup(user.email, 'volunteer');
+      }


Can we wrap all 3 service calls in one single transaction? Look at many of our other service functions (one example would be the create function in the donations service) to understand how we go about using a transactionManager. The boilerplate code should be identical to that.

dburkhart07 · 2026-06-03T08:14:50Z

  AdminCreateUserCommand,
 } from '@aws-sdk/client-cognito-identity-provider';

+import {


nit: include these imports in the same one as above

…the same as figma designs

…CognitoSub

dburkhart07 · 2026-06-09T07:36:21Z

+      expect(mockUserService.promoteVolunteerToAdmin).toHaveBeenCalledWith(1);
+    });
+
+    it('should throw NotFoundException from service when user not found', async () => {


we generally havent been testing for the sad cases in the controller, since primarily the service will handle bad input logic. controller tests for us are primarily just to make sure that the service is called once, and with the right parameters.

cuz of that, can we delete this test and the one below?

dburkhart07 · 2026-06-09T07:42:23Z

+  describe('promoteVolunteerToAdmin', () => {
+    it('should promote volunteer to admin successfully', async () => {
+      const userRepo = testDataSource.getRepository(User);
+      const volunteers = await userRepo.find({


in our dummy data, user id 7 is a volunteer i believe (from line 553). can we just use that for all of our promote checks here instead of finding all volunteers?

dburkhart07 · 2026-06-09T07:44:16Z

+      await userRepo.save(user);
+
+      await this.authService.addUserToGroup(user.email, 'admin');
+      await this.authService.removeUserFromGroup(user.email, 'volunteer');


if this removeUserFromGroup call fails, then our database will be inconsistent with Cognito (we will have an admin user in Cognito, but this will rollback and give us a volunteer in the database. For this second call, can we do a try catch and, in the catch, run await this.authService.removeUserFromGroup(user.email, 'admin'); so that we remove the just added admin, and then throw an error so that everything rolls back? can we also add a specific test for this?

dburkhart07 · 2026-06-09T07:45:21Z

+        where: { role: Role.VOLUNTEER },
+        relations: ['pantries'],
+      });
+      expect(volunteer).toBeDefined();


can we make sure hat the volunteer had pantries to begin with before, to show that the call removes them?

dburkhart07 · 2026-06-09T07:46:07Z

+      );
+    });
+
+    it('should throw BadRequestException when user is pantry', async () => {


nit: can we rephrase this to "when user is not a volunteer" since thats really what we want to test (any other role will work though).

dburkhart07 · 2026-06-09T07:47:36Z

+    });
+
+    it('should throw BadRequestException when user is already admin', async () => {
+      const admin = await testDataSource.getRepository(User).findOne({


same thing here, user id 1 should be an admin

dburkhart07 · 2026-06-09T07:47:47Z

+    });
+
+    it('should call Cognito addUserToGroup and removeUserFromGroup', async () => {
+      const volunteer = await testDataSource.getRepository(User).findOne({


see above comment about using id instead

dburkhart07 · 2026-06-09T07:48:13Z

+
+    it('should rollback if Cognito fails', async () => {
+      const userRepo = testDataSource.getRepository(User);
+      const volunteer = await userRepo.findOne({


dburkhart07 · 2026-06-09T07:51:10Z

+  return (
+    <Dialog.Root
+      open={isOpen}
+      onOpenChange={(e: { open: boolean }) => !e.open && onClose()}


nit: can we change this to if (!e.open) onClose(); like all our other modals?

dburkhart07 · 2026-06-09T08:03:49Z

    }
  }
+
+  async addUserToGroup(username: string, groupName: string): Promise<void> {


see slack comments

dburkhart07 · 2026-06-09T08:11:05Z

    return hmac.digest('base64');
  }

  async adminCreateUser({


i know this isnt part of your ticket, but can we also make this function take in the users role, so that they can be added to the appropriate group?

jxuistrying added 2 commits May 31, 2026 14:46

promote admin to volunteer feature

b372971

fix pantries.service.spec.ts missing DataSource

5b60321

jxuistrying changed the title ~~Jx/ssf 211 promote volunteer to admin~~ [SSF-211] promote volunteer to admin May 31, 2026

fix transaction rollback when Cognito fails

d932f0a

jiang-h-y self-requested a review May 31, 2026 23:13

jiang-h-y requested changes Jun 1, 2026

View reviewed changes

Yurika-Kan requested a review from dburkhart07 June 3, 2026 00:35

dburkhart07 self-assigned this Jun 3, 2026

dburkhart07 requested changes Jun 3, 2026

View reviewed changes

jxuistrying force-pushed the jx/SSF-211-promote-volunteer-to-admin branch from ae986a3 to d932f0a Compare June 7, 2026 19:54

jxuistrying added 5 commits June 7, 2026 17:09

simplfying promote volunteer endpoint and making promote to admin ui …

378d9e5

…the same as figma designs

fix failing tests: added DataSource provider and filter for null user…

d453689

…CognitoSub

fix test null for TypeORM

388487c

fix tests for userCognitoSub

5fcfab4

remove userCognitoSub check

2d30e74

jxuistrying requested review from dburkhart07 and jiang-h-y June 8, 2026 19:15

dburkhart07 requested changes Jun 9, 2026

View reviewed changes

Conversation

jxuistrying commented May 31, 2026

ℹ️ Issue

📝 Description

✔️ Verification

🏕️ (Optional) Future Work / Notes

Uh oh!

jiang-h-y left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiang-h-y commented Jun 1, 2026

Uh oh!

dburkhart07 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jiang-h-y left a comment •

edited

Loading