diff --git a/src/visualBuilder/components/FieldToolbar.tsx b/src/visualBuilder/components/FieldToolbar.tsx index d06f9f5f..879f0ec7 100644 --- a/src/visualBuilder/components/FieldToolbar.tsx +++ b/src/visualBuilder/components/FieldToolbar.tsx @@ -43,7 +43,11 @@ import { EntryPermissions } from "../utils/getEntryPermissions"; import { FieldLocationAppList } from "./FieldLocationAppList"; import { FieldLocationIcon } from "./FieldLocationIcon"; import { WorkflowStageDetails } from "../utils/getWorkflowStageDetails"; -import { getEntryEditRestrictionForField } from "../utils/fieldLockIndicator"; +import { + getEntryEditRestrictionForField, + getPeerLockForContainer, + peerLockMessage, +} from "../utils/fieldLockIndicator"; import { subscribeEntryFieldLockInfo } from "../utils/fieldLockStore"; import { ResolvedVariantPermissions } from "../utils/getResolvedVariantPermissions"; import { isCustomFieldMultipleInstance as checkIsCustomFieldMultipleInstance } from "../utils/isCustomFieldMultipleInstance"; @@ -201,12 +205,21 @@ function FieldToolbarComponent( const [restrictionReason, setRestrictionReason] = useState(() => getEntryEditRestrictionForField(fieldMetadata) ); + const [containerPeerLock, setContainerPeerLock] = useState(() => + getPeerLockForContainer(fieldMetadata) + ); useEffect(() => { - const update = () => + const update = () => { setRestrictionReason(getEntryEditRestrictionForField(fieldMetadata)); + setContainerPeerLock(getPeerLockForContainer(fieldMetadata)); + }; update(); return subscribeEntryFieldLockInfo(update); }, [fieldMetadata]); + // Add / move / delete change the whole container, so a collaborator anywhere in it blocks them. + const containerLockReason = containerPeerLock + ? peerLockMessage(containerPeerLock) + : null; let disableFieldActions = Boolean(restrictionReason); let isCustomFieldMultipleInstance = false; @@ -560,9 +573,10 @@ function FieldToolbarComponent( multipleFieldToolbarButtonClasses } data-tooltip={ - direction === "vertical" + containerLockReason ?? + (direction === "vertical" ? "Move up" - : "Move left" + : "Move left") } onClick={(e) => { e.preventDefault(); @@ -573,7 +587,9 @@ function FieldToolbarComponent( ); }} disabled={ - disableFieldActions || disableMoveLeft + disableFieldActions || + Boolean(containerLockReason) || + disableMoveLeft } > @@ -597,9 +614,10 @@ function FieldToolbarComponent( multipleFieldToolbarButtonClasses } data-tooltip={ - direction === "vertical" + containerLockReason ?? + (direction === "vertical" ? "Move down" - : "Move right" + : "Move right") } onClick={(e) => { e.preventDefault(); @@ -610,7 +628,9 @@ function FieldToolbarComponent( ); }} disabled={ - disableFieldActions || disableMoveRight + disableFieldActions || + Boolean(containerLockReason) || + disableMoveRight } > @@ -637,13 +658,18 @@ function FieldToolbarComponent( className={ multipleFieldToolbarButtonClasses } - data-tooltip={"Delete"} + data-tooltip={ + containerLockReason ?? "Delete" + } onClick={(e) => { e.preventDefault(); e.stopPropagation(); handleDeleteInstance(fieldMetadata); }} - disabled={disableFieldActions} + disabled={ + disableFieldActions || + Boolean(containerLockReason) + } > diff --git a/src/visualBuilder/components/__test__/addInstanceButton.test.tsx b/src/visualBuilder/components/__test__/addInstanceButton.test.tsx index dc4e774f..6af7d61a 100644 --- a/src/visualBuilder/components/__test__/addInstanceButton.test.tsx +++ b/src/visualBuilder/components/__test__/addInstanceButton.test.tsx @@ -10,6 +10,10 @@ import { singleLineFieldSchema } from "../../../__test__/data/fields"; import AddInstanceButtonComponent from "../addInstanceButton"; import visualBuilderPostMessageActual from "../../utils/visualBuilderPostMessage"; import { getDiscussionIdByFieldMetaData } from "../../utils/getDiscussionIdByFieldMetaData"; +import { + clearAllEntryFieldLockInfo, + setEntryFieldLockInfo, +} from "../../utils/fieldLockStore"; const visualBuilderPostMessage = vi.mocked(visualBuilderPostMessageActual); @@ -147,4 +151,66 @@ describe("AddInstanceButtonComponent", () => { expect(onFieldLockRefused).toHaveBeenCalled(); expect(onClickCallback).not.toHaveBeenCalled(); }); + describe("a collaborator inside the container", () => { + const fieldMetadata = { + entry_uid: "e1", + locale: "en-us", + fieldPathWithIndex: "group", + }; + const lockIn = (path: string) => + setEntryFieldLockInfo( + { entryUid: "e1", locale: "en-us" }, + { + [path]: { + user: { uid: "u2", name: "Watcher" }, + ttl: "2030-01-01", + isLocked: true, + isOwn: false, + }, + } + ); + const renderButton = async () => { + await act(() => { + render( + + ); + }); + return getByTestId( + document.body, + "visual-builder-add-instance-button" + ); + }; + + afterEach(() => clearAllEntryFieldLockInfo()); + + test("is disabled with the holder in the tooltip", async () => { + lockIn("group.1.title"); + const button = await renderButton(); + expect(button).toBeDisabled(); + expect(button).toHaveAttribute( + "data-tooltip", + "This field is locked by Watcher" + ); + }); + + test("re-enables when the lock goes", async () => { + lockIn("group.1"); + const button = await renderButton(); + await act(() => + setEntryFieldLockInfo({ entryUid: "e1", locale: "en-us" }, {}) + ); + expect(button).not.toBeDisabled(); + expect(button).toHaveAttribute("data-tooltip", "Add section"); + }); + }); }); diff --git a/src/visualBuilder/components/__test__/fieldToolbar.test.tsx b/src/visualBuilder/components/__test__/fieldToolbar.test.tsx index df5de50d..128f7788 100644 --- a/src/visualBuilder/components/__test__/fieldToolbar.test.tsx +++ b/src/visualBuilder/components/__test__/fieldToolbar.test.tsx @@ -30,6 +30,7 @@ import { VisualBuilderPostMessageEvents } from "../../utils/types/postMessage.ty import { clearAllEntryFieldLockInfo, setEntryEditRestriction, + setEntryFieldLockInfo, } from "../../utils/fieldLockStore"; import { VisualBuilder } from "../.."; @@ -817,6 +818,61 @@ describe("FieldToolbarComponent", () => { await waitFor(() => expect(deleteButton()).toBeDisabled()); }); + describe("a collaborator inside the container", () => { + const lockIn = (path: string) => + setEntryFieldLockInfo(scope, { + [path]: { + user: { uid: "u2", name: "Watcher" }, + ttl: "2030-01-01", + isLocked: true, + isOwn: false, + }, + }); + const button = (container: Element, name: string) => + container.querySelector( + `[data-testid="visual-builder__focused-toolbar__multiple-field-toolbar__${name}-button"]` + ); + + test("greys move and delete with the holder in the tooltip", async () => { + lockIn("group.link.1"); + const deleteButton = await renderToolbar(); + + await waitFor(() => expect(deleteButton()).toBeDisabled()); + expect(deleteButton()).toHaveAttribute( + "data-tooltip", + "This field is locked by Watcher" + ); + const toolbar = deleteButton()!.parentElement!; + expect(button(toolbar, "move-left")).toBeDisabled(); + expect(button(toolbar, "move-right")).toBeDisabled(); + }); + + test("re-enables them when the lock is released", async () => { + lockIn("group.link.1"); + const deleteButton = await renderToolbar(); + await waitFor(() => expect(deleteButton()).toBeDisabled()); + + act(() => setEntryFieldLockInfo(scope, {})); + + await waitFor(() => + expect(deleteButton()).not.toBeDisabled() + ); + expect(deleteButton()).toHaveAttribute( + "data-tooltip", + "Delete" + ); + }); + + test("ignores a lock in another field", async () => { + lockIn("group.title"); + const deleteButton = await renderToolbar(); + + await waitFor(() => + expect(deleteButton()).not.toBeDisabled() + ); + }); + }); + test("disables the field actions when the restriction arrives after the toolbar opened", async () => { const deleteButton = await renderToolbar(); await waitFor(() => expect(deleteButton()).not.toBeDisabled()); diff --git a/src/visualBuilder/components/addInstanceButton.tsx b/src/visualBuilder/components/addInstanceButton.tsx index f6dc621e..feda752b 100644 --- a/src/visualBuilder/components/addInstanceButton.tsx +++ b/src/visualBuilder/components/addInstanceButton.tsx @@ -1,4 +1,4 @@ -import React from "preact/compat"; +import React, { useEffect, useState } from "preact/compat"; import classNames from "classnames"; import { visualBuilderStyles } from "../visualBuilder.style"; import { PlusIcon } from "./icons"; @@ -8,6 +8,11 @@ import visualBuilderPostMessage from "../utils/visualBuilderPostMessage"; import { VisualBuilderPostMessageEvents } from "../utils/types/postMessage.types"; import { Signal } from "@preact/signals"; import { isFieldLockRefused } from "../utils/fieldLockRefused"; +import { + getPeerLockForContainer, + peerLockMessage, +} from "../utils/fieldLockIndicator"; +import { subscribeEntryFieldLockInfo } from "../utils/fieldLockStore"; interface AddInstanceButtonProps { value: any; @@ -28,6 +33,15 @@ function AddInstanceButtonComponent( const fieldMetadata = props.fieldMetadata; const index = props.index; const loading = props.loading; + const [containerPeerLock, setContainerPeerLock] = useState(() => + getPeerLockForContainer(fieldMetadata) + ); + useEffect(() => { + const update = () => + setContainerPeerLock(getPeerLockForContainer(fieldMetadata)); + update(); + return subscribeEntryFieldLockInfo(update); + }, [fieldMetadata]); const onClick = async (event: MouseEvent) => { loading.value = true; @@ -71,12 +85,17 @@ function AddInstanceButtonComponent( const isMaxInstances = maxInstances ? props.value.length >= maxInstances : false; - const disabled = loading.value || isMaxInstances; + const disabled = + loading.value || isMaxInstances || Boolean(containerPeerLock); return ( ; + }); + const button = generateAddInstanceButton({ + fieldSchema: singleLineFieldSchema, + value: "", + // @ts-expect-error mock field metadata + fieldMetadata: { hello: "world" }, + onClick: vi.fn(), + onFieldLockRefused: vi.fn(), + // @ts-expect-error mocking preact signal + loading: { value: false }, + index: 0, + }); + document.body.appendChild(button); + await vi.waitFor(() => expect(mounted).toHaveBeenCalled()); + + removeAddInstanceButton(button); + + expect(cleanup).toHaveBeenCalledTimes(1); + expect(button.isConnected).toBe(false); + }); + test("should call the AddInstanceButtonComponent with the correct props", () => { generateAddInstanceButton({ fieldSchema: singleLineFieldSchema, diff --git a/src/visualBuilder/generators/generateAddInstanceButtons.tsx b/src/visualBuilder/generators/generateAddInstanceButtons.tsx index de9d6367..5acc9a42 100644 --- a/src/visualBuilder/generators/generateAddInstanceButtons.tsx +++ b/src/visualBuilder/generators/generateAddInstanceButtons.tsx @@ -47,9 +47,22 @@ export function generateAddInstanceButton({ ); const button = wrapper.children[0] as HTMLButtonElement; + renderRoots.set(button, wrapper); return button; } +// The button's preact root, so removing the button can unmount it and run its +// effect cleanups (the lock-info subscription). +const renderRoots = new WeakMap(); + +/** Removes a button made by generateAddInstanceButton and unmounts its component. */ +export function removeAddInstanceButton(button: HTMLButtonElement): void { + const root = renderRoots.get(button); + renderRoots.delete(button); + if (root) render(null, root); + button.remove(); +} + /** * Returns an array of HTMLButtonElement instances that can be used to add new instances to the visual builder. * @param visualBuilderContainer - The HTMLDivElement that contains the visual builder. diff --git a/src/visualBuilder/utils/__test__/fieldLockIndicator.test.ts b/src/visualBuilder/utils/__test__/fieldLockIndicator.test.ts index a8337e92..2d62a992 100644 --- a/src/visualBuilder/utils/__test__/fieldLockIndicator.test.ts +++ b/src/visualBuilder/utils/__test__/fieldLockIndicator.test.ts @@ -1,5 +1,10 @@ import { describe, it, expect, beforeEach, vi } from "vitest"; -import { getPeerLockForField, lockAvatarInfo } from "../fieldLockIndicator"; +import { + getPeerLockForContainer, + getPeerLockForField, + lockAvatarInfo, + peerLockMessage, +} from "../fieldLockIndicator"; import { setEntryFieldLockInfo, clearAllEntryFieldLockInfo, @@ -121,6 +126,70 @@ describe("getPeerLockForField", () => { }); }); +describe("instance elements", () => { + // An instance element's cslp ends in its index: fieldPathWithIndex is the container. + const instance = (index: number) => + meta({ + fieldPath: "section.buckets", + fieldPathWithIndex: "section.0.buckets", + instance: { fieldPathWithIndex: `section.0.buckets.${index}` }, + }) as never; + + it("leaves an instance free when a peer is inside a sibling instance", () => { + setEntryFieldLockInfo(scope, { "section.0.buckets.1.title": peer() }); + expect(getPeerLockForField(instance(0))).toBeNull(); + expect(getPeerLockForField(instance(1))?.user.uid).toBe("u1"); + }); + + it("names the instance's own holder before a peer inside it", () => { + setEntryFieldLockInfo(scope, { + "section.0.buckets.1.title": peer({ user: { uid: "u2" } }), + "section.0.buckets.1": peer(), + }); + expect(getPeerLockForField(instance(1))?.user.uid).toBe("u1"); + }); + + it("does not match instance 1 against a lock on instance 10", () => { + setEntryFieldLockInfo(scope, { "section.0.buckets.10": peer() }); + expect(getPeerLockForField(instance(1))).toBeNull(); + }); + + it("locks every instance when the container itself is locked", () => { + setEntryFieldLockInfo(scope, { "section.0.buckets": peer() }); + expect(getPeerLockForField(instance(0))).not.toBeNull(); + }); + + it("blocks structural actions for a peer anywhere in the container", () => { + setEntryFieldLockInfo(scope, { "section.0.buckets.1.title": peer() }); + expect(getPeerLockForContainer(instance(0))?.user.uid).toBe("u1"); + }); + + it("blocks structural actions for a peer lock above the container", () => { + setEntryFieldLockInfo(scope, { "section.0": peer() }); + expect(getPeerLockForContainer(instance(0))).not.toBeNull(); + }); + + it("ignores own locks and other containers for structural actions", () => { + setEntryFieldLockInfo(scope, { + "section.0.buckets.1": peer({ isOwn: true }), + "section.0.buckets_two.0": peer(), + "section.1.buckets.0": peer(), + }); + expect(getPeerLockForContainer(instance(0))).toBeNull(); + }); +}); + +describe("peerLockMessage", () => { + it("names the holder, or another user", () => { + expect(peerLockMessage(peer() as never)).toBe( + "This field is locked by Ada Lovelace" + ); + expect(peerLockMessage(peer({ user: { uid: "u1" } }) as never)).toBe( + "This field is locked by another user" + ); + }); +}); + describe("lockAvatarInfo", () => { it("uses initials, color, and name from the lock", () => { expect(lockAvatarInfo(peer() as never)).toEqual({ diff --git a/src/visualBuilder/utils/fieldLockIndicator.ts b/src/visualBuilder/utils/fieldLockIndicator.ts index ae6e3745..0aa53861 100644 --- a/src/visualBuilder/utils/fieldLockIndicator.ts +++ b/src/visualBuilder/utils/fieldLockIndicator.ts @@ -13,13 +13,45 @@ import type { CslpData } from "../../cslp/types/cslp.types"; type FieldMetadataForLock = Pick< CslpData, "entry_uid" | "locale" | "variant" | "fieldPath" | "fieldPathWithIndex" ->; +> & + Partial>; /** A lock counts as a peer lock when it is locked and not the current user's. */ function asPeerLock(lock: EntryFieldLock | undefined): EntryFieldLock | null { return lock?.isLocked && lock.isOwn !== true ? lock : null; } +type FieldMetadataForContainerLock = Pick< + CslpData, + "entry_uid" | "locale" | "variant" | "fieldPathWithIndex" +>; + +function getScopeLocks(fieldMetadata: FieldMetadataForContainerLock) { + return getEntryFieldLockInfo({ + entryUid: fieldMetadata.entry_uid, + locale: fieldMetadata.locale, + ...(fieldMetadata.variant ? { variantUid: fieldMetadata.variant } : {}), + }); +} + +/** First peer lock on `path`, inside it, or on a container above it. */ +function findPeerLockAround( + scopeLocks: Record, + path: string +): EntryFieldLock | null { + for (const lockedPath of Object.keys(scopeLocks)) { + if ( + lockedPath === path || + path.startsWith(lockedPath + ".") || + lockedPath.startsWith(path + ".") + ) { + const peer = asPeerLock(scopeLocks[lockedPath]); + if (peer) return peer; + } + } + return null; +} + /** * Returns the peer lock (a lock held by another user) for a field, or null when * the field is unlocked or locked by the current user. Reads the SDK mirror the @@ -29,38 +61,45 @@ function asPeerLock(lock: EntryFieldLock | undefined): EntryFieldLock | null { * * Container fields propagate the lock, mirroring the entry editor's * useContainerFieldLock: a peer lock on an ancestor disables its descendants - * (parent locked => child locked) and a peer lock on a descendant disables the - * ancestor's structural actions (child locked => container locked). Mirror keys - * are cslp index-form, so the ancestor/descendant scan compares against - * fieldPathWithIndex. + * and a peer lock on a descendant disables the ancestor. Mirror keys are cslp + * index-form. An instance element (`buckets.1`) is matched on its own path, so + * a peer inside a sibling instance leaves it free; structural actions use + * `getPeerLockForContainer` instead. */ export function getPeerLockForField( fieldMetadata: FieldMetadataForLock ): EntryFieldLock | null { - const scopeLocks = getEntryFieldLockInfo({ - entryUid: fieldMetadata.entry_uid, - locale: fieldMetadata.locale, - ...(fieldMetadata.variant ? { variantUid: fieldMetadata.variant } : {}), - }); + const target = + fieldMetadata.instance?.fieldPathWithIndex || + fieldMetadata.fieldPathWithIndex || + fieldMetadata.fieldPath; + if (!target) return null; - const exact = asPeerLock( - scopeLocks[fieldMetadata.fieldPathWithIndex] ?? - scopeLocks[fieldMetadata.fieldPath] + // Own path first so its holder wins over a peer in a nested or parent field. + const scopeLocks = getScopeLocks(fieldMetadata); + return ( + asPeerLock(scopeLocks[target]) ?? findPeerLockAround(scopeLocks, target) ); - if (exact) return exact; +} - const target = fieldMetadata.fieldPathWithIndex || fieldMetadata.fieldPath; - if (!target) return null; - for (const lockedPath of Object.keys(scopeLocks)) { - if ( - target.startsWith(lockedPath + ".") || - lockedPath.startsWith(target + ".") - ) { - const peer = asPeerLock(scopeLocks[lockedPath]); - if (peer) return peer; - } - } - return null; +/** + * The peer lock that blocks add / move / delete on this field's multiple + * container: one on the container, inside any of its instances, or above it. + * Same rule the parent enforces before it applies the change. + */ +export function getPeerLockForContainer( + fieldMetadata: FieldMetadataForContainerLock +): EntryFieldLock | null { + if (!fieldMetadata.fieldPathWithIndex) return null; + return findPeerLockAround( + getScopeLocks(fieldMetadata), + fieldMetadata.fieldPathWithIndex + ); +} + +/** Tooltip / label text for a field a collaborator holds. */ +export function peerLockMessage(lock: EntryFieldLock): string { + return `This field is locked by ${lockAvatarInfo(lock).name || "another user"}`; } export const ENTRY_RESTRICTION_MESSAGES: Record = { diff --git a/src/visualBuilder/utils/multipleElementAddButton.ts b/src/visualBuilder/utils/multipleElementAddButton.ts index c4546b6a..9fa38fda 100644 --- a/src/visualBuilder/utils/multipleElementAddButton.ts +++ b/src/visualBuilder/utils/multipleElementAddButton.ts @@ -2,6 +2,7 @@ import { VisualBuilderCslpEventDetails } from "../types/visualBuilder.types"; import { generateAddInstanceButton, getAddInstanceButtons, + removeAddInstanceButton, } from "../generators/generateAddInstanceButtons"; import getChildrenDirection from "./getChildrenDirection"; import { hideOverlay } from "../generators/generateOverlay"; @@ -196,7 +197,7 @@ export function removeAddInstanceButtons( true ); - addInstanceButtons?.forEach((button) => button.remove()); + addInstanceButtons?.forEach(removeAddInstanceButton); } const addInstanceButtons = getAddInstanceButtons(visualBuilderContainer); @@ -219,8 +220,8 @@ export function removeAddInstanceButtons( return; } - nextButton.remove(); - previousButton.remove(); + removeAddInstanceButton(nextButton); + removeAddInstanceButton(previousButton); } /**