fix: [fixes #385] Ensure valid breeding pen references (#386)

* fix: [#385] allow any eligible cow to be moved to breeding pen * fix: [#385] avoid adding cows not in the inventory to the breeding pen * test: validate that cow not in inventory cannot be moved to breeding pen * refactor: rename id to playerId in Subheader component * refactor: simplify changeCowBreedingPenResident
jeremyckahn · Jan 29, 2023 · 5b52940 · 5b52940
1 parent da43b44
commit 5b52940
Show file tree

Hide file tree

Showing 4 changed files with 84 additions and 52 deletions.
diff --git a/src/components/CowCard/Subheader/Subheader.js b/src/components/CowCard/Subheader/Subheader.js
@@ -46,34 +46,29 @@ const Subheader = ({
   handleCowAutomaticHugChange,
   handleCowBreedChange,
   huggingMachinesRemain,
-  id,
+  id: playerId,
   isCowPurchased,
 }) => {
   const numberOfFullHearts = cow.happiness * 10
   const isInBreedingPen = isCowInBreedingPen(cow, cowBreedingPen)
-  const isBreedingPenFull =
-    cowBreedingPen.cowId1 !== null && cowBreedingPen.cowId2 !== null
+  const isRoomInBreedingPen =
+    cowBreedingPen.cowId1 === null || cowBreedingPen.cowId2 === null
+  const isThisCowOfferedForTrade = cowIdOfferedForTrade === cow.id
 
-  const isCowOfferedForTrade = !!cowInventory.find(
-    ({ id }) => id === cowIdOfferedForTrade
-  )
-
-  let canBeMovedToBreedingPen = !isBreedingPenFull && !isCowOfferedForTrade
-
-  if (canBeMovedToBreedingPen) {
-    const potentialMateId = cowBreedingPen.cowId2 ?? cowBreedingPen.cowId1
+  const mateId = cowBreedingPen.cowId1 ?? cowBreedingPen.cowId2
+  const mate = getCowMapById(cowInventory)[mateId]
+  const isEligibleToBreed = cow.gender !== mate?.gender
 
-    if (potentialMateId !== null) {
-      canBeMovedToBreedingPen =
-        cow.gender !== getCowMapById(cowInventory)[potentialMateId].gender
-    }
-  }
+  const canBeMovedToBreedingPen =
+    isRoomInBreedingPen && isEligibleToBreed && !isThisCowOfferedForTrade
 
   const disableBreedingControlTooltip =
     !canBeMovedToBreedingPen && !isInBreedingPen
 
   const showOriginalOwner =
-    isCowPurchased && id !== cow.originalOwnerId && id === cow.ownerId
+    isCowPurchased &&
+    playerId !== cow.originalOwnerId &&
+    playerId === cow.ownerId
 
   return (
     <div {...{ className: 'Subheader' }}>

diff --git a/src/game-logic/reducers/changeCowBreedingPenResident.js b/src/game-logic/reducers/changeCowBreedingPenResident.js
@@ -1,31 +1,48 @@
 import { COW_GESTATION_PERIOD_DAYS } from '../../constants'
 
+/**
+ * @param {farmhand.cow} cow
+ * @param {farmhand.cowBreedingPen} cowBreedingPen
+ * @param {Array.<farmhand.cow>} cowInventory
+ * @return {boolean}
+ */
+const cowCanBeAdded = (cow, cowBreedingPen, cowInventory) => {
+  const { cowId1, cowId2 } = cowBreedingPen
+  const isBreedingPenFull = cowId1 !== null && cowId2 !== null
+  const isCowInBreedingPen = cowId1 === cow.id || cowId2 === cow.id
+
+  const isCowInInventory = !!cowInventory.find(({ id }) => {
+    return id === cow.id
+  })
+
+  return isCowInInventory && !isBreedingPenFull && !isCowInBreedingPen
+}
+
 /**
  * @param {farmhand.state} state
  * @param {farmhand.cow} cow
- * @param {boolean} doAdd If true, cow will be added to the breeding pen. If
+ * @param {boolean} isAdding If true, cow will be added to the breeding pen. If
  * false, they will be removed.
  * @returns {farmhand.state}
  */
-export const changeCowBreedingPenResident = (state, cow, doAdd) => {
-  const { cowBreedingPen } = state
+export const changeCowBreedingPenResident = (state, cow, isAdding) => {
+  const { cowBreedingPen, cowInventory } = state
   const { cowId1, cowId2 } = cowBreedingPen
-  const isPenFull = cowId1 !== null && cowId2 !== null
-  const isCowInPen = cowId1 === cow.id || cowId2 === cow.id
+  const isCowInBreedingPen = cowId1 === cow.id || cowId2 === cow.id
   let newCowBreedingPen = { ...cowBreedingPen }
 
-  if (doAdd) {
-    if (isPenFull || isCowInPen) {
-      return state
-    }
+  if (isAdding && !cowCanBeAdded(cow, cowBreedingPen, cowInventory)) {
+    return state
+  }
 
-    const cowId = cowId1 === null ? 'cowId1' : 'cowId2'
-    newCowBreedingPen = { ...newCowBreedingPen, [cowId]: cow.id }
-  } else {
-    if (!isCowInPen) {
-      return state
-    }
+  if (!isAdding && !isCowInBreedingPen) {
+    return state
+  }
 
+  if (isAdding) {
+    const breedingPenCowId = cowId1 === null ? 'cowId1' : 'cowId2'
+    newCowBreedingPen = { ...newCowBreedingPen, [breedingPenCowId]: cow.id }
+  } else {
     if (cowId1 === cow.id) {
       newCowBreedingPen = {
         ...newCowBreedingPen,

diff --git a/src/game-logic/reducers/changeCowBreedingPenResident.test.js b/src/game-logic/reducers/changeCowBreedingPenResident.test.js
@@ -3,6 +3,10 @@ import { generateCow } from '../../utils'
 
 import { changeCowBreedingPenResident } from './changeCowBreedingPenResident'
 
+const cowA = generateCow({ id: 'cow-a' })
+const cowB = generateCow({ id: 'cow-b' })
+const cowC = generateCow({ id: 'cow-c' })
+
 describe('changeCowBreedingPenResident', () => {
   describe('doAdd === false', () => {
     describe('cow is not in breeding pen', () => {
@@ -13,13 +17,10 @@ describe('changeCowBreedingPenResident', () => {
             cowId2: 'cow-b',
             daysUntilBirth: COW_GESTATION_PERIOD_DAYS,
           },
+          cowInventory: [cowA, cowB, cowC],
         }
 
-        const state = changeCowBreedingPenResident(
-          inputState,
-          generateCow({ id: 'cow-c' }),
-          false
-        )
+        const state = changeCowBreedingPenResident(inputState, cowC, false)
 
         expect(state).toBe(inputState)
       })
@@ -34,8 +35,9 @@ describe('changeCowBreedingPenResident', () => {
               cowId2: 'cow-b',
               daysUntilBirth: COW_GESTATION_PERIOD_DAYS,
             },
+            cowInventory: [cowA, cowB],
           },
-          generateCow({ id: 'cow-a' }),
+          cowA,
           false
         )
 
@@ -45,6 +47,7 @@ describe('changeCowBreedingPenResident', () => {
             cowId2: null,
             daysUntilBirth: COW_GESTATION_PERIOD_DAYS,
           },
+          cowInventory: [cowA, cowB],
         })
       })
     })
@@ -58,8 +61,9 @@ describe('changeCowBreedingPenResident', () => {
               cowId2: 'cow-b',
               daysUntilBirth: COW_GESTATION_PERIOD_DAYS,
             },
+            cowInventory: [cowA, cowB],
           },
-          generateCow({ id: 'cow-b' }),
+          cowB,
           false
         )
 
@@ -69,6 +73,7 @@ describe('changeCowBreedingPenResident', () => {
             cowId2: null,
             daysUntilBirth: COW_GESTATION_PERIOD_DAYS,
           },
+          cowInventory: [cowA, cowB],
         })
       })
     })
@@ -83,13 +88,27 @@ describe('changeCowBreedingPenResident', () => {
             cowId2: 'cow-b',
             daysUntilBirth: COW_GESTATION_PERIOD_DAYS,
           },
+          cowInventory: [cowA, cowB],
         }
 
-        const state = changeCowBreedingPenResident(
-          inputState,
-          generateCow({ id: 'cow-a' }),
-          true
-        )
+        const state = changeCowBreedingPenResident(inputState, cowA, true)
+
+        expect(state).toBe(inputState)
+      })
+    })
+
+    describe('cow is not in inventory', () => {
+      test('no-ops', () => {
+        const inputState = {
+          cowBreedingPen: {
+            cowId1: 'cow-a',
+            cowId2: null,
+            daysUntilBirth: COW_GESTATION_PERIOD_DAYS,
+          },
+          cowInventory: [cowA],
+        }
+
+        const state = changeCowBreedingPenResident(inputState, cowB, true)
 
         expect(state).toBe(inputState)
       })
@@ -103,13 +122,10 @@ describe('changeCowBreedingPenResident', () => {
             cowId2: 'cow-b',
             daysUntilBirth: COW_GESTATION_PERIOD_DAYS,
           },
+          cowInventory: [cowA, cowB],
         }
 
-        const state = changeCowBreedingPenResident(
-          inputState,
-          generateCow({ id: 'cow-c' }),
-          true
-        )
+        const state = changeCowBreedingPenResident(inputState, cowC, true)
 
         expect(state).toBe(inputState)
       })
@@ -124,8 +140,9 @@ describe('changeCowBreedingPenResident', () => {
               cowId2: null,
               daysUntilBirth: COW_GESTATION_PERIOD_DAYS,
             },
+            cowInventory: [cowA, cowB],
           },
-          generateCow({ id: 'cow-a' }),
+          cowA,
           true
         )
 
@@ -135,6 +152,7 @@ describe('changeCowBreedingPenResident', () => {
             cowId2: null,
             daysUntilBirth: COW_GESTATION_PERIOD_DAYS,
           },
+          cowInventory: [cowA, cowB],
         })
       })
     })
@@ -148,8 +166,9 @@ describe('changeCowBreedingPenResident', () => {
               cowId2: null,
               daysUntilBirth: COW_GESTATION_PERIOD_DAYS,
             },
+            cowInventory: [cowA, cowB],
           },
-          generateCow({ id: 'cow-b' }),
+          cowB,
           true
         )
 
@@ -159,6 +178,7 @@ describe('changeCowBreedingPenResident', () => {
             cowId2: 'cow-b',
             daysUntilBirth: COW_GESTATION_PERIOD_DAYS,
           },
+          cowInventory: [cowA, cowB],
         })
       })
     })

diff --git a/src/handlers/peer-events.js b/src/handlers/peer-events.js
@@ -91,7 +91,7 @@ export const handleCowTradeRequest = async (
 
       if (!peerMetadata) {
         console.error(`No data for peer ${updatedCowOffered.ownerId}`)
-        return
+        return null
       }
 
       state = changeCowAutomaticHugState(state, cowToTradeAway, false)