LF-5277: Add crop/animal association section to expense add and edit forms#4179
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…isplays empty instead of 0
| const rawInitialValue = field.value ?? get(formState.defaultValues, name) ?? defaultValue; | ||
| const { inputProps, numericValue, increment, decrement, clear } = useNumberInput({ | ||
| initialValue: field.value || get(formState.defaultValues, name) || defaultValue, | ||
| initialValue: rawInitialValue === '' ? NaN : rawInitialValue, |
There was a problem hiding this comment.
field.value can be '' (empty string) when RHF requires an empty initial state, but useNumberInput treats '' as 0 via isNaN (isNaN("") === false). Converting to NaN here ensures the input displays empty instead. Using ?? instead of || prevents 0 from being incorrectly ignored.
kathyavini
left a comment
There was a problem hiding this comment.
This is lovely on so many levels!! The new form addition looks and works perfectly, and the form <--> API logic is so clean and readable 🔥 And I love the new options selectors, thank you!
This is not for this PR, but did you talk to Loïc about what's expected in the Transaction List Expanded Content for these allocated expenses? I don't think I ever saw a design, but I don't love that there is no sign of allocation in the <GeneralTransactionTable>; that felt a bit jarring to add the breakdowns and then not see them again until re-opening the form:
I don't know how he would prioritize that compared to e.g. the Weight/Unit quantities, but it seems like it would be a nice addition if we have some extra time left before release...!
| ); | ||
| const farmCropVarieties = useSelector(cropVarietiesSelector); | ||
|
|
||
| // Management plans determine which crop varieties are sale-eligible, |
There was a problem hiding this comment.
Oh a good example of a comment quickly becoming dead / junk, sorry. In fact it was a bad comment to start with because it was only in reference to moving off the old pattern 🙇
Could you please remove it? 🙏
There was a problem hiding this comment.
Oh sorry! Thank you for catching!!
| crop_translation_key: mp.crop_translation_key, | ||
| crop_variety_photo_url: mp.crop_variety_photo_url, | ||
| for (const cv of farmCropVarieties ?? []) { | ||
| if (!(cv.crop_variety_id in tileDataById)) { |
There was a problem hiding this comment.
This if isn't necessary anymore now that cropVarietiesSelector is being used since there will only be one of each crop variety (unlike before with management plans).
Just curious -- what you you think about using the cropVarietyOptionsSelector here too though, and just building the tile data separately?
const options = useSelector(cropVarietyOptionsSelector);
const farmCropVarieties = useSelector(cropVarietiesSelector);
const cropVarietyTileDataById: Record<number, CropVarietySaleTileData> = useMemo(
() =>
Object.fromEntries(
(farmCropVarieties ?? []).map((cv) => [
cv.crop_variety_id,
{
crop_variety_name: cv.crop_variety_name,
crop_translation_key: cv.crop_translation_key,
crop_variety_photo_url: cv.crop_variety_photo_url,
},
]),
),
[farmCropVarieties],
);I like the idea of revenue and expense using the same selector! The only thing is that the cropVarietyOptionsSelector is not currently sorted (while the optionList here was), but I wonder if sorting in the selector would be fine / good anyway, because it would then match the sorted animalsOptionsSelector?
There was a problem hiding this comment.
I added sorting in cropVarietyOptionsSelector and updated the useMemo!
| value: generateInventoryId(AnimalOrBatchKeys.BATCH, batch), | ||
| label: generateSelectOptionLabel(batch), | ||
| })) | ||
| .sort((a, b) => String(a.label).localeCompare(String(b.label))), |
There was a problem hiding this comment.
I'm guessing that the whole array was meant to be sorted? And not just the batches?
I think the difference is functionally silent in Expenses (due to sortAllocations) but you can just barely notice it in Sales, if you set up your animals right 😉
There was a problem hiding this comment.
What a mistake 😥 Thank you so much for catching!
| @@ -0,0 +1,39 @@ | |||
| /* | |||
There was a problem hiding this comment.
If we follow the current codebase convention, store/api/<entity>Api.ts has so far been used for API endpoints -- I believe this one looks like it should be in store/selectors/? Like the location.ts there?
Edit: oh sorry I just belatedly re-read this part of your description:
animalApi file is expected to contain animal APIs in the future
Does that mean that selectors/location.ts should be folded into the api/locationApi.ts as well? I don't know if I have a particularly strong preference but it would be nice to decide if selectors belong with the endpoints or not, going forward.
There was a problem hiding this comment.
Did you look at Duncan's PR!?? I completely forgot about the new folder 😅
I've moved this file there!
There was a problem hiding this comment.
Haha no I didn't 😂 I was actually looking for/remembering the old sensorSelectors.ts which I had once wrote a long time ago and put in store/api/selectors/. Actually I just now noticed that in addition to removing that file, the locations RTK Query upgrade moved that folder up a level into store/selectors 🤷
There was a problem hiding this comment.
Ah, I see! I'm sorry I didn't notice the folder path had changed!!
SayakaOno
left a comment
There was a problem hiding this comment.
Thank you so much for reviewing Joyce!!
I didn't ask Loïc about the allocations in the transaction list, but we can ask him on Wednesday!
Thank you for re-reviewing in advance 🙏
| ); | ||
| const farmCropVarieties = useSelector(cropVarietiesSelector); | ||
|
|
||
| // Management plans determine which crop varieties are sale-eligible, |
There was a problem hiding this comment.
Oh sorry! Thank you for catching!!
| crop_translation_key: mp.crop_translation_key, | ||
| crop_variety_photo_url: mp.crop_variety_photo_url, | ||
| for (const cv of farmCropVarieties ?? []) { | ||
| if (!(cv.crop_variety_id in tileDataById)) { |
There was a problem hiding this comment.
I added sorting in cropVarietyOptionsSelector and updated the useMemo!
| @@ -0,0 +1,39 @@ | |||
| /* | |||
There was a problem hiding this comment.
Did you look at Duncan's PR!?? I completely forgot about the new folder 😅
I've moved this file there!
| value: generateInventoryId(AnimalOrBatchKeys.BATCH, batch), | ||
| label: generateSelectOptionLabel(batch), | ||
| })) | ||
| .sort((a, b) => String(a.label).localeCompare(String(b.label))), |
There was a problem hiding this comment.
What a mistake 😥 Thank you so much for catching!
Description
Add
ExpenseEntitySectionto the expense forms. I had to try several form patterns to make validation work correctly. The final approach simplifies the form structure using anallocationsfield array shared between crop and animal types. Data is converted from DB format to form values before display and back before submission.When
entityTypeis switched or cleared,allocationsis reset viauseFieldArray.remove.ExpenseEntitySectioncomponent (EntityAssociationToggle+ allocation inputs)ExpenseEntitySectioninto:AddExpense(ExpenseItemInputs)FormProviderinstead of passing form props throughExpenseItemsForType➡️ExpenseItemInputsPureExpenseDetailFormProviderNumberInputlogicCheckboxMultiSelectto acceptnoOptionsMessageAnimalSaleInputsCropSaleInputsformatCropVarietyLabelAddExpenseandExpenseDetailcontainerscropVarietyOptionsandanimalOptionsto pure componentshandleSubmitto format allocationscropVarietyOptionsSelectorandanimalOptionsSelectoranimalApifile is expected to contain animal APIs in the futureJira link: https://lite-farm.atlassian.net/browse/LF-5277
Type of change
How Has This Been Tested?
Checklist:
pnpm i18nto help with this)