Lf 4380 api route for edit update route for edit animal and batch details #3484
Conversation
Essentially seems to be working. Groups needs to be provided in obect format not array of ids. TODO check identifier what happens on edit.
…quests To do: - fix orgiin_id being editable - sex detail has no constraints on multiple batch_id sex_id combination - handle new types and breeds on animals and batches - handle verifying count when sex details change and vice versa
…change plain insert with insertGraph
…d groups utilities
- change utility functions to use positive language versus negating the language - commetns will be removed at the end
Plain patches '{ id, default_type_id }' were resulting in an not-null contraint failures on organic_status.
Commenting out organic_status from the basecontroller.upsertGraph data portion fixes the problem -- but why?
- organic_status was not defined ever! (not necessary on patch, default value on post)
- the model should not validate patch the same as post (tried different setting on upsert graph and default on model)
- the model could not possibly validate anything until after removeAdditionalProperties() runs which also results in organic_status property not being present -- not defined
This commit corrects the problem, but I really cant see why it is happening.
Duncan-Brain
requested review from
antsgar,
SayakaOno and
a team
and removed request for
a team
… though unreachable test
…new custom type or breed
…re adding new custom type or breed
| (oneExists(breedKeyOptions, animalOrBatch) && default_breed_id) || | ||
| (oneExists(typeKeyOptions, animalOrBatch) && default_type_id) |
There was a problem hiding this comment.
I'm not really good at reading this function, but can't these be simply default_breed_id || default_type_id?
There was a problem hiding this comment.
Let me double check this! oneExists is used to discriminatebetween nulls and undefined -- its also quite possible I copied or changed existing logic and did not return to double check since it was harmless.
There was a problem hiding this comment.
The fewer oneExists and oneTruthy checks, the better for reviewers!
There was a problem hiding this comment.
I think I have removed as many as possible in the new branch: b643951
| checkIdIsNumber(animalOrBatchRecord?.custom_breed_id); | ||
| customBreed = await CustomAnimalBreedModel.query() | ||
| .whereNotDeleted() | ||
| .findById(animalOrBatchRecord.custom_breed_id); | ||
| if (!customBreed) { | ||
| // This should not be possible | ||
| throw customError('Custom breed does not exist'); | ||
| } |
There was a problem hiding this comment.
Can we just remove checkIdIsNumber and the error throwing?
There was a problem hiding this comment.
I kept checkIdIsNumber to be consistent with other logic --- but it could be handled by objection by throwing 500 or other objection error.
I could probably remove the second check since if it is from an existing animalOrBatchRecord -- it would be really rare for it not to exist (concurrency)
There was a problem hiding this comment.
animalOrBatchRecord?.custom_breed_id is the id already stored in the DB, isn't it? I thought it wasn't necessary to validate saved data. The concurrency is a good point, let's keep it!
There was a problem hiding this comment.
Yes it is the id stored in db -- should be mostly safe to remove if we prefer that. Unlikely but possible the animal is deleted in between record retrieval and this check, likelihood increases with number of animals being edited at once.
I can go either way -- really I would like to see trx used across middleware and controller to solve the issue instead of what I have here.
There was a problem hiding this comment.
Should I remove all checkIdIsNumber's? (..and their tests 😉 )
There was a problem hiding this comment.
I thought this was the only place validating the data in the DB.
possible the animal is deleted in between record retrieval and this check
This raises a question... where are you checking if the animal is deleted?
Even if you run checkIdIsNumber, it wouldn't tell you if the animal was deleted, right...? Also, if that's a concern, should we add a check for the animal's status right before editing?
There was a problem hiding this comment.
This raises a question... where are you checking if the animal is deleted?
getRecordIfExists() checks for the animal record and adds it to invalidIds[] if not present using whereNotDeleted().
Even if you run checkIdIsNumber, it wouldn't tell you if the animal was deleted, right...?
checkIdIsNumber() I do not think does much. It only early returns a preferred response code and message instead of the Objection 500 server error that would eventually result from update failure.
CheckIdIsNumber and InvaldIds check are both carried over from other endpoints. I was hesitant to change the existing order of checks or to add extra code or spend more time. Personally I would consider allowing Objection to handle the error from checkIdIsNumber(), accept the 500 error, and delete all instances and tests. Also I would consider moving checkInvalidIds() into the loop --- but I think invalidIds was maybe a step towards partial success. Also custom type and breeds I think could be more in the loop too. If you think any of these changes are valuable I will happily add them.
Also, if that's a concern, should we add a check for the animal's status right before editing?
Yeah we could do that too... I would prefer figuring out transactions to just keep every single check query and patch action atomic. But it is harder.. it would be nice if whereNotDeleted() worked with insert/upserts. Should I reuse getRecordIfExists right before upsert? I do not think concurrency is a big problem but it could help.
| } | ||
| } | ||
| // Check custom breed if exists | ||
| if (customBreed) { |
There was a problem hiding this comment.
Is it impossible to somehow "return" when the breed is not specified at some point?
There was a problem hiding this comment.
I am not sure I understand correctly -- nothing is returned from any of these functions unless there is an error. If breed is not specified, (and a type is also not specified that needs to be checked against an existing breed), all of these checks will get to the end of the code and go to the next function await checkBatchSexDetail.
But I think I misunderstood what you mean.
There was a problem hiding this comment.
Sorry for the lack of clarity. What I found more difficult in the review is the part where "If breed is not specified, all of these checks will get to the end of the code." If there's a way to "exit" (which I meant by "return") before running all these checks, that would be ideal.
There was a problem hiding this comment.
I think I understand you are asking if there is a way to do an "early return"
checkAnimalType is simple but it puts some burden on checkAnimalBreed to check the pre-existing breed if type is edited too not just breed.
I could probably wrap the entire thing with
!someExists(breedKeyOptions, animalOrBatch) && !someExists(typeKeyOptions, animalOrBatch)
And that might solve any overuse of it in the other body checks -- related to your other comment about overusing these too.
There was a problem hiding this comment.
Early return it is!
I could probably wrap the entire thing with
!someExists(breedKeyOptions, animalOrBatch) && !someExists(typeKeyOptions, animalOrBatch)
That sounds great!
There was a problem hiding this comment.
Early return added here in new pr: 00efca4
There was a problem hiding this comment.
Left a few comments, but amazing work @Duncan-Brain 👏 👏 👏 this seems to have been really complex!
Re animal use relationship and animal batch sex detail having different architectures, I think that any tables that contain relationships for attributes in the animals or the batches should actually support soft delete (like sex detail). There's no point IMO in supporting soft deletion of an animal or batch if this deletion can't be undone because the related attributes are hard deleted. Say at some point we introduced a feature to "undo" the delete action in the UI -- we'd bring back the animal but it would have the use field empty. Or let's say we wanted to delete something manually and soft deleted and then wanted to rollback that action -- no way to entirely revert it if some of the attributes are lost.
Re the size of the PR, I didn't find it difficult to follow! It seems a big chunk of the work was the validation portion, parsing the request in the middleware, making sure it looks right and returning the right errors. One option in the future would be to split out that validation layer into a separate PR -- get out the request > controller > response basic flow first, and then account for bad requests.
packages/api/src/util/animal.js
Outdated
| * | ||
| * @throws {Error} - If any database operation fails. | ||
| */ | ||
| export const checkAndAddCustomTypeAndBreed = async (req, animalOrBatch, farm_id, trx) => { |
There was a problem hiding this comment.
I know so far we've been putting all the domain logic in the controllers and keeping the models (in my opinion) too lean, so this feedback isn't exclusive to this PR, but I think this logic should live in the custom breed and custom type models rather than in a common util. The custom breed and type models should have a method to upsert a new record (check if the ID exists and if not create a new record), and this method should be called from the animal and animal batch controllers. This, however, would involve a bunch of refactoring which probably needs to be a separate endeavor.
Alternatively, what this util here is functioning as is essentially a service, which is a separate layer containing domain logic that can be accessed from multiple different controllers. What you've bumped into here is one of the reasons putting all of the logic in the controller is not a good idea -- there's no way to share logic between different controllers. Instead of calling this a util, I'd call it a service to make it clearer what its function is in our architecture -- this shared logic issue a problem we're going to bump into more and more if we keep going with the "lean model" approach, and we'll either end up with a lot of duplicate code across controllers, or with services to contain it. I'd suggest creating a services folder within api/src and setting this method and the one below it within an animalService object.
This is an interesting topic for us to chat about more in tech daily! If you want to read a bit about it, I found out today that the "lean model" approach we're using is called "Anemic domain model" approach by people who consider it an anti-pattern https://en.wikipedia.org/wiki/Anemic_domain_model
There was a problem hiding this comment.
Your totally right, I considered adding it to the model I forget why I didn't... maybe because it would double the code and I saw this lonely file here with one function haha.
I think we came to some agreement a while ago to prefer fat models over fat controllers. I think I like the idea of services because I like lean controllers, and lean models as just a structured description of the db -- but we don't have that services construct yet.
Happy to add a new folder called services or something! But it might be good to get buy in from team. @kathyavini & @SayakaOno
| checkIdIsNumber(animalOrBatchRecord?.custom_breed_id); | ||
| customBreed = await CustomAnimalBreedModel.query() | ||
| .whereNotDeleted() | ||
| .findById(animalOrBatchRecord.custom_breed_id); | ||
| if (!customBreed) { | ||
| // This should not be possible | ||
| throw customError('Custom breed does not exist'); | ||
| } |
There was a problem hiding this comment.
animalOrBatchRecord?.custom_breed_id is the id already stored in the DB, isn't it? I thought it wasn't necessary to validate saved data. The concurrency is a good point, let's keep it!
| } | ||
| } | ||
| // Check custom breed if exists | ||
| if (customBreed) { |
There was a problem hiding this comment.
Sorry for the lack of clarity. What I found more difficult in the review is the part where "If breed is not specified, all of these checks will get to the end of the code." If there's a way to "exit" (which I meant by "return") before running all these checks, that would be ideal.
| (oneExists(breedKeyOptions, animalOrBatch) && default_breed_id) || | ||
| (oneExists(typeKeyOptions, animalOrBatch) && default_type_id) |
There was a problem hiding this comment.
The fewer oneExists and oneTruthy checks, the better for reviewers!
| [updatedFirstAnimal, updatedSecondAnimal].forEach((animal) => { | ||
| // Should not cause an error | ||
| delete animal.extra_non_existant_property; | ||
| // Should not be able to update on edit | ||
| animal.animal_removal_reason_id = null; | ||
| // Return format different than post format | ||
| animal.group_ids = animal.group_ids.map((groupId) => groupId.animal_group_id); | ||
| animal.animal_use_relationships.forEach((rel) => { | ||
| rel.animal_id = animal.id; | ||
| rel.other_use = null; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
It seems a bit unusual to use the same object as a request body and an expected result.
Why don't you create expected animals using a map like this?
const [expectedFirstAnimal, expectedSecond...] = [updatedFirstAnimal, updatedSecondAnimal].map((animal) => {
const {extra_non_existant_property, ...rest} = animal;
return {
...rest,
animal_removal_reason_id: null;
group_ids: rest.group_ids.map((groupId) => groupId.animal_group_id);
animal_use_relationships: ...
});
});
There was a problem hiding this comment.
Yeah you are right, I have yet to learn what errors can be caused by mutating reference objects like this. I will use something like your proposal.
I also probably should not have extra tests inside this test. Testing posting non-existant-property would be most useful as a generic test across all endpoints as its own thing, I might remove some of these.
|
@antsgar and @SayakaOno Thanks for reviewing I will probably take almost all your suggestions! Should I do now or on follow up PR? Anto - just regarding the hard delete / soft delete and cascading delete -- to be clear its only on successful edit that it hard deletes unused relationships. I agree it shouldn't hard delete the relationship generally speaking on soft delete of the animal... At least how animal works now (and to be fair I needed to double check 😂 ) soft deleting the animal keeps the use_relationship even though it doesn't have base properties. We discussed what should happen during cascading delete sequences during |
|
@Duncan-Brain ah alright, that sounds good! In that case we should probably create a ticket to update the sex_detail table which is probably the only one not following that pattern? I wonder if we should also change its name to something like I'm good with a followup PR if we prefer to merge as is to unblock @kathyavini's work, up to you! |
… naming, handling false and zero
…BAtch and improve sex logic
|
Thanks for reviewing @SayakaOno and @antsgar. Will do a follow up PR for the rest of the comments! @kathyavini you should hopefully be unblocked but let me know if your testing leads to any other missing tests. |
|
Follow up PR here #3510 - will be resolving comments here addressed in other pr. |
Description
How to review this PR!
Main suggestion if you want to go through and understand it:
sex_detail,animal_identifiers,groups(for now)Why is this so huge?
.. I feel I need to justify this a bit, but would truly appreciate any peer feedback about the size of this PR.
crop.test.js+crop_variety.test.js= ~1400LOC,task.test.js= ~3400 LOCWhat the heck is in here?
Other notes:
default_type_id,custom_type_id,type_name,default_breed_id,custom_breed_id,breed_namedepending on available values.Key learnings:
sex_detailsandanimal_use_relationshiphave different architectures -- I think I prefer the composite key + hard deletion solution over the new unique id + soft-delete as analogous to losing information when we change any single column value. For soft delete information -- will we really want to keep this sub-detail?Future Improvements (should I make a ticket for any of these?)
origin_idandbrought_in_datesex_details- without good middleware we can have:{id: 4, batch_id: 2, sex: female, count:2 deleted: false} , {id: 4, batch_id: 2, sex: female, count:3 deleted: false}-- we could return the wrong one.default_type_idandtype_nameon same farmgroup_idcode on animalController, it would be easy to do this.Jira link: LF-4380
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: