pr05 Typescript Migration #14: Migrate User Controller #3681

clairep94 · 2025-10-10T00:57:36Z

Migration of the userController to ts

Initial rough work here

Changes:

Add unit tests to all userController methods, which can act as examples of how to test for other contributors to do migration work
Organise user routes into "subdomains" (related topics of routes)
Move userController methods into barrel structure based on these "subdomains" -- NO LOGIC CHANGES
Explicitly declare userController methods as Express RequestHandler & extract the types for each route --> added to relevant /server/types file:
- params
- responseBody
- requestBody
- query
- These are then intended to be legible by the frontend to create tighter contracts (eg. user preferences redux)
Extend the User type in the Express namespace so that each Request.user is our custom User type
Added jsdocs to each type & controller method
Added simple mock files for some mailer related modules that kept being used in user controllers

Notes:
I have some suggestions for logic improvements on the methods, but I will make those on a PR into this branch that I will link below when completed so that it's easier to view the diff

I have verified that this pull request:

has no linting errors (npm run lint)
has no test errors (npm run test)
is from a uniquely-named feature branch and is up to date with the develop branch.
is descriptively named and links to an issue number, i.e. Fixes #123
meets the standards outlined in the accessibility guidelines

…pes and jsdoc

…e, and resolve type errors

…ypes

…tePreferences, no-verify

…eferences to /userPreferences & add response and request types

…teCookiePreferences & move to /userPreferences

…e to /authmanagement and add request and response types

clairep94 · 2025-10-10T13:04:12Z

server/controllers/user.controller/__tests__/signup.test.ts

+  describe('createUser', () => {
+    it('should return 422 if email already exists', async () => {
+      User.findByEmailAndUsername = jest.fn().mockResolvedValue({
+        email: '[email protected]',
+        username: 'anyusername'
+      });
+
+      request.setBody({
+        username: 'testuser',
+        email: '[email protected]',
+        password: 'password'
+      });
+
+      await createUser(request, response, next);
+
+      expect(User.findByEmailAndUsername).toHaveBeenCalledWith(
+        '[email protected]',
+        'testuser'
+      );
+      expect(response.status).toHaveBeenCalledWith(422);
+      expect(response.send).toHaveBeenCalledWith({ error: 'Email is in use' });
+    });
+    it('should return 422 if username already exists', async () => {
+      User.findByEmailAndUsername = jest.fn().mockResolvedValue({
+        email: '[email protected]',
+        username: 'testuser'
+      });
+
+      request.setBody({
+        username: 'testuser',
+        email: '[email protected]',
+        password: 'password'
+      });
+
+      await createUser(request, response, next);
+
+      expect(User.findByEmailAndUsername).toHaveBeenCalledWith(
+        '[email protected]',
+        'testuser'
+      );
+      expect(response.status).toHaveBeenCalledWith(422);
+      expect(response.send).toHaveBeenCalledWith({
+        error: 'Username is in use'
+      });
+    });
+  });


this is the only controller method where not all the logic was tested

I couldn't work out how to control what new User({..}) does in the test environment, but everything is otherwise 100% tested

clairep94 · 2025-10-10T13:05:46Z

server/controllers/user.controller/apiKey.ts

+ * Description:
+ *   - Create API key
+ */
+export const createApiKey: RequestHandler<


I thought putting these here helped a lot with hints in the IDE when you hover the method, but let me know if it should go elsewhere, or if you prefer a different format

looks great!

clairep94 · 2025-10-10T13:07:33Z

server/controllers/user.controller/authManagement.ts

+      res.status(404).json({ error: 'User not found' });
+      return;
+    }
+    user.username = req.body.username;


Suggested change

user.username = req.body.username;

user.username = req.body.username ?? user.username;

Or error early if req.body doesn't have username or email

clairep94 · 2025-10-10T13:09:02Z

server/controllers/user.controller/authManagement.ts

+        res.status(401).json({ error: 'Current password is invalid.' });
+        return;
+      }
+      user.password = req.body.newPassword!;


i had to add ! to req.body.newPassword to resolve the typeError where newpassword might not exist, without changing the code logic, but I would propose to move this block within if (req.body.newPassword)

clairep94 · 2025-10-10T13:10:10Z

server/controllers/user.controller/authManagement.ts

+/**
+ * - Method: `DELETE`
+ * - Endpoint: `/auth/github`
+ * - Authenticated: `false` -- TODO: update to true?


this is currently not a route that goes after the isAuthenticated middleware, should we put it behind isAuthenticated and remove the login checks?

On the UI I think I can only see if I'm logged in

i agree!! that makes sense

clairep94 · 2025-10-10T13:10:19Z

server/controllers/user.controller/authManagement.ts

+/**
+ * - Method: `DELETE`
+ * - Endpoint: `/auth/google`
+ * - Authenticated: `false` -- TODO: update to true?


same as above

clairep94 · 2025-10-10T13:11:08Z

server/controllers/user.controller/signup.ts

+          domain: `${protocol}://${req.headers.host}`,
+          link: `${protocol}://${req.headers.host}/verify?t=${token}`
+        },
+        to: req.user!.email


added to resolve type error for user potentially not existing
safe to do so since this is after middleware

clairep94 · 2025-10-10T13:13:46Z

server/controllers/user.controller/signup.ts

+> = async (req, res) => {
+  try {
+    const token = await generateToken();
+    const user = await User.findById(req.user!.id).exec();


added to resolve type error for user potentially not existing
this isn't a route thats hidden past the middleware, but I think it's called after the user has signed up, so their new user is already attached to the request/passport right?

clairep94 · 2025-10-10T13:14:06Z

server/controllers/user.controller/userPreferences.ts

+  UpdatePreferencesRequestBody
+> = async (req, res) => {
+  try {
+    const user = await User.findById(req.user!.id).exec();


added to resolve type error for user potentially not existing
safe to do so since this is after middleware

clairep94 · 2025-10-10T13:14:16Z

server/controllers/user.controller/userPreferences.ts

+  UpdateCookieConsentRequestBody
+> = async (req, res) => {
+  try {
+    const user = await User.findById(req.user!.id).exec();


added to resolve type error for user potentially not existing
safe to do so since this is after middleware

clairep94 · 2025-10-10T13:14:55Z

server/middleware/__tests__/isAuthenticated.test.ts

this file was originally in server/utils but it's a middleware so I thought it was more appropriate here
added test before moving

clairep94 · 2025-10-10T13:16:09Z

server/tsconfig.json

    "lib": ["ES2022"],
-    "types": ["node", "jest"]
+    "types": ["node", "jest", "express"],
+    "typeRoots": ["./types", "../node_modules/@types"]


updated so that we can extend the express namespace user type to be our user type in each request

clairep94 · 2025-10-10T13:16:48Z

server/types/express/index.d.ts

Added so that we can extend the express namespace user type to be our user type in each request.user

… BDD, simplified after manual testingon client

clairep94 · 2025-10-11T16:57:43Z

server/controllers/user.controller/__tests__/authManagement/updateSettings.test.ts

+      describe.skip('when given old username, old email, and matching current password and no new password', () => {
+        beforeEach(async () => {
+          requestBody = {
+            ...minimumValidRequest,
+            currentPassword: OLD_PASSWORD
+          };
+          request.setBody(requestBody);
+          await updateSettings(request, response, next);
+        });
+
+        it('returns 401 with an "New password is required" message', () => {
+          expect(response.status).toHaveBeenCalledWith(400);
+          expect(response.json).toHaveBeenCalledWith({
+            error: 'New password is required.'
+          });
+        });
+
+        it('does not save the user with the new password', () => {
+          expect(saveUser).not.toHaveBeenCalled();
+        });
+        it('does not send a confirmation email to the user', () => {
+          expect(mailerService.send).not.toHaveBeenCalled();
+        });
+      });


There's client-side protection for this, but maybe we should guard against it just in case?

Currently it would save with the user.password: undefined

clairep94 · 2025-10-11T17:00:11Z

server/controllers/user.controller/__tests__/authManagement/updateSettings.test.ts

+      describe.skip('when missing username', () => {
+        beforeEach(async () => {
+          request.setBody({ email: OLD_EMAIL });
+          await updateSettings(request, response, next);
+        });
+
+        it('returns 401 with an "Missing username" message', () => {
+          expect(response.status).toHaveBeenCalledWith(400);
+          expect(response.json).toHaveBeenCalledWith({
+            error: 'Username is required.'
+          });
+        });
+
+        it('does not save the user with the new password', () => {
+          expect(saveUser).not.toHaveBeenCalled();
+        });
+        it('does not send a confirmation email to the user', () => {
+          expect(mailerService.send).not.toHaveBeenCalled();
+        });
+      });
+
+      describe.skip('when missing email', () => {
+        beforeEach(async () => {
+          request.setBody({ username: OLD_USERNAME });
+          await updateSettings(request, response, next);
+        });
+
+        it('returns 401 with an "Missing email" message', () => {
+          expect(response.status).toHaveBeenCalledWith(400);
+          expect(response.json).toHaveBeenCalledWith({
+            error: 'Email is required.'
+          });
+        });
+
+        it('does not save the user with the new password', () => {
+          expect(saveUser).not.toHaveBeenCalled();
+        });
+        it('does not send a confirmation email to the user', () => {
+          expect(mailerService.send).not.toHaveBeenCalled();
+        });
+      });


Currently doesn't return an error but maybe this would add safety, will propose on the logic-change PR

clairep94 · 2025-10-11T17:22:05Z

server/controllers/user.controller/__tests__/authManagement/updateSettings.test.ts

+      describe.skip('when given new username, new email, and new password with valid current password', () => {
+        beforeEach(async () => {
+          requestBody = {
+            username: NEW_USERNAME,
+            email: NEW_EMAIL,
+            currentPassword: OLD_PASSWORD,
+            newPassword: NEW_PASSWORD
+          };
+          request.setBody(requestBody);
+          await updateSettings(request, response, next);
+        });
+        it('saves the user with the correct details once', () => {
+          expect(saveUser).toHaveBeenCalledWith(response, {
+            ...startingUser,
+            username: NEW_USERNAME,
+            email: NEW_EMAIL,
+            verified: STATUSES.Sent,
+            verifiedToken: GENERATED_TOKEN,
+            verifiedTokenExpires: TOKEN_EXPIRY_TIME,
+            password: NEW_PASSWORD
+          });
+          expect(saveUser).toHaveBeenCalledTimes(1);
+        });
+        it('sends a confirmation email to the user', () => {
+          expect(mailerService.send).toHaveBeenCalledWith(
+            expect.objectContaining({
+              subject: 'Mock confirm your email'
+            })
+          );
+        });
+      });
+    });


This currently doesn't pass, but given the UI seems to enable users to change both at the same time, I'll propose an update to the logic of the PR to do that

clairep94 · 2025-10-11T17:24:50Z

server/controllers/user.controller/__tests__/authManagement/updateSettings.test.ts

+      describe.skip('when given old username, old email, and matching current password and no new password', () => {
+        beforeEach(async () => {
+          requestBody = {
+            ...minimumValidRequest,
+            currentPassword: OLD_PASSWORD
+          };
+          request.setBody(requestBody);
+          await updateSettings(request, response, next);
+        });
+
+        it('returns 401 with an "New password is required" message', () => {
+          expect(response.status).toHaveBeenCalledWith(401);
+          expect(response.json).toHaveBeenCalledWith({
+            error: 'New password is required.'
+          });
+        });
+
+        it('does not save the user with the new password', () => {
+          expect(saveUser).not.toHaveBeenCalled();
+        });
+        it('does not send a confirmation email to the user', () => {
+          expect(mailerService.send).not.toHaveBeenCalled();
+        });
+      });


There's client-side protection for this, but maybe we should guard against it just in case?

Currently it would save with the user.password: undefined

khanniie · 2025-10-11T20:03:41Z

server/controllers/user.controller/__tests__/apiKey.test.ts

-  let request;
-  let response;
+describe('user.controller > api key', () => {
+  let request: any;


should the type be MockRequest?

Ah yeah I wanted to do that but typing request: MockRequest and response: MockResponse results in a bunch of type errors because the MockRequest is missing a bunch of methods that actual express Request types have

We could cast it like below per test instance, but it felt a bit verbose, what do you think?

Actually I think it's kind of nice that we're being explicit about what is happening, just updated to implement this!

khanniie · 2025-10-11T20:07:20Z

server/controllers/user.controller/__tests__/authManagement/3rdPartyManagement.test.ts

+jest.mock('../../../../utils/mail');
+
+describe('user.controller > auth management > 3rd party auth', () => {
+  let request: any;


same here (and for other files) is there any chance the type can be MockRequest ?

khanniie · 2025-10-11T20:07:59Z

server/controllers/user.controller/__tests__/authManagement/updateSettings.test.ts

amazing, thank you so much for being thorough!

khanniie · 2025-10-11T20:10:30Z

server/controllers/user.controller/apiKey.ts

+ * Description:
+ *   - Create API key
+ */
+export const createApiKey: RequestHandler<


looks great!

khanniie · 2025-10-11T20:37:37Z

server/controllers/user.controller/authManagement.ts

+ * - Id: `UserController.resetPasswordInitiate`
+ *
+ * Description:
+ *   - Send an Reset Email email to the registered email account


small typo? "send a reset email..." ?

khanniie · 2025-10-11T20:39:21Z

server/controllers/user.controller/helpers.ts

+ * @returns Sanitised user
+ */
+export function userResponse(
+  user: PublicUser & Record<string, any>


maybe <string, unknown>, if possible?

also, i could be super wrong, but is there any chance the user parameter should be User or a similar type that has more fields? Otherwise it looks like we're not narrowing in on the fields

Updated to fix this!

khanniie · 2025-10-11T20:46:58Z

server/controllers/user.controller/authManagement.ts

+/**
+ * - Method: `DELETE`
+ * - Endpoint: `/auth/github`
+ * - Authenticated: `false` -- TODO: update to true?


i agree!! that makes sense

khanniie

thank you so much!!

…est and response

clairep94 added 30 commits October 8, 2025 14:26

package.json: add @types/express

dcae2b7

Merge with develop

e4e759c

server/utils/isAuthenticated: update to ts, no-verify

fbccc7d

server/utils/isAuthenticated: update to named export & add Express ty…

a84bf81

…pes and jsdoc

server/routes/user.routes: organise routes into subdomains

7a61ed3

isAuthenticated: add test for middleware

d8435a4

move isAuthenticated to /server/middleware folder

f62e9c4

server/controllers/user.controller/apiKey.test: migrate to ts, no-verify

eae9ca6

server/controllers/user.controller/apiKey.test: resolve type-errors

b19d61f

server/controllers/user/apiKey: migrate to ts, no-verify

c2ea2b1

types/express: add AuthenticatedRequest type, no-verify

8848ed5

correctly extend the Express User interface with custom properties

548e345

server/controllers/user.controller: finish typing request and respons…

6ab4b3d

…e, and resolve type errors

server/controllers/user.controller: Add tests for signup routes

3f012dd

user controller: migrate relevant routes to /signup

f4997b6

create user.controller/helpers.js

5c2e5d9

controllers/user/helper: add tests and update to ts

d293ee7

migrate emailVerificationInitiate to user.controller/signup & add tests

f8a7891

server/controllers/user.controller/signup: update to ts, no-verify

e0e2aa3

server/controllers/user.controller/signup: add response and request t…

fff6e51

…ypes

server/controllers/user.controller/userPreferences: add test for upda…

238f545

…tePreferences, no-verify

server/controllers/user.controller/userPreferences: move updateUserPr…

df35eb4

…eferences to /userPreferences & add response and request types

server/controllers/user.controller/userPreferences: add test for upda…

c455dd6

…teCookiePreferences & move to /userPreferences

user controller tests: correct names in top-level describe

6708adb

user controller: add 3rd party auth management tests

2350199

user controller/auth management: migrate unlinkGithub and unlinkGoogl…

1afa044

…e to /authmanagement and add request and response types

fix usercontroller helpers test

85ef277

add test for resetPasswordInitiate

13403dd

migrate resetPasswordInitiate to /authManagement and add req res types

f475d5a

user controller: validResetPasswordToken, add test

d61ef5f

clairep94 commented Oct 10, 2025

View reviewed changes

clairep94 force-pushed the pr05/migrate_user_controller_oct branch from 899c10c to 1426142 Compare October 10, 2025 13:17

clairep94 changed the title ~~Pr05/migrate User Controller~~ pr05 Typescript Migration #14: Migrate User Controller Oct 10, 2025

user.controller/authManagement: update tests for updateSettings to be…

b492d3e

… BDD, simplified after manual testingon client

clairep94 commented Oct 11, 2025

View reviewed changes

update tests to correct typos

60bb02a

clairep94 commented Oct 11, 2025

View reviewed changes

clairep94 mentioned this pull request Oct 11, 2025

pr05 Typescript Migration #15: Redux base files & migrate user preferences redux system #3683

Open

18 tasks

khanniie reviewed Oct 11, 2025

View reviewed changes

khanniie previously approved these changes Oct 11, 2025

View reviewed changes

clairep94 added 4 commits October 12, 2025 14:32

fix typo

f8fe2de

stronger typing on userResponse

e7730d6

extend Request type for jest-express

7eeac88

server/controller/user: update tests to have stronger typing for requ…

509e00a

…est and response

clairep94 dismissed khanniie’s stale review via 509e00a October 12, 2025 18:35

	user.username = req.body.username;
	user.username = req.body.username ?? user.username;

Uh oh!

pr05 Typescript Migration #14: Migrate User Controller #3681

Are you sure you want to change the base?

pr05 Typescript Migration #14: Migrate User Controller #3681

Conversation

clairep94 commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

clairep94 Oct 10, 2025 • 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!

clairep94 Oct 11, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

clairep94 Oct 11, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clairep94 Oct 12, 2025 • 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!

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!

khanniie left a comment

Choose a reason for hiding this comment

clairep94 commented Oct 10, 2025 •

edited

Loading

clairep94 Oct 10, 2025 •

edited

Loading

clairep94 Oct 11, 2025 •

edited

Loading

clairep94 Oct 11, 2025 •

edited

Loading

clairep94 Oct 12, 2025 •

edited

Loading