Fix(inventory, stock-location): Remove orphaned location levels and reservations #3460

pKorsholm · 2023-03-13T10:34:30Z

What

Remove related inventory levels and reservation items when a stock location is removed

How

Add bulk deletion methods for both inventory levels and reservation items to the inventory service api
invoke both on location removal

Fixes CORE-1232

changeset-bot · 2023-03-13T10:34:34Z

🦋 Changeset detected

Latest commit: a839372

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@medusajs/inventory	Patch
@medusajs/medusa	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel · 2023-03-13T10:39:20Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
medusa-docs	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Mar 15, 2023 at 8:57AM (UTC)

srindom · 2023-03-13T13:13:33Z

packages/inventory/src/services/inventory-level.ts

@@ -21,6 +21,7 @@ export default class InventoryLevelService extends TransactionBaseService {
 CREATED: "inventory-level.created",
 UPDATED: "inventory-level.updated",
 DELETED: "inventory-level.deleted",
+ DELETED_BY_LOCATION: "inventory-level.deleted-by-location",


suggestion: rename to BULK_DELETED and allow the event data to be dynamic based on the query. I.e.: Event data could be: { location_id: ... } but { inventory_item_id: ... } would also be valid.

This way we don't have to create DELETED_BY_X for every deletion dimension. People who want to build synchronization could simply listen to DELETED and BULK_DELETED events.

@adrien2p, @olivermrbl - curious to hear your thoughts on this too?

I would say that we reuse the delete event name and pass the location_id, if the user listen to delete, there is either an id or a other key. If there is an id the user will retrieve the id, if there is something else the user will list by. Does it make sense?

Could you give an example maybe? :)

something like that

// delete by location id await this.eventBusService_.emit(InventoryLevel.Events.DELETED, { location_id }) // delete by id await this.eventBusService_.emit(InventoryLevel.Events.DELETED, { id }) eventBusService_.subscribe(InventoryLevel.Events.DELETED, (eventName, data) => { if (data.id) { return await this.handleInventoryLevelDeleteById(data.id) } return await this.handleInventoryLevelDelete(data) }) async function handleInventoryLevelDeleteById(id) { // do something } async function handleInventoryLevelDelete(selector) { // do something // this.inventoryLevelService_.list(selector, { withDeleted: true }) // Or just use the selector for something else }

I splitted it for the example, but the id is also a selector in fact

I would be fine with that too -- would just have to be documented.

Eventually, second options is

const { raw } = levelRepository.delete({ location_id: locationId }) const events = raw.map(level => ({ eventName: InventoryLevel.Events.DELETED, data: { id: level.id }) await this.eventBusService_.withTransaction(manager).emit(events)

Wdyt @srindom ?

For the later, it require to merge master in develop :D

Going for the pure deleted event, where do you want it documented @srindom?

cc: @shahednasser, can we document this behavior in the events reference :)

@srindom sure, once it's published I'll include this behavior in the events reference (creating a linear issue for it) 👍🏻

packages/inventory/src/services/reservation-item.ts

srindom

Please add a test to ensure that this logic works correctly :)

…eservations

srindom

LGTM!

srindom · 2023-03-14T09:45:44Z

packages/admin-ui/ui/src/domain/inventory/locations/components/location-card/index.tsx

@@ -37,7 +37,7 @@ const LocationCard: React.FC<Props> = ({ location }) => {
 const onDelete = async () => {
 const shouldDelete = await dialog({
 heading: "Delete Location",
- text: "Are you sure you want to delete this location",
+ text: "Are you sure you want to delete this location. This will also delete all inventory levels and reservations associated with this location.",


praise: great UX addition!

…eservations

srindom · 2023-03-14T14:33:49Z

@pKorsholm - can you check up on the failing test :)

…eservations

srindom · 2023-03-14T22:10:57Z

The integration problem feels like a missing await - can't find any though; could it be because we don't do return await in delete functions?

adrien2p · 2023-03-15T08:21:58Z

The integration problem feels like a missing await - can't find any though; could it be because we don't do return await in delete functions?

on Master I had the same issue and it was due to missing transaction during the emit, might be worth looking at the diff with master. I think that the issue comes from the fact that the plugin integration use a mock of redis and might not support bulk

pKorsholm · 2023-03-15T08:51:37Z

Clutch @adrien2p! Thanks, that was the issue 😅

pKorsholm added 2 commits March 13, 2023 11:31

initial removal of reservations and inventory items on location deletion

8f2668f

add changeset

31cabe2

pKorsholm requested a review from a team as a code owner March 13, 2023 10:34

pKorsholm requested a review from srindom March 13, 2023 10:34

vercel bot deployed to Preview March 13, 2023 10:58 View deployment

srindom reviewed Mar 13, 2023

View reviewed changes

remove deleted bulk event

bc931f2

vercel bot deployed to Preview March 13, 2023 17:33 View deployment

srindom reviewed Mar 13, 2023

View reviewed changes

Merge branch 'develop' into fix/remove-orphaned-location-levels-and-r…

e1785cd

…eservations

vercel bot deployed to Preview March 13, 2023 22:03 View deployment

pKorsholm added 2 commits March 14, 2023 09:43

add test to validate that orphaned levels and reservations are removed

9b84688

add additional description to delete dialog

e90d0a3

pKorsholm requested a review from srindom March 14, 2023 08:47

vercel bot deployed to Preview March 14, 2023 08:52 View deployment

srindom approved these changes Mar 14, 2023

View reviewed changes

srindom added the automerge label Mar 14, 2023

Merge branch 'develop' into fix/remove-orphaned-location-levels-and-r…

66adc26

…eservations

vercel bot deployed to Preview March 14, 2023 12:17 View deployment

Merge branch 'develop' into fix/remove-orphaned-location-levels-and-r…

187b211

…eservations

vercel bot deployed to Preview March 14, 2023 20:56 View deployment

fix integration test

a839372

vercel bot deployed to Preview March 15, 2023 08:57 View deployment

kodiakhq bot merged commit 10bf05c into develop Mar 15, 2023

kodiakhq bot deleted the fix/remove-orphaned-location-levels-and-reservations branch March 15, 2023 09:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix(inventory, stock-location): Remove orphaned location levels and reservations #3460

Fix(inventory, stock-location): Remove orphaned location levels and reservations #3460

pKorsholm commented Mar 13, 2023 •

edited

Loading

changeset-bot bot commented Mar 13, 2023 •

edited

Loading

vercel bot commented Mar 13, 2023 •

edited

Loading

srindom Mar 13, 2023

adrien2p Mar 13, 2023

srindom Mar 13, 2023

adrien2p Mar 13, 2023 •

edited

Loading

srindom Mar 13, 2023

adrien2p Mar 13, 2023

adrien2p Mar 13, 2023

pKorsholm Mar 13, 2023

srindom Mar 13, 2023

shahednasser Mar 14, 2023 •

edited

Loading

srindom left a comment •

edited

Loading

srindom left a comment

srindom Mar 14, 2023

srindom commented Mar 14, 2023

srindom commented Mar 14, 2023

adrien2p commented Mar 15, 2023 •

edited

Loading

pKorsholm commented Mar 15, 2023

Fix(inventory, stock-location): Remove orphaned location levels and reservations #3460

Fix(inventory, stock-location): Remove orphaned location levels and reservations #3460

Conversation

pKorsholm commented Mar 13, 2023 • edited Loading

changeset-bot bot commented Mar 13, 2023 • edited Loading

🦋 Changeset detected

vercel bot commented Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrien2p Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahednasser Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

srindom left a comment • edited Loading

Choose a reason for hiding this comment

srindom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srindom commented Mar 14, 2023

srindom commented Mar 14, 2023

adrien2p commented Mar 15, 2023 • edited Loading

pKorsholm commented Mar 15, 2023

pKorsholm commented Mar 13, 2023 •

edited

Loading

changeset-bot bot commented Mar 13, 2023 •

edited

Loading

vercel bot commented Mar 13, 2023 •

edited

Loading

adrien2p Mar 13, 2023 •

edited

Loading

shahednasser Mar 14, 2023 •

edited

Loading

srindom left a comment •

edited

Loading

adrien2p commented Mar 15, 2023 •

edited

Loading