fix: simplify completedChallenges updates (#45862)

api-server/src/server/boot/challenge.js

@@ -3,10 +3,13 @@
  * Any ref to fixCompletedChallengesItem should be removed post
  * a db migration to fix all completedChallenges
  *
+ * NOTE: it's been 4 years, so any active users will have been migrated. We
+ * should still try to migrate the rest at some point.
+ *
  */
 import debug from 'debug';
 import dedent from 'dedent';
-import { isEmpty, pick, omit, find, uniqBy } from 'lodash';
+import { isEmpty, pick, omit, uniqBy } from 'lodash';
 import { ObjectID } from 'mongodb';
 import { Observable } from 'rx';
 import isNumeric from 'validator/lib/isNumeric';
@@ -17,7 +20,6 @@ import { jwtSecret } from '../../../../config/secrets';
 
 import { environment, deploymentEnv } from '../../../../config/env.json';
 import {
-  fixCompletedChallengeItem,
   fixPartiallyCompletedChallengeItem,
   fixSavedChallengeItem
 } from '../../common/utils';
@@ -144,32 +146,34 @@ export function buildUserUpdate(
     completedChallenge = omit(_completedChallenge, ['files']);
   }
   let finalChallenge;
-  const updateData = {};
+  const $push = {},
+    $set = {},
+    $pull = {};
   const {
     timezone: userTimezone,
     completedChallenges = [],
     needsModeration = false
   } = user;
 
-  const oldChallenge = find(
-    completedChallenges,
+  const oldIndex = completedChallenges.findIndex(
     ({ id }) => challengeId === id
   );
-  const alreadyCompleted = !!oldChallenge;
+
+  const alreadyCompleted = oldIndex !== -1;
+  const oldChallenge = alreadyCompleted ? completedChallenges[oldIndex] : null;
 
   if (alreadyCompleted) {
     finalChallenge = {
       ...completedChallenge,
       completedDate: oldChallenge.completedDate
     };
+    $set[`completedChallenges.${oldIndex}`] = finalChallenge;
   } else {
-    updateData.$push = {
-      ...updateData.$push,
-      progressTimestamps: completedDate
-    };
     finalChallenge = {
       ...completedChallenge
     };
+    $push.progressTimestamps = completedDate;
+    $push.completedChallenges = finalChallenge;
   }
 
   let newSavedChallenges;
@@ -183,44 +187,26 @@ export function buildUserUpdate(
     });
 
     // if savableChallenge, update saved array when submitting
-    updateData.$set = {
-      completedChallenges: uniqBy(
-        [finalChallenge, ...completedChallenges.map(fixCompletedChallengeItem)],
-        'id'
-      ),
-      savedChallenges: newSavedChallenges
-    };
-  } else {
-    updateData.$set = {
-      completedChallenges: uniqBy(
-        [finalChallenge, ...completedChallenges.map(fixCompletedChallengeItem)],
-        'id'
-      )
-    };
+    $set.savedChallenges = newSavedChallenges;
   }
 
   // remove from partiallyCompleted on submit
-  updateData.$pull = {
-    partiallyCompletedChallenges: { id: challengeId }
-  };
+  $pull.partiallyCompletedChallenges = { id: challengeId };
 
   if (
     timezone &&
     timezone !== 'UTC' &&
     (!userTimezone || userTimezone === 'UTC')
   ) {
-    updateData.$set = {
-      ...updateData.$set,
-      timezone: userTimezone
-    };
+    $set.timezone = userTimezone;
   }
 
-  if (needsModeration) {
-    updateData.$set = {
-      ...updateData.$set,
-      needsModeration: true
-    };
-  }
+  if (needsModeration) $set.needsModeration = true;
+
+  const updateData = {};
+  if (!isEmpty($set)) updateData.$set = $set;
+  if (!isEmpty($push)) updateData.$push = $push;
+  if (!isEmpty($pull)) updateData.$pull = $pull;
 
   return {
     alreadyCompleted,

api-server/src/server/boot_tests/challenge.test.js

@@ -1,4 +1,4 @@
-import { first, find } from 'lodash';
+import { find } from 'lodash';
 
 import {
   buildUserUpdate,
@@ -17,6 +17,7 @@ import {
   mockUser,
   mockGetFirstChallenge,
   mockCompletedChallenge,
+  mockCompletedChallengeNoFiles,
   mockCompletedChallenges
 } from './fixtures';
 
@@ -47,7 +48,7 @@ describe('boot/challenge', () => {
         mockCompletedChallenge,
         'UTC'
       );
-      expect(result).toHaveProperty('updateData.$set.completedChallenges');
+      expect(result).toHaveProperty('updateData.$push.completedChallenges');
     });
 
     // eslint-disable-next-line max-len
@@ -64,11 +65,9 @@ describe('boot/challenge', () => {
         completedChallenge,
         'UTC'
       );
-      const firstCompletedChallenge = first(
-        result.updateData.$set.completedChallenges
-      );
+      const newCompletedChallenge = result.updateData.$push.completedChallenges;
 
-      expect(firstCompletedChallenge).toEqual(completedChallenge);
+      expect(newCompletedChallenge).toEqual(completedChallenge);
     });
 
     it('preserves the original completed date of a challenge', () => {
@@ -89,11 +88,12 @@ describe('boot/challenge', () => {
         'UTC'
       );
 
-      const firstCompletedChallenge = first(
-        result.updateData.$set.completedChallenges
-      );
+      const updatedCompletedChallenge =
+        result.updateData.$set['completedChallenges.2'];
 
-      expect(firstCompletedChallenge.completedDate).toEqual(originalCompletion);
+      expect(updatedCompletedChallenge.completedDate).toEqual(
+        originalCompletion
+      );
     });
 
     // eslint-disable-next-line max-len
@@ -129,40 +129,19 @@ describe('boot/challenge', () => {
       expect(updateData.$push).toHaveProperty('progressTimestamps');
     });
 
-    it('removes repeat completions from the completedChallenges array', () => {
-      const completedChallengeId = 'aaa48de84e1ecc7c742e1124';
-      const completedChallenge = {
-        ...mockCompletedChallenge,
-        completedDate: Date.now(),
-        id: completedChallengeId
-      };
+    it('will $push newly completed challenges to the completedChallenges array', () => {
       const {
         updateData: {
-          $set: { completedChallenges }
+          $push: { completedChallenges }
         }
       } = buildUserUpdate(
         mockUser,
-        completedChallengeId,
-        completedChallenge,
+        '123abc',
+        mockCompletedChallengeNoFiles,
         'UTC'
       );
 
-      expect(completedChallenges.length).toEqual(
-        mockCompletedChallenges.length
-      );
-    });
-
-    // eslint-disable-next-line max-len
-    it('adds newly completed challenges to the completedChallenges array', () => {
-      const {
-        updateData: {
-          $set: { completedChallenges }
-        }
-      } = buildUserUpdate(mockUser, '123abc', mockCompletedChallenge, 'UTC');
-
-      expect(completedChallenges.length).toEqual(
-        mockCompletedChallenges.length + 1
-      );
+      expect(completedChallenges).toEqual(mockCompletedChallengeNoFiles);
     });
   });
 

api-server/src/server/boot_tests/fixtures.js

@@ -36,6 +36,12 @@ export const mockCompletedChallenge = {
   completedDate: Date.now()
 };
 
+export const mockCompletedChallengeNoFiles = {
+  id: '123abc456def',
+  challengeType: 0,
+  completedDate: Date.now()
+};
+
 export const mockCompletedChallenges = [
   {
     id: 'bd7123c8c441eddfaeb5bdef',