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-10T12:50:21Z

@khanniie @raclim

Ready for review!

I know this is a big one as I have also migrated the user controller methods into somewhat of a barrel structure for organisation

I have annotated the places where the original code content has changed, but other than adding ! on req.user, I have not changed much

I will also make a PR into this branch to share for a proposal on improving the logic in updateSettings as I noticed some aspects that could be improved, but I didn't want to change it inside the same PR since it's already tricky to view the diffs.

All controller methods have been unit tested before migrating to maximum safety!

clairep94 · 2025-10-10T12:51:18Z

server/controllers/user.controller.js

Migrated this to a barrel structure based on the 'subdomain' of what each route is for, please see server/routes/user

Made sure to add unit tests to each item prior to migrating & did not change any logic within functions other than asserting req.user! on authenticated routes

clairep94 · 2025-10-10T12:53:07Z

server/controllers/user.controller/__testUtils__.ts

helper to create user with a few more properties than the mock file

i wasn't sure how sinon worked, so I just mocked a simple object with the relevant properties, and devs can override per test basis

clairep94 · 2025-10-10T12:54:02Z

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

-/* @jest-environment node */
-
 import { last } from 'lodash';
-import { Request, Response } from 'jest-express';


this was highlighted as not being an export when the file was changed to ts
I did some digging and found the actual place where these are exported which fixed it

clairep94 · 2025-10-10T12:55:45Z

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

      User.findById = jest.fn().mockResolvedValue(null);

-      await createApiKey(request, response);
+      await createApiKey(request, response, next);


Express RequestHandler require a next function, so added one here

this doesn't seem to be an issue where it's used in the actual non-test code though (routes/user)

clairep94 · 2025-10-10T12:56:51Z

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

      const lastKey = last(user.apiKeys);

-      expect(lastKey.label).toBe('my key');
-      expect(typeof lastKey.hashedKey).toBe('string');
+      expect(lastKey?.label).toBe('my key');
+      expect(typeof lastKey?.hashedKey).toBe('string');

      const responseData = response.json.mock.calls[0][0];
      expect(responseData.apiKeys.length).toBe(1);
      expect(responseData.apiKeys[0]).toMatchObject({
        label: 'my key',
-        token: lastKey.hashedKey
+        token: lastKey?.hashedKey


? was to resolve a ts error where it said lastKey might not exist

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

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

I think the logic on this controller method is a bit wonky, I will propose some changes on another PR that I'll tag you guys on

Included some comments in the test below re thoughts on the method

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

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

+        // NOTE: This should not pass, but it currently does!!
+        describe('and when there is no new password on the request', () => {
+          beforeEach(async () => {
+            requestBody = {
+              ...minimumValidRequest,
+              newPassword: undefined,
+              currentPassword: OLD_PASSWORD
+            };
+
+            // simulate check for password match passing
+            startingUser.comparePassword = jest.fn().mockResolvedValue(true);
+
+            request.setBody(requestBody);
+            await updateSettings(request, response, next);
+          });
+          it('calls saveUser with the new empty password', () => {
+            expect(saveUser).toHaveBeenCalledWith(response, {
+              ...startingUser,
+              password: undefined
+            });
+          });
+        });


highlighting this

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

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

+      // currently frontend doesn't seem to call password-change related things the below
+      // not sure if we should update the logic to be cleaner?


highlighting this comment -- I couldn't find where we change password on the UI, so not sure if this is being used?

if not maybe we could remove this
I will otherwise propose how to make the logic cleaner on the other PR

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

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

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

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

clean up authManagement tests

47015cd

clairep94 marked this pull request as ready for review October 10, 2025 01:03

clairep94 added 4 commits October 10, 2025 05:45

split up authManagement tests into sub files for easier reading

b0548ea

clean up tests by migrating frequent mocks to mock files

fd98175

resolve updateSetting tests with current logic

e46db9b

cleanup jsdocs

1426142

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

		// currently frontend doesn't seem to call password-change related things the below
		// not sure if we should update the logic to be cleaner?

	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!

clairep94 commented Oct 10, 2025

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!

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 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!

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!

Uh oh!

clairep94 commented Oct 10, 2025 •

edited

Loading

clairep94 Oct 10, 2025 •

edited

Loading

clairep94 Oct 10, 2025 •

edited

Loading

clairep94 Oct 10, 2025 •

edited

Loading

clairep94 Oct 10, 2025 •

edited

Loading