Skip to content

feat: use Result<T> in email settings save path instead of throwing - #142

Merged
andregoepel merged 1 commit into
mainfrom
feature/email-settings-result-type
Jul 26, 2026
Merged

feat: use Result<T> in email settings save path instead of throwing#142
andregoepel merged 1 commit into
mainfrom
feature/email-settings-result-type

Conversation

@andregoepel

Copy link
Copy Markdown
Owner

Closes #141, scoped down from its literal wording after re-reading the actual code:

Fixed — the real convention violation:

  • IEmailSettingsStore.SaveAsync / MartenEmailSettingsStore.SaveAsync now return Result instead of throwing InvalidOperationException for the "no password on first save" case — that's an ordinary validation outcome, not an exceptional condition.
  • EmailSettingsPage.razor's OnValidSubmit branches on the result instead of catch (Exception ex), and shows result.Error (our own controlled validation message) instead of ex.Message.
  • Added a new test (Submit_StoreReturnsFailure_ShowsErrorNotificationWithResultMessage) covering the failure-notification path, which had no test coverage before.

Deliberately NOT changed, with reasoning:

  • EmailSettingsPage.razor's OnSendTest (catch (Exception ex) around the test-email send) — this wraps genuine external SMTP I/O with many real failure modes (network, auth, DNS), not an expected validation condition. Converting it to Result<T> wouldn't fix anything; the ex.Message shown there is also gated behind [Authorize(Roles = "Administrator")], so it's diagnostic info for the person testing their own mail settings, not a leak to an untrusted party.
  • Setup.razor's catch (Exception ex) (line ~232) — on closer read this already logs properly (Logger.LogError(ex, "Critical error during setup")) and shows a safe, generic message ("Setup could not be completed. Please try again.") — it never leaks ex.Message. It's a legitimate top-level safety net around a multi-step admin-bootstrap flow whose individual steps already branch on typed results (IdentityResult, SignInResult). This isn't the "exceptions for flow control" anti-pattern; converting it would add complexity without fixing anything.

Verification: dotnet csharpier format . clean, dotnet build 0 errors, dotnet test --filter "FullyQualifiedName!~E2ETests" 118/118 passed (117 existing + 1 new), dotnet list package --vulnerable --include-transitive clean.

Closes #141

🤖 Generated with Claude Code

@andregoepel
andregoepel merged commit 577162a into main Jul 26, 2026
4 checks passed
@andregoepel
andregoepel deleted the feature/email-settings-result-type branch July 26, 2026 03:16
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.

Adopt Result<T> in MartenEmailSettingsStore / EmailSettingsPage / Setup

1 participant