Lf 4528 / Lf 4564 / hide created in error option for animals with completed tasks - alternate option #3570

Duncan-Brain · 2024-12-05T17:47:21Z

Description

Invalidates required invalidation ticket https://lite-farm.atlassian.net/browse/LF-4564

Alternate option to #3554

~~Adds fix for new undocumented bug bug - missing backend check for animal tasks on delete endpoint, important for concurrency too.~~ Wrong about this Sayaka fixed last week in PR that got merged yesterday woopsie 🙏

Notes:

Mirrors /check_delete on /location
De-nests task under animals
- Punts the issue of how to search for animal tasks on the frontend to the future so it can be discussed
- I am concerned about nesting modules under other modules, if we nest everything under each other like we have in the past we risk having to fetch everything with every change
- Others might also be interested in discussing flatter normalized solutions: LF-4528: Hide created in error option for animals with completed tasks #3554 (comment)
new onClick function would set us up for dealing with other bad requests
could possible make dryRun another param and not make new route

Jira link: LF-4564, Alternate to merged LF-4528

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

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

Passes test case
UI components visually reviewed on desktop view
UI components visually reviewed on mobile view
Other (please explain):
abandon or complete a task with animals associated, try to remove the animal in inventory view, created in error option not visible.

Checklist:

I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
The precommit and linting ran successfully
I have added or updated language tags for text that's part of the UI
I have added "MISSING" for all new language tags to languages I don't speak
I have added the GNU General Public License to all new files

This reverts commit 9f26f35.

This reverts commit ed86d4d.

…d or completed tasks

…nimal or batch prior to deletion and allow it to bypass controller using dry run

…urn onClick function

SayakaOno

Thank you for adding the validation!

I like this simple approach.
I wish we rendered RemoveAnimalsModal only when necessary (it's rendered even if isOpen is false). This would allow us to move the logic from the hook to the component and avoid using state to hide the error option.

SayakaOno · 2024-12-05T22:00:05Z

packages/api/src/routes/animalBatchRoute.js

@@ -49,6 +49,11 @@ router.patch(
  checkRemoveAnimalOrBatch('batch'),
  AnimalBatchController.removeAnimalBatches(),
 );
+router.delete(


Is DELETE the right method?

GET was what I went with initially and makes more common sense. But I think the mutation was easier to use than the query.

On further looking I might actually be interested in a new pattern of reusing the existing delete endpoint with a new parameter called "dryRun" or something.

I could change if we think GET or other approach is best

SayakaOno · 2024-12-05T23:06:56Z

packages/webapp/src/containers/Animals/Inventory/useAnimalOrBatchRemoval.ts

      }
    }
-  }, [removalModalOpen, completedTasks, abandonedTasks, selectedInventoryIds]);
+    return false;


I'm not sure if returning false for some unknown error is expected...

So this is an unlikely scenario -- which we could handle differently like forcing a refresh or something because it indicates a concurrency issue (or api down) since those are the only other tests.

My thought was if there any other error -- it is okay to show the created in error option now accidentally -- because the api will now reject it if incorrect.

Edit: I see now there is also a date check!

SayakaOno · 2024-12-05T23:09:49Z

packages/webapp/src/containers/Animals/Inventory/useAnimalOrBatchRemoval.ts

+  const onClickRemoveAnimals = async () => {
+    const hideDelete = await checkHasFinalizedTasks();
+    setHasFinalizedTasks(hideDelete);
+    setRemovalModalOpen(true);
+  };


I think the ideal behaviour is to open the modal first and handle the loading status within the modal. The APIs seem fast enough, though.

I'm not sure ! I figured I would give it the extra millisecond to run the query haha

But that makes sense too.. should I update?

…for_animals_with_completed_Tasks-b

Duncan-Brain · 2024-12-06T02:03:51Z

Sorry @SayakaOno I did not realize you did the delete endpoint part here ! #3546 -- I am bad about knowing the other tickets and I threw this together in a rush so I should have searched.

Trying to resolve conflicts now...

SayakaOno · 2024-12-06T02:08:05Z

I prefer to merge before bug bash, so I will approve.
Could you remove unused code from the hook?

Duncan-Brain · 2024-12-06T02:23:05Z

@SayakaOno sounds good - I merged our two code into one. Mostly kept yours for the middleware/model with tiny tweaks.

Please tell me everything you still dislike and I can do a follow up! 🙏 appreciate your patience

Duncan-Brain · 2024-12-06T02:51:21Z

There were some failing tests due to extracting out idsSet I think it is still the right thing to do and that there are some unnecessary test in checkValidAnimalOrBatchIds. But its getting late and bug bash is tomorrow. I can troubleshoot tests on possible follow up pr.

antsgar

I really like the thought of de-nesting tasks to avoid having to re-fetch animals, and moving more logic to the BE. Great thinking!! 👏

The one thing I don't like, although it's a pre-existing pattern, is the definition of the new endpoint. I know we already have a couple of these check_delete endpoints for other entities, but I really want us to start trying to get more consistent with REST just to have a more standardized way of defining the endpoints that's easier to read, understand and maintain -- right now it's honestly all over the place, and with new code it's good for us to slowly start implementing cleaner patterns.

So my suggestion here would be to keep the rest of the code the same, but changing the route to something slightly more REST-y

GET /animals?ids=1,2,3&checkDelete=true

And then we'd have to call the function
Regardless of the shape of the URL, 100% this needs to be a GET route, since no resource is actually getting deleted.

antsgar · 2024-12-06T00:50:44Z

packages/api/src/middleware/validation/checkAnimalOrBatch.js

@@ -607,18 +614,24 @@ export function checkRemoveAnimalOrBatch(animalOrBatchKey) {
 * );
 *
 */
-export function checkDeleteAnimalOrBatch(animalOrBatchKey) {
+export function checkDeleteAnimalOrBatch(animalOrBatchKey, dryRun = false) {


It might just be me and I'm not sure what better name there'd be but I think I'd have a hard time understanding what dryRun means here if I didn't have this PR as context 🤔

I think I'd prefer the pattern from the other controllers where this function wouldn't take an extra param but there'd be an actual function on the controller for the new endpoint just to return the 200 status -- seems a bit over the top but it might be easier to understand at a glance

@antsgar
https://dx.apiture.com/docs/api/concepts/dry-runs/#:~:text=Performing%20a%20Dry%20Run,error%20data%20and%20response%20code.

https://stackoverflow.com/questions/31967684/dry-run-strategy-for-rest-api

https://www.google.com/search?q=dry+run+meaning&oq=dry+run+&gs_lcrp=EgZjaHJvbWUqCggBEAAYsQMYgAQyBggAEEUYOTIKCAEQABixAxiABDIHCAIQABiABDIHCAMQABiABDIHCAQQABiABDIHCAUQABiABDIHCAYQABiABDIHCAcQABiABDIHCAgQABiABDIHCAkQABiABNIBCDI5MDdqMGo3qAIAsAIA&sourceid=chrome&ie=UTF-8

I understand, but the strategy described in the links you posted is not what's being implemented here. In the links, what's being described is a situation where the client has the intention of doing a certain operation and needs to validate if that's possible -- translated to our situation here, an actual dry run would happen if we called this endpoint after the user selected the "Created in error" option. If that were the case, then yes, the strategy would be what's described in the links, we'd just be validating if we can delete before actually deleting. Here, on the other hand, we're checking if the operation is possible but we have no idea if we'll actually delete or not afterwards. It's for that same reason that using the "delete" HTTP method is wrong -- it's not the same as if we tried to delete and got an error, because when we call this endpoint we have no intention of deleting the resource yet.

Duncan-Brain · 2024-12-06T03:10:42Z

Thanks for reviewing @antsgar , I added a few links about dry runs in the comment to help display others views on the concept!

I don't yet know where I stand regarding using GET or not personally... my understanding is REST is a set of guidelines not rules so we just need to decide what we like best! We don't actually allow getting a single id on our GET endpoint -- so I don't actually know how REST-ful it is to work off of "collections" like we are for most of animals.

I could be down to switch to a GET --- but also using the exact endpoint you intend to use for a dry-run makes some sense to me too to reuse logic. I see no reason why you have to delete on a DELETE, when an error happens you don't delete haha

Duncan-Brain · 2024-12-06T03:19:43Z

I have got to sign off for the night -- happy to merge now and commit to changing before release ...or continue discussion tomorrow. It was mentioned we might want this functionality in before bash

antsgar · 2024-12-06T12:49:09Z

@Duncan-Brain REST does have a set of "rules" that we can follow as much as we want -- we could even decide not to do REST at all, but we need some sort of rules to solve the current lack of consistency. When we discussed the animal routes last year, we landed on routes that followed the REST guidelines better than what we had previously, so it's based on that decision that I always try to get us as close as possible to that when we add new changes. If as a team we want to follow a different set of rules altogether that's totally okay, but there should be a concrete proposal and we should discuss before moving towards that direction.

We don't actually allow getting a single id on our GET endpoint -- so I don't actually know how REST-ful it is to work off of "collections" like we are for most of animals.

Following REST definitely doesn't mean we need to add endpoints to GET single resources (although we could if we needed them) and does allow getting a whole collection or a subset of it. A different story is whether updating or deleting resources in batches like we are instead of by single resource is RESTful or not, that's something I read about a bit when we designed those endpoints so we could have that discussion and improve them. Our animal endpoints however do follow the RESTful principle of having the path of the endpoint describe the resource, and the HTTP verb describe the operation. The endpoint added here doesn't follow that principle.

All in all, I don't think we should merge this as is. As a quicker fix, we can always just add the tag invalidation to the previous solution -- and it's okay too to just call this out as a known issue if we need more time for it and it doesn't get in before bug bash.

antsgar · 2024-12-06T13:36:35Z

I've been taking a quick look at this, and I think there's a simpler fix at this point that unnests animals/batches from tasks and avoids unnecessarily needing to invalidate the animal/batches tags and refetching the whole inventory. We already have the tasks with their corresponding animals, so we just need to update the logic in the UI to check for that instead of checking the nested tasks in animals. I think that was what was originally suggested by @kathyavini here. I opened a PR with this fix here #3582

I know there are other changes in this PR beyond the fix to the invalidating tags issue that could be worth discussing, but I think we'll need more time for that and I'd prefer that we don't refactor the BE this much without being sure it's the direction we want to go towards.

Duncan-Brain added 9 commits December 4, 2024 20:02

Revert "LF-4528 Update animal models and controllers"

00fefb8

This reverts commit 9f26f35.

Revert "LF-4528 Update tests for GET animals/batches to expect tasks"

0cfe431

This reverts commit ed86d4d.

LF-4528 Add model service that checks animals or batches for abandone…

7c695f7

…d or completed tasks

LF-4528 Update middleware to check for completed abandoned tasks on a…

30b4465

…nimal or batch prior to deletion and allow it to bypass controller using dry run

LF-4528 Add route for check_delete

03bf927

LF-4528 Add new api endpoint to rtk

7ce2030

LF-4528 Rewrite hasFinalised function and to use new endpoint and ret…

2417d95

…urn onClick function

LF-4528 Implement new onclick function

85f93d4

LF-4528 Remove types from stories

90d23bc

Duncan-Brain requested review from a team as code owners December 5, 2024 17:47

Duncan-Brain requested review from antsgar, SayakaOno and kathyavini and removed request for a team December 5, 2024 17:47

Duncan-Brain changed the title ~~Lf 4528/hide created in error option for animals with completed tasks - alternate option~~ Lf 4528 / Lf 4564 / hide created in error option for animals with completed tasks - alternate option Dec 5, 2024

kathyavini assigned Duncan-Brain Dec 5, 2024

SayakaOno reviewed Dec 5, 2024

View reviewed changes

Merge branch 'integration' into LF-4528/Hide_Created_in_Error_option_…

553e88b

…for_animals_with_completed_Tasks-b

Duncan-Brain added 3 commits December 5, 2024 21:16

LF-4528 Resolve merge duplicate model and middleware function

ebb97c7

LF-4528 remove unused code and update error message

2f394fa

LF-4528 More unused code

e516222

SayakaOno previously approved these changes Dec 6, 2024

View reviewed changes

LF-4528 Fix failing tests

dc5bd6a

Duncan-Brain dismissed SayakaOno’s stale review via dc5bd6a December 6, 2024 02:49

Duncan-Brain requested a review from SayakaOno December 6, 2024 02:49

antsgar reviewed Dec 6, 2024

View reviewed changes

Duncan-Brain requested a review from antsgar December 6, 2024 03:17

Duncan-Brain closed this Dec 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lf 4528 / Lf 4564 / hide created in error option for animals with completed tasks - alternate option #3570

Lf 4528 / Lf 4564 / hide created in error option for animals with completed tasks - alternate option #3570

Duncan-Brain commented Dec 5, 2024 •

edited

Loading

SayakaOno left a comment

SayakaOno Dec 5, 2024

Duncan-Brain Dec 6, 2024 •

edited

Loading

SayakaOno Dec 5, 2024

Duncan-Brain Dec 6, 2024 •

edited

Loading

SayakaOno Dec 5, 2024

Duncan-Brain Dec 6, 2024

Duncan-Brain commented Dec 6, 2024

SayakaOno commented Dec 6, 2024

Duncan-Brain commented Dec 6, 2024

Duncan-Brain commented Dec 6, 2024 •

edited

Loading

antsgar left a comment

antsgar Dec 6, 2024

Duncan-Brain Dec 6, 2024 •

edited

Loading

antsgar Dec 6, 2024

Duncan-Brain commented Dec 6, 2024 •

edited

Loading

Duncan-Brain commented Dec 6, 2024

antsgar commented Dec 6, 2024

antsgar commented Dec 6, 2024

Lf 4528 / Lf 4564 / hide created in error option for animals with completed tasks - alternate option #3570

Lf 4528 / Lf 4564 / hide created in error option for animals with completed tasks - alternate option #3570

Conversation

Duncan-Brain commented Dec 5, 2024 • edited Loading

SayakaOno left a comment

Choose a reason for hiding this comment

SayakaOno Dec 5, 2024

Choose a reason for hiding this comment

Duncan-Brain Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

SayakaOno Dec 5, 2024

Choose a reason for hiding this comment

Duncan-Brain Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

SayakaOno Dec 5, 2024

Choose a reason for hiding this comment

Duncan-Brain Dec 6, 2024

Choose a reason for hiding this comment

Duncan-Brain commented Dec 6, 2024

SayakaOno commented Dec 6, 2024

Duncan-Brain commented Dec 6, 2024

Duncan-Brain commented Dec 6, 2024 • edited Loading

antsgar left a comment

Choose a reason for hiding this comment

antsgar Dec 6, 2024

Choose a reason for hiding this comment

Duncan-Brain Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

antsgar Dec 6, 2024

Choose a reason for hiding this comment

Duncan-Brain commented Dec 6, 2024 • edited Loading

Duncan-Brain commented Dec 6, 2024

antsgar commented Dec 6, 2024

antsgar commented Dec 6, 2024

Duncan-Brain commented Dec 5, 2024 •

edited

Loading

Duncan-Brain Dec 6, 2024 •

edited

Loading

Duncan-Brain Dec 6, 2024 •

edited

Loading

Duncan-Brain commented Dec 6, 2024 •

edited

Loading

Duncan-Brain Dec 6, 2024 •

edited

Loading

Duncan-Brain commented Dec 6, 2024 •

edited

Loading