[Not-urgent] LF-4380 - address comments and improvement suggestions

packages/api/src/middleware/validation/checkAnimalOrBatch.js

@@ -83,8 +83,8 @@ const checkAnimalType = async (animalOrBatch, farm_id, creating = true) => {
       'default_type_id, custom_type_id, or type_name',
     );
   }
+  // Overwrite with null in db if editing, post does not accept nulled values in oneOf schemas
   if (!creating && someExists(typeKeyOptions, animalOrBatch)) {
-    // Overwrite with null in db if editing
     setFalsyValuesToNull(typeKeyOptions, animalOrBatch);
   }
   if (custom_type_id) {
@@ -140,7 +140,7 @@ const checkCustomBreedMatchesType = (
   let customTypeId = custom_type_id;
   const typeKeyOptions = ['default_type_id', 'custom_type_id', 'type_name'];
 
-  // If not editing type, check record type
+  // If not editing type, get record type
   if (!someExists(typeKeyOptions, animalOrBatch) && preexistingAnimalOrBatch) {
     defaultTypeId = preexistingAnimalOrBatch.default_type_id;
     customTypeId = preexistingAnimalOrBatch.custom_type_id;
@@ -171,78 +171,71 @@ const checkAnimalBreed = async (
   } = animalOrBatch;
   const breedKeyOptions = ['default_breed_id', 'custom_breed_id', 'breed_name'];
   const typeKeyOptions = ['default_type_id', 'custom_type_id', 'type_name'];
-
-  // Check if breed is present
-  if (
-    (creating && someExists(breedKeyOptions, animalOrBatch)) ||
-    someTruthy([default_breed_id, custom_breed_id, breed_name])
-  ) {
-    checkExactlyOneIsProvided(
-      [default_breed_id, custom_breed_id, breed_name],
-      'default_breed_id, custom_breed_id, or breed_name',
-    );
-  }
-  // Check if breed is present
-  if (!creating && someExists(breedKeyOptions, animalOrBatch)) {
-    // Overwrite with null in db if editing
-    setFalsyValuesToNull(breedKeyOptions, animalOrBatch);
-  }
-
-  if (
-    someExists(breedKeyOptions, animalOrBatch) &&
-    !someTruthy([default_breed_id, custom_breed_id, breed_name])
-  ) {
-    // do nothing if nulling breed
-  } else {
-    // Check if default breed or default type is present
-    if (
-      (someExists(breedKeyOptions, animalOrBatch) && default_breed_id) ||
-      (someExists(typeKeyOptions, animalOrBatch) && default_type_id)
-    ) {
-      await checkDefaultBreedMatchesType(
-        preexistingAnimalOrBatch,
-        default_breed_id,
-        default_type_id,
+  // If neither breed or type is specified, skip checks
+  if (someExists(breedKeyOptions, animalOrBatch) || someExists(typeKeyOptions, animalOrBatch)) {
+    // Check only one breed option is truthy
+    if (someTruthy([default_breed_id, custom_breed_id, breed_name])) {
+      checkExactlyOneIsProvided(
+        [default_breed_id, custom_breed_id, breed_name],
+        'default_breed_id, custom_breed_id, or breed_name',
       );
     }
-    // Check if custom breed or custom type is present
-    if (
-      (someExists(breedKeyOptions, animalOrBatch) && custom_breed_id && !type_name) ||
-      (someExists(typeKeyOptions, animalOrBatch) &&
-        (default_type_id || custom_type_id) &&
-        !breed_name)
-    ) {
-      let customBreed;
-      // Find customBreed if exists
-      if (someExists(breedKeyOptions, animalOrBatch) && custom_breed_id) {
-        checkIdIsNumber(custom_breed_id);
-        customBreed = await CustomAnimalBreedModel.query()
-          .whereNotDeleted()
-          .findById(custom_breed_id);
-        if (!customBreed) {
-          throw customError('Custom breed does not exist');
-        }
-      } else if (preexistingAnimalOrBatch?.custom_breed_id) {
-        checkIdIsNumber(preexistingAnimalOrBatch?.custom_breed_id);
-        customBreed = await CustomAnimalBreedModel.query()
-          .whereNotDeleted()
-          .findById(preexistingAnimalOrBatch.custom_breed_id);
-        if (!customBreed) {
-          // This should not be possible
-          throw customError('Custom breed does not exist');
-        }
-      }
-      // Check custom breed if exists
-      if (customBreed) {
-        await checkRecordBelongsToFarm(customBreed, farm_id, 'custom breed');
-        checkCustomBreedMatchesType(
-          animalOrBatch,
+    // Overwrite all others with null in db if editing, post does not accept nulled values in oneOf schemas
+    if (!creating && someExists(breedKeyOptions, animalOrBatch)) {
+      setFalsyValuesToNull(breedKeyOptions, animalOrBatch);
+    }
+
+    const isNotNullingAllBreedOptions = !(
+      someExists(breedKeyOptions, animalOrBatch) &&
+      !someTruthy([default_breed_id, custom_breed_id, breed_name])
+    );
+    const isCreatingWithBreed =
+      creating && someTruthy([default_breed_id, custom_breed_id, breed_name]);
+    // Do checks on breed unless removing breed from the existing record or creating animal without breed
+    if (isNotNullingAllBreedOptions || isCreatingWithBreed) {
+      // Check if default breed or default type is present
+      if (default_breed_id || default_type_id) {
+        await checkDefaultBreedMatchesType(
           preexistingAnimalOrBatch,
-          customBreed,
+          default_breed_id,
           default_type_id,
-          custom_type_id,
         );
       }
+      // Check if custom breed or new type is present
+      // Skip checks in some cases where new custom type or breed is added
+      if ((custom_breed_id && !type_name) || ((default_type_id || custom_type_id) && !breed_name)) {
+        let customBreed;
+        // Find customBreed if exists
+        if (custom_breed_id) {
+          checkIdIsNumber(custom_breed_id);
+          customBreed = await CustomAnimalBreedModel.query()
+            .whereNotDeleted()
+            .findById(custom_breed_id);
+          if (!customBreed) {
+            throw customError('Custom breed does not exist');
+          }
+        } else if (preexistingAnimalOrBatch?.custom_breed_id) {
+          checkIdIsNumber(preexistingAnimalOrBatch?.custom_breed_id);
+          customBreed = await CustomAnimalBreedModel.query()
+            .whereNotDeleted()
+            .findById(preexistingAnimalOrBatch.custom_breed_id);
+          if (!customBreed) {
+            // This should not be possible unless concurrently deleted in between record get and this breed id check
+            throw customError('Custom breed does not exist');
+          }
+        }
+        // Check custom breed if exists
+        if (customBreed) {
+          await checkRecordBelongsToFarm(customBreed, farm_id, 'custom breed');
+          checkCustomBreedMatchesType(
+            animalOrBatch,
+            preexistingAnimalOrBatch,
+            customBreed,
+            default_type_id,
+            custom_type_id,
+          );
+        }
+      }
     }
   }
 };

packages/api/src/util/middleware.js

@@ -31,8 +31,6 @@ export const someTruthy = (values) => values.some((value) => !!value);
 // Sets falsy values to null for editing values that may have values for exclusive constraints
 export const setFalsyValuesToNull = (array, obj) => {
   for (const val of array) {
-    if (obj[val] !== 0 && obj[val] !== false) {
-      obj[val] ??= null;
-    }
+    obj[val] ??= null;
   }
 };

packages/api/tests/animal.test.js

@@ -835,21 +835,26 @@ describe('Animal Tests', () => {
         [updatedFirstAnimal, updatedSecondAnimal],
       );
 
-      // Remove or add properties not actually expected from get request
-      [updatedFirstAnimal, updatedSecondAnimal].forEach((animal) => {
-        // Should not cause an error
-        delete animal.extra_non_existant_property;
-        // Should not be able to update on edit
-        animal.animal_removal_reason_id = null;
-        // Return format different than post format
-        animal.group_ids = animal.group_ids.map((groupId) => groupId.animal_group_id);
-        animal.animal_use_relationships.forEach((rel) => {
-          rel.animal_id = animal.id;
-          rel.other_use = null;
-        });
+      const [expectedFirstAnimal, expectedSecondAnimal] = [
+        updatedFirstAnimal,
+        updatedSecondAnimal,
+      ].map((animal) => {
+        const { extra_non_existant_property, ...rest } = animal;
+        return {
+          ...rest,
+          animal_removal_reason_id: null,
+          group_ids: rest.group_ids.map((groupId) => groupId.animal_group_id),
+          animal_use_relationships: rest.animal_use_relationships.map((rel) => {
+            return {
+              animal_id: rest.id,
+              use_id: rel.use_id,
+              other_use: null,
+            };
+          }),
+        };
       });
 
-      return { res: patchRes, updatedFirstAnimal, updatedSecondAnimal };
+      return { res: patchRes, expectedFirstAnimal, expectedSecondAnimal };
     }
 
     test('Admin users should be able to edit animals', async () => {
@@ -864,11 +869,9 @@ describe('Animal Tests', () => {
           user,
         );
         expect(addRes.status).toBe(201);
-        expect(returnedFirstAnimal).toBeTruthy();
-        expect(returnedSecondAnimal).toBeTruthy();
 
         // Edit animals in db
-        const { res: editRes, updatedFirstAnimal, updatedSecondAnimal } = await editAnimals(
+        const { res: editRes, expectedFirstAnimal, expectedSecondAnimal } = await editAnimals(
           mainFarm,
           user,
           returnedFirstAnimal,
@@ -895,7 +898,7 @@ describe('Animal Tests', () => {
           delete record.deleted;
           delete record.updated_at;
           delete record.updated_by;
-          const updatedRecord = [updatedFirstAnimal, updatedSecondAnimal].find(
+          const updatedRecord = [expectedFirstAnimal, expectedSecondAnimal].find(
             (animal) => animal.id === record.id,
           );
           expect(record).toMatchObject(updatedRecord);
@@ -916,16 +919,14 @@ describe('Animal Tests', () => {
         fakeUserFarm(workerRole),
       );
 
-      // Add animals to db
+      // Use admin to add animals to db
       const { res: addRes, returnedFirstAnimal, returnedSecondAnimal } = await addAnimals(
         mainFarm,
         admin,
       );
       expect(addRes.status).toBe(201);
-      expect(returnedFirstAnimal).toBeTruthy();
-      expect(returnedSecondAnimal).toBeTruthy();
 
-      // Edit animals in db
+      // Edit animals in db with non-admin
       const { res: editRes } = await editAnimals(
         mainFarm,
         user,

packages/api/tests/animal_batch.test.js

@@ -828,21 +828,25 @@ describe('Animal Batch Tests', () => {
         [updatedFirstBatch, updatedSecondBatch],
       );
 
-      // Remove or add properties not actually expected from get request
-      [updatedFirstBatch, updatedSecondBatch].forEach((batch) => {
-        // Should not cause an error
-        delete batch.extra_non_existant_property;
-        // Should not be able to update on edit
-        batch.animal_removal_reason_id = null;
-        // Return format different than post format
-        batch.group_ids = batch.group_ids.map((groupId) => groupId.animal_group_id);
-        batch.animal_batch_use_relationships.forEach((rel) => {
-          rel.animal_batch_id = batch.id;
-          rel.other_use = null;
-        });
-      });
+      const [expectedFirstBatch, expectedSecondBatch] = [updatedFirstBatch, updatedSecondBatch].map(
+        (batch) => {
+          const { extra_non_existant_property, ...rest } = batch;
+          return {
+            ...rest,
+            animal_removal_reason_id: null,
+            group_ids: rest.group_ids.map((groupId) => groupId.animal_group_id),
+            animal_batch_use_relationships: rest.animal_batch_use_relationships.map((rel) => {
+              return {
+                animal_batch_id: rest.id,
+                use_id: rel.use_id,
+                other_use: null,
+              };
+            }),
+          };
+        },
+      );
 
-      return { res: patchRes, updatedFirstBatch, updatedSecondBatch };
+      return { res: patchRes, expectedFirstBatch, expectedSecondBatch };
     }
 
     test('Admin users should be able to edit batches', async () => {
@@ -857,11 +861,9 @@ describe('Animal Batch Tests', () => {
           user,
         );
         expect(addRes.status).toBe(201);
-        expect(returnedFirstBatch).toBeTruthy();
-        expect(returnedSecondBatch).toBeTruthy();
 
         // Edit batches in db
-        const { res: editRes, updatedFirstBatch, updatedSecondBatch } = await editAnimalBatches(
+        const { res: editRes, expectedFirstBatch, expectedSecondBatch } = await editAnimalBatches(
           mainFarm,
           user,
           returnedFirstBatch,
@@ -888,7 +890,7 @@ describe('Animal Batch Tests', () => {
           delete record.deleted;
           delete record.updated_at;
           delete record.updated_by;
-          const updatedRecord = [updatedFirstBatch, updatedSecondBatch].find(
+          const updatedRecord = [expectedFirstBatch, expectedSecondBatch].find(
             (batch) => batch.id === record.id,
           );
           expect(record).toMatchObject(updatedRecord);
@@ -909,16 +911,14 @@ describe('Animal Batch Tests', () => {
         fakeUserFarm(workerRole),
       );
 
-      // Add animals to db
+      // Add animals to db with admin
       const { res: addRes, returnedFirstBatch, returnedSecondBatch } = await addAnimalBatches(
         mainFarm,
         admin,
       );
       expect(addRes.status).toBe(201);
-      expect(returnedFirstBatch).toBeTruthy();
-      expect(returnedSecondBatch).toBeTruthy();
 
-      // Edit animals in db
+      // Edit animals in db with non-admin
       const { res: editRes } = await editAnimalBatches(
         mainFarm,
         user,
@@ -939,7 +939,6 @@ describe('Animal Batch Tests', () => {
       // Add animals to db
       const { res: addRes, returnedFirstBatch } = await addAnimalBatches(mainFarm, user);
       expect(addRes.status).toBe(201);
-      expect(returnedFirstBatch).toBeTruthy();
 
       // Change 1 thing
       returnedFirstBatch.sire = 'Changed';