Skip to content

feat: add waveBuildNotification user preference for build email notifications#1069

Merged
pditommaso merged 10 commits into
masterfrom
robnewman-user-build-notification
Jul 1, 2026
Merged

feat: add waveBuildNotification user preference for build email notifications#1069
pditommaso merged 10 commits into
masterfrom
robnewman-user-build-notification

Conversation

@robnewman

@robnewman robnewman commented Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

Behaviour

Preference Build succeeded Email sent?
null (default → Always On) yes yes
null (default → Always On) no yes
ALWAYS_ON yes yes
ALWAYS_ON no yes
ON_ERROR yes no
ON_ERROR no yes
ALWAYS_OFF yes no
ALWAYS_OFF no no

Null preference (older Tower clients that don't send the field) defaults to ALWAYS_ON, preserving existing behaviour. The existing noEmail system flag on BuildRequest (sub-build suppression) is unchanged and continues to take priority at the onBuildEvent level.

Test plan

  • WaveBuildNotificationTest — verifies 3 enum constants and defaultValue() returns ALWAYS_ON
  • UserTest — verifies field is nullable and stores enum values
  • MailServiceImplTest — 8-row parametrized test covering every cell of the decision table above; uses @MockBean(MailSpoolerImpl) to assert sendMail call count

Follow-up (post-merge)

  • Add a test case for the anonymous-build path (null user, fallback to config.from) to confirm it also defaults to ALWAYS_ON

🤖 Generated with Claude Code

robnewman and others added 7 commits June 26, 2026 12:41
@robnewman @claude
docs: add design spec for waveBuildNotification user preference
767557e 
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@robnewman @claude
docs: add implementation plan for waveBuildNotification
b2683f0 
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@robnewman @claude
feat: add WaveBuildNotification enum
6add7c3 
Create WaveBuildNotification enum with three constants (ALWAYS_ON, ON_ERROR, ALWAYS_OFF) and a defaultValue() method. Add comprehensive tests.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@robnewman @claude
feat: add waveBuildNotification field to User
f5b6a2b 
Add nullable waveBuildNotification field to the User class to support storing build notification preferences as a deserialized enum from JSON.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@robnewman @claude
feat: gate sendCompletionEmail on waveBuildNotification user preference
c797a75 
Add preference gate to MailServiceImpl.sendCompletionEmail: ALWAYS_OFF
suppresses all notifications, ON_ERROR suppresses successful-build emails,
null/ALWAYS_ON (default) preserves existing behaviour. Parametrized Spock
test covers all 8 pref × outcome combinations using @MockBean(MailSpoolerImpl).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@robnewman @claude
fix: move WaveBuildNotification import and add license headers to tes…
031543f 
…t files

Move WaveBuildNotification import to the io.seqera.wave group in MailServiceImpl, and add AGPL license headers to WaveBuildNotificationTest and UserTest.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@robnewman @claude
fix: null-coalesce build before accessing buildId in missing-recipien…
c943443 
…t log

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@robnewman

robnewman commented Jun 27, 2026

Copy link
Copy Markdown
Member Author

Related Platform PR

@robnewman

robnewman commented Jun 30, 2026

Copy link
Copy Markdown
Member Author

@claude please review with super powers

@claude

claude Bot commented Jun 30, 2026 •
edited
Loading

Copy link
Copy Markdown

Claude encountered an error —— View job

I'll analyze this and get back to you.

@robnewman

robnewman commented Jun 30, 2026

Copy link
Copy Markdown
Member Author

@claude review this PR with super powers. Also, we need to temporarily use Node 20, so set the ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION=true environment variable for the review CI/CD processes.

@claude

claude Bot commented Jun 30, 2026 •
edited
Loading

Copy link
Copy Markdown

Claude encountered an error —— View job

I'll analyze this and get back to you.

munishchouhan and others added 3 commits July 1, 2026 12:49
@munishchouhan @claude
chore: apply code review fixes for waveBuildNotification
0a77a01 
- restore jakarta imports above static imports in MailServiceImpl
- use request.buildId (not result.buildId which is '-' for unknown) in
  missing-recipient log line
- simplify PlatformId construction in test using @canonical default args
- bump copyright year on new files to 2023-2026

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@munishchouhan @claude
chore: remove docs/superpowers planning artifacts
679df23 
Internal agent-generated planning/design docs — belong outside the repo.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@pditommaso @claude
fix: tolerate unknown build-notification enum & simplify default
813ba90 
Address review feedback on the waveBuildNotification preference:

- Enable Jackson READ_UNKNOWN_ENUM_VALUES_AS_NULL so a waveBuildNotification
  value not (yet) known to Wave degrades to null (→ default ALWAYS_ON) instead
  of failing the entire Tower user-info deserialization.
- Inline the ALWAYS_ON default and drop the redundant defaultValue() helper.
- Document the new enum, the User field, and the mail gating logic.
- Replace the tautology defaultValue test with one asserting unknown enum
  values deserialize to null.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@pditommaso

pditommaso commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Review follow-up — addressed feedback (commit 813ba90)

Applied fixes from a correctness + over-engineering pass:

🐛 Correctness — unknown enum value no longer breaks user-info parsing
Tower's user-info payload is deserialized with JacksonHelper.fromJson, whose mapper did not set READ_UNKNOWN_ENUM_VALUES_AS_NULL. If Tower ever sent a waveBuildNotification value Wave's enum doesn't know (a new mode added Tower-side, a rename, different casing), Jackson would throw InvalidFormatException and fail the entire GetUserInfoResponse — breaking user resolution/auth for those builds, not just the preference.

  • Enabled DeserializationFeature.READ_UNKNOWN_ENUM_VALUES_AS_NULL on the mapper. Unknown values now degrade to null, which already falls through to the ALWAYS_ON default — so forward-compat with Tower is preserved.
  • Added a test asserting an unknown enum value deserializes to null instead of throwing.

🧹 Simplification

  • Inlined the ALWAYS_ON default at its single call site and removed the defaultValue() helper (and its tautology test).

📝 Comments

  • Javadoc on the WaveBuildNotification enum (the three modes + null semantics), on the User.waveBuildNotification field, and inline comments on the mail gating logic in MailServiceImpl, plus a comment explaining the Jackson setting.

All affected tests pass (WaveBuildNotificationTest, UserTest, MailServiceImplTest).

🤖 Generated with Claude Code

@pditommaso pditommaso merged commit b7822a0 into master Jul 1, 2026
5 checks passed
@pditommaso pditommaso deleted the robnewman-user-build-notification branch July 1, 2026 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pditommaso pditommaso pditommaso approved these changes
@munishchouhan munishchouhan munishchouhan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Accept user-profile level platform setting permission to opt-out of Wave build notification emails

3 participants

@robnewman @pditommaso @munishchouhan