-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Fix(inventory, stock-location): Remove orphaned location levels and reservations #3460
Fix(inventory, stock-location): Remove orphaned location levels and reservations #3460
Conversation
🦋 Changeset detectedLatest commit: a839372 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you give an example maybe? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be fine with that too -- would just have to be documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the later, it require to merge master in develop :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going for the pure deleted event, where do you want it documented @srindom?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc: @shahednasser, can we document this behavior in the events reference :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@srindom sure, once it's published I'll include this behavior in the events reference (creating a linear issue for it) 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test to ensure that this logic works correctly :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -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.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: great UX addition!
@pKorsholm - can you check up on the failing test :) |
The integration problem feels like a missing await - can't find any though; could it be because we don't do |
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 |
Clutch @adrien2p! Thanks, that was the issue 😅 |
What
How
Fixes CORE-1232