Skip to content

feat: PI-1274 Add SendResolution support to TeamsAlertMethod#893

Open
bsingern9 wants to merge 2 commits intomainfrom
PI-1274-teams-send-resolution
Open

feat: PI-1274 Add SendResolution support to TeamsAlertMethod#893
bsingern9 wants to merge 2 commits intomainfrom
PI-1274-teams-send-resolution

Conversation

@bsingern9
Copy link

Summary

  • Adds SendResolution field to TeamsAlertMethod struct, enabling all-clear (resolution) notifications for MS Teams alert methods
  • Adds validation for the new field, reusing the existing sendResolutionValidation (same pattern as PagerDuty and ServiceNow)

Context

JIRA: PI-1274

Customers want to receive "alert resolved" notifications via MS Teams when alerts are resolved or canceled. This change adds the manifest-level support so that the n9 backend can wire SendResolution for Teams alert methods.

Test plan

  • Existing validation tests continue to pass
  • New field follows the same pattern as PagerDuty/ServiceNow SendResolution
  • Companion PR in n9 repo wires the backend notification logic

🤖 Generated with Claude Code

Enables all-clear (resolved) notification configuration for MS Teams
alert method, matching the existing pattern used by PagerDuty and
ServiceNow alert methods.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Collaborator

@n9-machine-user n9-machine-user left a comment

Choose a reason for hiding this comment

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

Scope: XS — appropriate. Two issues need fixing before this can go forward.


1. Validation scope bug (inline comment on validation.go:273-275)

The sendResolution validation ends up inside the .When(!isHiddenValue(v.URL)) block because it's added to the same govy.New[TeamsAlertMethod](...) that the When() is chained on. When URL is empty or hidden, the entire validator is skipped — including sendResolution. PagerDuty and ServiceNow don't have this problem because their sendResolution validation is at the top-level validator without any conditional. This needs to be restructured.


2. Example not updated (manifest/v1alpha/examples/alert_method.go)

Both PagerDuty and ServiceNow set SendResolution in their example objects. The Teams case was not updated and still only sets URL. For consistency and discoverability, the Teams example should include SendResolution too.


3. No new tests (minor)

validation_teams_test.go was not modified. Existing PagerDuty/ServiceNow tests also lack sendResolution coverage, so this is consistent with the codebase norm — but since this PR is explicitly adding the field, at minimum a "passes with sendResolution set" case would be appropriate.

Comment on lines +273 to +275
govy.ForPointer(func(s TeamsAlertMethod) *SendResolution { return s.SendResolution }).
WithName("sendResolution").
Include(sendResolutionValidation),
Copy link
Collaborator

Choose a reason for hiding this comment

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

The sendResolution validation is placed inside the govy.New[TeamsAlertMethod](...) block that has .When(func(v TeamsAlertMethod) bool { return !isHiddenValue(v.URL) }, ...) applied to it (line 276). This means the sendResolution validation is skipped entirely when URL is empty or a hidden value.

This is inconsistent with PagerDuty and ServiceNow, where sendResolution validation is placed outside any When() condition and always runs.

A concrete broken case: a Teams alert method with no URL set and a sendResolution.message exceeding 128 chars will pass validation.

Fix: either move the When() condition to be field-level on the URL property (like PagerDuty does with integrationKey), or split into two validators — one for the URL (with the When()) and a separate top-level one for sendResolution.

Copy link
Author

Choose a reason for hiding this comment

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

Good catch — the When() guard was wrapping the entire validator, skipping sendResolution validation when the URL was hidden. Moved the When() to the URL field level only, matching the PagerDuty pattern. Fixed in 9a28d00.

The When(isHiddenValue) guard was wrapping the entire teamsValidation
block, which meant sendResolution validation was skipped when the URL
was hidden. Move the guard to the URL field only, matching PagerDuty's
pattern where sendResolution is always validated.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Collaborator

@n9-machine-user n9-machine-user left a comment

Choose a reason for hiding this comment

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

Validation bug is fixed correctly. When() is now field-level on url only — sendResolution validation runs unconditionally regardless of URL state, matching the serviceNowValidation structure exactly.

Remaining gaps for human reviewers to consider:

  1. manifest/v1alpha/examples/alert_method.go — Teams example still only sets URL, no SendResolution. PagerDuty and ServiceNow both include it. This means the e2e test fixture for Teams won't exercise the new field. Low risk, but worth a note — may want to add it once the companion n9 backend PR lands (so e2e tests don't hit an unsupported field).

  2. validation_teams_test.go — no new test cases for sendResolution. Consistent with the existing PagerDuty/ServiceNow tests which also lack sendResolution coverage, so this is more of a codebase-wide gap than a blocker here.

Scope: XS. No migrations, clean rollback, correct implementation.

@triluch
Copy link

triluch commented Mar 10, 2026

For human reviewers - please also review the process and review for automated review - what was sufficient, what would you like to tune, was it right to pass it to you with mentioned gaps etc

@nikodemrafalski
Copy link
Contributor

The validation bug could be avoided with validation unit testing - in this repo it a common practice. Agentic reviewer should emphasise this more

3. No new tests (minor)

validation_teams_test.go was not modified. Existing PagerDuty/ServiceNow tests also lack sendResolution coverage, so this is consistent with the codebase norm — but since this PR is explicitly adding the field, at minimum a "passes with sendResolution set" case would be appropriate.

@nikodemrafalski
Copy link
Contributor

Also on re-review it should not only check if the change it requested was implement, but also demand a unit test proving there is a test reproducing the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request go minor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants