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.

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

[Not urgent!] LF-4384(b): Refactor useAnimalOrBatchRemoval #3549

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

Conversation

SayakaOno
Copy link
Collaborator

Description

Currently the useAnimalOrBatchRemoval hook accepts inventory IDs and parses them. This parsing is redundant for the SingleAnimalView, which already has a parsed ID. This PR makes the hook more generic by:

  • Updating useAnimalOrBatchRemoval to accept:
    • ids with kind (animal or batch key) instead of inventory IDs
    • onSuccess instead of setSelectedInventoryIds
  • Move the inventory ID parsing and setSelectedInventoryIds logic to the Inventory component.

(This PR addresses the comment on #3534)

@SayakaOno SayakaOno added the enhancement New feature or request label Nov 26, 2024
@SayakaOno SayakaOno self-assigned this Nov 26, 2024
@SayakaOno SayakaOno requested review from a team as code owners November 26, 2024 01:37
@SayakaOno SayakaOno requested review from antsgar, kathyavini and Duncan-Brain and removed request for a team and kathyavini November 26, 2024 01:37
@SayakaOno SayakaOno mentioned this pull request Nov 26, 2024
16 tasks
@SayakaOno SayakaOno force-pushed the LF-4384_2/Refactor_useAnimalOrBatchRemoval branch from 984e09a to 5290430 Compare November 27, 2024 19:11
@SayakaOno SayakaOno changed the title LF-4384(b): Refactor useAnimalOrBatchRemoval [Not urgent!] LF-4384(b): Refactor useAnimalOrBatchRemoval Nov 27, 2024
Base automatically changed from LF-4384/End-to-End_Animal_Details to integration November 28, 2024 16:21
setSelectedInventoryIds((selectedInventoryIds) => {
return selectedInventoryIds.filter((i) => {
const { kind, id } = parseInventoryId(i);
return kind === animalOrBatchKey && ids.includes(id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The previous filter was : !selectedAnimalIds.includes(i))

Everything looks like it works so I am not sure it matters, but it looks like it is setting the selected inventory id to the ones that were successfully removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you so much for catching it! I remember getting confused about how to use .filter() suddenly and ended up flipping the condition! 🙏
It's fixed now!

Duncan-Brain
Duncan-Brain previously approved these changes Nov 29, 2024
Copy link
Collaborator

@Duncan-Brain Duncan-Brain left a comment

Choose a reason for hiding this comment

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

Left an inline comment but I am not sure it is valid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants