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.

Sign up for GitHub

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

Jump to bottom

LF-4410: Add limit on creating animals and batches and tests #3542

Open
wants to merge 3 commits into
base: integration
Choose a base branch
from
Open

LF-4410: Add limit on creating animals and batches and tests #3542

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
54be440
LF-4415: Add limit on creating animals and batches and tests
Tbrid Nov 20, 2024
7557db9
LF-4410: Limit when individuals checked
Tbrid Nov 25, 2024
d4d8765
LF-4410: Move logic to onChange of checkbox
Tbrid Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/api/src/middleware/validation/checkAnimalOrBatch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import CustomAnimalBreedModel from '../../models/customAnimalBreedModel.js';
import AnimalUseModel from '../../models/animalUseModel.js';
import AnimalOriginModel from '../../models/animalOriginModel.js';
import AnimalIdentifierType from '../../models/animalIdentifierTypeModel.js';
import { ANIMAL_CREATE_LIMIT } from '../../util/animal.js';

const AnimalOrBatchModel = {
animal: AnimalModel,
Expand Down Expand Up @@ -450,6 +451,9 @@ export function checkCreateAnimalOrBatch(animalOrBatchKey) {

checkIsArray(req.body, 'Request body');

if (req.body.length > ANIMAL_CREATE_LIMIT) {
return res.status(400).send(`Animal creation limit (${ANIMAL_CREATE_LIMIT}) exceeded.`);
}
for (const animalOrBatch of req.body) {
const { type_name, breed_name } = animalOrBatch;

Expand Down
1 change: 1 addition & 0 deletions packages/api/src/util/animal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import AnimalBatchModel from '../models/animalBatchModel.js';
import { checkIsArray, customError } from './customErrors.js';

export const ANIMAL_TASKS = ['animal_movement_task'];
export const ANIMAL_CREATE_LIMIT = 1000;

/**
* Assigns internal identifiers to records.
Expand Down
33 changes: 33 additions & 0 deletions packages/api/tests/animal.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import mocks from './mock.factories.js';
import CustomAnimalTypeModel from '../src/models/customAnimalTypeModel.js';
import CustomAnimalBreedModel from '../src/models/customAnimalBreedModel.js';
import AnimalUseRelationshipModel from '../src/models/animalUseRelationshipModel.js';
import { ANIMAL_CREATE_LIMIT } from '../src/util/animal.js';

describe('Animal Tests', () => {
let farm;
Expand Down Expand Up @@ -387,6 +388,38 @@ describe('Animal Tests', () => {
expect(res.status).toBe(400);
});

test('Should not be able to add >1000 animals', async () => {
const roles = [1, 2, 5];

for (const role of roles) {
const { mainFarm, user } = await returnUserFarms(role);

const [animalBreed] = await mocks.custom_animal_breedFactory({
promisedFarm: [mainFarm],
});

const animals = [];
for (let i = 0; i < ANIMAL_CREATE_LIMIT + 1; i++) {
animals.push(
mocks.fakeAnimal({
custom_type_id: animalBreed.custom_type_id,
custom_breed_id: animalBreed.id,
}),
);
}

const res = await postRequest(
{
user_id: user.user_id,
farm_id: mainFarm.farm_id,
},
animals,
);

expect(res.status).toBe(400);
}
});

describe('Create new types and/or breeds while creating animals', () => {
let farm;
let owner;
Expand Down
32 changes: 32 additions & 0 deletions packages/api/tests/animal_batch.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import CustomAnimalTypeModel from '../src/models/customAnimalTypeModel.js';
import CustomAnimalBreedModel from '../src/models/customAnimalBreedModel.js';
import AnimalBatchSexDetailModel from '../src/models/animalBatchSexDetailModel.js';
import AnimalBatchUseRelationshipModel from '../src/models/animalBatchUseRelationshipModel.js';
import { ANIMAL_CREATE_LIMIT } from '../src/util/animal.js';

describe('Animal Batch Tests', () => {
let farm;
Expand Down Expand Up @@ -294,6 +295,37 @@ describe('Animal Batch Tests', () => {
}
});

test('Users should not be able to create animal batch beyond ANIMAL_CREATE_LIMIT', async () => {
const roles = [1, 2, 5];
for (const role of roles) {
const { mainFarm, user } = await returnUserFarms(role);
const [animalBreed] = await mocks.custom_animal_breedFactory({
promisedFarm: [mainFarm],
});

const animalBatches = [];

for (let i = 0; i < ANIMAL_CREATE_LIMIT + 1; i++) {
animalBatches.push(
mocks.fakeAnimalBatch({
custom_type_id: animalBreed.custom_type_id,
custom_breed_id: animalBreed.id,
count: 6,
}),
);
}

const res = await postRequest(
{
user_id: user.user_id,
farm_id: mainFarm.farm_id,
},
animalBatches,
);
expect(res.status).toBe(400);
}
});

test('Non-admin users should not be able to create animal batch', async () => {
const roles = [3];

Expand Down
3 changes: 3 additions & 0 deletions packages/webapp/src/components/Animals/AddAnimalsFormCard/AddAnimalsFormCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ export default function AddAnimalsFormCard({
resetField,
formState: { errors },
} = useFormContext();

const ANIMAL_COUNT_LIMIT = 1000;
const { t } = useTranslation();
const watchAnimalCount = watch(`${namePrefix}${BasicsFields.COUNT}`);
const watchAnimalType = watch(`${namePrefix}${BasicsFields.TYPE}`);
Expand Down Expand Up @@ -112,6 +114,7 @@ export default function AddAnimalsFormCard({
<NumberInput
name={`${namePrefix}${BasicsFields.COUNT}`}
control={control}
max={ANIMAL_COUNT_LIMIT}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for working on this!

Screenshot 2024-11-20 at 3 55 23 PM

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 🙏

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

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 found that it doesn't propagate to the InputBase without specifying the value as a prop.

Copy link
Collaborator

@SayakaOno SayakaOno Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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!)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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:

  1. 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.
  2. 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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for your input, I'll discuss the solution with the team!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@navDhammu Thanks for explaining. @SayakaOno I think the 2nd way of using a key is the easiest.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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!

label={t('common:COUNT')}
className={styles.countInput}
allowDecimal={false}
Expand Down
Loading