Skip to content

[upstream #4918] fix(dashboard): Fix stock location deletion and add stock transfer on delete - #26

Open
ayim wants to merge 5 commits into
masterfrom
mirror/upstream-4918
Open

[upstream #4918] fix(dashboard): Fix stock location deletion and add stock transfer on delete#26
ayim wants to merge 5 commits into
masterfrom
mirror/upstream-4918

Conversation

@ayim

@ayim ayim commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Mirrored from vendurehq#4918 for the Overwatch review demo.

Original author: @biggamesmallworld


Summary

Fixes the dashboard bug where deleting a Stock Location always fails with a "Failed to delete N stock locations" toast (reported in vendurehq#4641 / OSS-493), and — since core already supports it — exposes transferring the location's stock to another location on delete.

Root cause

The generic DeleteBulkAction sends its mutation variables as { ids: [...] }, matching the deleteX(ids: [ID!]!) convention used by every other entity's bulk delete. But deleteStockLocations is the one exception — it takes input: [DeleteStockLocationInput!]! (so it can carry a per-location transferToLocationId):

deleteStockLocations(input: [DeleteStockLocationInput!]!): [DeletionResponse!]!

So the required $input variable was never provided and the request failed GraphQL validation:

Variable "$input" of required type "[DeleteStockLocationInput!]!" was not provided.

That error hits the DeleteBulkAction onError branch, producing the hard-coded Failed to delete N stock locations toast (no trailing message) — which matches the reporter's screenshot exactly. It fails for every stock location, with or without stock, so the "zero stock" detail in the report was a red herring.

Note: this is not a missing FK-cascade bug in core (as investigated on the issue) — a well-formed deleteStockLocation(s) request via the Admin API already works. The failure is purely the dashboard sending the wrong variable shape.

Reproduction

After reading the comment from @grolmus on the Linear task, I saw that he couldn't reproduce the error via the API. When I went to reproduce it via the UI I was able to reproduce it, with the API response in the network tab returning this:

{
    "errors": [
        {
            "message": "Variable \"$input\" of required type \"[DeleteStockLocationInput!]!\" was not provided.",
            "locations": [
                {
                    "line": 1,
                    "column": 31
                }
            ],
            "extensions": {
                "code": "BAD_USER_INPUT"
            }
        }
    ]
}

The true issue at hand, not the FK issue originally thought.

Fix + feature

DeleteStockLocationInput.transferToLocationId already lets core move a location's stock (levels + allocations) to another location on delete — but no admin UI ever surfaced it (neither the dashboard nor the legacy Angular app). So instead of only correcting the variable shape:

  • Replace the plain delete confirmation for stock locations with a dedicated delete dialog that asks what to do with remaining stock: transfer to another location or discard — then sends the correct input array with transferToLocationId per location.
  • The shared DeleteBulkAction is left untouched (still { ids } for all other entities).

Steps to verify

  1. Admin Dashboard → Settings → Stock Locations
  2. Select a location via its checkbox → Delete
  3. Before: toast "Failed to delete 1 stock locations"; $input ... was not provided in the network tab
  4. After: a dialog lets you transfer remaining stock to another location (or discard), then deletes successfully

Screen recording

Screen.Recording.2026-07-03.at.14.51.20.mov

Testing

Added a dashboard e2e test (packages/dashboard/e2e/tests/settings/stock-locations.spec.ts) that drives the real delete dialog end-to-end (select → Delete → choose "Discard remaining stock" → confirm) and asserts the location is removed — guarding against any regression back to the wrong mutation variable shape. The stock-location CRUD suite's generic bulk-delete (which drove the old confirm dialog) was replaced by this dialog-aware test.

Notes

  • Only delete path in the dashboard is the bulk action; the single-delete document was unused dead code.
  • useDetailPage/list pages unchanged; no core changes.
  • The transfer-target picker caps at 100 locations and invalidates its cached list after a delete.

Fixes vendurehq#4641

biggamesmallworld and others added 5 commits July 3, 2026 14:48
… delete

Deleting a stock location from the dashboard always failed with a "Failed to
delete N stock locations" toast. The shared DeleteBulkAction sends its
variables as `{ ids: [...] }` (the `deleteX(ids: [ID!]!)` convention used by
every other entity), but `deleteStockLocations` uniquely takes
`input: [DeleteStockLocationInput!]!`, so the required `$input` variable was
never provided and the request failed GraphQL validation — surfacing via the
onError branch as the toast, regardless of whether the location held stock.

Rather than only fix the variable shape, expose the transfer capability that
core already supports: `DeleteStockLocationInput.transferToLocationId` moves a
location's stock to another location on delete, but no admin UI ever surfaced
it. Replace the plain confirm with a dedicated delete dialog that lets the
admin choose, per delete, to transfer remaining stock to another location or
discard it, then sends the correct `input` array.

Fixes vendurehq#4641

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Invalidate the transfer-target list cache on delete so a just-deleted
  location can't be offered as a transfer target on the next open.
- Disable the target picker while its query is loading, so "Discard" cannot
  masquerade as "no transfer options exist".
- Type the mutation result via ResultOf instead of `any` + hand-rolled cast.
- Clarify dialog copy that all selected locations transfer into the single
  chosen target, and note the 100-location picker cap.
- Replace the generic CRUD bulk-delete coverage (which drove the old confirm
  dialog) with an e2e test that drives the new transfer/discard dialog and
  guards against regressing the mutation variable shape.

Relates to vendurehq#4641

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Reset the transfer target when the dialog opens, so a choice from a previous
  delete can't be sent as a to-be-deleted location's transferToLocationId.
- Only run success cleanup (refetch/reset/close) when at least one location was
  actually deleted; keep the dialog open if every item failed.
- Adapt to the v3.7 @vendure-io/ui Select: pass the required `items` map and
  handle the nullable onValueChange value.
- Keep the bulk-action dropdown open (closeOnClick={false}) so opening the
  dialog in the same tick doesn't race with the menu unmounting.
- Extract the new user-facing strings into the Lingui catalogs (they otherwise
  render as raw message IDs in the built app).
- Make the e2e test self-contained (seeds its own location via the API) and
  update it for the v3.7 bulk-action UI ("Actions" menu + custom dialog).

Skipped CodeRabbit's suggestions to import UI primitives from @vendure-io/ui
and api from @/graphql/api.js: every sibling route component uses
@/vdb/components/ui/* and @/vdb/graphql/api.js, and @/graphql/* isn't a tsconfig
path — following them would break the build and diverge from convention.

Relates to vendurehq#4641

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cap)

The delete dialog's transfer-target picker capped the query at 100 locations,
silently hiding valid destinations on stores with more. Paginate through all
pages instead so every location is offerable.

Relates to vendurehq#4641

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.

Not possible to delete a Stock Location

2 participants