Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: no notify for steps after reject step #7771

Merged
merged 2 commits into from
Oct 15, 2024

Conversation

kevin9foong
Copy link
Contributor

Problem

For rejected workflows, steps after the rejection should not be notified when a rejection happens earlier in the chain.

Closes FRM-1876

Solution

Do not notify subsequent steps if earlier step rejects.

Breaking Changes

  • No - this PR is backwards compatible

Tests

TC1: Regression test for approve

  • Create MRF workflow with 3 steps
  • Set step 2 and step 3 as approval step
  • Set steps 2 and 3 as notification step
  • Approve form completely.
  • See that steps 2 and 3 email get approve email.

TC2: No notify for steps after reject step

  • From MRF from TC1
  • Reject at step 2
  • Ensure only step 2 gets the reject email (Step 3 does not get an email)

TC3: Regression test for reject

  • From MRF from TC1
  • Reject at step 3
  • Ensure both step 2 and 3 gets the reject email

@kevin9foong kevin9foong self-assigned this Oct 14, 2024
@datadog-opengovsg
Copy link

datadog-opengovsg bot commented Oct 14, 2024

Datadog Report

Branch report: fix/no-notify-steps-after-rejection
Commit report: 07343cc
Test service: formsg

✅ 0 Failed, 109 Passed, 0 Skipped, 44.39s Total duration (11m 2.39s time saved)

@kevin9foong kevin9foong force-pushed the fix/no-notify-steps-after-rejection branch from b64e756 to 07343cc Compare October 14, 2024 08:29
},
} as FieldResponsesV3

const fourStepApprovalWorkflow: FormWorkflowStepDto[] = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this be regarded as 5 steps?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, i added another step but forgot to update the name. Thanks for this catch!

},
]

const currentStepNumber = 1 // 2nd step of 3 steps workflow
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const currentStepNumber = 1 // 2nd step of 3 steps workflow
const currentStepNumber = 1 // 2nd step of 5 steps workflow

Copy link
Contributor

@KenLSM KenLSM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of nits regarding comments, otherwise LGTM.

@kevin9foong kevin9foong merged commit 33d30db into develop Oct 15, 2024
9 checks passed
@kevin9foong kevin9foong deleted the fix/no-notify-steps-after-rejection branch October 15, 2024 10:47
@KenLSM KenLSM mentioned this pull request Oct 20, 2024
43 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants