feat: reorder step across approval branches

pregnantboy · pregnantboy · commit 6744420e3093 · 2025-12-01T01:25:34.000+08:00
diff --git a/packages/backend/src/graphql/__tests__/mutations/update-step-positions.itest.ts b/packages/backend/src/graphql/__tests__/mutations/update-step-positions.itest.ts
@@ -69,7 +69,9 @@ describe('updateStepPositions mutation', () => {
     // Set up flow patch and fetch spy first
     flowPatchAndFetchSpy = vi.fn().mockReturnValue({
       withGraphFetched: vi.fn().mockReturnValue({
-        orderBy: vi.fn().mockResolvedValue([]),
+        orderBy: vi.fn().mockResolvedValue({
+          steps: MOCK_STEPS,
+        }),
       }),
     })
     stepPatchSpy = vi.fn().mockResolvedValue({})
diff --git a/packages/backend/src/graphql/mutations/update-step-positions.ts b/packages/backend/src/graphql/mutations/update-step-positions.ts
@@ -1,6 +1,9 @@
 import { IStep } from '@plumber/types'
 
+import { PartialModelObject, raw } from 'objection'
+
 import { BadUserInputError } from '@/errors/graphql-errors'
+import logger from '@/helpers/logger'
 import Step from '@/models/step'
 
 import type { MutationResolvers } from '../__generated__/types.generated'
@@ -76,9 +79,17 @@ const updateStepPositions: MutationResolvers['updateStepPositions'] = async (
 
     // Patch each step individually with its new position
     for (const stepPosition of stepPositions) {
-      await Step.query(trx).findById(stepPosition.id).patch({
+      const patchData: PartialModelObject<Step> = {
         position: stepPosition.position,
-      })
+      }
+      if (stepPosition.config) {
+        patchData.config = stepPosition.config.approval
+          ? raw(`jsonb_set(config, '{approval}', ?::jsonb, true)`, [
+              JSON.stringify(stepPosition.config?.approval),
+            ])
+          : raw(`config - 'approval'`)
+      }
+      await Step.query(trx).findById(stepPosition.id).patch(patchData)
     }
 
     // Update the flow's lastUpdatedAt timestamp
@@ -90,6 +101,21 @@ const updateStepPositions: MutationResolvers['updateStepPositions'] = async (
       .withGraphFetched('steps')
       .orderBy('steps.position', 'asc')
 
+    // sanity check that all step positions are contiguous
+    const contiguousPositions = updatedFlow.steps.map((step) => step.position)
+    if (
+      !contiguousPositions.every((position, index) => position === index + 1)
+    ) {
+      logger.error({
+        message: 'Updated positions are no longer contiguous',
+        stepPositions,
+        flowId: flow.id,
+      })
+      throw new BadUserInputError(
+        'Failed to update: updated positions are no longer contiguous',
+      )
+    }
+
     return updatedFlow.steps
   })
 
diff --git a/packages/backend/src/graphql/schema.graphql b/packages/backend/src/graphql/schema.graphql
@@ -575,6 +575,7 @@ input StepPositionInput {
   id: String!
   position: Int!
   type: StepEnumType!
+  config: StepConfigInput
 }
 
 input UpdateStepPositionsInput {
@@ -680,8 +681,8 @@ type StepTemplateConfig {
 }
 
 type StepApprovalConfig {
-  branch: String
-  stepId: String
+  branch: String!
+  stepId: String!
 }
 
 input StepConnectionInput {
diff --git a/packages/frontend/src/components/Editor/components/StepsList.tsx b/packages/frontend/src/components/Editor/components/StepsList.tsx
@@ -1,14 +1,14 @@
 import { IStep } from '@plumber/types'
 
-import { useContext } from 'react'
+import { useCallback, useContext } from 'react'
 import { Center, Flex } from '@chakra-ui/react'
 
 import PrimarySpinner from '@/components/PrimarySpinner'
 import { SortableList } from '@/components/SortableList'
 import { EditorContext } from '@/contexts/Editor'
+import { MrfContext } from '@/contexts/MrfContext'
 import { StepsToDisplayContext } from '@/contexts/StepsToDisplay'
 import { FlowStepGroup } from '@/exports/components'
-import { StepEnumType } from '@/graphql/__generated__/graphql'
 import { TOOLBOX_ACTIONS } from '@/helpers/toolbox'
 import useReorderSteps from '@/hooks/useReorderSteps'
 
@@ -30,26 +30,43 @@ export function StepsList({ isNested }: StepsListProps) {
     groupingActions,
   } = useContext(StepsToDisplayContext)
   const { flow, isDrawerOpen, isMobile, readOnly } = useContext(EditorContext)
+  const { mrfSteps, mrfApprovalSteps, approvalBranches } =
+    useContext(MrfContext)
 
-  const { handleReorderUpdate } = useReorderSteps(flow.id)
+  const { calculateReorderedSteps, handleReorderUpdate } = useReorderSteps(
+    flow.id,
+  )
 
-  const handleReorderSteps = async (reorderedSteps: IStep[]) => {
-    const stepPositions = reorderedSteps.map((step, index) => ({
-      id: step.id,
-      position: index + 2, // trigger position is 1
-      type: step.type as StepEnumType,
-    }))
+  const handleReorderSteps = useCallback(
+    async (reorderedSteps: IStep[]) => {
+      const allSteps = flow.steps
+      const allReorderedSteps = calculateReorderedSteps({
+        reorderedSteps,
+        allSteps,
+        mrfSteps,
+        mrfApprovalSteps,
+        approvalBranches,
+      })
 
-    try {
-      await handleReorderUpdate(stepPositions)
-    } catch (error) {
-      console.error(
-        'Error updating step positions: ',
-        error,
-        JSON.stringify(stepPositions),
-      )
-    }
-  }
+      try {
+        await handleReorderUpdate(allReorderedSteps)
+      } catch (error) {
+        console.error(
+          'Error updating step positions: ',
+          error,
+          JSON.stringify(allReorderedSteps),
+        )
+      }
+    },
+    [
+      flow.steps,
+      calculateReorderedSteps,
+      mrfSteps,
+      mrfApprovalSteps,
+      approvalBranches,
+      handleReorderUpdate,
+    ],
+  )
 
   const nonIfThenActionSteps = actionStepsBeforeGroup.filter(
     (step) => step.key !== TOOLBOX_ACTIONS.IfThen,
diff --git a/packages/frontend/src/hooks/useReorderSteps.ts b/packages/frontend/src/hooks/useReorderSteps.ts
@@ -1,22 +1,126 @@
-import { IStep } from '@plumber/types'
+import { IStep, IStepApprovalBranch } from '@plumber/types'
 
+import { useCallback } from 'react'
 import { useMutation } from '@apollo/client'
 import { useToast } from '@opengovsg/design-system-react'
 
-import { StepEnumType } from '@/graphql/__generated__/graphql'
+import {
+  StepEnumType,
+  StepPositionInput,
+} from '@/graphql/__generated__/graphql'
 import { UPDATE_STEP_POSITIONS } from '@/graphql/mutations/update-step-positions'
 import { GET_FLOW } from '@/graphql/queries/get-flow'
 
-interface StepPositionInput {
-  id: string
-  position: number
-  type: StepEnumType
-}
-
 const useReorderSteps = (flowId: string) => {
   const toast = useToast()
   const [updateStepPositions] = useMutation(UPDATE_STEP_POSITIONS)
 
+  // TODO: write unit test for this function
+  const calculateReorderedSteps = useCallback(
+    ({
+      reorderedSteps,
+      allSteps,
+      mrfSteps,
+      mrfApprovalSteps,
+      approvalBranches,
+    }: {
+      reorderedSteps: IStep[]
+      allSteps: IStep[]
+      mrfSteps: IStep[]
+      mrfApprovalSteps: IStep[]
+      approvalBranches: { [stepId: string]: IStepApprovalBranch }
+    }): StepPositionInput[] => {
+      // we start from 2 because the first step is the trigger step and not part of the sortable list
+      let nextPosition = 2
+      let currentApprovalBranch: {
+        stepId: string
+        branch: 'approve' | 'reject'
+      } | null = null
+      const allReorderedSteps: StepPositionInput[] = []
+      reorderedSteps.forEach((reorderingStep) => {
+        const isMrfStep = mrfSteps.some(
+          (mrfStep) => mrfStep.id === reorderingStep.id,
+        )
+
+        if (isMrfStep) {
+          // if the previous approval config is in the approve branch
+          // we need to add the steps in the reject branch that was hidden
+          if (currentApprovalBranch?.branch === 'approve') {
+            const stepsInRejectBranch = allSteps.filter(
+              (step) =>
+                step.config?.approval?.branch === 'reject' &&
+                step.config?.approval?.stepId === currentApprovalBranch?.stepId,
+            ) as IStep[]
+            stepsInRejectBranch.forEach((step) => {
+              allReorderedSteps.push({
+                id: step.id,
+                position: nextPosition++,
+                type: step.type as StepEnumType,
+              })
+            })
+          }
+          currentApprovalBranch = null
+        }
+
+        // add the current step to the list
+        allReorderedSteps.push({
+          id: reorderingStep.id,
+          position: nextPosition++,
+          type: reorderingStep.type as StepEnumType,
+          config:
+            currentApprovalBranch?.branch === 'reject'
+              ? {
+                  approval: currentApprovalBranch,
+                }
+              : { approval: null }, // this sets it to approval branch
+        })
+
+        const isMrfApprovalStep = mrfApprovalSteps.some(
+          (mrfApprovalStep) => mrfApprovalStep.id === reorderingStep.id,
+        )
+        if (isMrfApprovalStep) {
+          currentApprovalBranch = {
+            branch: approvalBranches[reorderingStep.id],
+            stepId: reorderingStep.id,
+          }
+          // if the current approval config is in the reject branch
+          // we need to add the steps in the approve branch that was hidden
+          if (currentApprovalBranch?.branch === 'reject') {
+            const reorderingStepIndex = allSteps.findIndex(
+              (step) => step.id === reorderingStep.id,
+            )
+            const stepsInApproveBranch = []
+            // Search for non-mrf steps directly after the approval step
+            for (let i = reorderingStepIndex + 1; i < allSteps.length; i++) {
+              const step = allSteps[i]
+              if (
+                !step.config?.approval &&
+                !mrfSteps.some((mrfStep) => mrfStep.id === step.id)
+              ) {
+                stepsInApproveBranch.push(step)
+              } else {
+                break
+              }
+            }
+            stepsInApproveBranch.forEach((step) => {
+              allReorderedSteps.push({
+                id: step.id,
+                position: nextPosition++,
+                type: step.type as StepEnumType,
+              })
+            })
+          }
+        }
+      })
+
+      // all later steps not in the sortable list need not be updated since
+      // reordering of visible steps will not affect them
+
+      return allReorderedSteps
+    },
+    [],
+  )
+
   const handleReorderUpdate = async (stepPositions: StepPositionInput[]) => {
     try {
       await updateStepPositions({
@@ -25,6 +129,7 @@ const useReorderSteps = (flowId: string) => {
           updateStepPositions: stepPositions.map((sp) => ({
             id: sp.id,
             position: sp.position,
+            config: sp.config ? { approval: sp.config.approval } : undefined,
             __typename: 'Step' as const,
           })),
         },
@@ -37,15 +142,22 @@ const useReorderSteps = (flowId: string) => {
 
           if (flow) {
             // Create a map of step positions for quick lookup
-            const positionMap = new Map(
-              stepPositions.map((sp) => [sp.id, sp.position]),
+            const updatedStepMap = new Map(
+              stepPositions.map((sp) => [
+                sp.id,
+                { position: sp.position, config: sp.config },
+              ]),
             )
 
             // Update steps with new positions
             const updatedSteps = flow.steps.map((step: IStep) => {
-              const newPosition = positionMap.get(step.id)
-              return newPosition !== undefined
-                ? { ...step, position: newPosition }
+              const updatedStep = updatedStepMap.get(step.id)
+              return updatedStep !== undefined
+                ? {
+                    ...step,
+                    position: updatedStep.position,
+                    config: { ...step.config, ...updatedStep.config },
+                  }
                 : step
             })
 
@@ -91,7 +203,7 @@ const useReorderSteps = (flowId: string) => {
     }
   }
 
-  return { handleReorderUpdate }
+  return { handleReorderUpdate, calculateReorderedSteps }
 }
 
 export default useReorderSteps

Original file line number	Diff line number	Diff line change
`@@ -575,6 +575,7 @@ input StepPositionInput {`
`575`	`575`	`id: String!`
`576`	`576`	`position: Int!`
`577`	`577`	`type: StepEnumType!`
	`578`	`+ config: StepConfigInput`
`578`	`579`	`}`
`579`	`580`
`580`	`581`	`input UpdateStepPositionsInput {`
`@@ -680,8 +681,8 @@ type StepTemplateConfig {`
`680`	`681`	`}`
`681`	`682`
`682`	`683`	`type StepApprovalConfig {`
`683`		`- branch: String`
`684`		`- stepId: String`
	`684`	`+ branch: String!`
	`685`	`+ stepId: String!`
`685`	`686`	`}`
`686`	`687`
`687`	`688`	`input StepConnectionInput {`