Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add experience part 1 #448

Merged
merged 11 commits into from
Aug 30, 2023
1 change: 1 addition & 0 deletions .env.development.local
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
REACT_APP_API_ROOT=http://localhost:3001/
REACT_APP_ENABLE_KEGS=true
REACT_APP_ENABLE_EXPERIENCE=true
40 changes: 27 additions & 13 deletions src/components/Farmhand/Farmhand.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,17 +56,16 @@ import NotificationSystem, {
} from '../NotificationSystem'
import DebugMenu from '../DebugMenu'
import theme from '../../mui-theme'
import { levelAchieved } from '../../utils/levelAchieved'
import {
computeMarketPositions,
createNewField,
doesMenuObstructStage,
farmProductsSold,
generateCow,
getAvailableShopInventory,
getItemCurrentValue,
getPeerMetadata,
inventorySpaceRemaining,
levelAchieved,
moneyTotal,
nullArray,
reduceByPersistedKeys,
Expand Down Expand Up @@ -381,7 +380,7 @@ export default class Farmhand extends FarmhandReducers {

get levelEntitlements() {
return getLevelEntitlements(
levelAchieved(farmProductsSold(this.state.itemsSold))
levelAchieved({ itemsSold: this.state.itemsSold })
)
}

Expand Down Expand Up @@ -425,6 +424,7 @@ export default class Farmhand extends FarmhandReducers {
cowTradeTimeoutId: -1,
cropsHarvested: {},
dayCount: 0,
experience: 0,
farmName: 'Unnamed',
field: createNewField(),
fieldMode: OBSERVE,
Expand Down Expand Up @@ -497,6 +497,7 @@ export default class Farmhand extends FarmhandReducers {
[toolType.WATERING_CAN]: toolLevel.DEFAULT,
},
useAlternateEndDayButtonPosition: false,
useLegacyLevelingSystem: true,
jeremyckahn marked this conversation as resolved.
Show resolved Hide resolved
valueAdjustments: {},
version: process.env.REACT_APP_VERSION ?? '',
}
Expand Down Expand Up @@ -589,17 +590,30 @@ export default class Farmhand extends FarmhandReducers {
})
const { isCombineEnabled, newDayNotifications } = sanitizedState

this.setState({ ...sanitizedState, newDayNotifications: [] }, () => {
newDayNotifications.forEach(({ message, severity }) => {
// Defer these notifications so that notistack doesn't swallow all
// but the last one.
setTimeout(() => this.showNotification(message, severity), 0)
// saved games will likely have itemsSold but not experience since it's a new
// feature so this will "sync" them up
let { experience } = state
if (experience === 0) {
experience = Object.values(state.itemsSold).reduce(
(acc, value) => acc + value,
0
)
}

if (isCombineEnabled) {
this.forRange(reducers.harvestPlot, Infinity, 0, 0)
}
})
})
this.setState(
{ ...sanitizedState, newDayNotifications: [], experience },
() => {
newDayNotifications.forEach(({ message, severity }) => {
// Defer these notifications so that notistack doesn't swallow all
// but the last one.
setTimeout(() => this.showNotification(message, severity), 0)

if (isCombineEnabled) {
this.forRange(reducers.harvestPlot, Infinity, 0, 0)
}
})
}
)
} else {
// Initialize new game
await this.incrementDay(true)
Expand Down
10 changes: 3 additions & 7 deletions src/components/Field/Field.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,8 @@ import Plot from '../Plot'
import QuickSelect from '../QuickSelect'
import { fieldMode } from '../../enums'
import tools from '../../data/tools'
import {
doesInventorySpaceRemain,
farmProductsSold,
levelAchieved,
nullArray,
} from '../../utils'
import { levelAchieved } from '../../utils/levelAchieved'
import { doesInventorySpaceRemain, nullArray } from '../../utils'
import { getLevelEntitlements } from '../../utils/getLevelEntitlements'

import './Field.sass'
Expand Down Expand Up @@ -74,7 +70,7 @@ export const isInHoverRange = ({
switch (fieldMode) {
case SET_SPRINKLER:
hoveredPlotRangeSizeToRender = getLevelEntitlements(
levelAchieved(farmProductsSold(itemsSold))
levelAchieved({ itemsSold })
).sprinklerRange

break
Expand Down
6 changes: 3 additions & 3 deletions src/components/Navigation/Navigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,13 @@ import Typography from '@material-ui/core/Typography'
import { array, bool, func, number, object, string } from 'prop-types'

import FarmhandContext from '../Farmhand/Farmhand.context'
import { farmProductsSold } from '../../utils/farmProductsSold'
import { levelAchieved } from '../../utils/levelAchieved'
import {
doesInventorySpaceRemain,
farmProductSalesVolumeNeededForLevel,
farmProductsSold,
integerString,
inventorySpaceConsumed,
levelAchieved,
scaleNumber,
} from '../../utils'
import { dialogView } from '../../enums'
Expand Down Expand Up @@ -225,7 +225,7 @@ export const Navigation = ({
viewList,

totalFarmProductsSold = farmProductsSold(itemsSold),
currentLevel = levelAchieved(totalFarmProductsSold),
currentLevel = levelAchieved({ itemsSold }),
levelPercent = scaleNumber(
totalFarmProductsSold,
farmProductSalesVolumeNeededForLevel(currentLevel),
Expand Down
13 changes: 3 additions & 10 deletions src/components/OnlinePeersView/OnlinePeer/OnlinePeer.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,8 @@ import CardHeader from '@material-ui/core/CardHeader'
import CowCard from '../../CowCard'

import { moneyString } from '../../../utils/moneyString'
import {
getPlayerName,
farmProductsSold,
integerString,
levelAchieved,
} from '../../../utils'
import { levelAchieved } from '../../../utils/levelAchieved'
import { getPlayerName, integerString } from '../../../utils'

import './OnlinePeer.sass'

Expand All @@ -28,10 +24,7 @@ const OnlinePeer = ({
subheader: (
<div>
<p>Day: {integerString(dayCount)}</p>
<p>
Level:{' '}
{integerString(levelAchieved(farmProductsSold(itemsSold)))}
</p>
<p>Level: {integerString(levelAchieved({ itemsSold }))}</p>
<p>Money: {moneyString(money)}</p>
</div>
),
Expand Down
5 changes: 3 additions & 2 deletions src/components/OnlinePeersView/OnlinePeersView.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ import { array, number, object, string } from 'prop-types'

import BailOutErrorBoundary from '../BailOutErrorBoundary'

import { getPlayerName, farmProductsSold, levelAchieved } from '../../utils'
import { levelAchieved } from '../../utils/levelAchieved'
import { getPlayerName } from '../../utils'
import FarmhandContext from '../Farmhand/Farmhand.context'

import CowCard from '../CowCard'
Expand Down Expand Up @@ -58,7 +59,7 @@ const OnlinePeersView = ({
{sortBy(populatedPeers, [
peerId =>
// Use negative value to reverse sort order
-levelAchieved(farmProductsSold(peers[peerId].itemsSold || 0)),
-levelAchieved({ itemsSold: peers[peerId].itemsSold || 0 }),
]).map(peerId => (
<BailOutErrorBoundary {...{ key: peerId }}>
<OnlinePeer {...{ peer: peers[peerId] }} />
Expand Down
2 changes: 1 addition & 1 deletion src/components/Recipe/Recipe.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@ import CardActions from '@material-ui/core/CardActions'
import Typography from '@material-ui/core/Typography'
import { array, func, number, object } from 'prop-types'

import { totalIngredientsInRecipe } from '../../utils/totalIngredientsInRecipe'
import {
canMakeRecipe,
doesInventorySpaceRemain,
dollarString,
maxYieldOfRecipe,
integerString,
totalIngredientsInRecipe,
} from '../../utils'
import { itemsMap } from '../../data/maps'
import { craftedItems } from '../../img'
Expand Down
20 changes: 19 additions & 1 deletion src/components/SettingsView/SettingsView.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React, { useState } from 'react'
import { bool, func } from 'prop-types'
import { bool, func, object } from 'prop-types'
import Button from '@material-ui/core/Button'
import Dialog from '@material-ui/core/Dialog'
import DialogActions from '@material-ui/core/DialogActions'
Expand All @@ -21,6 +21,7 @@ import './SettingsView.sass'

const SettingsView = ({
allowCustomPeerCowNames,
features,
handleAllowCustomPeerCowNamesChange,
handleClearPersistedDataClick,
handleExportDataClick,
Expand All @@ -29,8 +30,10 @@ const SettingsView = ({
handleShowNotificationsChange,
handleUseAlternateEndDayButtonPositionChange,
handleShowHomeScreenChange,
handleUseLegacyLevelSystemChange,
showNotifications,
useAlternateEndDayButtonPosition,
useLegacyLevelingSystem,
showHomeScreen,
}) => {
const [isClearDataDialogOpen, setIsClearDataDialogOpen] = useState(false)
Expand Down Expand Up @@ -99,6 +102,19 @@ const SettingsView = ({
}
label="Display custom names for cows received from other players"
/>
{features.EXPERIENCE ? (
<FormControlLabel
control={
<Switch
color="primary"
checked={useLegacyLevelingSystem}
onChange={handleUseLegacyLevelSystemChange}
name="use-legacy-leveling-system"
/>
}
label="Use legacy leveling system (experience is only gained by selling items)"
/>
) : null}
</FormGroup>
</FormControl>

Expand Down Expand Up @@ -199,6 +215,7 @@ const SettingsView = ({

SettingsView.propTypes = {
allowCustomPeerCowNames: bool.isRequired,
features: object.isRequired,
handleAllowCustomPeerCowNamesChange: func.isRequired,
handleClearPersistedDataClick: func.isRequired,
handleExportDataClick: func.isRequired,
Expand All @@ -210,6 +227,7 @@ SettingsView.propTypes = {
showHomeScreen: bool.isRequired,
showNotifications: bool.isRequired,
useAlternateEndDayButtonPosition: bool.isRequired,
useLegacyLevelingSystem: bool,
}

export default function Consumer(props) {
Expand Down
6 changes: 3 additions & 3 deletions src/components/StatsView/StatsView.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@ import sortBy from 'lodash.sortby'
import { itemsMap } from '../../data/maps'
import FarmhandContext from '../Farmhand/Farmhand.context'
import { moneyString } from '../../utils/moneyString'
import { farmProductsSold } from '../../utils/farmProductsSold'
import { levelAchieved } from '../../utils/levelAchieved'
import {
farmProductSalesVolumeNeededForLevel,
farmProductsSold,
get7DayAverage,
getProfit,
getProfitRecord,
integerString,
levelAchieved,
moneyTotal,
} from '../../utils'
import {
Expand Down Expand Up @@ -51,7 +51,7 @@ const StatsView = ({
todaysRevenue,

totalFarmProductsSold = farmProductsSold(itemsSold),
currentLevel = levelAchieved(totalFarmProductsSold),
currentLevel = levelAchieved({ itemsSold }),
}) => (
<div className="StatsView">
<TableContainer {...{ component: ElevatedPaper }}>
Expand Down
7 changes: 2 additions & 5 deletions src/components/UpgradePurchase/UpgradePurchase.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,8 @@ import { array, func, number, object } from 'prop-types'

import IngredientsList from '../IngredientsList'

import {
canMakeRecipe,
doesInventorySpaceRemain,
totalIngredientsInRecipe,
} from '../../utils'
import { totalIngredientsInRecipe } from '../../utils/totalIngredientsInRecipe'
import { canMakeRecipe, doesInventorySpaceRemain } from '../../utils'

import { craftedItems } from '../../img'

Expand Down
2 changes: 2 additions & 0 deletions src/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ export const PERSISTED_STATE_KEYS = [
'cowsTraded',
'cropsHarvested',
'dayCount',
'experience',
'farmName',
'field',
'historicalDailyLosses',
Expand Down Expand Up @@ -182,6 +183,7 @@ export const PERSISTED_STATE_KEYS = [
'todaysStartingInventory',
'toolLevels',
'useAlternateEndDayButtonPosition',
'useLegacyLevelingSystem',
'valueAdjustments',
'version',
]
Expand Down
20 changes: 20 additions & 0 deletions src/game-logic/reducers/addExperience.js
jeremyckahn marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { levelAchieved } from '../../utils/levelAchieved'

import { processLevelUp } from './processLevelUp'

/**
* @param {farmhand.state} state
* @param {number} amount
* @returns {farmhand.state}
*/
export const addExperience = (state, amount) => {
const newExperience = state.experience + amount
const oldLevel = levelAchieved({ itemsSold: state.itemsSold })

state = processLevelUp(state, oldLevel)

return {
...state,
experience: newExperience,
}
}
20 changes: 20 additions & 0 deletions src/game-logic/reducers/addExperience.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { addExperience } from './addExperience'
import { processLevelUp } from './processLevelUp'

jest.mock('./processLevelUp')

describe('addExperience', () => {
const gameState = { experience: 0, itemsSold: {} }

it('adds experience to current experience', () => {
const newState = addExperience(gameState, 100)

expect(newState.experience).toEqual(100)
})

it('will check for leveling up when experience is added', () => {
addExperience(gameState, 100)

expect(processLevelUp).toHaveBeenCalled()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this unit test would not be concerned with implementation details like we're doing here. Could this test be changed to examine gameState to detect whether the level-up was processed instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have mixed feelings about this. not strong enough that I won't change it, but I do want to offer my perspective. i did things this way because I see this test as a unit test, and if it were to check that a level changed it would have to know about game logic conditions that cause the level to change (i.e. how much experience it takes to get from one level to another). the way i see, addExperience is only concerned with making sure that processLevelUp is invoked, which is what mocking it and checking for it to be called does. i think the place for doing a test on game logic belongs elsewhere, which is what might be missing from this PR, but i haven't found that spec yet 😄 .

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the way i see, addExperience is only concerned with making sure that processLevelUp is invoked

I think this is where our views diverge. Based on the semantics and API of this reducer, I would assume that addExperience only cares about adding experience. The fact that level-ups are processed within this reducer seems like an implementation detail that could conceivably be handled outside of this function (though I'm not suggesting we do that).

We definitely do want to test that adding experience causes a level up, but perhaps this isn't the appropriate place to validate that? Specifically, I'm thinking that it may be better to validate that gaining experience results in a level-up in a higher-level React Test Library test. I'm thinking something similar to this:

test('item in inventory can be sold', async () => {
const loadedState = saveDataStubFactory({
inventory: [{ id: 'carrot-seed', quantity: 1 }],
})
await farmhandStub({
localforage: {
getItem: () => Promise.resolve(loadedState),
setItem: (_key, data) => Promise.resolve(data),
},
})
const menu = screen.getByRole('complementary')
const carrotSeedMenuItem = within(menu)
.getByText('Carrot Seed')
.closest('.Item')
const sellButton = within(carrotSeedMenuItem).getByText('Sell')
userEvent.click(sellButton)
expect(within(menu).queryByText('Carrot Seed')).not.toBeInTheDocument()
})

I realize that this is expanding the scope of what you set out to do here, and the current state isn't something I'd block this PR over or anything. It's just a difference of opinion. If you feel good about what we have here, then I'm okay with sticking with it. Just let me know which way you'd like to go. 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i actually think i'm with you on processing level ups as being the thing that is out of place here. so i think i'll remove that. it definitely feels like an unexpected side effect not that you've mentioned it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok i've removed it. i see that sellItem is what actually does processLevelUp but I don't see any tests in there explicitly checking for level up. i always forget that level isn't part of farmhand state and that it's calculated when needed so testing that a level up was achieved is trickier than i expected (at least, without just mocking and checking for the call.) if you have suggestions on how to test it let me know and i am happy to add it to this PR

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making that change!

testing that a level up was achieved is trickier than i expected (at least, without just mocking and checking for the call.)

If we're speaking in terms of React Testing Library tests, I'd approach this by checking to see if the level up notification text is present on the page:

farmhand/src/templates.js

Lines 184 to 214 in d37d410

export const LEVEL_GAINED_NOTIFICATION = (_, newLevel, randomCropSeed) => {
let chunks = [
`You reached **level ${newLevel}!** Way to go!
`,
]
const levelObject = levels[newLevel]
if (itemUnlockLevels[newLevel]) {
chunks.push(
`Now available in the shop: **${
itemsMap[itemUnlockLevels[newLevel]].name
}**.`
)
} else if (levelObject && levelObject.increasesSprinklerRange) {
chunks.push(`Sprinkler range has increased.`)
} else if (randomCropSeed) {
chunks.push(
`You got **${getRandomLevelUpRewardQuantity(newLevel)} units of ${
randomCropSeed.name
}** as a reward!`
)
} else if (levelObject && levelObject.unlocksTool) {
// todo: there is only one tool that can be unlocked currently, but this is a bit
// short-sighted if we ever introduce other tool unlocks
chunks.push(`You've unlocked a new tool for the field, The **Shovel**!`)
}
return chunks.join(' ')
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh yes that makes sense. the sellItem test doesn't seem like the right place in that case (it isn't doing any rendering). i suppose we could check the notifications in state from there though?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be worth spinning up a new test file sibling to src/game-logic/item-sell.test.js just for testing level-ups. This is game logic that cuts across functional units, so an integration test seems appropriate.

})
})
5 changes: 2 additions & 3 deletions src/game-logic/reducers/generatePriceEvents.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import { levelAchieved } from '../../utils/levelAchieved'
import {
farmProductsSold,
filterItemIdsToSeeds,
getPriceEventForCrop,
getRandomUnlockedCrop,
levelAchieved,
} from '../../utils'
import { getLevelEntitlements } from '../../utils/getLevelEntitlements'
import { PRICE_EVENT_CHANCE } from '../../constants'
Expand All @@ -29,7 +28,7 @@ export const generatePriceEvents = state => {
// less-than check.
if (random() < PRICE_EVENT_CHANCE) {
const { items: unlockedItems } = getLevelEntitlements(
levelAchieved(farmProductsSold(state.itemsSold))
levelAchieved({ itemsSold: state.itemsSold })
)

const cropItem = getRandomUnlockedCrop(
Expand Down
Loading
Loading