From 1fbbadf08feaa8cef17b9f6f155c3cc583b3bf9b Mon Sep 17 00:00:00 2001 From: dolphin Date: Mon, 10 Aug 2026 23:03:55 +0800 Subject: [PATCH 01/38] fix(permission): carry grant assignee ids as strings so revoke can find them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 撤销和改权限模型在 116 上必然失败，报「Grant assignee is missing or ambiguous」。 授权行的 id 是 60~62 位整数（API 新增走 secrets.randbits(62)，所有者/复制路径走 sha256[:15]），超过 JSON number 在浏览器里能表示的 2^53。以数字下发时 JSON.parse 当场四舍五入，前端原样回传的 id 与任何一条 assignee 都对不上，_find_source 找到 0 条即抛错。实测约 99% 的 id 会失真，也就是基本必挂。ADD 不受影响，它按主体识别而不 是按行 id。 只把 id 挪到线上的十进制字符串：领域层、仓储和 BigInteger 列仍是整数，无需数据迁移。 入参改为正整数字符串并拒绝数字形态——混合部署下宁可显式报参数错，也不要重新掉进 静默截断。新增 test_f048_assignee_id_wire_format.py 覆盖两种分配器的上界值。 顺带修掉 client 端的撤销确认弹窗：原本是手搓的 AlertDialog，确认按钮取顶层 key `confirm`，而三个语言包都没有这个 key，i18next 直接把 key 当文案渲染成英文 "confirm"。改用共享的 useConfirm() destructive 变体，与会话删除等危险操作一致，并在 正文里点名被撤销的主体——原来只问「确认撤销该主体吗」，不告诉你是哪个。 已知未处理：撤销成功后 grants:mutate 的返回体把 scope 写死 LOCAL、主体名置空、模型 名退化成 model_key，而前端拿它整体替换了列表，成功后剩余行会短暂「变糙」，刷新恢复。 此前撤销从未成功，所以这个问题一直没暴露。 Co-Authored-By: Claude Opus 5 (1M context) --- .../permission/application/resource_api.py | 6 +- .../bisheng/permission/domain/schemas/f048.py | 10 +- .../test_f048_assignee_id_wire_format.py | 101 ++++++++++++++++++ .../test/permission/test_f048_grant_api.py | 4 +- .../client/src/api/permission.test.ts | 4 +- src/frontend/client/src/api/permission.ts | 6 +- .../permission/PermissionGrantTab.test.tsx | 12 +-- .../permission/PermissionGrantTab.tsx | 4 +- .../permission/PermissionListTab.test.tsx | 8 +- .../permission/PermissionListTab.tsx | 58 +++------- .../bs-comp/permission/PermissionListTab.tsx | 2 +- .../src/controllers/API/permission.ts | 6 +- .../src/test/f048PermissionApi.test.ts | 4 +- .../src/test/f048PermissionDialog.test.tsx | 2 +- .../src/test/f048PermissionGrantTab.test.tsx | 8 +- .../src/test/f048PermissionRoster.test.tsx | 4 +- 16 files changed, 160 insertions(+), 79 deletions(-) create mode 100644 src/backend/test/permission/test_f048_assignee_id_wire_format.py diff --git a/src/backend/bisheng/permission/application/resource_api.py b/src/backend/bisheng/permission/application/resource_api.py index 270cf56876..90c929b167 100644 --- a/src/backend/bisheng/permission/application/resource_api.py +++ b/src/backend/bisheng/permission/application/resource_api.py @@ -183,7 +183,7 @@ async def list_grants( model_names = {item.snapshot.model_key: item.name for item in catalog.models} data = [ { - "assignee_id": row.source_id, + "assignee_id": str(row.source_id), "assignee_version": row.source_version, "subject": { "type": row.subject_type, @@ -294,7 +294,7 @@ async def mutate_grants( operation=change.op.value, model_key=change.model_key, source=source, - assignee_id=change.assignee_id, + assignee_id=change.assignee_row_id, expected_assignee_version=(change.expected_assignee_version), target_model_key=change.target_model_key, ) @@ -309,7 +309,7 @@ async def mutate_grants( ) items = [ { - "assignee_id": source.source_id, + "assignee_id": str(source.source_id), "assignee_version": source.version, "subject": { "type": source.subject_type, diff --git a/src/backend/bisheng/permission/domain/schemas/f048.py b/src/backend/bisheng/permission/domain/schemas/f048.py index 90172d0eba..53b97736eb 100644 --- a/src/backend/bisheng/permission/domain/schemas/f048.py +++ b/src/backend/bisheng/permission/domain/schemas/f048.py @@ -225,10 +225,16 @@ class GrantMutationChange(StrictRequestModel): op: GrantMutationOperation model_key: str | None = Field(default=None, max_length=64) subject: GrantSubjectInput | None = None - assignee_id: int | None = Field(default=None, gt=0) + # Carried as a decimal string: row ids are 60-62 bit, past the 2^53 that a + # JSON number survives in a browser, and a rounded id matches no assignee. + assignee_id: str | None = Field(default=None, pattern=r"^[1-9][0-9]{0,19}$") expected_assignee_version: int | None = Field(default=None, ge=0) target_model_key: str | None = Field(default=None, max_length=64) + @property + def assignee_row_id(self) -> int | None: + return None if self.assignee_id is None else int(self.assignee_id) + @model_validator(mode="after") def validate_operation_shape(self) -> GrantMutationChange: if self.op == GrantMutationOperation.ADD: @@ -279,7 +285,7 @@ class GrantSourceDTO(BaseModel): class GrantAssigneeDTO(BaseModel): - assignee_id: int + assignee_id: str assignee_version: int subject: GrantSubjectDTO model: GrantModelDTO diff --git a/src/backend/test/permission/test_f048_assignee_id_wire_format.py b/src/backend/test/permission/test_f048_assignee_id_wire_format.py new file mode 100644 index 0000000000..e8f0eb66e3 --- /dev/null +++ b/src/backend/test/permission/test_f048_assignee_id_wire_format.py @@ -0,0 +1,101 @@ +"""Assignee row ids must cross the wire as strings. + +Row ids are allocated as 60-62 bit integers (``secrets.randbits(62)`` for API +adds, ``sha256(...)[:15]`` for owner/copy paths), well past the 2^53 a JSON +number survives in a browser. Sent as a number, ``JSON.parse`` silently rounds +it and the id the client echoes back matches no assignee, so every REMOVE/MOVE +fails with "Grant assignee is missing or ambiguous". +""" + +import json + +import pytest +from pydantic import ValidationError + +from bisheng.permission.domain.schemas.f048 import ( + GrantAssigneeDTO, + GrantMutationChange, +) + +# Both allocators produce ids in this range; 2**53 is the JS safe-integer limit. +UNSAFE_IDS = ( + 2**53 + 1, + 4_611_686_018_427_387_903, # secrets.randbits(62) upper bound + 1_152_921_504_606_846_975, # int(sha256(...)[:15], 16) upper bound +) + + +@pytest.mark.parametrize("row_id", UNSAFE_IDS) +def test_remove_change_preserves_ids_beyond_js_safe_integers(row_id: int) -> None: + change = GrantMutationChange.model_validate( + { + "op": "REMOVE", + "assignee_id": str(row_id), + "expected_assignee_version": 2, + } + ) + assert change.assignee_id == str(row_id) + assert change.assignee_row_id == row_id + + +def test_numeric_assignee_id_is_rejected() -> None: + """A number is the shape that loses precision in the browser.""" + + with pytest.raises(ValidationError): + GrantMutationChange.model_validate( + { + "op": "REMOVE", + "assignee_id": 91, + "expected_assignee_version": 2, + } + ) + + +@pytest.mark.parametrize("bad", ["0", "-1", "01", "9e18", "", "12a"]) +def test_assignee_id_must_be_a_positive_decimal_string(bad: str) -> None: + with pytest.raises(ValidationError): + GrantMutationChange.model_validate( + { + "op": "REMOVE", + "assignee_id": bad, + "expected_assignee_version": 2, + } + ) + + +def test_add_still_rejects_an_assignee_identity() -> None: + with pytest.raises(ValidationError): + GrantMutationChange.model_validate( + { + "op": "ADD", + "model_key": "standard-viewer", + "subject": {"type": "user", "id": "7"}, + "assignee_id": "91", + } + ) + + +@pytest.mark.parametrize("row_id", UNSAFE_IDS) +def test_roster_row_serializes_the_id_as_a_json_string(row_id: int) -> None: + dto = GrantAssigneeDTO.model_validate( + { + "assignee_id": str(row_id), + "assignee_version": 2, + "subject": {"type": "user", "id": "7", "name": "Alice"}, + "model": { + "key": "standard-viewer", + "name": "Viewer", + "level": 1, + "active": True, + }, + "source": {"type": "DIRECT", "include_children": False}, + "scope": "LOCAL", + "inherited_from": None, + "protected": False, + "editable": True, + } + ) + payload = json.loads(dto.model_dump_json()) + assert payload["assignee_id"] == str(row_id) + # Quoted in the raw JSON, so a browser never parses it as a number. + assert f'"assignee_id":"{row_id}"' in dto.model_dump_json() diff --git a/src/backend/test/permission/test_f048_grant_api.py b/src/backend/test/permission/test_f048_grant_api.py index 8f9c2f6a94..5f79d81a1c 100644 --- a/src/backend/test/permission/test_f048_grant_api.py +++ b/src/backend/test/permission/test_f048_grant_api.py @@ -50,7 +50,7 @@ async def list_grants(self, **kwargs) -> dict: return { "data": [ { - "assignee_id": 91, + "assignee_id": "91", "assignee_version": 2, "subject": { "type": "department", @@ -190,7 +190,7 @@ def test_protected_mutation_and_stale_cursor_preserve_business_codes() -> None: "changes": [ { "op": "REMOVE", - "assignee_id": 91, + "assignee_id": "91", "expected_assignee_version": 2, } ], diff --git a/src/frontend/client/src/api/permission.test.ts b/src/frontend/client/src/api/permission.test.ts index 255c1731c1..651715a348 100644 --- a/src/frontend/client/src/api/permission.test.ts +++ b/src/frontend/client/src/api/permission.test.ts @@ -86,13 +86,13 @@ describe("F048 Client permission API", () => { }, { op: "MOVE" as const, - assignee_id: 8, + assignee_id: "8", expected_assignee_version: 3, target_model_key: "standard-editor", }, { op: "REMOVE" as const, - assignee_id: 9, + assignee_id: "9", expected_assignee_version: 4, }, ], diff --git a/src/frontend/client/src/api/permission.ts b/src/frontend/client/src/api/permission.ts index 80e13600ae..7dcd39cd50 100644 --- a/src/frontend/client/src/api/permission.ts +++ b/src/frontend/client/src/api/permission.ts @@ -45,7 +45,7 @@ export interface PermissionGrantSource { } export interface PermissionGrantAssignee { - assignee_id: number; + assignee_id: string; assignee_version: number; subject: PermissionGrantSubject; model: GrantablePermissionModel; @@ -85,13 +85,13 @@ export type PermissionGrantMutationChange = } | { op: "MOVE"; - assignee_id: number; + assignee_id: string; expected_assignee_version: number; target_model_key: string; } | { op: "REMOVE"; - assignee_id: number; + assignee_id: string; expected_assignee_version: number; }; diff --git a/src/frontend/client/src/components/permission/PermissionGrantTab.test.tsx b/src/frontend/client/src/components/permission/PermissionGrantTab.test.tsx index ac5edbc029..881f2b7f1d 100644 --- a/src/frontend/client/src/components/permission/PermissionGrantTab.test.tsx +++ b/src/frontend/client/src/components/permission/PermissionGrantTab.test.tsx @@ -79,7 +79,7 @@ const context: ResourcePermissionContext = { }; function existing( - id: number, + id: string, overrides: Partial = {}, ): PermissionGrantAssignee { return { @@ -155,7 +155,7 @@ describe("F048 Client PermissionGrantTab", () => { resourceType="channel" resourceId="channel-1" context={context} - assignees={[existing(1), existing(2)]} + assignees={[existing("1"), existing("2")]} onSuccess={jest.fn()} />, ); @@ -225,8 +225,8 @@ describe("F048 Client PermissionGrantTab", () => { resourceId="channel-1" context={context} assignees={[ - existing(3, { protected: true, editable: false }), - existing(4, { scope: "INHERITED", editable: false }), + existing("3", { protected: true, editable: false }), + existing("4", { scope: "INHERITED", editable: false }), ]} onSuccess={jest.fn()} />, @@ -251,7 +251,7 @@ describe("F048 Client PermissionGrantTab", () => { resourceType="channel" resourceId="channel-1" context={context} - assignees={[existing(99)]} + assignees={[existing("99")]} onSuccess={jest.fn()} />, ); @@ -319,7 +319,7 @@ describe("F048 Client PermissionGrantTab", () => { resourceType="channel" resourceId="channel-1" context={context} - assignees={[existing(1)]} + assignees={[existing("1")]} onSuccess={jest.fn()} />, ); diff --git a/src/frontend/client/src/components/permission/PermissionGrantTab.tsx b/src/frontend/client/src/components/permission/PermissionGrantTab.tsx index 04d7e868fe..97bbe3d640 100644 --- a/src/frontend/client/src/components/permission/PermissionGrantTab.tsx +++ b/src/frontend/client/src/components/permission/PermissionGrantTab.tsx @@ -79,8 +79,8 @@ export function PermissionGrantTab({ ); const [selectedModelKey, setSelectedModelKey] = useState(""); const [internalIncludeChildren, setInternalIncludeChildren] = useState(false); - const [targetModels, setTargetModels] = useState>({}); - const [removedIds, setRemovedIds] = useState>(new Set()); + const [targetModels, setTargetModels] = useState>({}); + const [removedIds, setRemovedIds] = useState>(new Set()); const [queuedAdds, setQueuedAdds] = useState< PermissionGrantMutationChange[] >([]); diff --git a/src/frontend/client/src/components/permission/PermissionListTab.test.tsx b/src/frontend/client/src/components/permission/PermissionListTab.test.tsx index 9dbc341139..a71eec0942 100644 --- a/src/frontend/client/src/components/permission/PermissionListTab.test.tsx +++ b/src/frontend/client/src/components/permission/PermissionListTab.test.tsx @@ -48,7 +48,7 @@ const context: ResourcePermissionContext = { }; function assignee( - id: number, + id: string, source: "DIRECT" | "DEPARTMENT", overrides: Partial = {}, ): PermissionGrantAssignee { @@ -72,7 +72,7 @@ describe("F048 Client PermissionListTab", () => { { key: "standard-viewer", name: "Viewer", level: 1, active: true }, ]); mockedGetGrants.mockResolvedValue({ - data: [assignee(1, "DIRECT"), assignee(2, "DEPARTMENT")], + data: [assignee("1", "DIRECT"), assignee("2", "DEPARTMENT")], page_size: 2, has_more: true, next_cursor: "cursor-2", @@ -112,7 +112,7 @@ describe("F048 Client PermissionListTab", () => { mockedGetGrants .mockResolvedValueOnce({ data: [ - assignee(3, "DIRECT", { + assignee("3", "DIRECT", { protected: true, editable: false, scope: "INHERITED", @@ -124,7 +124,7 @@ describe("F048 Client PermissionListTab", () => { next_cursor: "cursor-2", }) .mockResolvedValueOnce({ - data: [assignee(4, "DIRECT", { subject: { type: "user", id: "8", name: "Bob" } })], + data: [assignee("4", "DIRECT", { subject: { type: "user", id: "8", name: "Bob" } })], page_size: 1, has_more: false, next_cursor: null, diff --git a/src/frontend/client/src/components/permission/PermissionListTab.tsx b/src/frontend/client/src/components/permission/PermissionListTab.tsx index 37d920804b..8cc7f46df5 100644 --- a/src/frontend/client/src/components/permission/PermissionListTab.tsx +++ b/src/frontend/client/src/components/permission/PermissionListTab.tsx @@ -24,17 +24,9 @@ import type { ResourceType, SubjectType, } from "~/api/permission"; -import { - AlertDialog, - AlertDialogAction, - AlertDialogCancel, - AlertDialogContent, - AlertDialogFooter, - AlertDialogHeader, - AlertDialogTitle, - Button, -} from "~/components/ui"; +import { Button } from "~/components/ui"; import { useLocalize } from "~/hooks"; +import { useConfirm } from "~/Providers"; import { SourceBadge } from "./SourceBadge"; interface PermissionListTabProps { @@ -223,6 +215,7 @@ export function PermissionListTab({ onMutationSuccess, }: PermissionListTabProps) { const localize = useLocalize(); + const confirm = useConfirm(); const [assignees, setAssignees] = useState([]); const [models, setModels] = useState([]); const [summary, setSummary] = useState(null); @@ -231,11 +224,9 @@ export function PermissionListTab({ const [hasMore, setHasMore] = useState(false); const [loading, setLoading] = useState(false); const [loadingMore, setLoadingMore] = useState(false); - const [pendingAssigneeId, setPendingAssigneeId] = useState( + const [pendingAssigneeId, setPendingAssigneeId] = useState( null, ); - const [removeTarget, setRemoveTarget] = - useState(null); const [failed, setFailed] = useState(false); useEffect(() => { @@ -382,10 +373,20 @@ export function PermissionListTab({ setFailed(true); } finally { setPendingAssigneeId(null); - setRemoveTarget(null); } }; + const handleRemove = async (assignee: PermissionGrantAssignee) => { + const confirmed = await confirm({ + variant: "destructive", + title: localize("com_permission.confirm_revoke"), + description: assignee.subject.name || assignee.subject.id, + confirmText: localize("com_permission.action_revoke"), + }); + if (!confirmed) return; + await mutateAssignee(assignee, { op: "REMOVE" }); + }; + const handleLoadMore = async () => { if (!nextCursor || loadingMore) return; setLoadingMore(true); @@ -457,7 +458,7 @@ export function PermissionListTab({ target_model_key: modelKey, }) } - onRemove={setRemoveTarget} + onRemove={(item) => void handleRemove(item)} /> ))} {visibleAssignees.length === 0 && ( @@ -484,33 +485,6 @@ export function PermissionListTab({ )} - - { - if (!nextOpen) setRemoveTarget(null); - }} - > - - - - {localize("com_permission.confirm_revoke")} - - - - {localize("cancel")} - { - if (removeTarget) { - void mutateAssignee(removeTarget, { op: "REMOVE" }); - } - }} - > - {localize("confirm")} - - - - ); } diff --git a/src/frontend/platform/src/components/bs-comp/permission/PermissionListTab.tsx b/src/frontend/platform/src/components/bs-comp/permission/PermissionListTab.tsx index cdd1a11e0f..692b3a09f5 100644 --- a/src/frontend/platform/src/components/bs-comp/permission/PermissionListTab.tsx +++ b/src/frontend/platform/src/components/bs-comp/permission/PermissionListTab.tsx @@ -213,7 +213,7 @@ export function PermissionListTab({ const [hasMore, setHasMore] = useState(false) const [loading, setLoading] = useState(false) const [loadingMore, setLoadingMore] = useState(false) - const [pendingAssigneeId, setPendingAssigneeId] = useState(null) + const [pendingAssigneeId, setPendingAssigneeId] = useState(null) const [failed, setFailed] = useState(false) useEffect(() => { diff --git a/src/frontend/platform/src/controllers/API/permission.ts b/src/frontend/platform/src/controllers/API/permission.ts index b43d399ebe..9e79f50b13 100644 --- a/src/frontend/platform/src/controllers/API/permission.ts +++ b/src/frontend/platform/src/controllers/API/permission.ts @@ -145,7 +145,7 @@ export interface PermissionGrantSource { } export interface PermissionGrantAssignee { - assignee_id: number + assignee_id: string assignee_version: number subject: PermissionGrantSubject model: GrantablePermissionModel @@ -185,13 +185,13 @@ export type PermissionGrantMutationChange = } | { op: "MOVE" - assignee_id: number + assignee_id: string expected_assignee_version: number target_model_key: string } | { op: "REMOVE" - assignee_id: number + assignee_id: string expected_assignee_version: number } diff --git a/src/frontend/platform/src/test/f048PermissionApi.test.ts b/src/frontend/platform/src/test/f048PermissionApi.test.ts index ad2eb2b283..bc38f38996 100644 --- a/src/frontend/platform/src/test/f048PermissionApi.test.ts +++ b/src/frontend/platform/src/test/f048PermissionApi.test.ts @@ -103,13 +103,13 @@ describe("F048 Platform permission API", () => { }, { op: "MOVE", - assignee_id: 91, + assignee_id: "91", expected_assignee_version: 2, target_model_key: "editor", }, { op: "REMOVE", - assignee_id: 92, + assignee_id: "92", expected_assignee_version: 3, }, ], diff --git a/src/frontend/platform/src/test/f048PermissionDialog.test.tsx b/src/frontend/platform/src/test/f048PermissionDialog.test.tsx index 08cc364abe..102c61f6cd 100644 --- a/src/frontend/platform/src/test/f048PermissionDialog.test.tsx +++ b/src/frontend/platform/src/test/f048PermissionDialog.test.tsx @@ -31,7 +31,7 @@ const customContext = { } const protectedAssignee = { - assignee_id: 81, + assignee_id: "81", assignee_version: 4, subject: { type: "user" as const, id: "3", name: "Creator" }, model: { diff --git a/src/frontend/platform/src/test/f048PermissionGrantTab.test.tsx b/src/frontend/platform/src/test/f048PermissionGrantTab.test.tsx index a5e83af36b..3338ec2a90 100644 --- a/src/frontend/platform/src/test/f048PermissionGrantTab.test.tsx +++ b/src/frontend/platform/src/test/f048PermissionGrantTab.test.tsx @@ -50,7 +50,7 @@ const context: ResourcePermissionContext = { } const editableAssignee: PermissionGrantAssignee = { - assignee_id: 41, + assignee_id: "41", assignee_version: 2, subject: { type: "user", id: "10", name: "Bob" }, model: { key: "viewer", name: "Viewer", level: 1, active: true }, @@ -63,7 +63,7 @@ const editableAssignee: PermissionGrantAssignee = { const protectedAssignee: PermissionGrantAssignee = { ...editableAssignee, - assignee_id: 42, + assignee_id: "42", assignee_version: 5, subject: { type: "user", id: "11", name: "Creator" }, model: { key: "owner", name: "Owner", level: 4, active: true }, @@ -196,7 +196,7 @@ describe("F048 PermissionGrantTab", () => { changes: [ { op: "MOVE", - assignee_id: 41, + assignee_id: "41", expected_assignee_version: 2, target_model_key: "editor", }, @@ -233,7 +233,7 @@ describe("F048 PermissionGrantTab", () => { changes: [ { op: "REMOVE", - assignee_id: 41, + assignee_id: "41", expected_assignee_version: 2, }, ], diff --git a/src/frontend/platform/src/test/f048PermissionRoster.test.tsx b/src/frontend/platform/src/test/f048PermissionRoster.test.tsx index a47808aaa8..8265957adc 100644 --- a/src/frontend/platform/src/test/f048PermissionRoster.test.tsx +++ b/src/frontend/platform/src/test/f048PermissionRoster.test.tsx @@ -26,7 +26,7 @@ const customContext: ResourcePermissionContext = { } const directAssignee = { - assignee_id: 101, + assignee_id: "101", assignee_version: 3, subject: { type: "user" as const, id: "7", name: "Alice" }, model: { @@ -43,7 +43,7 @@ const directAssignee = { } const departmentAssignee = { - assignee_id: 102, + assignee_id: "102", assignee_version: 4, subject: { type: "user" as const, id: "7", name: "Alice" }, model: { From 8860ced1fcf2ef32465064115af97125667f09d9 Mon Sep 17 00:00:00 2001 From: dolphin Date: Tue, 11 Aug 2026 12:26:08 +0800 Subject: [PATCH 02/38] fix(permission): scope Catalog publish reads and label the state a failed one leaves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 116 上发布 catalog 跑了 423 秒然后 500，日志里是 OpenFGA 的 `validation_error`。一次事故牵出四个问题，都在这条冷路径上。 **发布必然失败。** `read_active_release_keys` 只给了 user 和 relation，没给对象 类型。Read API 要求对象类型必填，且对象 id 与 user 不能同时为空，否则一律拒绝。 它在发布的最后一步 commit 才被调用，所以前面的活全白干，死在终点线上。 **读取量与库大小成正比，而不是与发布内容成正比。** staging 和校验各发一次 **不带过滤条件**的 Read，那是全库扫描，客户端每页 100 条 —— 77348 条 tuple 要 774 次往返，两次就是 1548 次强一致读。而发布真正要核对的只是自己那几百条 catalog tuple，涉及两种对象类型。改成按计划涉及的对象逐个读、并发 8 路。 116 实测：全量扫描 77348 条 / 774 次请求 / 206.8s；改后 active 指针 1 条 / 1 次 / 0.02s，模型发布标记 354 条 / 4 次 / 0.04s。语义未变，仍是同一批 tuple、 同样的强一致存在性比对。 **恢复路径依赖那个坏掉的函数。** commit 失败会转入 `_resolve_unknown_commit`， 而它第一行又调 `read_active_release_keys`，于是二次抛出、异常穿透整个 publish， `fail_closed` 根本没机会执行。发布前给 CURRENT release 上的写锁就此留在库里。 封锁本身是设计意图（失败即封锁，只能前向修复），但**封锁却不打标记**不是： release 停在 PROJECTING，没有 FAILED_CLOSED，没有事件，没有任何记录。运行中的 进程用着内存里已初始化的运行时照常服务，直到下一次重启才全线 500 —— 这次就是 隔了 4 天才发作。现在指针读不出来也会落进同一个带原因的终态。 **报错指错了方向。** 门禁被 latch 后一律报「Permission data migration is required」，而真实原因是「CURRENT Permission Catalog is write fenced」，害人去 找一个根本不存在的迁移。改为把 latch 时的真实原因透出来。 顺带给客户端加了本地校验：不合法的 Read 过滤在发出前就报清楚缺什么，而不是等 服务端回一个含糊的 validation_error。过滤规则是在真实 OpenFGA 上逐组合验证的。 测试：新增 8 个用例（客户端过滤校验、投影器只读计划内对象、失败必留标记、门禁 报真实原因）。test_f048_catalog_runtime 里的假 OpenFGA 原本只认精确对象匹配， 按线上语义补上了类型前缀过滤。全量权限测试 626 通过 / 13 失败，与改动前基线的 13 个完全同批，无新增。 Co-Authored-By: Claude Opus 5 (1M context) --- src/backend/bisheng/core/openfga/client.py | 12 ++ .../permission/application/catalog_api.py | 46 ++++-- .../permission/application/process_runtime.py | 16 +- .../domain/services/catalog_service.py | 40 ++++- .../test_f048_catalog_projector_reads.py | 100 ++++++++++++ .../permission/test_f048_catalog_runtime.py | 7 +- .../test/permission/test_f048_fga_client.py | 42 +++++ .../test_f048_publish_failure_is_labelled.py | 153 ++++++++++++++++++ 8 files changed, 398 insertions(+), 18 deletions(-) create mode 100644 src/backend/test/permission/test_f048_catalog_projector_reads.py create mode 100644 src/backend/test/permission/test_f048_publish_failure_is_labelled.py diff --git a/src/backend/bisheng/core/openfga/client.py b/src/backend/bisheng/core/openfga/client.py index ad3ce8138e..f52dd2c937 100644 --- a/src/backend/bisheng/core/openfga/client.py +++ b/src/backend/bisheng/core/openfga/client.py @@ -268,6 +268,12 @@ async def read_tuples( ) -> list[dict]: """Read tuples matching the given filter. + Pass no filter at all to walk the whole Store. Any other combination must + satisfy the Read API: the object type is mandatory, and the object id and + the user cannot both be empty. Callers used to learn this the hard way — + the server answers a filter it dislikes with a generic validation_error, + which surfaced as a 500 only after the surrounding work had already run. + Returns list of {"key": {"user": ..., "relation": ..., "object": ...}, "timestamp": ...}. """ tuple_key: dict[str, str] = {} @@ -277,6 +283,12 @@ async def read_tuples( tuple_key["relation"] = relation if object: tuple_key["object"] = object + if tuple_key: + object_type, separator, object_id = (object or "").partition(":") + if not object_type or not separator: + raise ValueError(f"OpenFGA read filter needs an object type, got object={object!r}") + if not object_id and not user: + raise ValueError("OpenFGA read filter needs an object id or a user, got neither") tuples: list[dict] = [] continuation_token: str | None = None while True: diff --git a/src/backend/bisheng/permission/application/catalog_api.py b/src/backend/bisheng/permission/application/catalog_api.py index f9907d9406..cb350ca280 100644 --- a/src/backend/bisheng/permission/application/catalog_api.py +++ b/src/backend/bisheng/permission/application/catalog_api.py @@ -2,6 +2,7 @@ from __future__ import annotations +import asyncio from collections.abc import AsyncIterator, Callable from contextlib import AbstractAsyncContextManager, asynccontextmanager from dataclasses import dataclass, replace @@ -78,6 +79,7 @@ ZERO_CHECKSUM = "0" * 64 HIGHER_CONSISTENCY = "HIGHER_CONSISTENCY" CATALOG_STAGE_BATCH_SIZE = 80 +CATALOG_READ_CONCURRENCY = 8 ACTIVE_OPERATION_STATUSES = ( "PREPARED", "STAGING", @@ -1087,10 +1089,7 @@ async def stage_model_releases( ) -> None: expected = self._expected_tuples(draft) await self._persist_plan(draft.release_id, expected) - present = { - (row["user"], row["relation"], row["object"]) - for row in await self._client.read_tuples(consistency=HIGHER_CONSISTENCY) - } + present = await self._read_present(expected) missing = [row for row in expected if (row["user"], row["relation"], row["object"]) not in present] for index in range(0, len(missing), CATALOG_STAGE_BATCH_SIZE): batch = missing[index : index + CATALOG_STAGE_BATCH_SIZE] @@ -1102,17 +1101,43 @@ async def run_model_tests( self, draft: CatalogDraftSnapshot, ) -> None: - expected = {(row["user"], row["relation"], row["object"]) for row in self._expected_tuples(draft)} - present = { - (row["user"], row["relation"], row["object"]) - for row in await self._client.read_tuples(consistency=HIGHER_CONSISTENCY) - } + planned = self._expected_tuples(draft) + expected = {(row["user"], row["relation"], row["object"]) for row in planned} + present = await self._read_present(planned) missing = expected - present if missing: raise PermissionProjectionFailedError(msg=f"Catalog staged tuple verification failed: {len(missing)}") if draft.model_release is None: raise PermissionPublishNotReadyError(msg="Catalog model release is missing") + async def _read_present( + self, + planned: list[dict[str, str]], + ) -> set[tuple[str, str, str]]: + """Read only the tuples this plan touches, one concrete object at a time. + + An unfiltered Read walks the whole Store at 100 tuples per request, so + checking a few hundred Catalog tuples against a 77k-tuple Store cost + ~774 round trips — twice per publish, at HIGHER_CONSISTENCY. Scoping the + reads to the planned objects makes the cost proportional to the plan + instead of the Store, and the objects are independent so they overlap. + """ + + objects = sorted({row["object"] for row in planned}) + if not objects: + return set() + semaphore = asyncio.Semaphore(CATALOG_READ_CONCURRENCY) + + async def read(object_key: str) -> list[dict]: + async with semaphore: + return await self._client.read_tuples( + object=object_key, + consistency=HIGHER_CONSISTENCY, + ) + + pages = await asyncio.gather(*(read(key) for key in objects)) + return {(row["user"], row["relation"], row["object"]) for page in pages for row in page} + async def arm_recent_marker( self, draft: CatalogDraftSnapshot, @@ -1149,9 +1174,12 @@ async def commit_active( return _commit_checksum(changes) async def read_active_release_keys(self) -> frozenset[str]: + # OpenFGA rejects a tuple_key without an object type, so the filter has + # to name the type even though the prefix check below already does. rows = await self._client.read_tuples( user="user:*", relation="active", + object="permission_catalog_release:", consistency=HIGHER_CONSISTENCY, ) prefix = "permission_catalog_release:" diff --git a/src/backend/bisheng/permission/application/process_runtime.py b/src/backend/bisheng/permission/application/process_runtime.py index 08fa1a0637..2f640fe04d 100644 --- a/src/backend/bisheng/permission/application/process_runtime.py +++ b/src/backend/bisheng/permission/application/process_runtime.py @@ -38,7 +38,7 @@ async def bind_catalog_runtime( async def heartbeat(self) -> bool: ... - async def mark_migration_required(self) -> None: ... + async def mark_migration_required(self, *, reason: str = ...) -> None: ... def readiness(self) -> dict: ... @@ -78,15 +78,21 @@ def register_f048_permission_runtime_context( async def initialize() -> ProcessPermissionRuntime: manager = app_context.get_context("openfga") client = await manager.async_get_instance() - if manager.readiness().get("migration_required"): - raise PermissionPublishNotReadyError(msg="Permission data migration is required") + readiness = manager.readiness() + if readiness.get("migration_required"): + # Report what actually latched the gate. A fenced Catalog left by a + # crashed publish reported "migration is required" here, which sent + # the operator looking for a migration that was never pending. + raise PermissionPublishNotReadyError( + msg=str(readiness.get("error") or "Permission data migration is required") + ) try: runtime = await initializer(client) except ( AuthorizationModelMismatchError, PermissionPublishNotReadyError, - ): - await manager.mark_migration_required() + ) as exc: + await manager.mark_migration_required(reason=str(exc) or "permission_data_migration_required") raise await bind_f048_process_runtime( manager, diff --git a/src/backend/bisheng/permission/domain/services/catalog_service.py b/src/backend/bisheng/permission/domain/services/catalog_service.py index 0da63e1bc1..6cee2fcfb0 100644 --- a/src/backend/bisheng/permission/domain/services/catalog_service.py +++ b/src/backend/bisheng/permission/domain/services/catalog_service.py @@ -437,7 +437,21 @@ async def _resolve_unknown_commit( original_error: Exception, allow_retry: bool, ) -> CatalogPublishOutcome: - active = await self._projector.read_active_release_keys() + try: + active = await self._projector.read_active_release_keys() + except Exception as exc: + # Reading the pointer is how this path tells "committed" from "never + # committed". If that read itself fails the publication is + # unresolvable, so land it in the fenced terminal state instead of + # letting the error escape — an escaping error leaves the CURRENT + # release fenced with no FAILED_CLOSED marker and no event, which is + # invisible until a restart takes the whole permission runtime down. + return await self._fail_closed( + context, + reason=( + f"Catalog active pointer is unreadable after commit: error={exc}, original_error={original_error}" + ), + ) old_key = context.current_release_key new_key = context.draft.release_key @@ -467,9 +481,29 @@ async def _resolve_unknown_commit( reconciled=True, ) - reason = ( - f"Catalog active pointer invariant violated after commit: active={sorted(active)}, error={original_error}" + await self._fail_closed( + context, + reason=( + f"Catalog active pointer invariant violated after commit: " + f"active={sorted(active)}, error={original_error}" + ), ) + + async def _fail_closed( + self, + context: CatalogPublishContext, + *, + reason: str, + ) -> CatalogPublishOutcome: + """Record the fenced terminal state, then raise. + + Publishing fences the CURRENT release and only a resolved commit lifts + that fence, so an unresolvable publication is meant to stay fenced. What + it must never do is stay fenced *unlabelled*: the release has to end up + FAILED_CLOSED with a reason, or the next process restart refuses to serve + permissions with nothing on record explaining why. + """ + await self._state.fail_closed(context, reason=reason) await self._emit( context, diff --git a/src/backend/test/permission/test_f048_catalog_projector_reads.py b/src/backend/test/permission/test_f048_catalog_projector_reads.py new file mode 100644 index 0000000000..daf58fed31 --- /dev/null +++ b/src/backend/test/permission/test_f048_catalog_projector_reads.py @@ -0,0 +1,100 @@ +"""Catalog publish reads OpenFGA in proportion to the plan, not the Store. + +Both staging and verification used to issue an unfiltered Read, which walks the +whole Store 100 tuples per request. On a 77k-tuple Store that is ~774 round +trips each, at HIGHER_CONSISTENCY — one observed publish spent 423s and then +died at its commit step, where a filter without an object type earned a generic +OpenFGA validation_error. +""" + +from __future__ import annotations + +import pytest + +from bisheng.permission.application.catalog_api import OpenFGACatalogProjector + + +class _RecordingClient: + """Answer Reads from a tiny Store and record exactly how they were filtered.""" + + def __init__(self, tuples: set[tuple[str, str, str]]) -> None: + self.store_id = "store-1" + self.model_id = "model-f048" + self.tuples = tuples + self.read_filters: list[dict[str, str | None]] = [] + + async def read_tuples( + self, + user: str | None = None, + relation: str | None = None, + object: str | None = None, + consistency: str | None = None, + ) -> list[dict]: + del consistency + self.read_filters.append({"user": user, "relation": relation, "object": object}) + if object is not None and not object.partition(":")[0]: + raise AssertionError(f"read filter without an object type: {object!r}") + if object is not None and object.endswith(":") and not user: + raise AssertionError("type-only object filter needs a user") + matched = [ + {"user": row_user, "relation": row_relation, "object": row_object} + for row_user, row_relation, row_object in sorted(self.tuples) + if (user is None or row_user == user) + and (relation is None or row_relation == relation) + and (object is None or row_object == object or (object.endswith(":") and row_object.startswith(object))) + ] + return matched + + +PLANNED = [ + {"user": "permission_model_release:r1", "relation": "release", "object": "permission_model:viewer"}, + {"user": "permission_catalog_release:c1", "relation": "catalog", "object": "permission_model_release:r1"}, + {"user": "user:*", "relation": "enabled_marker", "object": "permission_model_release:r1"}, +] + +# Everything a real Store also holds and a publish has no business reading. +UNRELATED = {("user:9", "ordinary_assignee", f"permission_grant:g{index}") for index in range(500)} + + +def _projector(client: _RecordingClient) -> OpenFGACatalogProjector: + return OpenFGACatalogProjector(client=client, marker=None) + + +@pytest.mark.asyncio +async def test_present_reads_only_the_planned_objects() -> None: + present_rows = {(row["user"], row["relation"], row["object"]) for row in PLANNED[:2]} + client = _RecordingClient(present_rows | UNRELATED) + + present = await _projector(client)._read_present(PLANNED) + + assert present == present_rows + # One Read per distinct planned object — never an unfiltered Store walk. + assert len(client.read_filters) == 2 + assert {entry["object"] for entry in client.read_filters} == { + "permission_model:viewer", + "permission_model_release:r1", + } + assert all(entry["object"] for entry in client.read_filters) + + +@pytest.mark.asyncio +async def test_present_is_empty_without_a_plan() -> None: + client = _RecordingClient(UNRELATED) + + assert await _projector(client)._read_present([]) == set() + assert client.read_filters == [] + + +@pytest.mark.asyncio +async def test_active_release_keys_filter_names_the_object_type() -> None: + client = _RecordingClient({("user:*", "active", "permission_catalog_release:c1")} | UNRELATED) + + assert await _projector(client).read_active_release_keys() == frozenset({"c1"}) + + assert client.read_filters == [ + { + "user": "user:*", + "relation": "active", + "object": "permission_catalog_release:", + } + ] diff --git a/src/backend/test/permission/test_f048_catalog_runtime.py b/src/backend/test/permission/test_f048_catalog_runtime.py index db8d3b6ec9..84221b335d 100644 --- a/src/backend/test/permission/test_f048_catalog_runtime.py +++ b/src/backend/test/permission/test_f048_catalog_runtime.py @@ -124,12 +124,17 @@ async def read_tuples( consistency: str | None = None, ) -> list[dict]: del consistency + # An object ending in ":" is a type filter, matching every id of that + # type — the same shape the real Read API accepts (and it requires a + # user alongside it, which this fake asserts rather than silently allows). + if object is not None and object.endswith(":") and not user: + raise AssertionError("type-only object filter needs a user") return [ {"user": item_user, "relation": item_relation, "object": item_object} for item_user, item_relation, item_object in sorted(self.tuples) if (user is None or item_user == user) and (relation is None or item_relation == relation) - and (object is None or item_object == object) + and (object is None or item_object == object or (object.endswith(":") and item_object.startswith(object))) ] @staticmethod diff --git a/src/backend/test/permission/test_f048_fga_client.py b/src/backend/test/permission/test_f048_fga_client.py index 065fa282bd..b57570b98e 100644 --- a/src/backend/test/permission/test_f048_fga_client.py +++ b/src/backend/test/permission/test_f048_fga_client.py @@ -270,3 +270,45 @@ def test_for_model_reuses_store_without_a_legacy_client(client: FGAClient) -> No assert migration_client.store_id == client.store_id assert migration_client.model_id == "model-target" assert migration_client is not client + + +@pytest.mark.asyncio +async def test_read_filter_requires_an_object_type( + client: FGAClient, +) -> None: + """A filtered Read without an object type is rejected before it is sent. + + OpenFGA answers such a filter with a generic validation_error. The Catalog + publish path issued one at its final commit step, so the failure only showed + up as a 500 after the whole publish had already run. + """ + + client._post = AsyncMock() + + with pytest.raises(ValueError, match="object type"): + await client.read_tuples(user="user:*", relation="active") + + with pytest.raises(ValueError, match="object type"): + await client.read_tuples(object="permission_catalog_release") + + client._post.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_read_filter_requires_an_object_id_or_a_user( + client: FGAClient, +) -> None: + """A type-only object filter is legal only when a user narrows it.""" + + client._post = AsyncMock() + with pytest.raises(ValueError, match="object id or a user"): + await client.read_tuples(object="permission_model_release:") + client._post.assert_not_awaited() + + client._post = AsyncMock(return_value={"tuples": []}) + assert await client.read_tuples(object="permission_model_release:", user="user:*") == [] + _, body = client._post.call_args.args + assert body["tuple_key"] == { + "user": "user:*", + "object": "permission_model_release:", + } diff --git a/src/backend/test/permission/test_f048_publish_failure_is_labelled.py b/src/backend/test/permission/test_f048_publish_failure_is_labelled.py new file mode 100644 index 0000000000..d2bb30032e --- /dev/null +++ b/src/backend/test/permission/test_f048_publish_failure_is_labelled.py @@ -0,0 +1,153 @@ +"""An unresolvable publish must leave a labelled state, not a silent fence. + +Publishing fences the CURRENT Catalog and only a resolved commit lifts that +fence, so staying fenced after an unresolvable commit is deliberate. Staying +fenced *unlabelled* is not: a crashed publish left the release fenced with no +FAILED_CLOSED marker and no event, which stayed invisible until the next restart +refused to initialize the permission runtime — reporting a data migration that +was never pending. +""" + +from __future__ import annotations + +import pytest + +from bisheng.common.errcode.permission import ( + PermissionProjectionFailedError, + PermissionPublishNotReadyError, +) +from bisheng.core.openfga.exceptions import FGAClientError +from bisheng.permission.application.process_runtime import ( + register_f048_permission_runtime_context, +) + + +class _StubProjector: + """Fail the active-pointer read the way a rejected Read filter did.""" + + def __init__(self, error: Exception) -> None: + self.error = error + self.read_calls = 0 + + async def read_active_release_keys(self): + self.read_calls += 1 + raise self.error + + +class _RecordingState: + def __init__(self) -> None: + self.fail_closed_calls: list[str] = [] + + async def fail_closed(self, context, *, reason: str) -> None: + del context + self.fail_closed_calls.append(reason) + + +def _service(projector: _StubProjector, state: _RecordingState): + from bisheng.permission.domain.services.catalog_service import CatalogService + + service = CatalogService.__new__(CatalogService) + service._projector = projector + service._state = state + + async def _emit(context, *, status, error=None): + del context, status, error + + service._emit = _emit + return service + + +@pytest.mark.asyncio +async def test_unreadable_active_pointer_still_fails_closed() -> None: + projector = _StubProjector( + FGAClientError('OpenFGA 400: {"code":"validation_error","message":"object type is required"}') + ) + state = _RecordingState() + service = _service(projector, state) + + with pytest.raises(PermissionProjectionFailedError): + await service._resolve_unknown_commit( + object(), + original_error=RuntimeError("commit blew up"), + allow_retry=True, + ) + + assert projector.read_calls == 1 + # The release is recorded as FAILED_CLOSED, carrying both errors, instead of + # the read error escaping and leaving the fence unexplained. + assert len(state.fail_closed_calls) == 1 + reason = state.fail_closed_calls[0] + assert "unreadable" in reason + assert "validation_error" in reason + assert "commit blew up" in reason + + +class _StubManager: + def __init__(self, readiness: dict) -> None: + self._readiness = readiness + self.marked: list[str] = [] + + def readiness(self) -> dict: + return self._readiness + + async def mark_migration_required(self, *, reason: str = "permission_data_migration_required") -> None: + self.marked.append(reason) + + async def async_get_instance(self): + return object() + + +def _capture_initializer(monkeypatch, manager: _StubManager): + """Register the runtime context against a stub manager and return its init.""" + + from bisheng.core.context.manager import app_context + + def get_context(name: str): + if name == "openfga": + return manager + raise KeyError(name) + + monkeypatch.setattr(app_context, "get_context", get_context) + + captured: dict = {} + + def register_context(context, **kwargs): + del kwargs + captured["init"] = context.init_func + + monkeypatch.setattr(app_context, "register_context", register_context) + + async def initializer(client): + raise AssertionError("the initializer must not run while the gate is latched") + + register_f048_permission_runtime_context(initializer) + return captured["init"] + + +@pytest.mark.asyncio +async def test_latched_gate_reports_the_reason_that_latched_it(monkeypatch) -> None: + manager = _StubManager( + { + "migration_required": True, + "error": "CURRENT Permission Catalog is write fenced", + } + ) + initialize = _capture_initializer(monkeypatch, manager) + + with pytest.raises(PermissionPublishNotReadyError) as excinfo: + await initialize() + + # Not the generic "migration is required", which sent an operator hunting a + # migration that was never pending. + assert "write fenced" in str(excinfo.value) + + +@pytest.mark.asyncio +async def test_gate_falls_back_when_no_reason_was_recorded(monkeypatch) -> None: + manager = _StubManager({"migration_required": True, "error": None}) + initialize = _capture_initializer(monkeypatch, manager) + + with pytest.raises(PermissionPublishNotReadyError) as excinfo: + await initialize() + + assert "migration is required" in str(excinfo.value) From c0a6b7e364a905c28d587d09146f388298600420 Mon Sep 17 00:00:00 2001 From: dolphin Date: Tue, 11 Aug 2026 12:28:32 +0800 Subject: [PATCH 03/38] style(permission): render the publish impact expiry as local time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 发布影响确认弹窗直接把后端的 ISO 串塞进文案，显示成 `2026-08-11T03:27:14.567728+00:00`。不只是不好看：那是 UTC，对 CST 用户比本地 时间早 8 小时，而这个影响窗口后端只给 10 分钟，照它判断还剩多久会直接误判成 早已过期。改为按本地时间渲染成 `2026-08-11 11:27:14`，窗口只有 10 分钟所以保留 到秒；无法解析的值原样返回。 Co-Authored-By: Claude Opus 5 (1M context) --- .../components/permission/ImpactDialog.tsx | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/frontend/platform/src/pages/SystemPage/components/permission/ImpactDialog.tsx b/src/frontend/platform/src/pages/SystemPage/components/permission/ImpactDialog.tsx index 61f4281b96..b753caf98b 100644 --- a/src/frontend/platform/src/pages/SystemPage/components/permission/ImpactDialog.tsx +++ b/src/frontend/platform/src/pages/SystemPage/components/permission/ImpactDialog.tsx @@ -11,6 +11,7 @@ import type { PermissionCatalogDraft, PublishPermissionCatalogDraftRequest, } from "@/controllers/API/permission" +import { formatDate } from "@/util/utils" import { AlertTriangle } from "lucide-react" import { useState } from "react" import { useTranslation } from "react-i18next" @@ -46,6 +47,15 @@ function ImpactMetric({ label, value, testId }: ImpactMetricProps) { ) } +// The backend sends UTC ISO-8601 with microseconds; rendering it raw showed +// both an unreadable string and a time 8 hours off for CST operators. The +// impact window is only 10 minutes, so keep seconds. +function formatExpiry(iso: string): string { + const parsed = new Date(iso) + if (Number.isNaN(parsed.getTime())) return iso + return formatDate(parsed, "yyyy-MM-dd HH:mm:ss") +} + function createPublishIdempotencyKey(): string { return `catalog-publish-${Date.now()}-${Math.random() .toString(36) @@ -155,7 +165,7 @@ export function ImpactDialog({ )}

- {t("impact.expiresAt", { value: draft.impact.expires_at })} + {t("impact.expiresAt", { value: formatExpiry(draft.impact.expires_at) })}

From be1e914064a30abd356d2bcab93d6a2065d8b65f Mon Sep 17 00:00:00 2001 From: dolphin Date: Tue, 11 Aug 2026 15:02:16 +0800 Subject: [PATCH 04/38] fix(permission): let one Catalog draft carry the whole edit batch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 「拖拽完就保存了」是合理预期，但动作等级看板不是这么工作的：每拖一次就以 **当前生效版本**为基准新建一份草案，而一份草案只装一个改动。连改三处会得到三份 互不相干的草案，点发布只发布最后一份，**前两处即使走完完整发布流程也会静默丢失**。 库里因此堆了 45 份废弃草案，且前后端都没有删除接口。 而且这个 tab 上根本没有发布按钮 —— 只有一个描边小按钮「查看影响」，真正的 「确认发布」藏在它弹出的对话框里；页面上唯一的提示写的是「预计影响 N 个资源」， 只讲影响量，从头到尾没有一个字说尚未发布（整个权限模块搜不到「未发布」字样）。 所以用户拖完就走，改动全丢。 后端：`CatalogDraftRequest.change` 改为 `changes` 元组，`_apply_change` 改为 `_apply_changes`，把整批改动折叠到基准版本上再做一次校验。校验放在折叠之后而不是 每步之后——一批编辑可能途经某个它根本不会发布的中间状态，只有真正要发布的那个 状态需要成立。 前端：编辑改为即时落本地、不发请求；待发布改动由「本地状态与已发布版本的差异」 推导，所以把卡片拖回原处会自动撤销该改动，而不是再排一条反向的。头部换成 「发布更改」主按钮 + 「放弃更改」，提示条改为「有 N 项改动尚未发布」，草案生成 失败会明确报错（原先是 `void` 掉的未处理拒绝，静默无声）。 测试：后端新增 2 个（整批折叠、同一动作后写覆盖先写）；前端看板用例按新行为重写 （编辑不触网、发布时一次提交全部、移回原处撤销改动、放弃更改、草案失败提示）。 后端 628 通过 / 13 失败与基线同批；前端 22 个相关用例全过，tsc-strict 与 eslint 干净。`pnpm check-i18n` 仍失败，但基线一模一样（250xx 权限错误码缺前端文案，与本 次无关）。 Co-Authored-By: Claude Opus 5 (1M context) --- .../permission/application/catalog_api.py | 178 ++++++++++-------- .../bisheng/permission/domain/schemas/f048.py | 5 +- .../test/permission/test_f048_catalog_api.py | 26 +-- .../permission/test_f048_catalog_runtime.py | 136 +++++++++++-- .../public/locales/en-US/permission.json | 7 +- .../public/locales/ja/permission.json | 7 +- .../public/locales/zh-Hans/permission.json | 7 +- .../src/controllers/API/permission.ts | 2 +- .../components/RolesAndPermissions.tsx | 6 +- .../permission/ActionLevelBoard.tsx | 153 ++++++++------- .../components/permission/ModelEditor.tsx | 14 +- .../src/test/f048ActionLevelBoard.test.tsx | 117 +++++++++--- .../src/test/f048ModelEditor.test.tsx | 56 +++--- .../src/test/f048PermissionApi.test.ts | 12 +- .../src/test/f048RolesAndPermissions.test.tsx | 28 +-- 15 files changed, 497 insertions(+), 257 deletions(-) diff --git a/src/backend/bisheng/permission/application/catalog_api.py b/src/backend/bisheng/permission/application/catalog_api.py index cb350ca280..97089784b6 100644 --- a/src/backend/bisheng/permission/application/catalog_api.py +++ b/src/backend/bisheng/permission/application/catalog_api.py @@ -1348,8 +1348,8 @@ async def create_draft( before = await self._state.load_snapshot(reservation.predecessor_id) if before.action_release is None or before.model_release is None: raise PermissionPublishNotReadyError() - actions, customs, standard_policy = await self._apply_change( - request.change, + actions, customs, standard_policy = await self._apply_changes( + request.changes, before, ) try: @@ -1482,15 +1482,22 @@ async def _release_payload( "published_at": (_as_utc(row.published_at).isoformat() if row.published_at is not None else None), } - async def _apply_change( + async def _apply_changes( self, - change: CatalogChangeRequest, + changes: tuple[CatalogChangeRequest, ...], before: CatalogDraftSnapshot, ) -> tuple[ tuple[CatalogAction, ...], tuple[CustomModelSelection, ...], dict[str, bool], ]: + """Fold the whole edit batch onto the base release, then validate once. + + Validation runs on the resulting state rather than after each change: a + batch may pass through an intermediate arrangement it never publishes, + and only the state that actually ships has to hold. + """ + assert before.action_release is not None assert before.model_release is not None actions = list(before.action_release.actions) @@ -1508,90 +1515,95 @@ async def _apply_change( } standard_by_key = {model.model_key: model for model in before.model_release.models if model.kind == "STANDARD"} standard_policy = {key: model.allow_same_level for key, model in standard_by_key.items()} - kind = change.type - if kind in { - CatalogChangeType.ASSIGN_ACTION_LEVEL, - CatalogChangeType.SET_ACTION_ACTIVE, - }: - index = next( - (index for index, action in enumerate(actions) if action.code == change.action_code), - None, - ) - if index is None: - raise InvalidCatalogActionError() - if kind == CatalogChangeType.ASSIGN_ACTION_LEVEL: - actions[index] = replace( - actions[index], - level=(int(change.level) if change.level is not None else None), - ) - elif change.active is None: - raise InvalidCatalogActionError() - else: - actions[index] = replace( - actions[index], - active=change.active, + touched_standard_keys: set[str] = set() + for change in changes: + kind = change.type + if kind in { + CatalogChangeType.ASSIGN_ACTION_LEVEL, + CatalogChangeType.SET_ACTION_ACTIVE, + }: + index = next( + (index for index, action in enumerate(actions) if action.code == change.action_code), + None, ) - elif kind == CatalogChangeType.CREATE_MODEL: - key = change.model_key or uuid4().hex - if key in custom_by_key or key in standard_by_key or not change.name or not change.action_codes: - raise PermissionModelStateConflictError() - custom_by_key[key] = CustomModelSelection( - model_key=key, - name=change.name, - action_codes=change.action_codes, - active=change.active is not False, - allow_same_level=bool(change.allow_same_level), - ) - elif kind == CatalogChangeType.UPDATE_MODEL: - model = self._custom_model(change.model_key, custom_by_key) - custom_by_key[model.model_key] = replace( - model, - name=change.name if change.name is not None else model.name, - action_codes=(change.action_codes if change.action_codes is not None else model.action_codes), - active=(change.active if change.active is not None else model.active), - allow_same_level=( - change.allow_same_level if change.allow_same_level is not None else model.allow_same_level - ), - ) - elif kind == CatalogChangeType.SET_MODEL_ACTIVE: - if change.active is None: - raise PermissionModelStateConflictError() - model = self._custom_model(change.model_key, custom_by_key) - custom_by_key[model.model_key] = replace( - model, - active=change.active, - ) - elif kind == CatalogChangeType.DELETE_MODEL: - model = self._custom_model(change.model_key, custom_by_key) - derived = next(item for item in before.model_release.models if item.model_key == model.model_key) - references = await self._state.grant_references() - try: - ensure_model_deletable( - derived, - reference_count=len(references.get(model.model_key, ())), + if index is None: + raise InvalidCatalogActionError() + if kind == CatalogChangeType.ASSIGN_ACTION_LEVEL: + actions[index] = replace( + actions[index], + level=(int(change.level) if change.level is not None else None), + ) + elif change.active is None: + raise InvalidCatalogActionError() + else: + actions[index] = replace( + actions[index], + active=change.active, + ) + elif kind == CatalogChangeType.CREATE_MODEL: + key = change.model_key or uuid4().hex + if key in custom_by_key or key in standard_by_key or not change.name or not change.action_codes: + raise PermissionModelStateConflictError() + custom_by_key[key] = CustomModelSelection( + model_key=key, + name=change.name, + action_codes=change.action_codes, + active=change.active is not False, + allow_same_level=bool(change.allow_same_level), ) - except ValueError as exc: - raise PermissionModelStateConflictError( - exception=exc, - msg=str(exc), - ) from exc - del custom_by_key[model.model_key] - elif kind == CatalogChangeType.SET_ALLOW_SAME_LEVEL: - if change.allow_same_level is None or not change.model_key: - raise PermissionModelStateConflictError() - if change.model_key in standard_by_key: - standard_policy[change.model_key] = change.allow_same_level - else: - model = self._custom_model( - change.model_key, - custom_by_key, + elif kind == CatalogChangeType.UPDATE_MODEL: + model = self._custom_model(change.model_key, custom_by_key) + custom_by_key[model.model_key] = replace( + model, + name=change.name if change.name is not None else model.name, + action_codes=(change.action_codes if change.action_codes is not None else model.action_codes), + active=(change.active if change.active is not None else model.active), + allow_same_level=( + change.allow_same_level if change.allow_same_level is not None else model.allow_same_level + ), ) + elif kind == CatalogChangeType.SET_MODEL_ACTIVE: + if change.active is None: + raise PermissionModelStateConflictError() + model = self._custom_model(change.model_key, custom_by_key) custom_by_key[model.model_key] = replace( model, - allow_same_level=change.allow_same_level, + active=change.active, ) - else: - raise InvalidCatalogActionError() + elif kind == CatalogChangeType.DELETE_MODEL: + model = self._custom_model(change.model_key, custom_by_key) + derived = next(item for item in before.model_release.models if item.model_key == model.model_key) + references = await self._state.grant_references() + try: + ensure_model_deletable( + derived, + reference_count=len(references.get(model.model_key, ())), + ) + except ValueError as exc: + raise PermissionModelStateConflictError( + exception=exc, + msg=str(exc), + ) from exc + del custom_by_key[model.model_key] + elif kind == CatalogChangeType.SET_ALLOW_SAME_LEVEL: + if change.allow_same_level is None or not change.model_key: + raise PermissionModelStateConflictError() + if change.model_key in standard_by_key: + standard_policy[change.model_key] = change.allow_same_level + else: + model = self._custom_model( + change.model_key, + custom_by_key, + ) + custom_by_key[model.model_key] = replace( + model, + allow_same_level=change.allow_same_level, + ) + else: + raise InvalidCatalogActionError() + if change.model_key and change.model_key in standard_by_key: + if change.type is not CatalogChangeType.SET_ALLOW_SAME_LEVEL: + touched_standard_keys.add(change.model_key) try: action_release = derive_action_release(actions) derive_permission_models( @@ -1600,7 +1612,7 @@ async def _apply_change( standard_allow_same_level=standard_policy, ) except ValueError as exc: - if change.model_key in standard_by_key and kind not in {CatalogChangeType.SET_ALLOW_SAME_LEVEL}: + if touched_standard_keys: raise ImmutableStandardModelError( exception=exc, msg=str(exc), diff --git a/src/backend/bisheng/permission/domain/schemas/f048.py b/src/backend/bisheng/permission/domain/schemas/f048.py index 53b97736eb..749d2e729d 100644 --- a/src/backend/bisheng/permission/domain/schemas/f048.py +++ b/src/backend/bisheng/permission/domain/schemas/f048.py @@ -186,7 +186,10 @@ class CatalogChangeRequest(StrictRequestModel): class CatalogDraftRequest(StrictRequestModel): idempotency_key: str = Field(min_length=1, max_length=64) base_release_id: int = Field(gt=0) - change: CatalogChangeRequest + # A draft carries the whole edit batch. One change per draft forced the UI to + # open a fresh draft off the CURRENT release for every edit, so publishing + # applied only the last one and silently dropped the rest. + changes: tuple[CatalogChangeRequest, ...] = Field(min_length=1, max_length=50) class CatalogImpactDTO(BaseModel): diff --git a/src/backend/test/permission/test_f048_catalog_api.py b/src/backend/test/permission/test_f048_catalog_api.py index b321f54cf1..abcf12d99a 100644 --- a/src/backend/test/permission/test_f048_catalog_api.py +++ b/src/backend/test/permission/test_f048_catalog_api.py @@ -99,11 +99,13 @@ def test_catalog_get_and_draft_round_trip_use_unified_response() -> None: json={ "idempotency_key": "draft-1", "base_release_id": 12, - "change": { - "type": "ASSIGN_ACTION_LEVEL", - "action_code": "edit", - "level": 2, - }, + "changes": [ + { + "type": "ASSIGN_ACTION_LEVEL", + "action_code": "edit", + "level": 2, + } + ], }, ).json() fetched = client.get("/api/v1/permissions/catalog/drafts/13").json() @@ -143,12 +145,14 @@ def test_catalog_semantic_errors_are_translated_to_unified_codes() -> None: json={ "idempotency_key": "draft-invalid", "base_release_id": 12, - "change": { - "type": "UPDATE_MODEL", - "model_key": "owner", - "name": "forbidden", - "action_codes": ["unknown-action"], - }, + "changes": [ + { + "type": "UPDATE_MODEL", + "model_key": "owner", + "name": "forbidden", + "action_codes": ["unknown-action"], + } + ], }, ).json() assert body["status_code"] == expected diff --git a/src/backend/test/permission/test_f048_catalog_runtime.py b/src/backend/test/permission/test_f048_catalog_runtime.py index 84221b335d..a55434134b 100644 --- a/src/backend/test/permission/test_f048_catalog_runtime.py +++ b/src/backend/test/permission/test_f048_catalog_runtime.py @@ -345,10 +345,12 @@ async def test_action_level_draft_rebuilds_every_standard_and_custom_model( request=CatalogDraftRequest( idempotency_key="raise-edit", base_release_id=int(current.id), - change=CatalogChangeRequest( - type=CatalogChangeType.ASSIGN_ACTION_LEVEL, - action_code="edit", - level=3, + changes=( + CatalogChangeRequest( + type=CatalogChangeType.ASSIGN_ACTION_LEVEL, + action_code="edit", + level=3, + ), ), ), operator_id=7, @@ -451,10 +453,12 @@ async def test_catalog_publish_allows_visibility_only_grant_after_action_level_c request=CatalogDraftRequest( idempotency_key="raise-download", base_release_id=int(current.id), - change=CatalogChangeRequest( - type=CatalogChangeType.ASSIGN_ACTION_LEVEL, - action_code="download", - level=2, + changes=( + CatalogChangeRequest( + type=CatalogChangeType.ASSIGN_ACTION_LEVEL, + action_code="download", + level=2, + ), ), ), operator_id=7, @@ -491,10 +495,12 @@ async def test_catalog_publish_stages_complete_release_and_switches_once( request=CatalogDraftRequest( idempotency_key="disable-share", base_release_id=int(current.id), - change=CatalogChangeRequest( - type=CatalogChangeType.SET_ACTION_ACTIVE, - action_code="share", - active=False, + changes=( + CatalogChangeRequest( + type=CatalogChangeType.SET_ACTION_ACTIVE, + action_code="share", + active=False, + ), ), ), operator_id=7, @@ -567,10 +573,12 @@ async def test_catalog_create_is_idempotent_and_current_shape_is_complete( request = CatalogDraftRequest( idempotency_key="same-draft", base_release_id=int(current.id), - change=CatalogChangeRequest( - type=CatalogChangeType.SET_ALLOW_SAME_LEVEL, - model_key="manager", - allow_same_level=True, + changes=( + CatalogChangeRequest( + type=CatalogChangeType.SET_ALLOW_SAME_LEVEL, + model_key="manager", + allow_same_level=True, + ), ), ) @@ -829,3 +837,99 @@ async def test_inherited_roster_uses_nearest_custom_ancestor( (30, "LOCAL", None), ] assert has_more is False + + +async def test_draft_folds_every_change_in_the_batch( + session_factory: SessionFactory, +) -> None: + """A batch must publish all of its edits, not just the last one. + + The board used to open a fresh draft off the CURRENT release per edit, so a + session of three tweaks produced three one-change drafts and publishing any + of them silently dropped the other two. + """ + + fga = InMemoryCatalogFGA() + marker = FakeCatalogMarker() + current = await _seed_current(session_factory, fga) + api = _api(session_factory, fga, marker) + + draft = await api.create_draft( + request=CatalogDraftRequest( + idempotency_key="batch-of-three", + base_release_id=int(current.id), + changes=( + CatalogChangeRequest( + type=CatalogChangeType.ASSIGN_ACTION_LEVEL, + action_code="edit", + level=3, + ), + CatalogChangeRequest( + type=CatalogChangeType.ASSIGN_ACTION_LEVEL, + action_code="rename", + level=4, + ), + CatalogChangeRequest( + type=CatalogChangeType.SET_ACTION_ACTIVE, + action_code="unpublish", + active=False, + ), + ), + ), + operator_id=7, + ) + + async with session_factory() as session: + rows = list( + ( + await session.execute( + select(PermissionAction).where(PermissionAction.catalog_release_id == draft["draft_id"]) + ) + ).scalars() + ) + by_code = {row.code: row for row in rows} + assert by_code["edit"].level == 3 + assert by_code["rename"].level == 4 + assert by_code["unpublish"].active is False + # Untouched actions keep the base release's values. + assert by_code["delete"].level == 4 + + +async def test_a_later_change_in_the_batch_wins_over_an_earlier_one( + session_factory: SessionFactory, +) -> None: + fga = InMemoryCatalogFGA() + marker = FakeCatalogMarker() + current = await _seed_current(session_factory, fga) + api = _api(session_factory, fga, marker) + + draft = await api.create_draft( + request=CatalogDraftRequest( + idempotency_key="batch-overwrite", + base_release_id=int(current.id), + changes=( + CatalogChangeRequest( + type=CatalogChangeType.ASSIGN_ACTION_LEVEL, + action_code="edit", + level=3, + ), + CatalogChangeRequest( + type=CatalogChangeType.ASSIGN_ACTION_LEVEL, + action_code="edit", + level=4, + ), + ), + ), + operator_id=7, + ) + + async with session_factory() as session: + row = ( + await session.execute( + select(PermissionAction).where( + PermissionAction.catalog_release_id == draft["draft_id"], + PermissionAction.code == "edit", + ) + ) + ).scalar_one() + assert row.level == 4 diff --git a/src/frontend/platform/public/locales/en-US/permission.json b/src/frontend/platform/public/locales/en-US/permission.json index 7bd8c808d1..cf004a2b06 100644 --- a/src/frontend/platform/public/locales/en-US/permission.json +++ b/src/frontend/platform/public/locales/en-US/permission.json @@ -68,7 +68,12 @@ "change": "Change action level", "active": "Action status", "inactive": "Inactive", - "empty": "No actions in this area" + "empty": "No actions in this area", + "pendingChanges_one": "1 change is not published yet", + "pendingChanges_other": "{{count}} changes are not published yet", + "publishChanges": "Publish changes", + "discardChanges": "Discard changes", + "draftFailed": "Could not prepare the change draft. Try again." }, "model": { "title": "Permission Model", diff --git a/src/frontend/platform/public/locales/ja/permission.json b/src/frontend/platform/public/locales/ja/permission.json index c2bf46a369..90aba80ed6 100644 --- a/src/frontend/platform/public/locales/ja/permission.json +++ b/src/frontend/platform/public/locales/ja/permission.json @@ -68,7 +68,12 @@ "change": "アクションレベルを変更", "active": "アクション状態", "inactive": "無効", - "empty": "この領域にアクションはありません" + "empty": "この領域にアクションはありません", + "pendingChanges_one": "未公開の変更が 1 件あります", + "pendingChanges_other": "未公開の変更が {{count}} 件あります", + "publishChanges": "変更を公開", + "discardChanges": "変更を破棄", + "draftFailed": "変更ドラフトの作成に失敗しました。再試行してください。" }, "model": { "title": "権限モデル", diff --git a/src/frontend/platform/public/locales/zh-Hans/permission.json b/src/frontend/platform/public/locales/zh-Hans/permission.json index 9ceedf189b..217dc017cf 100644 --- a/src/frontend/platform/public/locales/zh-Hans/permission.json +++ b/src/frontend/platform/public/locales/zh-Hans/permission.json @@ -68,7 +68,12 @@ "change": "调整动作等级", "active": "动作状态", "inactive": "已停用", - "empty": "此区域暂无动作" + "empty": "此区域暂无动作", + "pendingChanges_one": "有 1 项改动尚未发布", + "pendingChanges_other": "有 {{count}} 项改动尚未发布", + "publishChanges": "发布更改", + "discardChanges": "放弃更改", + "draftFailed": "生成变更草案失败，请重试" }, "model": { "title": "权限模型", diff --git a/src/frontend/platform/src/controllers/API/permission.ts b/src/frontend/platform/src/controllers/API/permission.ts index 9e79f50b13..2651852a12 100644 --- a/src/frontend/platform/src/controllers/API/permission.ts +++ b/src/frontend/platform/src/controllers/API/permission.ts @@ -83,7 +83,7 @@ export interface PermissionCatalogChange { export interface CreatePermissionCatalogDraftRequest { idempotency_key: string base_release_id: number - change: PermissionCatalogChange + changes: PermissionCatalogChange[] } export interface PermissionCatalogImpact { diff --git a/src/frontend/platform/src/pages/SystemPage/components/RolesAndPermissions.tsx b/src/frontend/platform/src/pages/SystemPage/components/RolesAndPermissions.tsx index 07c1de9029..667c4ef7f0 100644 --- a/src/frontend/platform/src/pages/SystemPage/components/RolesAndPermissions.tsx +++ b/src/frontend/platform/src/pages/SystemPage/components/RolesAndPermissions.tsx @@ -68,7 +68,7 @@ interface ModelCatalogPanelProps { selectedModelKey: string | null onSelectModel: (modelKey: string) => void onCreateDraft: ( - change: PermissionCatalogChange, + changes: PermissionCatalogChange[], ) => Promise onReviewImpact: (draft: PermissionCatalogDraft) => void } @@ -205,13 +205,13 @@ export function RolesAndPermissions() { }, [isPlatformSuperAdmin, loadCatalog]) const handleCreateDraft = async ( - change: PermissionCatalogChange, + changes: PermissionCatalogChange[], ): Promise => { if (!catalog) throw new Error("permission Catalog is not loaded") return await createPermissionCatalogDraftApi({ idempotency_key: createIdempotencyKey("catalog-draft"), base_release_id: catalog.id, - change, + changes, }) } diff --git a/src/frontend/platform/src/pages/SystemPage/components/permission/ActionLevelBoard.tsx b/src/frontend/platform/src/pages/SystemPage/components/permission/ActionLevelBoard.tsx index d5219ea3bc..83ba86c216 100644 --- a/src/frontend/platform/src/pages/SystemPage/components/permission/ActionLevelBoard.tsx +++ b/src/frontend/platform/src/pages/SystemPage/components/permission/ActionLevelBoard.tsx @@ -15,7 +15,7 @@ interface ActionLevelBoardProps { actions: PermissionCatalogAction[] disabled?: boolean onCreateDraft: ( - change: PermissionCatalogChange, + changes: PermissionCatalogChange[], ) => Promise onReviewImpact: (draft: PermissionCatalogDraft) => void } @@ -50,10 +50,10 @@ export function ActionLevelBoard({ const normalizedActions = useMemo(() => uniqueActions(actions), [actions]) const [levels, setLevels] = useState>({}) const [activeStates, setActiveStates] = useState>({}) - const [pendingCode, setPendingCode] = useState(null) - const [draft, setDraft] = useState(null) + const [submitting, setSubmitting] = useState(false) + const [draftFailed, setDraftFailed] = useState(false) - useEffect(() => { + const resetToRelease = () => { setLevels( Object.fromEntries( normalizedActions.map((action) => [action.code, action.level]), @@ -64,56 +64,60 @@ export function ActionLevelBoard({ normalizedActions.map((action) => [action.code, action.active]), ), ) - setDraft(null) - }, [normalizedActions]) + setDraftFailed(false) + } - const createDraft = async ( - actionCode: string, - change: PermissionCatalogChange, - applyLocalState: () => void, - ) => { - if (disabled || pendingCode) return - setPendingCode(actionCode) - try { - const nextDraft = await onCreateDraft(change) - applyLocalState() - setDraft(nextDraft) - } finally { - setPendingCode(null) + useEffect(resetToRelease, [normalizedActions]) + + // Derived from the diff against the published release rather than accumulated + // per edit, so moving a card back where it came from drops the change instead + // of queueing a second one. + const pendingChanges = useMemo(() => { + const changes: PermissionCatalogChange[] = [] + for (const action of normalizedActions) { + const level = levels[action.code] + if (level !== undefined && level !== action.level) { + changes.push({ + type: "ASSIGN_ACTION_LEVEL", + action_code: action.code, + level, + }) + } + const active = activeStates[action.code] + if (active !== undefined && active !== action.active) { + changes.push({ + type: "SET_ACTION_ACTIVE", + action_code: action.code, + active, + }) + } } - } + return changes + }, [normalizedActions, levels, activeStates]) - const handleLevelChange = ( - actionCode: string, - level: ActionLevelValue, - ) => { - if (levels[actionCode] === level) return - void createDraft( - actionCode, - { - type: "ASSIGN_ACTION_LEVEL", - action_code: actionCode, - level, - }, - () => setLevels((current) => ({ ...current, [actionCode]: level })), - ) + const handleLevelChange = (actionCode: string, level: ActionLevelValue) => { + if (disabled || submitting || levels[actionCode] === level) return + setDraftFailed(false) + setLevels((current) => ({ ...current, [actionCode]: level })) } const handleActiveChange = (actionCode: string, active: boolean) => { - if (activeStates[actionCode] === active) return - void createDraft( - actionCode, - { - type: "SET_ACTION_ACTIVE", - action_code: actionCode, - active, - }, - () => - setActiveStates((current) => ({ - ...current, - [actionCode]: active, - })), - ) + if (disabled || submitting || activeStates[actionCode] === active) return + setDraftFailed(false) + setActiveStates((current) => ({ ...current, [actionCode]: active })) + } + + const handlePublishChanges = async () => { + if (submitting || pendingChanges.length === 0) return + setSubmitting(true) + setDraftFailed(false) + try { + onReviewImpact(await onCreateDraft(pendingChanges)) + } catch { + setDraftFailed(true) + } finally { + setSubmitting(false) + } } return ( @@ -130,32 +134,50 @@ export function ActionLevelBoard({ {t("actionLevel.description")}

- +
+ + +
- {draft && ( + {pendingChanges.length > 0 && (
)} + {draftFailed && ( +

+ {t("actionLevel.draftFailed")} +

+ )} +
{LEVELS.map((level) => { const key = levelKey(level) @@ -202,20 +224,19 @@ export function ActionLevelBoard({
{zoneActions.map((action) => { const active = activeStates[action.code] ?? action.active - const pending = pendingCode === action.code return (
{ event.dataTransfer.effectAllowed = "move" event.dataTransfer.setData("text/plain", action.code) }} className={cn( "rounded-lg border bg-background p-3 shadow-sm transition-opacity", - pending && "opacity-60", + submitting && "opacity-60", )} >
@@ -236,7 +257,7 @@ export function ActionLevelBoard({ handleActiveChange(action.code, checked) } @@ -267,7 +288,7 @@ export function ActionLevelBoard({