add experience part 1 #448

lstebner · 2023-08-27T20:40:58Z

What this PR does

The goal of this PR is to lay some groundwork for an experience based level system as outlined in #371

add 'experience' to farmhand state
add 'useLegacyLevelingSystem' to farmhand state and settings screen (only shown when the EXPERIENCE feature is enabled)
update levelAchieved to be more flexible about how level is calculated based on which leveling system is being used
update all calls to levelAchieved
update utils unit tests for levelAchieved to test both systems
add 'EXPERIENCE' feature flag
add reducer for adding experience + unit test
some utils were moved into their own files to make them easier to work with (mostly to avoid circular dependencies when one util relies on another util) and imports across the codebase were updated
initialize experience value to match number of items sold when loading a saved game
add 1 experience point per farm item sold (crops, milks, and crafted or forged items)
items that are not farm items should not reward the player with any experience (seeds, weeds, other supplies)
importing a save without experience should calculate experience based on items sold

How this change can be validated

This PR would be easiest to test if checked out and ran locally so that the feature flag can be flipped on and off

With the feature flag enabled:

there should be a setting in the settings window to toggle between the legacy system and the new system
the use legacy setting should be persist

With the flag disabled:

the settings menu should not show the toggle for changing which leveling system is used

Regardless of flag enabled or disabled, these things should happen:

an experience value can be seen when inspecting the Farmhand root component
the experience value in state should match the total number of items sold when loading a saved game that previously did not have experience
experience should increment by 1 for each item sold

Questions or concerns about this change

Additional information

- add 'experience' to farmhand state - add 'useLegacyLevelingSystem' to farmhand state and settings screen - update `levelAchieved` to be more flexible about how level is calculated based on which leveling system is being used - update all calls to `levelAchieved` - update utils unit tests for levelAchieved to test both systems - add 'EXPERIENCE' feature flag

vercel · 2023-08-27T20:41:02Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
farmhand	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 30, 2023 5:10am

jeremyckahn

I'm still working through a fuller review, I just wanted to make my in-progress comments available.

src/utils/levelAchieved.js

jeremyckahn · 2023-08-27T22:59:08Z

src/utils/levelAchieved.js

Thanks for moving this to a new file!

src/components/SettingsView/SettingsView.js

src/components/Farmhand/Farmhand.js

src/components/SettingsView/SettingsView.js

src/game-logic/reducers/addExperience.js

jeremyckahn

Just a few more notes. I've gotten through all the files now. Great work here so far!

I haven't tested this out yet, but I will do so once this is promoted to "Ready for review."

src/utils/index.js

jeremyckahn · 2023-08-27T23:19:25Z

src/utils/index.test.js

+  const cases = [
+    [1, 0],
+    [2, 100],
+    [2, 150],
+    [3, 400],
+    [100, 980100],
+  ]
+
+  describe('with legacy system', () => {
+    test.each(cases)(


This is a much better approach! 👏

- hook up SettingsView for useLegacyLevelSystem toggle - persist useLegacyLevelSystem value - unit tests for addExperience and fix for oldLevel value - wording update for setting

jeremyckahn

Thanks for following up with my previous review items! I found a few more things to discuss with the latest revision. Crucially, this is currently breaking new game sessions (I've called out where in the Farmhand component) so we'll need to get that addressed by adding an initialValue to the reduce.

We're getting there!

src/components/Farmhand/Farmhand.js

src/components/SettingsView/SettingsView.js

jeremyckahn · 2023-08-28T02:03:37Z

src/game-logic/reducers/addExperience.test.js

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


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?

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 😄 .

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:

farmhand/src/game-logic/item-sell.test.js

Lines 9 to 29 in f791e83

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. 🙂

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

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

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(' ')

}

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?

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.

jeremyckahn · 2023-08-28T02:13:33Z

src/utils/index.js

-export { default as totalIngredientsInRecipe } from './totalIngredientsInRecipe'
-export { farmProductsSold } from './farmProductsSold'
-export { isItemAFarmProduct } from './isItemAFarmProduct'
-export { isItemAGrownCrop } from './isItemAGrownCrop'
-export { levelAchieved } from './levelAchieved'


Thanks for making this change and getting all of the imports straightened out!

jeremyckahn · 2023-08-28T02:15:06Z

src/utils/totalIngredientsInRecipe.js

@@ -1,4 +1,4 @@
-export default function totalIngredientsInRecipe(recipe, amount = 1) {


Oh good call to update this. Thanks for switching to a named export!

src/utils/index.js

jeremyckahn

The code looks good to merge, but I still haven't had a chance to QA this (sorry about that!). I will approve once I'm able to run through everything!

jeremyckahn · 2023-08-28T14:48:44Z

src/game-logic/reducers/addExperience.test.js

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


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:

farmhand/src/game-logic/item-sell.test.js

Lines 9 to 29 in f791e83

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. 🙂

jeremyckahn

I did a bit of testing. Here's what I found:

the experience value in state should match the total number of items sold when loading a saved game that previously did not have experience

I imported one of my high-level save files and saw that experience was 0. I saved the game and refreshed the page, and experience was appropriately updated. This wouldn't introduce instability if this PR was merged as-is, but I imagine that this would be perceived as a pretty significant bug once the Experience feature is complete and shipped. It seems like this issue should be addressed in this function:

farmhand/src/utils/index.js

Lines 941 to 952 in 89bb2ee

    
           /** 
        
            * @param {farmhand.state} state 
        
            * @return {farmhand.state} 
        
            */ 
        
           export const transformStateDataForImport = state => { 
        
             const sanitizedState = { ...state } 
        
             const rejectedKeys = ['version'] 
        
             rejectedKeys.forEach(rejectedKey => delete sanitizedState[rejectedKey]) 
        
             return sanitizedState 
        
           }

experience should increment by 1 for each item sold

This is working as expected. I noticed that experience is also incremented when Weeds are sold. Is this intentional? I don't think we discussed this specifically, but it seems like that shouldn't be the case because Weeds do not currently count towards experience. What do you think?

Aside from this, things are looking good. Great work so far!

lstebner · 2023-08-30T05:00:53Z

thanks for calling those things out @jeremyckahn . i've made some updates now to handle importing saves and making sure that experience is only rewarded for farm products sold.

…o need to do it in Farmhand.js anymore

jeremyckahn

This looks ready to merge! Thanks for sticking with this and addressing all the issues. Awesome work! 🚀

jeremyckahn · 2023-08-30T14:18:23Z

src/utils/index.test.js

@@ -1046,3 +1047,52 @@ describe('getCowImage', () => {
    expect(image).toEqual(animals.cow.rainbow)
  })
 })
+
+describe('transformStateDataForImport', () => {


Thank you for setting up test cases for this function! 🙏

jeremyckahn · 2023-08-30T14:27:48Z

src/utils/isItemAFarmProduct.js

+const FARM_PRODUCT_TYPES = [itemType.MILK, itemType.CRAFTED_ITEM]
+
 /**
 * @param {farmhand.item} item
 * @returns {boolean}
 */
 export const isItemAFarmProduct = item =>
-  Boolean(
-    isItemAGrownCrop(item) ||
-      item.type === itemType.MILK ||
-      item.type === itemType.CRAFTED_ITEM
-  )
+  Boolean(isItemAGrownCrop(item) || FARM_PRODUCT_TYPES.includes(item.type))


This is an excellent refactor! 💯

jeremyckahn reviewed Aug 27, 2023

View reviewed changes

jeremyckahn requested changes Aug 27, 2023

View reviewed changes

updates from code review

00d7a56

- hook up SettingsView for useLegacyLevelSystem toggle - persist useLegacyLevelSystem value - unit tests for addExperience and fix for oldLevel value - wording update for setting

vercel bot deployed to Preview August 27, 2023 23:50 View deployment

remove exports from utils/index and update imports across codebase

3296199

vercel bot deployed to Preview August 28, 2023 00:08 View deployment

initialize experience when loading game and add to it when selling

91583f7

vercel bot deployed to Preview August 28, 2023 00:28 View deployment

add jsdoc to addExperience

fd88b39

lstebner marked this pull request as ready for review August 28, 2023 00:34

vercel bot deployed to Preview August 28, 2023 00:37 View deployment

jeremyckahn requested changes Aug 28, 2023

View reviewed changes

couple of fixups

f791e83

vercel bot deployed to Preview August 28, 2023 03:40 View deployment

jeremyckahn reviewed Aug 28, 2023

View reviewed changes

remove processLevelUp from addExperience

89bb2ee

vercel bot deployed to Preview August 28, 2023 16:10 View deployment

jeremyckahn requested changes Aug 29, 2023

View reviewed changes

only add experience for items classified as farm products

1f5e5f7

vercel bot deployed to Preview August 30, 2023 04:37 View deployment

lstebner added 2 commits August 29, 2023 21:42

set experience when importing a game save that doesn't have experience

023e1c7

add some tests for transformStateDataForImport

0556d30

vercel bot deployed to Preview August 30, 2023 05:00 View deployment

since transformStateDataForImport will calculate experience there's n…

de09e52

…o need to do it in Farmhand.js anymore

vercel bot deployed to Preview August 30, 2023 05:10 View deployment

jeremyckahn approved these changes Aug 30, 2023

View reviewed changes

lstebner merged commit 99b4212 into develop Aug 30, 2023
5 checks passed

lstebner deleted the lstebner/experience-pt1 branch August 30, 2023 16:17

jeremyckahn mentioned this pull request Oct 13, 2023

make selling ores and ignots give xp #460

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add experience part 1 #448

add experience part 1 #448

lstebner commented Aug 27, 2023 •

edited by jeremyckahn

Loading

vercel bot commented Aug 27, 2023 •

edited

Loading

jeremyckahn left a comment

jeremyckahn Aug 27, 2023

jeremyckahn left a comment

jeremyckahn Aug 27, 2023

jeremyckahn left a comment

jeremyckahn Aug 28, 2023

lstebner Aug 28, 2023

jeremyckahn Aug 28, 2023

lstebner Aug 28, 2023

lstebner Aug 28, 2023

jeremyckahn Aug 28, 2023

lstebner Aug 28, 2023

jeremyckahn Aug 29, 2023

jeremyckahn Aug 28, 2023

jeremyckahn Aug 28, 2023

jeremyckahn left a comment

jeremyckahn Aug 28, 2023

jeremyckahn left a comment

lstebner commented Aug 30, 2023

jeremyckahn left a comment

jeremyckahn Aug 30, 2023

jeremyckahn Aug 30, 2023

	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()
	})

	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(' ')
	}

		@@ -1,4 +1,4 @@
		export default function totalIngredientsInRecipe(recipe, amount = 1) {

	/**
	* @param {farmhand.state} state
	* @return {farmhand.state}
	*/
	export const transformStateDataForImport = state => {
	const sanitizedState = { ...state }

	const rejectedKeys = ['version']
	rejectedKeys.forEach(rejectedKey => delete sanitizedState[rejectedKey])

	return sanitizedState
	}

add experience part 1 #448

add experience part 1 #448

Conversation

lstebner commented Aug 27, 2023 • edited by jeremyckahn Loading

What this PR does

How this change can be validated

Questions or concerns about this change

Additional information

vercel bot commented Aug 27, 2023 • edited Loading

jeremyckahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyckahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyckahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyckahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyckahn left a comment

Choose a reason for hiding this comment

lstebner commented Aug 30, 2023

jeremyckahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lstebner commented Aug 27, 2023 •

edited by jeremyckahn

Loading

vercel bot commented Aug 27, 2023 •

edited

Loading