LF-4410: Add limit on creating animals and batches and tests #3542
Conversation
Tbrid
requested review from
antsgar and
SayakaOno
and removed request for
a team
| @@ -112,6 +114,7 @@ export default function AddAnimalsFormCard({ | |||
| <NumberInput | |||
| name={`${namePrefix}${BasicsFields.COUNT}`} | |||
| control={control} | |||
| max={ANIMAL_COUNT_LIMIT} | |||
There was a problem hiding this comment.
Thank you for working on this!
Sorry, we should've explained this first... When the checkbox is selected and the count is 1000, 1000 animals will be created. If the checkbox is not selected, a batch with 1000 animals (one record in the DB) will be created instead. The latter is a valid case, so it would be great if you could add a condition to set a maximum limit when the checkbox is selected 🙏
There was a problem hiding this comment.
Hi, Sayaka. So I put a hook to deal with this, but then I found it wouldn't change the displayed value in the input when I only use setValue, (i.e. if number is 2000 and user clicks the checkbox, the value will be set to the limit, but still displayed as 2000) so I put a "value" prop for the NumberedInput, let me know what you think about it.
There was a problem hiding this comment.
Sorry for the late reply! Sending value to NumberInput seems good, but it looks like value will be automatically passed through without modifying NumberInput, since ...props is added to InputBase. Could you double check?
Would it work if we move the logic from the useEffect to the checkbox's onChange instead?
There was a problem hiding this comment.
I found that it doesn't propagate to the InputBase without specifying the value as a prop.
There was a problem hiding this comment.
Oh right, inputProps's value overrides it.
@navDhammu The number in the NumberInput doesn't update when the value is changed via setValue. Do you think we should update the logic in useNumberInput to handle this? (I believe that's the right thing to do!)
There was a problem hiding this comment.
@SayakaOno The reason it doesn't update with setValue is because NumberInput manages its own state, so when changing value with setValue it doesn't update NumberInput's internal state, which is why you still see the previous number.
I don't think its a good idea to update the logic inside the useNumberInput to handle this because then you would probably have to add a useEffect to update the state based on changes to some prop, and this isn't best practices for react.
Better ways of handling this in my opinion:
- Lift the state up to where its needed. This would involve refactoring out the useNumberInput hook from NumberInput and using it in each component that uses NumberInput.
- Use a key to rerender NumberInput.
Also, sending value to NumberInput is not a good idea because it overrides the value given by useNumberInput which is a formatted string based on the locale. So overriding that breaks the localization feature.
There was a problem hiding this comment.
Thank you for your input, I'll discuss the solution with the team!
There was a problem hiding this comment.
@navDhammu Thanks for explaining. @SayakaOno I think the 2nd way of using a key is the easiest.
There was a problem hiding this comment.
I agree, but I'm hesitant to go with that approach because we may encounter the same issue in the future. Needing to add a key each time doesn't seem like a sustainable solution...
I'll bring this up in tech daily early next week (feel free to join if you'd like) and get back to you!
Tbrid
changed the title
Description
Put 1000 limit on creating animals and animal batches. Added tests on api package. Added max limit on UI count input.
Jira link: https://lite-farm.atlassian.net/browse/LF-4410
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: