chore: allow ifthen and foreach in different branches

Author: pregnantboy

packages/frontend/src/components/FlowStep/components/DuplicateStepButton.tsx

@@ -23,6 +23,7 @@ export default function DuplicateStepButton(props: DuplicateStepButtonProps) {
   const { flow, isMobile, onDrawerOpen, setCurrentStepId } =
     useContext(EditorContext)
 
+  // TODO: use onCreateStep from EditorContext instead
   const [createStep] = useMutation(CREATE_STEP, { refetchQueries: [GET_FLOW] })
   const onDuplicateStep = useCallback(async () => {
     const duplicateConfig = {

packages/frontend/src/components/FlowStepConfigurationModal/hooks/useIsAppSelectable.ts

@@ -1,11 +1,11 @@
-import { IFlow, IStep } from '@plumber/types'
+import { IStep } from '@plumber/types'
 
 import { useContext } from 'react'
 
 import { BranchContext } from '@/components/FlowStepGroup/Content/IfThen/BranchContext'
 import { NESTED_IFTHEN_FEATURE_FLAG } from '@/config/flags'
-import { EditorContext } from '@/contexts/Editor'
 import { LaunchDarklyContext } from '@/contexts/LaunchDarkly'
+import { StepsToDisplayContext } from '@/contexts/StepsToDisplay'
 import { TOOLBOX_ACTIONS } from '@/helpers/toolbox'
 
 /**
@@ -16,68 +16,26 @@ import { TOOLBOX_ACTIONS } from '@/helpers/toolbox'
  *
  */
 function isDelaySelectable({
-  flow,
+  hasForEach,
+  groupedSteps,
   step,
   prevStepId,
 }: {
-  flow: IFlow
+  hasForEach: boolean
+  groupedSteps: IStep[][]
   step?: IStep
   prevStepId?: string
 }) {
-  const forEachStepPosition = flow?.steps.find(
-    (s) => s.key === TOOLBOX_ACTIONS.ForEach,
-  )?.position
-
-  let prevStepPosition = null
-  if (step) {
-    prevStepPosition = step.position
-  } else if (prevStepId) {
-    prevStepPosition = flow?.steps?.find((s) => s.id === prevStepId)?.position
-  }
-
-  let isDelaySelectable: boolean
-  if (!forEachStepPosition) {
-    isDelaySelectable = true
-  } else if (forEachStepPosition && prevStepPosition) {
-    isDelaySelectable = prevStepPosition < forEachStepPosition
-  } else {
-    isDelaySelectable = false
+  if (!hasForEach) {
+    return true
   }
-
-  return isDelaySelectable
-}
-
-/**
- * Helper function to check if For-each action should be selectable; supports edge
- * case in ChooseEvent component.
- *
- * For-each should only be selectable if:
- * - We're the last step.
- * - We are not inside a for-each action.
- * - We are not inside an if-then action.
- * - There is no if-then action in the flow,
- *
- * Using many consts as purpose of the conditions may not be immediately
- * apparent.
- */
-function isForEachSelectable({
-  flow,
-  hasIfThen,
-  isLastStep,
-}: {
-  flow: IFlow
-  hasIfThen: boolean
-  isLastStep: boolean
-}) {
-  const hasForEach = flow?.steps?.some(
-    (step: IStep) => step.key === TOOLBOX_ACTIONS.ForEach,
-  )
-
-  if (hasForEach || hasIfThen || !isLastStep) {
+  const isStepWithinForEach = groupedSteps.flat().some((groupedStep) => {
+    if (step?.id === groupedStep.id || prevStepId === groupedStep.id) {
+      return true
+    }
     return false
-  }
-
-  return true
+  })
+  return !isStepWithinForEach
 }
 
 /**
@@ -116,6 +74,7 @@ function isIfThenSelectable({
   }
 
   const isNestedBranch = depth > 0
+
   return !isNestedBranch
 }
 
@@ -129,21 +88,31 @@ export const useIsAppSelectable = ({
   prevStepId?: string
 }): Record<string, boolean> => {
   const { depth } = useContext(BranchContext)
-  const { flow, hasIfThen } = useContext(EditorContext)
+  const { groupedSteps } = useContext(StepsToDisplayContext)
   const { getFlagValue } = useContext(LaunchDarklyContext)
 
+  const hasIfThen = groupedSteps.some((group) =>
+    group.some((step) => step.key === TOOLBOX_ACTIONS.IfThen),
+  )
+
+  const hasForEach = groupedSteps.some((group) =>
+    group.some((step) => step.key === TOOLBOX_ACTIONS.ForEach),
+  )
+
   return {
     [TOOLBOX_ACTIONS.IfThen]: isIfThenSelectable({
       depth,
       hasIfThen,
       isLastStep,
       getFlagValue,
     }),
-    [TOOLBOX_ACTIONS.ForEach]: isForEachSelectable({
-      flow,
-      hasIfThen,
-      isLastStep,
-    }),
-    delay: isDelaySelectable({ flow, step, prevStepId }),
+    /**
+     * For-each should only be selectable if:
+     * - We're the last step.
+     * - There's no other for-each action
+     * - There's no other if-then action
+     */
+    [TOOLBOX_ACTIONS.ForEach]: isLastStep && !hasIfThen && !hasForEach,
+    delay: isDelaySelectable({ hasForEach, groupedSteps, step, prevStepId }),
   }
 }

packages/frontend/src/components/FlowStepGroup/Content/IfThen/index.tsx

@@ -7,8 +7,10 @@ import { Flex } from '@chakra-ui/react'
 import { Button } from '@opengovsg/design-system-react'
 
 import { EditorContext } from '@/contexts/Editor'
+import { MrfContext } from '@/contexts/MrfContext'
 import { CREATE_STEP } from '@/graphql/mutations/create-step'
 import { GET_FLOW } from '@/graphql/queries/get-flow'
+import { getMrfApprovalConfig } from '@/helpers/formsg'
 import { TOOLBOX_ACTIONS, TOOLBOX_APP_KEY } from '@/helpers/toolbox'
 
 import Branch from './Branch'
@@ -24,6 +26,7 @@ export default function IfThen(props: IfThenProps): JSX.Element {
   const { groupedSteps, stepsBeforeGroup } = props
 
   const { depth } = useContext(BranchContext)
+  const { approvalBranches } = useContext(MrfContext)
   const {
     flowId,
     readOnly: isEditorReadOnly,
@@ -50,6 +53,11 @@ export default function IfThen(props: IfThenProps): JSX.Element {
     const lastGroup = groupedSteps[groupedSteps.length - 1]
     const lastStep = lastGroup[lastGroup.length - 1]
 
+    const approvalConfig = getMrfApprovalConfig({
+      previousStep: lastStep,
+      approvalBranches,
+    })
+
     const branchStep = await createStep({
       variables: {
         input: {
@@ -65,6 +73,9 @@ export default function IfThen(props: IfThenProps): JSX.Element {
             depth,
             branchName: `Branch ${numBranches + 1}`,
           },
+          config: {
+            approval: approvalConfig,
+          },
         },
       },
     })
@@ -80,6 +91,9 @@ export default function IfThen(props: IfThenProps): JSX.Element {
           flow: {
             id: flowId,
           },
+          config: {
+            approval: approvalConfig,
+          },
         },
       },
     })
@@ -92,6 +106,7 @@ export default function IfThen(props: IfThenProps): JSX.Element {
     createStep,
     flowId,
     depth,
+    approvalBranches,
     numBranches,
     onDrawerOpen,
     setCurrentStepId,

packages/frontend/src/hooks/useReorderSteps.ts

@@ -1,11 +1,10 @@
-import { IStep, IStepApprovalBranch, IStepApprovalConfig } 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 {
-  StepConfigInput,
   StepEnumType,
   StepPositionInput,
 } from '@/graphql/__generated__/graphql'
@@ -33,68 +32,81 @@ const useReorderSteps = (flowId: string) => {
     }): StepPositionInput[] => {
       // we start from 2 because the first step is the trigger step and not part of the sortable list
       let nextPosition = 2
-      let currentApprovalconfig: IStepApprovalConfig | null = null
+      let currentApprovalBranch: {
+        stepId: string
+        branch: 'approve' | 'reject'
+      } | null = null
       const allReorderedSteps: StepPositionInput[] = []
-      reorderedSteps.forEach((step) => {
-        const isMrfStep = mrfSteps.some((mrfStep) => mrfStep.id === step.id)
+      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 (currentApprovalconfig?.branch === 'approve') {
+          if (currentApprovalBranch?.branch === 'approve') {
             const stepsInRejectBranch = allSteps.filter(
               (step) =>
                 step.config?.approval?.branch === 'reject' &&
-                step.config?.approval?.stepId === currentApprovalconfig?.stepId,
+                step.config?.approval?.stepId === currentApprovalBranch?.stepId,
             ) as IStep[]
             stepsInRejectBranch.forEach((step) => {
               allReorderedSteps.push({
                 id: step.id,
                 position: nextPosition++,
                 type: step.type as StepEnumType,
-                config: {
-                  approval: step.config.approval,
-                } as StepConfigInput,
               })
             })
           }
-          currentApprovalconfig = null
+          currentApprovalBranch = null
         }
 
         // add the current step to the list
         allReorderedSteps.push({
-          id: step.id,
+          id: reorderingStep.id,
           position: nextPosition++,
-          type: step.type as StepEnumType,
-          config: {
-            approval: currentApprovalconfig,
-          } as StepConfigInput,
+          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 === step.id,
+          (mrfApprovalStep) => mrfApprovalStep.id === reorderingStep.id,
         )
         if (isMrfApprovalStep) {
-          currentApprovalconfig = {
-            branch: approvalBranches[step.id],
-            stepId: step.id,
+          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 (currentApprovalconfig?.branch === 'reject') {
-            const stepsInApproveBranch = allSteps.filter(
-              (step) =>
-                step.config?.approval?.branch === 'approve' &&
-                step.config?.approval?.stepId === currentApprovalconfig?.stepId,
-            ) as IStep[]
+          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,
-                config: {
-                  approval: step.config.approval,
-                } as StepConfigInput,
               })
             })
           }

packages/frontend/src/hooks/useStepMetadata.ts

@@ -119,11 +119,8 @@ export function useStepMetadata(
         break
       }
     }
-    // if is approval step, return whether
+    // this means that the step is after the trigger step
     if (!immediatePriorMrfStep) {
-      console.warn(
-        'No immediate prior mrf step found... this should not happen tho',
-      )
       return null
     }
     /**