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

Refactor button reservable from another library #797

Conversation

kasperbirch1
Copy link
Contributor

Link to issue

https://reload.atlassian.net/browse/DDFSOEG-528

Description

This pull request refactors the order buttons by moving the logic for displaying the MaterialButtonReservableFromAnotherLibrary component and refactoring the getReservablePidsFromAnotherLibrary function into a useReservableFromAnotherLibrary hook. That also checks if the material is not reservable and should be ordered via openOrder.

Screenshot of the result

If the material only can be reserved on another library, we don't want to show other buttons like `MaterialButtonFindOnShelf`
…notherLibrary` hook

Added validation with FBS to verify manifestation's reservable status (must be false) for Overbygningsmaterial/OpenOrder reservations.

in addition, I have removed out the tests that were for `getReservablePidsFromAnotherLibrary` as they must be rewritten for the new hook
@kasperbirch1 kasperbirch1 changed the title Move material button reservable from another library Refactor button reservable from another library Dec 20, 2023
@spaceo spaceo self-requested a review December 20, 2023 10:30
Copy link
Contributor

@spaceo spaceo left a comment

Choose a reason for hiding this comment

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

Looks good. But I think it is a bad idea to delete tests without adding new

src/core/utils/useReservableFromAnotherLibrary.tsx Outdated Show resolved Hide resolved
Following the update from DBC, we now have a material that meets all requirements to activate the `MaterialButtonReservableFromAnotherLibrary` (openOrder) button.
Simplified return value to directly return the array, eliminating the need for an intermediate object.
@kasperbirch1 kasperbirch1 force-pushed the move-material-button-reservable-from-another-library branch from 86f17a4 to 862f61f Compare December 21, 2023 15:41
Due to the `UseReservableManifestations` hook validating whether a manifestation is reservable, we cannot use the `reservableManifestations` variable it returns in the new `useReservableFromAnotherLibrary` hook. Therefore, I have used the `selectedManifestations` variable within `useReservableFromAnotherLibrary`.

Additionally, I have implemented minor changes within `saveReservation`. Specifically, I removed the guard check for an early return. Now, it directly checks for the required criteria to determine whether to use `mutateAddReservations` or `mutateOpenOrder`.
Replace fixture with a material requiring reservation via openOrder (overbygningsmateriale).
This line was confusing...  the status will be set in the `translateOpenOrderStatus` function, and the translations for them should be descriptive of the current status of the order.
Copy link
Contributor

@spaceo spaceo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@kasperg kasperg removed their assignment Jan 3, 2024
@kasperg
Copy link
Contributor

kasperg commented Jan 3, 2024

@spaceo already gave this a 👍 😄

@kasperg kasperg changed the base branch from release/2024-2-0 to develop January 9, 2024 12:49
@kasperbirch1 kasperbirch1 changed the base branch from develop to release/2024-4-0 January 11, 2024 07:22
@kasperbirch1 kasperbirch1 merged commit 3cd7e08 into release/2024-4-0 Jan 11, 2024
16 checks passed
@kasperbirch1 kasperbirch1 deleted the move-material-button-reservable-from-another-library branch January 11, 2024 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants