LF-4121 Create PATCH endpoints for animals and batches #3137
Conversation
Also added a note about hasFarmAccess for discussion; it should not be used here
It was just for discussion this morning and has been discussed :)
…le for use in models as well
Animal and animal batch had too many string properties to handle them one by one in the controller as had been the previous pattern. I also don't think strings were being trimmed on insert (add controllers) and this will handle that as well.
…y deleted license
Realized in reading the test cases that a more general patch would need to restate all of the various checks in addAnimal, which should be handled in some other way when those requirements have been scoped.
Add logging to function since this is required for Sentry reporting
…POST Database check is enforced and already handled by error handler
kathyavini
requested review from
antsgar and
SayakaOno
and removed request for
a team
| await super.$beforeUpdate(opt, queryContext); | ||
| this.trimStringProperties(); | ||
| } | ||
|
|
There was a problem hiding this comment.
I added this here because:
- There were too many string properties on this table to check individually in the controller. I added it originally for a general PATCH, but right now it will apply to the POST endpoints and the single string property --
removal_explanationof PATCH - String trimming had not been included on the POST endpoint. Since
addAnimals()is already a pretty lengthy controller, I did not want to add to it.
Curious if people like this pattern enough to generalize it; for now it is just on these two models.
There was a problem hiding this comment.
I think it's a great idea!
kathyavini
changed the base branch from
integration
to
LF-4120-add-fields-to-animal-and-animal-batch-tables
| @@ -268,19 +268,4 @@ export default { | |||
|
|
|||
| return query.first(); | |||
| }, | |||
|
|
|||
There was a problem hiding this comment.
Extracted from baseController to general /utils so it could be used in models as well. Also upon reflection it never needed to be in a controller to start with.
kathyavini
changed the title
This is much easier than messing with the JSON schema validator
| const sentAnimalIds = req.body.map(({ id }) => id); | ||
| const invalidAnimalIds = []; | ||
|
|
||
| for (const id of sentAnimalIds) { |
There was a problem hiding this comment.
I think it would be better to loop req.body as you did on line 206.
There was a problem hiding this comment.
Ahhh, then there is no need to create a new array!
| return Object.entries(this.jsonSchema.properties) | ||
| .filter(([, value]) => value.type.includes('string')) | ||
| .map(([key]) => key); |
There was a problem hiding this comment.
It would be better to do filter and map in a for loop for better performance.
There was a problem hiding this comment.
Would the improvement in performance be specifically from using for...of, or from not having to iterate two times? For instance, if the filter and map were combined in a reduce instead?
static get stringProperties() {
return Object.entries(this.jsonSchema.properties).reduce((acc, [key, value]) => {
if (value.type.includes('string')) {
acc.push(key);
}
return acc;
}, []);
}I've love to read about method efficiency, do you know of a good source?
Edit: Or I guess you didn't specify for...of! So it could also be
static get stringProperties() {
let stringProperties = [];
for (const key in this.jsonSchema.properties) {
if (this.jsonSchema.properties[key].type.includes('string')) {
stringProperties.push(key);
}
}
return stringProperties;
}There was a problem hiding this comment.
Sorry! It seems better to prioritize readability, so no changes are necessary!
My suggestion was to make the array in one loop instead of two.
I know reduce() and looping an object keys (for in) are not great in terms of performance, but I can't find good sources...
I learned data structure from a Udemy course, but I will share other resources when I find!
If I were to do it in one loop, I would do:
const stringProperties = [];
for (const [key, value] of Object.entries(this.jsonSchema.properties)) {
if (value.type.includes('string')) {
stringProperties.push(key);
}
}
There was a problem hiding this comment.
I like your version the best!
I'm happy to hear reduce is not good for performance because in my opinion it's also not nice for readability 😂
| await super.$beforeUpdate(opt, queryContext); | ||
| this.trimStringProperties(); | ||
| } | ||
|
|
There was a problem hiding this comment.
I think it's a great idea!
kathyavini
changed the base branch from
LF-4120-add-fields-to-animal-and-animal-batch-tables
to
integration
kathyavini
changed the base branch from
integration
to
LF-4120-add-fields-to-animal-and-animal-batch-tables
…imals-and-batches
kathyavini
changed the base branch from
LF-4120-add-fields-to-animal-and-animal-batch-tables
to
integration
There was a problem hiding this comment.
Now that removed is removed, I feel like we should have separate routes for removal. It feels unstable to rely on just animal_removal_reason_id which is optional...
- I was referring to
taskController, we should probably check if animals/batches have been already removed? - I can't remember, but did we need timestamp for removal...?
@antsgar can you remind me if we have had any discussion of how removed + timeline are going to interact? It seems that we would need a removal date, which means the user has to supply it? |
|
|
@SayakaOno uh oh, there it is then
Probably should have been caught at grooming or planning that the date didn't make it into the UI 😦 Time to check back in with @loicsans and @antsgar Edit: Although on second read I guess it could equally be that we create a date but don't show it until some future screen / flow and that is what the table cell refers to 🤔 |
|
@kathyavini ah that's my bad, I didn't notice the date thing! We'd probably need to add a removed_date field to the tables then. My assumption is that this doesn't mean a date for when the animal actually died / got sold / etc, but the timestamp for when it was registered as removed in the UI, which would mean we wouldn't have an actual UI field to fill it in but just set it when the record is marked as deleted, but could we double check with Loïc just in case? |
|
Some updates following group discussion:
|
|
Are we going to allow |
|
@SayakaOno we discussed yesterday (sorry maybe we should have repeated today) that we pretty much don't care about that property, since it is true optional and we will only display it in client when there is a removal reason id provided (i.e. when the animal is removed). Obviously the client also won't send a PATCH request where it the explanation is present without reason id, but is there another situation to be concerned about? |
There was a problem hiding this comment.
@kathyavini No, I can compromise. When looking at the data, I would wonder why active animals have removal_explanation. I'm thinking about not only the webapp but also postman and external API users (which we probably don't need to think about anymore) as its clients.
|
@SayakaOno got it, that makes sense. Well we have to migrate on the table anyway for date, so I like the idea of a constraint in that same migration for explanation. @antsgar may I add that to the date ticket if that sounds right -- that db table should never show date or explanation if the animal is currently active? (I also feel like we just came full circle from removing db checks in the previous backend ticket 😅) |
|
@kathyavini yeah that sounds reasonable! Lol yeah back to constraints, but at least there's one less field to worry about |
antsgar
deleted the
LF-4121-create-patch-endpoints-for-animals-and-batches
branch
Description
This adds PATCH endpoints and tests specifically for handling animal/animal batch removal. Other properties cannot be updated via this iteration of controller.
Notes:
animal_removal_reason_idJira link: https://lite-farm.atlassian.net/browse/LF-4121
Type of change
How Has This Been Tested?
Checklist: