Fix team gets cleared when user is checked in

.gitignore

@@ -1,4 +1,4 @@
-package-lock.json
+yarn.lock
 node_modules/
 dist/
 coverage/

Dockerfile

@@ -5,21 +5,21 @@ WORKDIR /app
 # bcrypt depends on node-pre-gyp
 RUN apk add --no-cache --virtual .gyp python3 make g++
 
-COPY --chown=node:node package.json yarn.lock ./
-RUN yarn install
+COPY --chown=node:node package.json package-lock.json ./
+RUN npm install
 
 COPY --chown=node:node entrypoint.sh /app
 COPY --chown=node:node backend/ /app/backend/
 COPY --chown=node:node frontend/ /app/frontend/
 
 ENV NODE_OPTIONS=--openssl-legacy-provider
 
-RUN yarn backend::build && \
-    API_BASE_URL=/api yarn frontend::build && \
+RUN npm run backend::build && \
+    API_BASE_URL=/api npm run frontend::build && \
     mv backend/dist/ tmp && rm -rf backend/ && mv tmp backend && \
     mv frontend/dist/ tmp && rm -rf frontend/ && mv tmp frontend
 
-RUN yarn install --production
+RUN npm install --production
 
 FROM node:alpine
 

backend/src/controllers/application-controller.ts

@@ -153,8 +153,8 @@ export class ApplicationController {
   @Authorized(UserRole.Moderator)
   public async admit(@Body() { data: userIDs }: IDsRequestDTO): Promise<void> {
     const users = await this._users.findUsersByIDs(userIDs);
-    const firstMissingIndex = users.findIndex((user) => user == null);
 
+    const firstMissingIndex = users.findIndex((user) => user == null);
     if (firstMissingIndex !== -1) {
       throw new NotFoundError(
         `no user with id ${userIDs[firstMissingIndex]} found`,

backend/src/services/application-service.ts

@@ -17,7 +17,7 @@ import {
   QuestionGraphServiceToken,
 } from "./question-service";
 import { ISettingsService, SettingsServiceToken } from "./settings-service";
-import { IUserService, UserServiceToken } from "./user-service";
+import { IUserService, UserServiceToken, PartialUser } from "./user-service";
 
 /**
  * A form containing questions and given answers.
@@ -402,14 +402,19 @@ export class ApplicationService implements IApplicationService {
 
     await this.replaceAnswers(user, questions, answers);
 
+    // partial updates to avoid accidentally setting relations that haven't been loaded
+    // to null.
+    const userUpdate: PartialUser = {
+      id: user.id,
+    };
+
     if (user.initialProfileFormSubmittedAt == null) {
-      user.initialProfileFormSubmittedAt = new Date();
+      userUpdate.initialProfileFormSubmittedAt = new Date();
       // send mail to user about successful submission
       await this._email.sendSubmissionEmail(user);
-      await this._users.updateUser(user);
     }
-    user.profileSubmitted = true;
-    await this._users.updateUser(user);
+    userUpdate.profileSubmitted = true;
+    await this._users.updateUser(userUpdate);
   }
 
   /**
@@ -420,15 +425,18 @@ export class ApplicationService implements IApplicationService {
     const now = Date.now();
     const millisecondsInHour = 60 * 60 * 1000;
 
-    for (const user of users) {
-      user.confirmationExpiresAt = new Date(
-        now + settings.application.hoursToConfirm * millisecondsInHour,
-      );
-
-      user.admitted = true;
+    const updates: PartialUser[] = [];
+    for (const { id } of users) {
+      updates.push({
+        id,
+        confirmationExpiresAt: new Date(
+          now + settings.application.hoursToConfirm * millisecondsInHour,
+        ),
+        admitted: true,
+      });
     }
 
-    await this._users.updateUsers(users);
+    await this._users.updateUsers(updates);
 
     const emailPromises = users.map((user) =>
       this._email.sendAdmittedEmail(user),
@@ -515,8 +523,10 @@ export class ApplicationService implements IApplicationService {
     await this.replaceAnswers(user, questions, answers);
 
     if (!user.confirmed) {
-      user.confirmed = true;
-      await this._users.updateUser(user);
+      await this._users.updateUser({
+        id: user.id,
+        confirmed: true,
+      });
     }
   }
 
@@ -568,16 +578,20 @@ export class ApplicationService implements IApplicationService {
    * @inheritdoc
    */
   public async declineSpot(user: User): Promise<void> {
-    user.declined = true;
-    await this._users.updateUser(user);
+    await this._users.updateUser({
+      id: user.id,
+      declined: true,
+    });
   }
 
   /**
    * @inheritdoc
    */
   public async checkIn(user: User): Promise<void> {
-    user.checkedIn = true;
-    await this._users.updateUser(user);
+    await this._users.updateUser({
+      id: user.id,
+      checkedIn: true,
+    });
   }
 }
 

backend/src/services/user-service.ts

@@ -1,6 +1,6 @@
 import { compare, genSalt, hash } from "bcrypt";
 import { Inject, Service, Token } from "typedi";
-import { Repository } from "typeorm";
+import { Repository, In } from "typeorm";
 import { IService } from ".";
 import { User } from "../entities/user";
 import { UserRole } from "../entities/user-role";
@@ -20,6 +20,14 @@ import {
 } from "./haveibeenpwned-service";
 import { UserListDto } from "../controllers/dto";
 
+/**
+ * To partially update a user (not insert, enforce id to be present).
+ * Beware that if you hvaen't loaded the team and teamRequest relations (they are not
+ * eagerly loaded), they are null and will therefore be cleared in the database on save.
+ * Prefer partial updates with only those fields you want to update.
+ */
+export type PartialUser = { id: number } & Partial<Omit<User, "id">>;
+
 /**
  * An interface describing user handling.
  */
@@ -103,13 +111,13 @@ export interface IUserService extends IService {
    * Updates the given user.
    * @param user The user to update
    */
-  updateUser(user: User): Promise<void>;
+  updateUser(user: PartialUser): Promise<void>;
 
   /**
    * Updates all given users.
    * @param users The users to update
    */
-  updateUsers(users: readonly User[]): Promise<void>;
+  updateUsers(users: readonly PartialUser[]): Promise<void>;
 
   /**
    * Finds all users.
@@ -384,21 +392,26 @@ export class UserService implements IUserService {
   public async findUsersByIDs(
     userIDs: readonly number[],
   ): Promise<ReadonlyArray<User | null>> {
-    const users = await this._users.findByIds(userIDs as number[]);
+    const users = await this._users.find({
+      where: {
+        id: In(userIDs),
+      },
+      relations: ["team", "teamRequest"],
+    });
     return users.map((user) => user ?? null);
   }
 
   /**
    * @inheritdoc
    */
-  public async updateUser(user: User): Promise<void> {
+  public async updateUser(user: PartialUser): Promise<void> {
     await this._users.save(user);
   }
 
   /**
    * @inheritdoc
    */
-  public async updateUsers(users: readonly User[]): Promise<void> {
+  public async updateUsers(users: readonly PartialUser[]): Promise<void> {
     await this._users.save(users as User[]);
   }
 

backend/test/services/application-service.spec.ts

@@ -40,6 +40,11 @@ describe(ApplicationService.name, () => {
   let settings: ISettingsService;
   let user: User;
 
+  const refreshUser = async (): Promise<void> => {
+    const userRepo = database.getRepository(User);
+    user = (await userRepo.findOne({ where: { id: 1 } }))!;
+  };
+
   const createQuestion = <T>(): Question<T> => {
     const question = new Question<T>();
     question.description = "";
@@ -128,7 +133,6 @@ describe(ApplicationService.name, () => {
     await settings.bootstrap();
 
     const users = new MockUserService();
-
     emails = new MockEmailTemplateService();
     service = new ApplicationService(
       questionGraph,
@@ -137,6 +141,23 @@ describe(ApplicationService.name, () => {
       users.instance,
       emails.instance,
     );
+
+    users.mocks.updateUser.mockImplementation(async (update) => {
+      if (update.id !== user.id) {
+        throw new Error("test bug");
+      }
+      await userRepo.save(update);
+      await refreshUser();
+    });
+
+    users.mocks.updateUsers.mockImplementation(async (updates) => {
+      if (updates.length !== 1 && updates[0].id !== updates[0].id) {
+        throw new Error("test bug");
+      }
+      await userRepo.save(updates);
+      await refreshUser();
+    });
+
     await service.bootstrap();
   });
 
@@ -576,8 +597,8 @@ describe(ApplicationService.name, () => {
     await patchSettingsServiceToReturnProfileFormQuestionsFromTheFuture();
 
     await service.admit([user]);
-    const { questions } = await service.getConfirmationForm(user);
 
+    const { questions } = await service.getConfirmationForm(user);
     expect(questions).toHaveLength(2);
 
     const updatedSettings = await settings.getSettings();

backend/test/services/application-user-team-integration.spec.ts

@@ -0,0 +1,114 @@
+import { User } from "../../src/entities/user";
+import { Team } from "../../src/entities/team";
+import { UserRole } from "../../src/entities/user-role";
+import {
+  ApplicationService,
+  IApplicationService,
+} from "../../src/services/application-service";
+import { UserService } from "../../src/services/user-service";
+import { TestDatabaseService } from "./mock/mock-database-service";
+import { MockEmailTemplateService } from "./mock/mock-email-template-service";
+import { MockLoggerService } from "./mock/mock-logger-service";
+import { MockSettingsService } from "../services/mock/mock-settings-service";
+import { MockQuestionGraphService } from "../services/mock/mock-question-graph-service";
+import { MockHaveibeenpwnedService } from "../services/mock/mock-haveibeenpwned-service";
+import { MockTokenService } from "./mock/mock-token-service";
+import { MockTeamsService } from "./mock/mock-teams-service";
+import { ApplicationController } from "../../src/controllers/application-controller";
+
+// To reproduce this particular bug, we need a test setup that uses a proper
+// application service and user service.
+describe("Application, User and Team Integration Spec", () => {
+  let applicationService: IApplicationService;
+  let database: TestDatabaseService;
+
+  let user: User;
+  let team: Team;
+
+  let controller: ApplicationController;
+
+  beforeAll(async () => {
+    database = new TestDatabaseService();
+    await database.bootstrap();
+  });
+
+  beforeEach(async () => {
+    await database.nuke();
+
+    user = new User();
+    user.firstName = "";
+    user.lastName = "";
+    user.email = "";
+    user.password = "";
+    user.role = UserRole.User;
+    user.verifyToken = "";
+    user.tokenSecret = "";
+    user.forgotPasswordToken = "";
+
+    const userRepo = database.getRepository(User);
+    const savedUser = await userRepo.save(user);
+
+    // Create team for the user
+    team = new Team();
+    team.title = "";
+    team.description = "";
+    team.teamImg = "";
+    const teamRepo = database.getRepository(Team);
+    team.owner = savedUser;
+    team = await teamRepo.save(team);
+
+    user.team = team;
+    user = await userRepo.save(user);
+
+    const emails = new MockEmailTemplateService();
+    const userService = new UserService(
+      new MockHaveibeenpwnedService().instance,
+      database,
+      new MockLoggerService().instance,
+      new MockTokenService().instance,
+      emails.instance,
+    );
+
+    const settingsService = new MockSettingsService();
+    settingsService.mocks.getSettings.mockResolvedValue({
+      application: { hoursToConfirm: 24 },
+    } as any);
+
+    applicationService = new ApplicationService(
+      new MockQuestionGraphService().instance,
+      database,
+      settingsService.instance,
+      userService,
+      emails.instance,
+    );
+    await applicationService.bootstrap();
+    await userService.bootstrap();
+
+    controller = new ApplicationController(
+      applicationService,
+      userService,
+      new MockTeamsService().instance,
+    );
+  });
+
+  it("does not clear team and teamRequest on checkIn", async () => {
+    expect.assertions(1);
+
+    await applicationService.checkIn(user);
+
+    const userRepo = database.getRepository(User);
+    const foundUser = await userRepo.findOne({ where: { id: user.id } });
+    expect(foundUser?.team?.id).toEqual(team.id);
+  });
+
+  it("does not clear team and teamRequest on admit", async () => {
+    // Based on a bug. team relation was not included in findUsersByIDs,
+    // team got overwritten with null on update
+    expect.assertions(1);
+    await controller.admit({ data: [user.id] });
+
+    const userRepo = database.getRepository(User);
+    const foundUser = await userRepo.findOne({ where: { id: user.id } });
+    expect(foundUser?.team?.id).toEqual(team.id);
+  });
+});