# Issue 744: Approval Action Correctness Issue: https://github.com/pingcap-inc/NexusCRM/issues/744 Related issues: - https://github.com/pingcap-inc/NexusCRM/issues/745 - https://github.com/pingcap-inc/NexusCRM/issues/746 ## Problem Approval actions currently have two coupled correctness failures: - A non-final approve can finalize the `ProcessInstance` as `Approved` when the action transaction cannot load a usable `ProcessDefinition` or cannot match the current runtime node to the ordered definition nodes. - The record detail approval UI can derive a pending status from display-only upcoming steps even when the authoritative `ProcessInstance.Status` is terminal. Both are source-of-truth bugs. The backend must never treat missing definition context as "no next step", and the frontend must not treat chain visualization rows as the process status. ## Constraints - Approval process definitions, nodes, actions, and runtime rows are generic approval substrate. The fix must not branch on object names, status values outside the approval runtime contract, profiles, record types, or tenant metadata. - The action transaction must remain atomic: deleting current work items, completing the current node, creating the next node/work items, and finalizing the instance must succeed or roll back together. - `ProcessInstance.Status` remains the authoritative record-level approval status. Chain rows are visualization. - Existing unanimous-approver and reject-rollback behavior must keep working. ## Backend Design ### 1. Fail closed on missing process definition `processAction` already knows `ProcessInstance.ProcessDefinitionId`. Inside the transaction it must load that `ProcessDefinition` through a helper that returns an explicit error when the query fails or returns no record. The action transaction will abort before deleting work items or finalizing the process if the definition is unavailable. This directly prevents the current nil-definition path from becoming `advancedToNext=false`. ### 2. Fail closed when the current step cannot be reconciled `tryAdvanceToNextStep` and `tryRejectBackToPrevious` must distinguish "current node is final" from "current node is missing from the definition list": - Match by `ProcessNode.Id` first. - If the id is not found, allow a unique fallback match by current node `Name` or `DeveloperName`. - If the fallback is absent or ambiguous, return an explicit error and roll back the transaction. This preserves older runtime rows that can still be matched by name, while avoiding accidental finalization when the engine cannot prove where it is in the chain. ### 3. Propagate next-step creation failures Creating a next-step `ProcessInstanceNode` and its `ProcessInstanceWorkitem` rows is part of advancement. Any insert failure must return an error to the caller and roll back the transaction. It must not return `advanced=false`, because that is semantically "terminal step". ### 4. Preserve action lifecycle boundaries Current step-level actions still run after the current work item is processed. Final actions run only when `advancedToNext=false` and the step is not still pending. Next-step notification behavior stays as-is. ### 5. Return the actual action result status The approval service already returns `InstanceStatus` as `Pending` when an approve advances or a reject rolls back, and as terminal only when the process finalizes. REST response builders should use that service value for both approve and reject, with the action verb's terminal status only as a fallback. ## Frontend Design ### 1. Carry authoritative process status through the chain API `ApprovalChainResult` will include `processInstanceStatus`, sourced from the raw `ProcessInstance.Status` or the Nexus summary endpoint status. `buildApprovalChain` is still pure; it simply returns the authoritative status alongside the visualization chain. ### 2. Use authoritative status in `useRecordApproval` `useRecordApproval` derives the badge status from `processInstanceStatus` when available. Chain-based inference remains only as a backward-compatible fallback when a remote source does not provide status. `pendingStatus` is produced only when the authoritative derived status is `Pending`. Terminal `Approved`, `Rejected`, or `Removed` instances keep the chain visible through `hasProcess`, but the pending banner and disabled submit state clear. ### 3. Keep record-page refresh behavior explicit The record detail page should refresh both record data and approval data after approval actions. The current code already does this for banner and history callbacks; the PR will keep it covered with a component regression test so it does not regress. ## Tests Backend unit tests: - First-step approve in a two-step process creates the second runtime node/work item, keeps `ProcessInstance.Status` pending, and does not run finalization. - Final-step approve finalizes as approved and does not create another work item. - Missing in-transaction `ProcessDefinition` returns an explicit error and does not finalize the instance. Frontend tests: - `useRecordApproval` returns `Approved` and clears `pendingStatus` when the chain includes upcoming display steps but `processInstanceStatus` is `Approved`. - `useRecordApproval` keeps a pending banner payload when the authoritative status is `Pending`. - `MetadataRecordDetail` invokes record reload after an approval action completion callback. ## Iteration Log 1. Initial design considered only passing the pre-transaction `ProcessDefinition` into `tryAdvanceToNextStep`. That would make the happy path advance, but it would still allow stale or missing transactional state to commit. Rejected in favor of an in-transaction load that fails closed. 2. Current-node mismatch could be handled by always matching names. That is unsafe if names are duplicated or metadata changed materially. Final design matches by id first, then only by a unique name/developer-name fallback, otherwise errors. 3. The frontend could hide the banner by scanning for a pending work item. That still leaves the badge derived from visualization rows. Final design carries the authoritative `ProcessInstance.Status` through the API result and makes pending banner state a consequence of that status. 4. Review of the REST handler found reject responses were still hardcoded to `Rejected`. Final design uses the service-returned `InstanceStatus` for reject as well, preserving rollback-to-previous semantics across the wire. ## Confidence Checklist - A non-final step can only finalize when the ordered definition list is loaded and the current node is proven to be the last matching node. - A failed definition load, missing current-node match, or failed next-step insert rolls back before terminal actions fire. - Terminal backend state is exposed to the frontend as `processInstanceStatus`. - Display-only `Upcoming` rows cannot force a terminal instance back to pending. - Record data and approval data refresh are both covered by tests after action completion.