feat: calculate new step position for reject branch

Author: pregnantboy

packages/backend/src/apps/formsg/common/types.ts

@@ -50,3 +50,8 @@ export interface ParsedMrfWorkflow {
   trigger: Omit<ParsedMrfWorkflowStep, 'approvalField'>
   actions: ParsedMrfWorkflowStep[]
 }
+
+export const stepApprovalConfigSchema = z.object({
+  branch: z.enum(['approve', 'reject']),
+  stepId: z.string().uuid(),
+})

packages/backend/src/graphql/mutations/create-step.ts

@@ -31,26 +31,26 @@ const createStep: MutationResolvers['createStep'] = async (
       throw new BadUserInputError('Action can only be created by system')
     }
   }
-  return await Step.transaction(async (trx) => {
-    await trx.raw('SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;')
-
-    if (input.connection?.id) {
-      // if connectionId is specified, verify that the connection exists and belongs to the user
-      const connection = await context.currentUser
-        .$relatedQuery('connections')
-        .findOne({ id: input.connection.id })
-      if (!connection) {
-        throw new BadUserInputError('Connection not found')
-      }
+  if (input.connection?.id) {
+    // if connectionId is specified, verify that the connection exists and belongs to the user
+    const connection = await context.currentUser
+      .$relatedQuery('connections')
+      .findOne({ id: input.connection.id })
+    if (!connection) {
+      throw new BadUserInputError('Connection not found')
     }
+  }
 
-    // Put SELECTs in transaction just in case there's concurrent modification.
-    const flow = await context.currentUser
-      .$relatedQuery('flows', trx)
-      .findOne({
-        id: input.flow.id,
-      })
-      .throwIfNotFound()
+  // Put SELECTs in transaction just in case there's concurrent modification.
+  const flow = await context.currentUser
+    .$relatedQuery('flows')
+    .findOne({
+      id: input.flow.id,
+    })
+    .throwIfNotFound()
+
+  return await Step.transaction(async (trx) => {
+    await trx.raw('SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;')
 
     const previousStep = await flow
       .$relatedQuery('steps', trx)
@@ -59,11 +59,11 @@ const createStep: MutationResolvers['createStep'] = async (
       })
       .throwIfNotFound()
 
-    const isApprovalConfigValid = validateApprovalConfig(
+    const validationResult = await validateApprovalConfig(
       input.config,
       previousStep,
     )
-    if (!isApprovalConfigValid) {
+    if (!validationResult.isApprovalConfigValid) {
       throw new BadUserInputError('Invalid approval config')
     }
 
@@ -72,13 +72,13 @@ const createStep: MutationResolvers['createStep'] = async (
       .patch({
         position: raw(`position + 1`),
       })
-      .where('position', '>=', previousStep.position + 1)
+      .where('position', '>=', validationResult.newStepPosition)
 
     const step = await flow.$relatedQuery('steps', trx).insertAndFetch({
       key: input.key,
       appKey: input.appKey,
       type: 'action',
-      position: previousStep.position + 1,
+      position: validationResult.newStepPosition,
       parameters: input.parameters,
       connectionId: input.connection?.id,
       config: input.config,

packages/backend/src/helpers/validate-approval-config.ts

@@ -2,42 +2,150 @@ import { IStep, IStepConfig } from '@plumber/types'
 
 import get from 'lodash.get'
 
-export function validateApprovalConfig(config: IStepConfig, prevStep: IStep) {
+import { stepApprovalConfigSchema } from '@/apps/formsg/common/types'
+import Step from '@/models/step'
+
+import logger from './logger'
+
+/**
+ * TODO: write unit test for this function
+ */
+export async function validateApprovalConfig(
+  config: IStepConfig,
+  prevStep: IStep,
+): Promise<
+  | {
+      isApprovalConfigValid: true
+      newStepPosition: number
+    }
+  | {
+      isApprovalConfigValid: false
+    }
+> {
   /**
-   * if approval config is not present, or is a trigger, return true
+   * Case 1: new step is a trigger step, return early
    */
-  if (!config?.approval || !prevStep) {
-    return true
+  if (!prevStep) {
+    return {
+      isApprovalConfigValid: true,
+      newStepPosition: 1,
+    }
   }
 
+  const isPreviousStepMrfApprovalStep =
+    prevStep.appKey === 'formsg' &&
+    prevStep.key === 'mrfSubmission' &&
+    // if no approval field, it's just a normal mrf step
+    !!get(prevStep.parameters, 'mrf.approvalField')
+
   /**
-   * Both approval branch and approval step id must come together
+   * Case 2: Prev step has no approval config and is not an mrf approval step,
+   * current step should have no approval config either
    */
-  if (!(config?.approval?.branch && config?.approval?.stepId)) {
-    return false
+  if (!prevStep.config.approval && !isPreviousStepMrfApprovalStep) {
+    if (!config?.approval) {
+      return {
+        isApprovalConfigValid: true,
+        newStepPosition: prevStep.position + 1,
+      }
+    }
+    logger.error(
+      'Invalid approval config: previous step has no approval config and is not an mrf approval step',
+    )
+    return {
+      isApprovalConfigValid: false,
+    }
   }
 
   /**
-   * If previous step an approval step return true
+   * Case 3: Prev step is part of an approval branch, check that
+   * the new step has the same approval config
    */
-  if (
-    prevStep.id === config.approval?.stepId &&
-    prevStep.appKey === 'formsg' &&
-    prevStep.key === 'mrfSubmission' &&
-    !!get(prevStep.parameters, 'mrf.approvalField')
-  ) {
-    return true
+  if (prevStep.config.approval) {
+    const isSameApprovalConfig =
+      prevStep.config.approval?.branch === config?.approval?.branch &&
+      prevStep.config.approval?.stepId === config?.approval?.stepId
+    if (isSameApprovalConfig) {
+      return {
+        isApprovalConfigValid: true,
+        newStepPosition: prevStep.position + 1,
+      }
+    }
+    logger.error(
+      'Invalid approval config: new step approval config is not the same as previous step',
+    )
+    return {
+      isApprovalConfigValid: false,
+    }
   }
 
   /**
-   * If previous step has the same approval branch and step id, return true
+   * Case 4: Previous step is an approval step
    */
-  if (
-    prevStep.config.approval?.branch === config.approval?.branch &&
-    prevStep.config.approval?.stepId === config.approval?.stepId
-  ) {
-    return true
+  if (isPreviousStepMrfApprovalStep) {
+    // validate approval config
+    const { success } = stepApprovalConfigSchema.safeParse(config?.approval)
+    // validate approval config step id is the same as prev step id
+    if (!success || config.approval.stepId !== prevStep.id) {
+      logger.error(
+        'Invalid approval config (after approval step): invalid approval config or new step id is not the same as approval step id',
+        {
+          approvalConfig: config?.approval,
+        },
+      )
+      return {
+        isApprovalConfigValid: false,
+      }
+    }
+    /**
+     * If approval branch, simply add 1 to the prev step position
+     */
+    if (config.approval.branch === 'approve') {
+      return {
+        isApprovalConfigValid: true,
+        newStepPosition: prevStep.position + 1,
+      }
+    }
+    // If reject branch, find the end of the approval branch and add one to that step
+    // first find the next mrf step (if any)
+    const nextMrfStep = await Step.query()
+      .where('flow_id', prevStep.flowId)
+      .andWhere('type', 'action')
+      .andWhere('key', 'mrfSubmission')
+      .andWhere('position', '>', prevStep.position)
+      .orderBy('position', 'asc')
+      .first()
+    // then find the last approval step in the curr branch (if any)
+    const lastApprovalStepQuery = Step.query()
+      .where('flow_id', prevStep.flowId)
+      .andWhere('position', '>', prevStep.position)
+      .andWhereRaw(`config->'approval'->>'branch' = ?`, ['approve'])
+      .orderBy('position', 'desc')
+      .first()
+
+    if (nextMrfStep) {
+      lastApprovalStepQuery.andWhere('position', '<', nextMrfStep.position)
+    }
+    const lastApprovalStep = await lastApprovalStepQuery.first()
+    // If last approval step exists, return the position after that
+    if (lastApprovalStep) {
+      return {
+        isApprovalConfigValid: true,
+        newStepPosition: lastApprovalStep.position + 1,
+      }
+    } else {
+      // If no last approval step, return the position after the previous step
+      return {
+        isApprovalConfigValid: true,
+        newStepPosition: prevStep.position + 1,
+      }
+    }
   }
 
-  return false
+  /**
+   * Catch all branch, something seems fishy
+   */
+  return {
+    isApprovalConfigValid: false,
+  }
 }