From 3fd61d93bb9930b761313afff8906e331bb559f7 Mon Sep 17 00:00:00 2001 From: Carlos Virreira Date: Fri, 31 Jul 2026 14:31:40 +0200 Subject: [PATCH 1/4] feat(assets): bulk assign and remove asset model from the asset index Adds "Update asset model" and "Remove from asset model" to the asset index Actions menu, so an existing library can be grouped into asset models without editing assets one at a time or re-importing a CSV. Requested via in-app feedback. - New `bulkUpdateAssetModel` links or unlinks the resolved selection. Quantity-tracked assets are skipped rather than failing the batch, because asset models are individually-tracked only. Only an all-quantity-tracked link errors; the equivalent unlink is a legitimate no-op. - Toasts report real counts: grouped, moved off another model, skipped, plus a distinct grey "No assets updated" with a different sentence for each zero-row cause. - Dialog titles use the loader total, so a cross-page "select all" states the true batch size instead of the page size. - Assign and remove are separate menu items and dialogs, matching the Assign/Remove tags and Add/Remove from kit pairs, so the model picker lists only real asset models. - Forwards `assetModels`/`totalAssetModels` from the simple mode loader. These were already queried by `getEntitiesWithSelectedValues` and discarded, so the picker no longer opens blank in simple mode at no extra query cost. - No activity events or notes: the singular `updateAsset` path emits neither and `ActivityAction` has no ASSET_MODEL action, per .claude/rules/bulk-event-parity.md. --- apps/webapp/app/atoms/bulk-update-dialog.ts | 2 + .../assets/bulk-actions-dropdown.tsx | 20 ++ .../assets/bulk-asset-model-remove-dialog.tsx | 87 +++++++ .../bulk-asset-model-update-dialog.test.ts | 54 +++++ .../assets/bulk-asset-model-update-dialog.tsx | 181 ++++++++++++++ .../bulk-update-dialog/bulk-update-dialog.tsx | 2 + .../app/components/shared/icons-map.tsx | 6 + apps/webapp/app/modules/asset/data.server.ts | 9 + .../app/modules/asset/service.server.test.ts | 226 ++++++++++++++++++ .../app/modules/asset/service.server.ts | 215 +++++++++++++++++ .../api+/assets.bulk-update-asset-model.ts | 144 +++++++++++ 11 files changed, 946 insertions(+) create mode 100644 apps/webapp/app/components/assets/bulk-asset-model-remove-dialog.tsx create mode 100644 apps/webapp/app/components/assets/bulk-asset-model-update-dialog.test.ts create mode 100644 apps/webapp/app/components/assets/bulk-asset-model-update-dialog.tsx create mode 100644 apps/webapp/app/routes/api+/assets.bulk-update-asset-model.ts diff --git a/apps/webapp/app/atoms/bulk-update-dialog.ts b/apps/webapp/app/atoms/bulk-update-dialog.ts index 5ac3761b5a..1e33f0dc0b 100644 --- a/apps/webapp/app/atoms/bulk-update-dialog.ts +++ b/apps/webapp/app/atoms/bulk-update-dialog.ts @@ -8,6 +8,8 @@ import type { BulkDialogType } from "~/components/bulk-update-dialog/bulk-update const DEFAULT_STATE: Record = { location: false, category: false, + "asset-model": false, + "asset-model-remove": false, "assign-custody": false, "release-custody": false, "tag-add": false, diff --git a/apps/webapp/app/components/assets/bulk-actions-dropdown.tsx b/apps/webapp/app/components/assets/bulk-actions-dropdown.tsx index f4aaa41f1a..f1942a97ff 100644 --- a/apps/webapp/app/components/assets/bulk-actions-dropdown.tsx +++ b/apps/webapp/app/components/assets/bulk-actions-dropdown.tsx @@ -17,6 +17,8 @@ import { userHasPermission } from "~/utils/permissions/permission.validator.clie import { tw } from "~/utils/tw"; import BulkAddToAuditDialog from "./bulk-add-to-audit-dialog"; import BulkAddToKitDialog from "./bulk-add-to-kit-dialog"; +import BulkAssetModelRemoveDialog from "./bulk-asset-model-remove-dialog"; +import BulkAssetModelUpdateDialog from "./bulk-asset-model-update-dialog"; import BulkAssignCustodyDialog from "./bulk-assign-custody-dialog"; import BulkAssignTagsDialog from "./bulk-assign-tags-dialog"; import BulkCategoryUpdateDialog from "./bulk-category-update-dialog"; @@ -156,6 +158,8 @@ function ConditionalDropdown() { + + @@ -365,6 +369,22 @@ function ConditionalDropdown() { disabled={isLoading} /> + + + + + + (); + const zo = useZorm("BulkAssetModelRemove", BulkAssetModelRemoveSchema); + + const selectedItems = useAtomValue(selectedBulkItemsAtom); + + /** Same reason as the assign dialog: the shared count atom would report the page size. */ + const totalSelected = isSelectingAllItems(selectedItems) + ? totalItems + : selectedItems.length; + + return ( + + {({ disabled, handleCloseDialog, fetcherError }) => ( +
+ {/* Empty is what the service reads as "unlink". */} + + + {fetcherError ? ( +

{fetcherError}

+ ) : null} + +
+ + +
+
+ )} + + ); +} diff --git a/apps/webapp/app/components/assets/bulk-asset-model-update-dialog.test.ts b/apps/webapp/app/components/assets/bulk-asset-model-update-dialog.test.ts new file mode 100644 index 0000000000..8c38b13e04 --- /dev/null +++ b/apps/webapp/app/components/assets/bulk-asset-model-update-dialog.test.ts @@ -0,0 +1,54 @@ +/** + * Schema contract for the bulk asset model dialogs. + * + * The assign and remove dialogs post to the SAME endpoint and are told apart + * only by whether `assetModelId` is empty. A regression during development made + * the endpoint reject the empty value, which silently broke removal while every + * other gate stayed green, so the split is pinned here. + * + * @see {@link file://./bulk-asset-model-update-dialog.tsx} + * @see {@link file://./bulk-asset-model-remove-dialog.tsx} + * @see {@link file://./../../routes/api+/assets.bulk-update-asset-model.ts} + */ +import { describe, expect, it } from "vitest"; +import { + BulkAssetModelActionSchema, + BulkAssetModelUpdateSchema, +} from "./bulk-asset-model-update-dialog"; + +describe("bulk asset model schemas", () => { + it("accepts an empty assetModelId on the wire, because that is how removal is requested", () => { + const result = BulkAssetModelActionSchema.safeParse({ + assetIds: ["asset-1"], + assetModelId: "", + }); + + expect(result.success).toBe(true); + }); + + it("rejects an empty assetModelId in the assign form, so it cannot silently ungroup", () => { + const result = BulkAssetModelUpdateSchema.safeParse({ + assetIds: ["asset-1"], + assetModelId: "", + }); + + expect(result.success).toBe(false); + expect(result.success ? null : result.error.issues[0]?.message).toBe( + "Please select an asset model" + ); + }); + + it("accepts a model id in both", () => { + const payload = { assetIds: ["asset-1"], assetModelId: "model-1" }; + + expect(BulkAssetModelActionSchema.safeParse(payload).success).toBe(true); + expect(BulkAssetModelUpdateSchema.safeParse(payload).success).toBe(true); + }); + + it("requires at least one asset in both, so a stray POST cannot target everything", () => { + const payload = { assetIds: [], assetModelId: "model-1" }; + + expect(BulkAssetModelActionSchema.safeParse(payload).success).toBe(false); + expect(BulkAssetModelUpdateSchema.safeParse(payload).success).toBe(false); + }); +}); diff --git a/apps/webapp/app/components/assets/bulk-asset-model-update-dialog.tsx b/apps/webapp/app/components/assets/bulk-asset-model-update-dialog.tsx new file mode 100644 index 0000000000..1db43c3081 --- /dev/null +++ b/apps/webapp/app/components/assets/bulk-asset-model-update-dialog.tsx @@ -0,0 +1,181 @@ +/** + * Bulk Asset Model Update Dialog + * + * Lets a user group many existing assets into one AssetModel from the asset + * index "Actions" menu, instead of opening each asset's edit form or + * re-importing a CSV. + * + * Assign only. Ungrouping is its own menu item and dialog + * ({@link file://./bulk-asset-model-remove-dialog.tsx}) so that this picker + * contains nothing but real asset models: an in-list "remove" row reads as a + * model called "Remove from asset model" to anyone who has not made one yet. + * That also matches how the menu already treats tags and kits, which each have + * a separate Remove item. + * + * Reads the selection from `selectedBulkItemsAtom` and posts to + * `/api/assets/bulk-update-asset-model` via {@link BulkUpdateDialogContent}, + * which also forwards the active index filters so a cross-page "select all" + * resolves server-side. + * + * @see {@link file://./../../routes/api+/assets.bulk-update-asset-model.ts} action + * @see {@link file://./../../modules/asset/service.server.ts} `bulkUpdateAssetModel` + * @see {@link file://./asset-model-form-row.tsx} the single-asset equivalent + */ +import { useAtomValue } from "jotai"; +import { useLoaderData } from "react-router"; +import { useZorm } from "react-zorm"; +import { z } from "zod"; +import { selectedBulkItemsAtom } from "~/atoms/list"; +import { isQuantityTracked } from "~/modules/asset/utils"; +import type { AssetIndexLoaderData } from "~/routes/_layout+/assets._index"; +import { isSelectingAllItems } from "~/utils/list"; +import { BulkUpdateDialogContent } from "../bulk-update-dialog/bulk-update-dialog"; +import DynamicSelect from "../dynamic-select/dynamic-select"; +import InlineEntityCreationDialog from "../inline-entity-creation-dialog/inline-entity-creation-dialog"; +import { Button } from "../shared/button"; +import { WarningBox } from "../shared/warning-box"; + +/** + * Wire format for `/api/assets/bulk-update-asset-model`, which serves BOTH the + * assign dialog and {@link file://./bulk-asset-model-remove-dialog.tsx}. + * + * `assetModelId` is deliberately not `.min(1)`: an empty value is how the + * remove dialog asks for an unlink. Keep it that way, or removal breaks. + * + * It lives here rather than in the route because a route file may only export + * `loader`/`action` (see .claude/rules/no-server-module-in-route-client-exports), + * and this is the same place the sibling bulk dialogs keep their schemas. + */ +export const BulkAssetModelActionSchema = z.object({ + assetIds: z.array(z.string()).min(1), + assetModelId: z.string(), +}); + +/** + * What the assign form validates client-side. A model is required here: an + * empty submit from THIS dialog would silently ungroup, which is the other + * dialog's job, so it has to fail with a message instead. + */ +export const BulkAssetModelUpdateSchema = BulkAssetModelActionSchema.extend({ + assetModelId: z.string().min(1, "Please select an asset model"), +}); + +export default function BulkAssetModelUpdateDialog() { + const { totalItems } = useLoaderData(); + const zo = useZorm("BulkAssetModelUpdate", BulkAssetModelUpdateSchema); + + const selectedItems = useAtomValue(selectedBulkItemsAtom); + const isSelectingAll = isSelectingAllItems(selectedItems); + + /** + * The shared count atom counts array entries, and a cross-page "select all" + * is stored as a single sentinel entry — so it would report the page size + * while the action changed every filtered asset. Use the loader's total in + * that case, the same way the bulk delete dialog does. + */ + const totalSelected = isSelectingAll ? totalItems : selectedItems.length; + + /** + * Asset models describe N distinguishable units of one template, so they + * only apply to individually tracked assets, and `bulkUpdateAssetModel` + * skips quantity-tracked ones. Warn before the user commits rather than + * after. + * + * The gate is the current page's contents in both modes. Under select-all + * the client holds only the current page plus a sentinel, so the copy drops + * the number rather than stating one that understates the batch. Gating on + * the page (instead of "always warn when selecting all") keeps the warning + * out of workspaces that track nothing by quantity, where it would be noise + * on every sweep. If a later page turns out to hold quantity-tracked assets, + * the result toast still reports exactly how many were skipped. + */ + const quantityTrackedCount = selectedItems.filter((item) => + isQuantityTracked(item) + ).length; + + return ( + + {({ disabled, handleCloseDialog, fetcherError }) => ( +
+ {quantityTrackedCount > 0 ? ( +
+ + + {isSelectingAll + ? "Any quantity-tracked assets in your selection will be skipped." + : `${quantityTrackedCount} quantity-tracked asset(s) in your selection will be skipped.`}{" "} + Asset models can only be linked to individually tracked + assets. + + +
+ ) : null} + +
+ ( + { + if (created?.type !== "assetModel") return; + const assetModel = created.entity; + onItemCreated({ + id: assetModel.id, + name: assetModel.name, + metadata: { ...assetModel }, + }); + closePopover(); + }} + /> + )} + /> + {zo.errors.assetModelId()?.message ? ( +

+ {zo.errors.assetModelId()?.message} +

+ ) : null} + {fetcherError ? ( +

{fetcherError}

+ ) : null} +
+ +
+ + +
+
+ )} + + ); +} diff --git a/apps/webapp/app/components/bulk-update-dialog/bulk-update-dialog.tsx b/apps/webapp/app/components/bulk-update-dialog/bulk-update-dialog.tsx index a1d18802af..a47099d5ab 100644 --- a/apps/webapp/app/components/bulk-update-dialog/bulk-update-dialog.tsx +++ b/apps/webapp/app/components/bulk-update-dialog/bulk-update-dialog.tsx @@ -31,6 +31,8 @@ import { type BulkDialogType = | "location" | "category" + | "asset-model" + | "asset-model-remove" | "assign-custody" | "release-custody" | "trash" diff --git a/apps/webapp/app/components/shared/icons-map.tsx b/apps/webapp/app/components/shared/icons-map.tsx index f343f60cae..5b2e9471c4 100644 --- a/apps/webapp/app/components/shared/icons-map.tsx +++ b/apps/webapp/app/components/shared/icons-map.tsx @@ -1,6 +1,7 @@ import type { JSX } from "react"; import { CalendarIcon, RowsIcon } from "@radix-ui/react-icons"; import { + Boxes, CalendarCheck, ClipboardList, MapPinIcon, @@ -11,6 +12,7 @@ import { PackagePlus, QrCode, SlidersHorizontal, + Ungroup, } from "lucide-react"; import { Spinner } from "./spinner"; @@ -93,6 +95,8 @@ export type IconType = | "tag-add" | "category" | "location" + | "asset-model" + | "asset-model-remove" | "gps" | "duplicate" | "asset" @@ -168,6 +172,8 @@ export const iconsMap: IconsMap = { "tag-remove": , category: , location: , + "asset-model": , + "asset-model-remove": , gps: , duplicate: , asset: , diff --git a/apps/webapp/app/modules/asset/data.server.ts b/apps/webapp/app/modules/asset/data.server.ts index a720574bca..9cfd8b9895 100644 --- a/apps/webapp/app/modules/asset/data.server.ts +++ b/apps/webapp/app/modules/asset/data.server.ts @@ -205,6 +205,8 @@ export async function simpleModeLoader({ totalTags, locations, totalLocations, + assetModels, + totalAssetModels, teamMembers, totalTeamMembers, }, @@ -392,6 +394,13 @@ export async function simpleModeLoader({ totalTags, locations, totalLocations, + /** + * Seeds the asset model picker in the bulk "Update asset model" dialog. + * Advanced mode already returned these; simple mode was querying them and + * throwing them away, so this adds no database work. + */ + assetModels, + totalAssetModels, teamMembers, totalTeamMembers, currentUserTeamMember, diff --git a/apps/webapp/app/modules/asset/service.server.test.ts b/apps/webapp/app/modules/asset/service.server.test.ts index db2098880f..267186af9a 100644 --- a/apps/webapp/app/modules/asset/service.server.test.ts +++ b/apps/webapp/app/modules/asset/service.server.test.ts @@ -16,6 +16,7 @@ import { bulkAssignKitCustody } from "~/modules/kit/service.server"; import { getQr } from "~/modules/qr/service.server"; import { ShelfError } from "~/utils/error"; import { createSignedUrl } from "~/utils/storage.server"; +import { resolveAssetIdsForBulkOperation } from "./bulk-operations-helper.server"; import { BULK_CREATE_MAX, bulkAssignAssetTags, @@ -23,6 +24,7 @@ import { bulkCreateAssetsFromModel, bulkDeleteAssets, bulkUpdateAssetCategory, + bulkUpdateAssetModel, buildAssetKitCreateData, checkOutQuantity, createAsset, @@ -69,6 +71,13 @@ vitest.mock("~/database/db.server", () => ({ category: { findFirst: vitest.fn().mockResolvedValue(null), }, + // why: `~/utils/org-validation.server` is NOT mocked in this file, so + // `assertAssetModelBelongsToOrg` runs for real inside + // `bulkUpdateAssetModel` and hits this stub. Without the key the guard + // throws a TypeError instead of exercising the org check. + assetModel: { + findFirst: vitest.fn().mockResolvedValue(null), + }, location: { findFirst: vitest.fn().mockResolvedValue(null), }, @@ -2035,6 +2044,223 @@ describe("bulkUpdateAssetCategory", () => { }); }); +describe("bulkUpdateAssetModel", () => { + beforeEach(() => { + vitest.clearAllMocks(); + // why: this file pins shared db mocks with sticky `mockReturnValue` in + // other suites and `clearAllMocks` does not undo those. Re-arm the two + // stubs this suite drives so it never reads a leaked value. + //@ts-expect-error mock setup + db.asset.findMany.mockResolvedValue([]); + //@ts-expect-error mock setup + db.assetModel.findFirst.mockResolvedValue({ + id: "model-1", + name: "Panasonic PT-VZ580", + }); + }); + + it("links the individually tracked assets and skips quantity-tracked ones", async () => { + expect.assertions(4); + //@ts-expect-error mock setup + db.asset.findMany.mockResolvedValue([ + { id: "asset-1", type: "INDIVIDUAL", assetModelId: null }, + { id: "asset-2", type: "QUANTITY_TRACKED", assetModelId: null }, + { id: "asset-3", type: "INDIVIDUAL", assetModelId: null }, + ]); + + const result = await bulkUpdateAssetModel({ + userId: "user-1", + assetIds: ["asset-1", "asset-2", "asset-3"], + organizationId: "org-1", + assetModelId: "model-1", + currentSearchParams: "assetModel=is:without-model", + // @ts-expect-error settings shape not relevant, only pass-through is + settings: { mode: "ADVANCED" }, + }); + + expect(result).toEqual({ + linked: true, + resolved: 3, + updated: 2, + moved: 0, + skippedQuantityTracked: 1, + modelName: "Panasonic PT-VZ580", + }); + // The active filters and index mode must reach the resolver, or a + // cross-page "select all" silently operates on the wrong set. + expect(resolveAssetIdsForBulkOperation).toHaveBeenCalledWith({ + assetIds: ["asset-1", "asset-2", "asset-3"], + organizationId: "org-1", + currentSearchParams: "assetModel=is:without-model", + settings: { mode: "ADVANCED" }, + }); + expect(db.asset.updateMany).toHaveBeenCalledWith({ + where: { id: { in: ["asset-1", "asset-3"] }, organizationId: "org-1" }, + data: { assetModelId: "model-1" }, + }); + // The qty-tracked asset must never reach the write. + expect( + (db.asset.updateMany as ReturnType).mock.calls[0][0] + .where.id.in + ).not.toContain("asset-2"); + }); + + it("counts assets moved off another model separately from first-time grouping", async () => { + expect.assertions(1); + //@ts-expect-error mock setup + db.asset.findMany.mockResolvedValue([ + { id: "asset-1", type: "INDIVIDUAL", assetModelId: null }, + { id: "asset-2", type: "INDIVIDUAL", assetModelId: "model-other" }, + // already on the target model → not a change at all + { id: "asset-3", type: "INDIVIDUAL", assetModelId: "model-1" }, + ]); + + const result = await bulkUpdateAssetModel({ + userId: "user-1", + assetIds: ["asset-1", "asset-2", "asset-3"], + organizationId: "org-1", + assetModelId: "model-1", + // @ts-expect-error settings not relevant for this test + settings: {}, + }); + + expect(result).toMatchObject({ updated: 2, moved: 1 }); + }); + + it("removes the link when no model is given, without touching the model table", async () => { + expect.assertions(3); + //@ts-expect-error mock setup + db.asset.findMany.mockResolvedValue([ + { id: "asset-1", type: "INDIVIDUAL", assetModelId: "model-1" }, + // already unlinked → no write + { id: "asset-2", type: "INDIVIDUAL", assetModelId: null }, + ]); + + const result = await bulkUpdateAssetModel({ + userId: "user-1", + assetIds: ["asset-1", "asset-2"], + organizationId: "org-1", + // why: the dialog posts an EMPTY STRING for "remove from asset model", + // never null — this is the shape the route actually parses. + assetModelId: "", + // @ts-expect-error settings not relevant for this test + settings: {}, + }); + + expect(result).toMatchObject({ + linked: false, + updated: 1, + moved: 0, + modelName: null, + }); + expect(db.assetModel.findFirst).not.toHaveBeenCalled(); + expect(db.asset.updateMany).toHaveBeenCalledWith({ + where: { id: { in: ["asset-1"] }, organizationId: "org-1" }, + data: { assetModelId: null }, + }); + }); + + it("does not error when unlinking a selection that is entirely quantity-tracked", async () => { + expect.assertions(2); + //@ts-expect-error mock setup + db.asset.findMany.mockResolvedValue([ + { id: "asset-1", type: "QUANTITY_TRACKED", assetModelId: null }, + ]); + + // Removing a model from assets that can never have had one is a no-op, + // not a rule violation. Only the LINK direction rejects. + const result = await bulkUpdateAssetModel({ + userId: "user-1", + assetIds: ["asset-1"], + organizationId: "org-1", + assetModelId: "", + // @ts-expect-error settings not relevant for this test + settings: {}, + }); + + expect(result).toMatchObject({ + linked: false, + updated: 0, + skippedQuantityTracked: 0, + }); + expect(db.asset.updateMany).not.toHaveBeenCalled(); + }); + + it("throws a 400 when every selected asset is quantity-tracked", async () => { + expect.assertions(2); + //@ts-expect-error mock setup + db.asset.findMany.mockResolvedValue([ + { id: "asset-1", type: "QUANTITY_TRACKED", assetModelId: null }, + ]); + + await expect( + bulkUpdateAssetModel({ + userId: "user-1", + assetIds: ["asset-1"], + organizationId: "org-1", + assetModelId: "model-1", + // @ts-expect-error settings not relevant for this test + settings: {}, + }) + // The status AND the message must survive the catch-all wrapper, or the + // dialog shows "Something went wrong" instead of the eligibility rule. + ).rejects.toMatchObject({ + status: 400, + message: expect.stringContaining("quantity-tracked"), + }); + + expect(db.asset.updateMany).not.toHaveBeenCalled(); + }); + + it("throws when the asset model belongs to a different organization", async () => { + expect.assertions(2); + // why: emulate a foreign-org model — the org-scoped guard finds nothing + //@ts-expect-error mock setup + db.assetModel.findFirst.mockResolvedValue(null); + + await expect( + bulkUpdateAssetModel({ + userId: "user-1", + assetIds: ["asset-1"], + organizationId: "org-1", + assetModelId: "foreign-model", + // @ts-expect-error settings not relevant for this test + settings: {}, + }) + // Same reason as above: the guard's own 404 message has to reach the UI. + ).rejects.toMatchObject({ + status: 404, + message: expect.stringContaining("workspace"), + }); + + // The guard must run before the assets are read or written. + expect(db.asset.updateMany).not.toHaveBeenCalled(); + }); + + it("writes nothing when the selection resolves to no assets", async () => { + expect.assertions(2); + + const result = await bulkUpdateAssetModel({ + userId: "user-1", + assetIds: [], + organizationId: "org-1", + assetModelId: "model-1", + // @ts-expect-error settings not relevant for this test + settings: {}, + }); + + expect(result).toEqual({ + linked: true, + resolved: 0, + updated: 0, + moved: 0, + skippedQuantityTracked: 0, + modelName: null, + }); + expect(db.asset.updateMany).not.toHaveBeenCalled(); + }); +}); + describe("bulkAssignAssetTags", () => { beforeEach(() => { vitest.clearAllMocks(); diff --git a/apps/webapp/app/modules/asset/service.server.ts b/apps/webapp/app/modules/asset/service.server.ts index 68da4c76a6..687f8d22a0 100644 --- a/apps/webapp/app/modules/asset/service.server.ts +++ b/apps/webapp/app/modules/asset/service.server.ts @@ -4123,6 +4123,8 @@ export async function getPaginatedAndFilterableAssets({ totalCategories, locations, totalLocations, + assetModels, + totalAssetModels, }, teamMembersData, { assets, totalAssets }, @@ -4179,6 +4181,15 @@ export async function getPaginatedAndFilterableAssets({ cookie, locations: excludeLocationQuery ? [] : locations, totalLocations, + /** + * `getEntitiesWithSelectedValues` already queries these on every simple + * mode load and the results were being dropped at the destructure above. + * Forwarding them costs no extra query and is what seeds the asset model + * picker in the bulk "Update asset model" dialog — without it the picker + * opens blank for every simple mode workspace. + */ + assetModels, + totalAssetModels, ...teamMembersData, }; } catch (cause) { @@ -6612,6 +6623,210 @@ export async function bulkUpdateAssetCategory({ } } +/** Outcome of a bulk asset model update, used to build an honest toast. */ +export type BulkUpdateAssetModelResult = { + /** + * Whether the action linked assets to a model (`true`) or removed the link + * (`false`). The caller must branch on this rather than on `modelName`, + * which is only a label and can be blank. + */ + linked: boolean; + /** + * How many assets the selection resolved to. Distinguishes "your filters + * matched nothing" from "nothing needed changing", which read identically + * from `updated: 0`. + */ + resolved: number; + /** Assets whose `assetModelId` actually changed. */ + updated: number; + /** Of `updated`, how many were moved off a different model rather than grouped for the first time. */ + moved: number; + /** Selected assets left untouched because models are individually-tracked only. */ + skippedQuantityTracked: number; + /** Name of the target model, or `null` when the action removed the link. */ + modelName: string | null; +}; + +/** + * Links (or unlinks) many assets to a single AssetModel in one action. + * + * This is the bulk counterpart of the Asset Model picker on the asset edit + * form. It exists so an existing library can be grouped into models without + * editing assets one at a time or round-tripping through a CSV re-import. + * + * Behaviour worth knowing before you change it: + * - QUANTITY_TRACKED assets are **skipped**, not rejected. An asset model + * describes N distinguishable units of one template, while a qty-tracked + * asset is a stock pool, so `createAsset`/`updateAsset` both refuse that + * link. A mixed selection is normal in a real library, so the batch applies + * to the individually-tracked assets and reports the skip. Only a selection + * with nothing eligible in it is an error. + * - The model's `defaultCategoryId` / `defaultValuation` are **not** applied. + * Those are create-time conveniences (see `bulkCreateAssetsFromModel`); + * retro-applying them here would silently overwrite curated data on assets + * that already exist. + * - No activity events and no notes are written. `ActivityAction` has no + * ASSET_MODEL action and the singular `updateAsset` path writes neither, so + * staying silent is what `.claude/rules/bulk-event-parity.md` requires: + * bulk must emit exactly what singular emits. Adding the event properly + * needs an additive enum migration plus the singular call site plus the + * model-delete SetNull cascade, which is its own change. + * + * @param params.assetIds - Selected asset ids, possibly `[ALL_SELECTED_KEY]` + * @param params.assetModelId - Target model, or `null`/`""` to remove the link + * @param params.currentSearchParams - Active index filters, used to resolve a + * cross-page "select all" into real ids + * @param params.settings - Asset index settings; decides simple vs advanced + * filter resolution for that select-all + * @returns Counts describing what actually happened, for the caller's toast + * @throws {ShelfError} 404 when the model is not in this organization, 400 + * when every selected asset is quantity-tracked + */ +export async function bulkUpdateAssetModel({ + userId, + assetIds, + organizationId, + assetModelId, + currentSearchParams, + settings, +}: { + userId: string; + assetIds: Asset["id"][]; + organizationId: Asset["organizationId"]; + assetModelId: Asset["assetModelId"]; + currentSearchParams?: string | null; + settings: AssetIndexSettings; +}): Promise { + try { + // Resolve IDs (works for both simple and advanced mode) + const resolvedIds = await resolveAssetIdsForBulkOperation({ + assetIds, + organizationId, + currentSearchParams, + settings, + }); + + /** An empty `assetModelId` is the "remove from asset model" request. */ + const newAssetModelId = assetModelId || null; + const linked = newAssetModelId !== null; + + if (resolvedIds.length === 0) { + return { + linked, + resolved: 0, + updated: 0, + moved: 0, + skippedQuantityTracked: 0, + modelName: null, + }; + } + + // why: `connect`-style writes are not org-scoped by Prisma, so a crafted + // foreign-org model id would otherwise be written verbatim onto this + // org's assets. Shared guard per .claude/rules/org-scope-user-supplied-ids. + let modelName: string | null = null; + if (newAssetModelId) { + await assertAssetModelBelongsToOrg({ + assetModelId: newAssetModelId, + organizationId, + }); + + const model = await db.assetModel.findFirst({ + where: { id: newAssetModelId, organizationId }, + select: { name: true }, + }); + modelName = model?.name ?? null; + } + + /** + * Before-state, org-scoped. This read is also the ownership proof for the + * asset ids: `resolveAssetIdsForBulkOperation` returns a caller-supplied + * list verbatim when it is not a select-all, so nothing upstream has + * checked them against this organization yet. + */ + const assetsBeforeUpdate = await db.asset.findMany({ + where: { id: { in: resolvedIds }, organizationId }, + select: { id: true, type: true, assetModelId: true }, + }); + + const individuals = assetsBeforeUpdate.filter( + (asset) => asset.type !== AssetType.QUANTITY_TRACKED + ); + const skippedQuantityTracked = + assetsBeforeUpdate.length - individuals.length; + + /** + * Only the LINK direction errors here. Removing a link from a set of + * quantity-tracked assets is a legitimate no-op (they can never have had a + * model), so failing it would teach a rule the user did not break. + */ + if (linked && individuals.length === 0 && skippedQuantityTracked > 0) { + throw new ShelfError({ + cause: null, + title: "Asset model not allowed", + message: + "All selected assets are quantity-tracked. Asset models can only be linked to individually tracked assets.", + additionalData: { organizationId, userId, assetModelId }, + label, + status: 400, + shouldBeCaptured: false, + }); + } + + /** Skip rows that already point at the target so the counts stay honest. */ + const assetsThatChange = individuals.filter( + (asset) => asset.assetModelId !== newAssetModelId + ); + + /** + * Assets taken off another model rather than grouped for the first time. + * Surfaced in the toast because it is the only signal that the previous + * model's book-by-model availability pool just shrank. + */ + const moved = newAssetModelId + ? assetsThatChange.filter((asset) => asset.assetModelId !== null).length + : 0; + + if (assetsThatChange.length > 0) { + await db.asset.updateMany({ + where: { + id: { in: assetsThatChange.map((asset) => asset.id) }, + organizationId, + }, + data: { assetModelId: newAssetModelId }, + }); + } + + return { + linked, + resolved: resolvedIds.length, + updated: assetsThatChange.length, + moved, + /** + * Only meaningful when linking. On the unlink path a quantity-tracked + * asset was never going to change, so reporting it as "skipped" would + * invent a failure. + */ + skippedQuantityTracked: linked ? skippedQuantityTracked : 0, + modelName, + }; + } catch (cause) { + // why: the eligibility 400 and the cross-org 404 are the two errors the + // user is meant to read. The generic wrapper keeps the status but replaces + // the message, so re-throw our own errors untouched. + if (isLikeShelfError(cause)) { + throw cause; + } + + throw new ShelfError({ + cause, + message: "Something went wrong while bulk updating the asset model.", + additionalData: { userId, assetIds, organizationId, assetModelId }, + label, + }); + } +} + export async function bulkAssignAssetTags({ userId, assetIds, diff --git a/apps/webapp/app/routes/api+/assets.bulk-update-asset-model.ts b/apps/webapp/app/routes/api+/assets.bulk-update-asset-model.ts new file mode 100644 index 0000000000..e62f2b13e1 --- /dev/null +++ b/apps/webapp/app/routes/api+/assets.bulk-update-asset-model.ts @@ -0,0 +1,144 @@ +/** + * Bulk Asset Model Update API + * + * Links (or unlinks) every selected asset to a single AssetModel. Backs the + * "Update asset model" item in the asset index Actions menu. + * + * The reported counts are deliberately specific: a select-all can resolve to + * a different set than the one on screen, and quantity-tracked assets are + * skipped, so a flat "Assets updated" toast would hide both. + * + * @see {@link file://./../../components/assets/bulk-asset-model-update-dialog.tsx} assign dialog + * @see {@link file://./../../components/assets/bulk-asset-model-remove-dialog.tsx} remove dialog + * @see {@link file://./../../modules/asset/service.server.ts} `bulkUpdateAssetModel` + */ +import { data, type ActionFunctionArgs } from "react-router"; +import { BulkAssetModelActionSchema } from "~/components/assets/bulk-asset-model-update-dialog"; +import { bulkUpdateAssetModel } from "~/modules/asset/service.server"; +import { CurrentSearchParamsSchema } from "~/modules/asset/utils.server"; +import { getAssetIndexSettings } from "~/modules/asset-index-settings/service.server"; +import { sendNotification } from "~/utils/emitter/send-notification.server"; +import { makeShelfError } from "~/utils/error"; +import { assertIsPost, payload, error, parseData } from "~/utils/http.server"; +import { + PermissionAction, + PermissionEntity, +} from "~/utils/permissions/permission.data"; +import { requirePermission } from "~/utils/roles.server"; + +export async function action({ context, request }: ActionFunctionArgs) { + const authSession = context.getSession(); + const userId = authSession.userId; + + try { + assertIsPost(request); + + const formData = await request.formData(); + + const { organizationId, canUseBarcodes, role } = await requirePermission({ + userId, + request, + entity: PermissionEntity.asset, + action: PermissionAction.update, + }); + + // Fetch asset index settings to determine mode + const settings = await getAssetIndexSettings({ + userId, + organizationId, + canUseBarcodes, + role, + }); + + const { assetIds, assetModelId, currentSearchParams } = parseData( + formData, + BulkAssetModelActionSchema.and(CurrentSearchParamsSchema) + ); + + const { + linked, + resolved, + updated, + moved, + skippedQuantityTracked, + modelName, + } = await bulkUpdateAssetModel({ + userId, + assetIds, + assetModelId, + organizationId, + currentSearchParams, + settings, + }); + + /** + * Appended when linking, never when unlinking: the service already zeroes + * the count on the unlink path, where a quantity-tracked asset was never + * going to change. + */ + const skippedSuffix = + skippedQuantityTracked > 0 + ? ` ${skippedQuantityTracked} quantity-tracked asset(s) were skipped.` + : ""; + + /** + * A zero-row result is a real outcome, not a success. Saying "updated" + * here would train users to trust a no-op, and a select-all can resolve + * to a narrower set than the screen shows. The three causes read very + * differently to the user, so they get their own sentences. + */ + if (updated === 0) { + const reason = + resolved === 0 + ? "Your filters no longer match any assets." + : linked + ? "The selected assets are already in this asset model." + : "None of the selected assets were in an asset model."; + + sendNotification({ + title: "No assets updated", + message: `${reason}${skippedSuffix}`, + icon: { name: "asset-model", variant: "gray" }, + senderId: userId, + }); + + return data(payload({ success: true })); + } + + if (linked) { + /** + * `moved` is the only warning that another model's book-by-model + * availability pool just shrank, so it is stated rather than folded + * into the total. + */ + const movedSuffix = + moved > 0 + ? ` ${moved} of them were moved from another asset model.` + : ""; + + // Branch on `linked`, never on `modelName`: the name is only a label. + sendNotification({ + title: "Assets grouped", + message: `${updated} asset(s) were grouped into ${ + modelName || "the selected asset model" + }.${movedSuffix}${skippedSuffix}`, + icon: { name: "success", variant: "success" }, + senderId: userId, + }); + + return data(payload({ success: true })); + } + + sendNotification({ + title: "Assets updated", + message: `${updated} asset(s) were removed from their asset model.`, + icon: { name: "success", variant: "success" }, + senderId: userId, + }); + + return data(payload({ success: true })); + } catch (cause) { + const reason = makeShelfError(cause, { userId }); + return data(error(reason), { status: reason.status }); + } +} From d9312657c4b317cbd92278d96c8e182a2bc9ef3d Mon Sep 17 00:00:00 2001 From: Carlos Virreira Date: Fri, 31 Jul 2026 14:43:04 +0200 Subject: [PATCH 2/4] fix(assets): report the org-verified count in bulkUpdateAssetModel `resolveAssetIdsForBulkOperation` returns a caller-supplied id list verbatim, so `resolvedIds.length` counted ids that may not belong to the organization. The zero-row toast then claimed "The selected assets are already in this asset model" when nothing had matched at all. Count the org-scoped read instead. --- .../app/modules/asset/service.server.test.ts | 22 +++++++++++++++++++ .../app/modules/asset/service.server.ts | 8 ++++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/apps/webapp/app/modules/asset/service.server.test.ts b/apps/webapp/app/modules/asset/service.server.test.ts index 267186af9a..2579f2a691 100644 --- a/apps/webapp/app/modules/asset/service.server.test.ts +++ b/apps/webapp/app/modules/asset/service.server.test.ts @@ -2237,6 +2237,28 @@ describe("bulkUpdateAssetModel", () => { expect(db.asset.updateMany).not.toHaveBeenCalled(); }); + it("reports resolved 0 when the ids are not in this organization", async () => { + expect.assertions(2); + // why: the resolver returns a caller-supplied id list verbatim, so the + // org check is the org-scoped read. Foreign ids resolve to a non-empty + // list but match no rows, and the caller must be able to tell that apart + // from "these are already on the model". + //@ts-expect-error mock setup + db.asset.findMany.mockResolvedValue([]); + + const result = await bulkUpdateAssetModel({ + userId: "user-1", + assetIds: ["foreign-asset-1", "foreign-asset-2"], + organizationId: "org-1", + assetModelId: "model-1", + // @ts-expect-error settings not relevant for this test + settings: {}, + }); + + expect(result).toMatchObject({ resolved: 0, updated: 0 }); + expect(db.asset.updateMany).not.toHaveBeenCalled(); + }); + it("writes nothing when the selection resolves to no assets", async () => { expect.assertions(2); diff --git a/apps/webapp/app/modules/asset/service.server.ts b/apps/webapp/app/modules/asset/service.server.ts index 687f8d22a0..395c89bfcd 100644 --- a/apps/webapp/app/modules/asset/service.server.ts +++ b/apps/webapp/app/modules/asset/service.server.ts @@ -6799,7 +6799,13 @@ export async function bulkUpdateAssetModel({ return { linked, - resolved: resolvedIds.length, + /** + * The org-verified count, not `resolvedIds.length`. A caller-supplied id + * list comes back from the resolver verbatim, so counting it would report + * "already in this asset model" for ids that simply are not in this + * organization, when "matched no assets" is the truth. + */ + resolved: assetsBeforeUpdate.length, updated: assetsThatChange.length, moved, /** From 2f8b7881ae7e67cf1df1b683971b408c45363909 Mon Sep 17 00:00:00 2001 From: Donkoko Date: Thu, 6 Aug 2026 12:17:56 +0300 Subject: [PATCH 3/4] fix(assets): announce asset model picker errors and de-duplicate the model read Follow-ups from the review of the bulk asset model actions. Accessibility: DynamicSelect gains an optional `error` prop that renders the message with role="alert" and links it to the trigger via aria-describedby. Callers previously hand-rolled a plain paragraph, so a screen-reader user who submitted the picker empty got nothing at all - the form simply refused to submit. Adopted in the two bulk asset model dialogs; the remaining consumers are left for a separate sweep. Deliberately no aria-invalid: ARIA permits it only on input widgets and the trigger's implicit role is button. Performance: assertAssetModelBelongsToOrg now selects and returns { id, name }, so bulkUpdateAssetModel drops the second findFirst it issued purely for the toast label. Keeping the shared guard preserves the org-scope rule that inlining the lookup would have broken. Tests: new route tests for /api/assets/bulk-update-asset-model covering every branch of its notification composition - the three zero-row reasons and the moved/skipped suffixes - plus permission and service-forwarding assertions. They post a URLSearchParams body because happy-dom drops empty-valued FormData fields on the Request round trip, which would silently delete the field this endpoint reads as "unlink". --- .../assets/bulk-asset-model-remove-dialog.tsx | 4 +- .../assets/bulk-asset-model-update-dialog.tsx | 18 +- .../dynamic-select/dynamic-select.test.tsx | 44 ++ .../dynamic-select/dynamic-select.tsx | 44 +- .../app/modules/asset/service.server.test.ts | 7 +- .../app/modules/asset/service.server.ts | 11 +- .../app/utils/org-validation.server.test.ts | 13 +- .../webapp/app/utils/org-validation.server.ts | 16 +- ...api.assets.bulk-update-asset-model.test.ts | 409 ++++++++++++++++++ 9 files changed, 541 insertions(+), 25 deletions(-) create mode 100644 apps/webapp/test/routes-tests/api.assets.bulk-update-asset-model.test.ts diff --git a/apps/webapp/app/components/assets/bulk-asset-model-remove-dialog.tsx b/apps/webapp/app/components/assets/bulk-asset-model-remove-dialog.tsx index 695b57e1ad..a30ec6bd8a 100644 --- a/apps/webapp/app/components/assets/bulk-asset-model-remove-dialog.tsx +++ b/apps/webapp/app/components/assets/bulk-asset-model-remove-dialog.tsx @@ -58,7 +58,9 @@ export default function BulkAssetModelRemoveDialog() { {fetcherError ? ( -

{fetcherError}

+

+ {fetcherError} +

) : null}
diff --git a/apps/webapp/app/components/assets/bulk-asset-model-update-dialog.tsx b/apps/webapp/app/components/assets/bulk-asset-model-update-dialog.tsx index 1db43c3081..d9b1dc4ecf 100644 --- a/apps/webapp/app/components/assets/bulk-asset-model-update-dialog.tsx +++ b/apps/webapp/app/components/assets/bulk-asset-model-update-dialog.tsx @@ -120,6 +120,13 @@ export default function BulkAssetModelUpdateDialog() {
)} /> - {zo.errors.assetModelId()?.message ? ( -

- {zo.errors.assetModelId()?.message} -

- ) : null} + {/* Form-level server error, not a field error — announced, but + deliberately not folded into the picker's aria-describedby. */} {fetcherError ? ( -

{fetcherError}

+

+ {fetcherError} +

) : null}
diff --git a/apps/webapp/app/components/dynamic-select/dynamic-select.test.tsx b/apps/webapp/app/components/dynamic-select/dynamic-select.test.tsx index 8266885f9b..09961fb554 100644 --- a/apps/webapp/app/components/dynamic-select/dynamic-select.test.tsx +++ b/apps/webapp/app/components/dynamic-select/dynamic-select.test.tsx @@ -880,4 +880,48 @@ describe("DynamicSelect", () => { expect(screen.getByRole("button")).toHaveTextContent("Item 2"); }); }); + + describe("Validation error", () => { + function renderWithError(error?: string) { + return render( + + ); + } + + it("renders nothing extra when there is no error", () => { + renderWithError(); + + expect(screen.queryByRole("alert")).not.toBeInTheDocument(); + expect(screen.getByRole("button")).not.toHaveAttribute( + "aria-describedby" + ); + }); + + it("announces the message, because a picker that refuses to submit is otherwise silent", () => { + renderWithError("Please select an asset model"); + + // `role="alert"` is what makes submit-time validation audible — without + // it a screen-reader user only experiences the form not submitting. + expect(screen.getByRole("alert")).toHaveTextContent( + "Please select an asset model" + ); + }); + + it("points the trigger at the message via aria-describedby", () => { + renderWithError("Please select an asset model"); + + const describedBy = screen + .getByRole("button") + .getAttribute("aria-describedby"); + + expect(describedBy).toBeTruthy(); + expect(screen.getByRole("alert")).toHaveAttribute("id", describedBy); + }); + }); }); diff --git a/apps/webapp/app/components/dynamic-select/dynamic-select.tsx b/apps/webapp/app/components/dynamic-select/dynamic-select.tsx index 3b23172c45..58adbbfe83 100644 --- a/apps/webapp/app/components/dynamic-select/dynamic-select.tsx +++ b/apps/webapp/app/components/dynamic-select/dynamic-select.tsx @@ -1,4 +1,4 @@ -import { useEffect, useMemo, useRef, useState } from "react"; +import { useEffect, useId, useMemo, useRef, useState } from "react"; import type { CSSProperties, ReactNode } from "react"; import { ChevronDownIcon } from "@radix-ui/react-icons"; import { @@ -55,6 +55,20 @@ type Props = ModelFilterProps & { /** Is this input required. Used to show a required star */ required?: boolean; + + /** + * Validation message for the current selection. + * + * Pass this instead of rendering your own paragraph next to the picker: a + * hand-rolled one is invisible to assistive tech, so a screen-reader user who + * submits an empty required picker gets nothing at all — the form just + * silently refuses to submit. Rendering it here lets the component announce + * the message (`role="alert"`) and point `aria-describedby` at it, which only + * the component can do because it owns the trigger element. + * + * Naming matches `~/components/forms/input.tsx`, which takes the same prop. + */ + error?: string; searchIcon?: IconType; showSearch?: boolean; defaultValue?: string; @@ -115,6 +129,7 @@ export default function DynamicSelect({ label, hideLabel, required, + error, searchIcon = "search", showSearch = true, defaultValue, @@ -139,6 +154,8 @@ export default function DynamicSelect({ const [createdItems, setCreatedItems] = useState([]); const [isPopoverOpen, setIsPopoverOpen] = useState(false); const triggerRef = useRef(null); + /** Stable id linking the trigger to its error text via `aria-describedby`. */ + const errorId = useId(); /** * Focus the search input when the popover opens — replaces a bare * `autoFocus` prop which is flagged by jsx-a11y because it can surprise @@ -361,6 +378,17 @@ export default function DynamicSelect({ "w-full", disabled && "cursor-not-allowed opacity-60" )} + // Radix's `asChild` trigger injects aria-expanded/haspopup/controls + // onto this element but never this one, so there is no collision. + // + // No `aria-invalid`: ARIA only allows it on input widgets, and + // this trigger's implicit role is `button` (jsx-a11y flags it as + // an error). Re-roling it to `combobox` would satisfy the + // attribute but lie about the popup — Radix Popover advertises + // `aria-haspopup="dialog"`, not a listbox. The announcement comes + // from the message's `role="alert"` and the association from + // `aria-describedby`; the red border carries the visual half. + aria-describedby={error ? errorId : undefined} > {label && ( @@ -370,7 +398,11 @@ export default function DynamicSelect({
+ + {/* Inserting a role="alert" node announces it, which is the whole point: + submit-time validation on a picker is otherwise silent. */} + {error ? ( + + ) : null}
); diff --git a/apps/webapp/app/modules/asset/service.server.test.ts b/apps/webapp/app/modules/asset/service.server.test.ts index 2579f2a691..1fbb267eec 100644 --- a/apps/webapp/app/modules/asset/service.server.test.ts +++ b/apps/webapp/app/modules/asset/service.server.test.ts @@ -2060,7 +2060,7 @@ describe("bulkUpdateAssetModel", () => { }); it("links the individually tracked assets and skips quantity-tracked ones", async () => { - expect.assertions(4); + expect.assertions(5); //@ts-expect-error mock setup db.asset.findMany.mockResolvedValue([ { id: "asset-1", type: "INDIVIDUAL", assetModelId: null }, @@ -2086,6 +2086,11 @@ describe("bulkUpdateAssetModel", () => { skippedQuantityTracked: 1, modelName: "Panasonic PT-VZ580", }); + // The model is read ONCE. `assertAssetModelBelongsToOrg` returns the row it + // already had to fetch, so the toast label costs no second round trip — + // pinned here because re-adding a `findFirst` for the name is the easy + // regression. + expect(db.assetModel.findFirst).toHaveBeenCalledTimes(1); // The active filters and index mode must reach the resolver, or a // cross-page "select all" silently operates on the wrong set. expect(resolveAssetIdsForBulkOperation).toHaveBeenCalledWith({ diff --git a/apps/webapp/app/modules/asset/service.server.ts b/apps/webapp/app/modules/asset/service.server.ts index 395c89bfcd..94a1af5ea8 100644 --- a/apps/webapp/app/modules/asset/service.server.ts +++ b/apps/webapp/app/modules/asset/service.server.ts @@ -6726,16 +6726,13 @@ export async function bulkUpdateAssetModel({ // org's assets. Shared guard per .claude/rules/org-scope-user-supplied-ids. let modelName: string | null = null; if (newAssetModelId) { - await assertAssetModelBelongsToOrg({ + // The guard returns the row it already had to read, so the toast label + // costs no extra query. + const model = await assertAssetModelBelongsToOrg({ assetModelId: newAssetModelId, organizationId, }); - - const model = await db.assetModel.findFirst({ - where: { id: newAssetModelId, organizationId }, - select: { name: true }, - }); - modelName = model?.name ?? null; + modelName = model.name; } /** diff --git a/apps/webapp/app/utils/org-validation.server.test.ts b/apps/webapp/app/utils/org-validation.server.test.ts index ee94a291fe..13f5c4e580 100644 --- a/apps/webapp/app/utils/org-validation.server.test.ts +++ b/apps/webapp/app/utils/org-validation.server.test.ts @@ -538,22 +538,27 @@ describe("single-entity guards reject foreign/missing with 400", () => { expect(err.title).toBe("Invalid asset model"); expect(tx.assetModel.findFirst).toHaveBeenCalledWith({ where: { id: "am-foreign", organizationId: ORG }, - select: { id: true }, + select: { id: true, name: true }, }); }); - it("assertAssetModelBelongsToOrg resolves when the model is in the org", async () => { + it("assertAssetModelBelongsToOrg returns the row so callers skip a second read", async () => { const tx = txWith({ assetModel: { - findFirst: vitest.fn().mockResolvedValue({ id: "am-1" }), + findFirst: vitest + .fn() + .mockResolvedValue({ id: "am-1", name: "Panasonic PT-VZ580" }), }, }); + + // `name` is part of the contract: `bulkUpdateAssetModel` builds its toast + // label from this row instead of querying the model a second time. await expect( assertAssetModelBelongsToOrg( { assetModelId: "am-1", organizationId: ORG }, tx ) - ).resolves.toBeUndefined(); + ).resolves.toEqual({ id: "am-1", name: "Panasonic PT-VZ580" }); }); }); diff --git a/apps/webapp/app/utils/org-validation.server.ts b/apps/webapp/app/utils/org-validation.server.ts index 878ab31999..4f839ba64e 100644 --- a/apps/webapp/app/utils/org-validation.server.ts +++ b/apps/webapp/app/utils/org-validation.server.ts @@ -108,10 +108,13 @@ export type OrgValidationTxClient = { }) => Promise<{ id: string }[]>; }; assetModel: { + // `name` is selected (not just `id`) because the guard hands the row back — + // see `assertAssetModelBelongsToOrg`, whose callers need the label and would + // otherwise repeat the same query. findFirst: (args: { where: { id: string; organizationId: string }; - select: { id: true }; - }) => Promise<{ id: string } | null>; + select: { id: true; name: true }; + }) => Promise<{ id: string; name: string } | null>; }; }; @@ -537,6 +540,9 @@ export async function assertLocationBelongsToOrg( * @param params.assetModelId - AssetModel ID sourced from request/form input * @param params.organizationId - The caller's (validated) organization ID * @param tx - Optional Prisma transaction client; defaults to the global `db` + * @returns The org-scoped model row. `name` is selected so callers that need a + * label for a note or toast (e.g. `bulkUpdateAssetModel`) do not have to issue + * a second query with the same `{ id, organizationId }` predicate. * @throws {ShelfError} 404 if the model is missing or in another org */ export async function assertAssetModelBelongsToOrg( @@ -545,12 +551,12 @@ export async function assertAssetModelBelongsToOrg( organizationId, }: { assetModelId: AssetModel["id"]; organizationId: string }, tx?: OrgValidationTxClient -): Promise { +): Promise> { const client = tx ?? db; const found = await client.assetModel.findFirst({ where: { id: assetModelId, organizationId }, - select: { id: true }, + select: { id: true, name: true }, }); if (!found) { @@ -565,6 +571,8 @@ export async function assertAssetModelBelongsToOrg( additionalData: { organizationId, assetModelId }, }); } + + return found; } /** diff --git a/apps/webapp/test/routes-tests/api.assets.bulk-update-asset-model.test.ts b/apps/webapp/test/routes-tests/api.assets.bulk-update-asset-model.test.ts new file mode 100644 index 0000000000..0f9a866b41 --- /dev/null +++ b/apps/webapp/test/routes-tests/api.assets.bulk-update-asset-model.test.ts @@ -0,0 +1,409 @@ +/** + * Route tests for `/api/assets/bulk-update-asset-model`. + * + * The service is unit-tested separately; what lives ONLY here is the + * notification the user actually reads. That copy is assembled from five + * independent inputs (`linked`, `resolved`, `updated`, `moved`, + * `skippedQuantityTracked`) into three different titles and three distinct + * zero-row explanations, and getting it wrong is silent: a mislabelled toast + * still returns `success: true`, so the dialog closes and the selection clears + * exactly as it would on a real update. + * + * `parseData` and `assertIsPost` are deliberately NOT mocked — the tests post a + * real request through the real wire schema, so the `assetIds[n]` bracket + * parsing and the empty-`assetModelId`-means-unlink split are covered too. + * + * @see {@link file://./../../app/routes/api+/assets.bulk-update-asset-model.ts} + * @see {@link file://./../../app/modules/asset/service.server.ts} `bulkUpdateAssetModel` + */ +import { OrganizationRoles } from "@prisma/client"; +import type { ActionFunctionArgs } from "react-router"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import type { BulkUpdateAssetModelResult } from "~/modules/asset/service.server"; +import { bulkUpdateAssetModel } from "~/modules/asset/service.server"; +import { action } from "~/routes/api+/assets.bulk-update-asset-model"; +import { sendNotification } from "~/utils/emitter/send-notification.server"; +import { ShelfError } from "~/utils/error"; +import { + PermissionAction, + PermissionEntity, +} from "~/utils/permissions/permission.data"; +import { requirePermission } from "~/utils/roles.server"; + +// why: React Router v7 single fetch — `data()` outside a request context returns +// an internal wrapper, not a Response. Returning a real Response makes status +// and body directly assertable. +const createDataMock = vi.hoisted( + () => () => + vi.fn((payload: unknown, init?: ResponseInit) => { + return new Response(JSON.stringify(payload), { + status: init?.status || 200, + headers: { "Content-Type": "application/json" }, + }); + }) +); + +vi.mock("react-router", async () => { + const actual = await vi.importActual("react-router"); + return { ...actual, data: createDataMock() }; +}); + +// why: the route never touches the database directly; the service does, and it +// is mocked below. An empty client keeps a real Prisma connection out of tests. +vi.mock("~/database/db.server", () => ({ db: {} })); + +// why: exercising the route's own logic, not the permission system +vi.mock("~/utils/roles.server", () => ({ requirePermission: vi.fn() })); + +// why: the service is unit-tested in modules/asset/service.server.test.ts; here +// it is the input to the message composition, so each test drives its result +vi.mock("~/modules/asset/service.server", () => ({ + bulkUpdateAssetModel: vi.fn(), +})); + +// why: index settings only decide simple vs advanced select-all resolution, +// which the service owns — the route just forwards them +vi.mock("~/modules/asset-index-settings/service.server", () => ({ + getAssetIndexSettings: vi.fn().mockResolvedValue({ mode: "SIMPLE" }), +})); + +// why: the assertion target. Capturing it beats sending real notifications. +vi.mock("~/utils/emitter/send-notification.server", () => ({ + sendNotification: vi.fn(), +})); + +const requirePermissionMock = vi.mocked(requirePermission); +const bulkUpdateAssetModelMock = vi.mocked(bulkUpdateAssetModel); +const sendNotificationMock = vi.mocked(sendNotification); + +/** Builds the result shape the service returns, with link-success defaults. */ +function serviceResult( + overrides: Partial = {} +): BulkUpdateAssetModelResult { + return { + linked: true, + resolved: 3, + updated: 3, + moved: 0, + skippedQuantityTracked: 0, + modelName: "Panasonic PT-VZ580", + ...overrides, + }; +} + +/** + * Posts a real request through the route and returns its Response. + * + * `assetIds` is expanded into the `assetIds[n]` bracket names the dialogs + * actually submit, so the array parsing is covered rather than assumed. + */ +async function callAction({ + assetIds = ["asset-1"], + assetModelId = "model-1", + currentSearchParams = "", +}: { + assetIds?: string[]; + assetModelId?: string; + currentSearchParams?: string; +} = {}) { + // why: a URLSearchParams body, not FormData. happy-dom's multipart serializer + // DROPS empty-valued fields on the Request round trip, which would silently + // delete the very field this endpoint uses to mean "unlink" — Node's undici + // (what the server actually runs) and real browsers both keep it. Url-encoded + // round-trips faithfully in both, so the tests exercise the real + // `assertIsPost` + `parseData` + wire-schema path instead of mocking it away. + const body = new URLSearchParams(); + assetIds.forEach((id, i) => body.append(`assetIds[${i}]`, id)); + body.append("assetModelId", assetModelId); + body.append("currentSearchParams", currentSearchParams); + + const request = new Request( + "https://example.com/api/assets/bulk-update-asset-model", + { method: "POST", body } + ); + + // The `as unknown` hop is required: the route's declared return type is + // React Router's `DataWithResponseInit`, while the mocked `data()` above + // hands back a real Response so status and body are assertable. + return (await action({ + context: { getSession: () => ({ userId: "user-123" }) }, + request, + params: {}, + } as unknown as ActionFunctionArgs)) as unknown as Response; +} + +/** + * The one notification the route sent. Asserting the count here means a test + * can never accidentally read the first of several and call it a pass. + */ +function sentNotification() { + expect(sendNotificationMock).toHaveBeenCalledTimes(1); + return sendNotificationMock.mock.calls[0][0]; +} + +beforeEach(() => { + vi.clearAllMocks(); + requirePermissionMock.mockResolvedValue({ + organizationId: "org-1", + role: OrganizationRoles.ADMIN, + canUseBarcodes: false, + } as any); + bulkUpdateAssetModelMock.mockResolvedValue(serviceResult()); +}); + +describe("api/assets/bulk-update-asset-model", () => { + describe("authorization and forwarding", () => { + it("requires asset:update — the action is admin/owner territory", async () => { + await callAction(); + + expect(requirePermissionMock).toHaveBeenCalledWith( + expect.objectContaining({ + entity: PermissionEntity.asset, + action: PermissionAction.update, + }) + ); + }); + + it("forwards the selection, the model, the active filters and the index mode", async () => { + // The filters and mode are what turn a cross-page "select all" into the + // right set of assets. Dropping either silently operates on the wrong one. + await callAction({ + assetIds: ["asset-1", "asset-2"], + assetModelId: "model-7", + currentSearchParams: "category=cat-1&status=AVAILABLE", + }); + + expect(bulkUpdateAssetModelMock).toHaveBeenCalledWith({ + userId: "user-123", + assetIds: ["asset-1", "asset-2"], + assetModelId: "model-7", + organizationId: "org-1", + currentSearchParams: "category=cat-1&status=AVAILABLE", + settings: { mode: "SIMPLE" }, + }); + }); + + it("passes an empty assetModelId straight through, because that is how removal is requested", async () => { + bulkUpdateAssetModelMock.mockResolvedValue( + serviceResult({ linked: false, updated: 1, modelName: null }) + ); + + await callAction({ assetModelId: "" }); + + expect(bulkUpdateAssetModelMock).toHaveBeenCalledWith( + expect.objectContaining({ assetModelId: "" }) + ); + }); + + it("rejects an empty selection before reaching the service", async () => { + const response = await callAction({ assetIds: [] }); + + expect(response.status).toBe(400); + expect(bulkUpdateAssetModelMock).not.toHaveBeenCalled(); + }); + }); + + describe("link succeeded", () => { + it("names the model and states the count", async () => { + const response = await callAction(); + + expect(response.status).toBe(200); + expect(sentNotification()).toMatchObject({ + title: "Assets grouped", + message: "3 asset(s) were grouped into Panasonic PT-VZ580.", + icon: { name: "success", variant: "success" }, + senderId: "user-123", + }); + }); + + it("calls out assets taken off another model", async () => { + // The only signal that a different model's book-by-model pool shrank, so + // it is stated rather than folded into the total. + bulkUpdateAssetModelMock.mockResolvedValue(serviceResult({ moved: 1 })); + + await callAction(); + + expect(sentNotification().message).toBe( + "3 asset(s) were grouped into Panasonic PT-VZ580. 1 of them were moved from another asset model." + ); + }); + + it("reports quantity-tracked assets that were skipped", async () => { + bulkUpdateAssetModelMock.mockResolvedValue( + serviceResult({ resolved: 5, skippedQuantityTracked: 2 }) + ); + + await callAction(); + + expect(sentNotification().message).toBe( + "3 asset(s) were grouped into Panasonic PT-VZ580. 2 quantity-tracked asset(s) were skipped." + ); + }); + + it("states moved before skipped when both apply", async () => { + bulkUpdateAssetModelMock.mockResolvedValue( + serviceResult({ resolved: 5, moved: 1, skippedQuantityTracked: 2 }) + ); + + await callAction(); + + expect(sentNotification().message).toBe( + "3 asset(s) were grouped into Panasonic PT-VZ580. 1 of them were moved from another asset model. 2 quantity-tracked asset(s) were skipped." + ); + }); + + it("falls back to a generic label when the model has no usable name", async () => { + // The branch is on `linked`, never on `modelName` — the name is only a + // label, and a blank one must not flip the sentence to the removal copy. + bulkUpdateAssetModelMock.mockResolvedValue( + serviceResult({ modelName: null }) + ); + + await callAction(); + + expect(sentNotification()).toMatchObject({ + title: "Assets grouped", + message: "3 asset(s) were grouped into the selected asset model.", + }); + }); + }); + + describe("unlink succeeded", () => { + it("uses the removal copy and never mentions skipped assets", async () => { + // A quantity-tracked asset can never have had a model, so counting it as + // "skipped" here would invent a failure the user did not cause. The + // service zeroes the count; this pins that the route does not re-add it. + bulkUpdateAssetModelMock.mockResolvedValue( + serviceResult({ + linked: false, + resolved: 2, + updated: 2, + modelName: null, + }) + ); + + await callAction({ assetModelId: "" }); + + expect(sentNotification()).toMatchObject({ + title: "Assets updated", + message: "2 asset(s) were removed from their asset model.", + icon: { name: "success", variant: "success" }, + }); + }); + }); + + describe("nothing changed", () => { + it("says the filters matched nothing when the selection resolved to no assets", async () => { + bulkUpdateAssetModelMock.mockResolvedValue( + serviceResult({ resolved: 0, updated: 0, modelName: null }) + ); + + const response = await callAction(); + + // Still a 200 with `success: true` — the dialog closes on a real + // outcome, it just must not claim an update happened. + expect(response.status).toBe(200); + expect(await response.json()).toMatchObject({ success: true }); + expect(sentNotification()).toMatchObject({ + title: "No assets updated", + message: "Your filters no longer match any assets.", + icon: { name: "asset-model", variant: "gray" }, + }); + }); + + it("says the assets are already on the model when linking changed nothing", async () => { + bulkUpdateAssetModelMock.mockResolvedValue( + serviceResult({ resolved: 3, updated: 0 }) + ); + + await callAction(); + + expect(sentNotification()).toMatchObject({ + title: "No assets updated", + message: "The selected assets are already in this asset model.", + }); + }); + + it("says none had a model when unlinking changed nothing", async () => { + bulkUpdateAssetModelMock.mockResolvedValue( + serviceResult({ + linked: false, + resolved: 3, + updated: 0, + modelName: null, + }) + ); + + await callAction({ assetModelId: "" }); + + expect(sentNotification()).toMatchObject({ + title: "No assets updated", + message: "None of the selected assets were in an asset model.", + }); + }); + + it("still reports the skip alongside the zero-row reason", async () => { + bulkUpdateAssetModelMock.mockResolvedValue( + serviceResult({ resolved: 5, updated: 0, skippedQuantityTracked: 2 }) + ); + + await callAction(); + + expect(sentNotification().message).toBe( + "The selected assets are already in this asset model. 2 quantity-tracked asset(s) were skipped." + ); + }); + }); + + describe("errors", () => { + it("preserves the service's status so the eligibility rule reaches the dialog", async () => { + bulkUpdateAssetModelMock.mockRejectedValue( + new ShelfError({ + cause: null, + title: "Asset model not allowed", + message: + "All selected assets are quantity-tracked. Asset models can only be linked to individually tracked assets.", + label: "Assets", + status: 400, + shouldBeCaptured: false, + }) + ); + + const response = await callAction(); + + expect(response.status).toBe(400); + // `error()` surfaces the ShelfError's own title and message. Both have to + // survive, or the user gets "Something went wrong" instead of the rule + // they broke — which is exactly why the service re-throws its own errors + // rather than letting the generic wrapper replace the message. + expect(sentNotification()).toMatchObject({ + title: "Asset model not allowed", + message: expect.stringContaining("quantity-tracked"), + icon: { name: "x", variant: "error" }, + }); + }); + + it("preserves the 404 raised for a model outside the organization", async () => { + bulkUpdateAssetModelMock.mockRejectedValue( + new ShelfError({ + cause: null, + title: "Invalid asset model", + message: + "The selected asset model could not be found in your workspace. Please reload and try again.", + label: "Assets", + status: 404, + shouldBeCaptured: false, + }) + ); + + const response = await callAction({ assetModelId: "foreign-model" }); + + expect(response.status).toBe(404); + expect(sentNotification()).toMatchObject({ + title: "Invalid asset model", + message: expect.stringContaining("workspace"), + }); + }); + }); +}); From ba9a3dfbfccc9548b12d9bfe5e3d80ac5e424f35 Mon Sep 17 00:00:00 2001 From: Donkoko Date: Thu, 6 Aug 2026 12:33:20 +0300 Subject: [PATCH 4/4] test(assets): type the requirePermission fixture instead of casting to any CLAUDE.md forbids `any` as a shortcut, and requirePermission is first-party code so the third-party-shape exception does not apply. Asserts the three-key literal to `Awaited>` rather than reconstructing the full return: the real result also carries currentOrganization / organizations / userOrganizations, which are Prisma payloads running to ~30 fields this route never reads. The assertion still fails the build if one of the three fields is renamed or retyped, which `as any` did not. Same approach as test/routes-tests/api+/user.entity-counts.test.ts. The identical casts in sibling route tests are untouched by this PR; sweeping them belongs in its own change. --- ...api.assets.bulk-update-asset-model.test.ts | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/apps/webapp/test/routes-tests/api.assets.bulk-update-asset-model.test.ts b/apps/webapp/test/routes-tests/api.assets.bulk-update-asset-model.test.ts index 0f9a866b41..a407c2f1f2 100644 --- a/apps/webapp/test/routes-tests/api.assets.bulk-update-asset-model.test.ts +++ b/apps/webapp/test/routes-tests/api.assets.bulk-update-asset-model.test.ts @@ -77,6 +77,21 @@ const requirePermissionMock = vi.mocked(requirePermission); const bulkUpdateAssetModelMock = vi.mocked(bulkUpdateAssetModel); const sendNotificationMock = vi.mocked(sendNotification); +/** + * The three fields this route actually destructures from `requirePermission`. + * + * Asserted to the real return type instead of `any`: the full result also + * carries `currentOrganization` / `organizations` / `userOrganizations`, which + * are Prisma payloads running to ~30 fields the route never reads, so building + * a complete literal would be noise. The assertion still fails the build if one + * of these three is renamed or retyped, which the `any` cast would not. + */ +const permissionResult = { + organizationId: "org-1", + role: OrganizationRoles.ADMIN, + canUseBarcodes: false, +} as Awaited>; + /** Builds the result shape the service returns, with link-success defaults. */ function serviceResult( overrides: Partial = {} @@ -144,11 +159,7 @@ function sentNotification() { beforeEach(() => { vi.clearAllMocks(); - requirePermissionMock.mockResolvedValue({ - organizationId: "org-1", - role: OrganizationRoles.ADMIN, - canUseBarcodes: false, - } as any); + requirePermissionMock.mockResolvedValue(permissionResult); bulkUpdateAssetModelMock.mockResolvedValue(serviceResult()); });