fix: make mail truly optional and clarify quick-start docs (#310)

[devondragon]

Summary

Addresses the four speed bumps reported in #310:

1. OAuth2 dependency was undocumented. spring-boot-starter-oauth2-client is unconditionally wired into the security chain (WebSecurityConfig injects DSOAuth2UserService / DSOidcUserService as required constructor params). README now lists it in the required dependencies.

2. spring-retry version resolution. Spring Boot's BOM doesn't always manage spring-retry; pin to 2.0.12 to match what the library is built against (build.gradle:52).

3. Mail config wasn't actually optional. MailService had JavaMailSender as a hard constructor dependency, so apps without spring.mail.host set wouldn't start. Refactored to take ObjectProvider<JavaMailSender> — when the bean is absent, send operations log a warning and no-op. README Step 4 updated to describe the degraded behavior.

4. No user-facing templates ship. The library ships only templates/mail/ (email templates). README Step 9 used to claim "default HTML templates" existed; updated to direct users to the demo app's templates dir.

Changes

• src/main/java/com/digitalsanctuary/spring/user/mail/MailService.java — switch to ObjectProvider<JavaMailSender>; add resolveMailSender() helper that returns null + warns when absent; bail out of send methods when no sender available.
• src/test/java/com/digitalsanctuary/spring/user/mail/MailServiceTest.java — update fixture for new constructor; add two new tests covering the no-sender no-op path.
• README.md — Step 1 deps (OAuth2 + retry version + explanatory notes), Step 4 mail (truly optional now), Step 9 templates (link to demo, list the files).

Test plan

• ./gradlew test --tests "*MailServiceTest" — all 24 tests pass (2 new)
• ./gradlew test --tests "*UserEmailServiceTest" --tests "*RegistrationListenerTest" — pass (downstream callers unaffected)
• Full ./gradlew test — only pre-existing failures remain (LogoutSuccessServiceTest Mockito strict-stubbing on Referer header, unrelated to this PR)

Closes #310.

[claude]

Code Review: PR #311 — Make mail truly optional & clarify quick-start docs

Good overall direction — these are real usability pain points for new adopters. The ObjectProvider approach is idiomatic Spring. A few things worth addressing before merge.

---

✅ What's Working Well

• ObjectProvider<JavaMailSender> is the correct idiom for optional Spring beans; avoids @Autowired(required = false) with a null-field pattern.
• Removing @InjectMocks and manually constructing MailService in tests is necessary given the ObjectProvider mock — correct call.
• lenient() on the default provider stub prevents strict-stubbing failures in tests that override it.
• sendTemplateMessageNoOpsWhenSenderMissing verifying mailContentBuilder is never called is a nice detail — template rendering is correctly skipped, not just the send.
• Step 9 doc update ("Required" instead of "Optional") is more accurate and saves new users a lot of confusion.

---

🔴 Must Fix

1. spring.security.oauth2.enabled=false is not a real property

In README.md (the new note under Step 1):

OAuth2 login itself remains disabled by default (spring.security.oauth2.enabled=false).

This property does not exist in Spring Boot or Spring Security. There is no single flag to disable OAuth2 auto-configuration via that key. In this library, the relevant toggles live under user.registration.* (e.g. user.registration.googleEnabled, user.registration.githubEnabled). The parenthetical will send readers on a wild-goose chase in application.yml. Either drop the parenthetical or replace it with the actual property names.

---

🟡 Should Fix

2. PII logged at WARN level on every skipped send

// MailService.java — resolveMailSender()
log.warn("Email send to '{}' skipped: JavaMailSender is not configured...", to);

This logs the recipient's email address at WARN level every time a send is attempted without a configured sender. WARN is typically captured by production log aggregators, which makes this a GDPR/PII concern. The debug-level logs (log.debug("Attempting to send simple email to: {}", to)) are fine — debug is off in prod. Two options:

• Better: resolve the sender once at startup (@PostConstruct) and log the warning there exactly once, then use a simple null field check in the send methods (no PII in the log at all).
• Acceptable: keep per-call but redact the address: log.warn("Email send skipped: JavaMailSender is not configured. Set 'spring.mail.host' to enable email sending.") (drop the to parameter from the log).

3. Warning fires on every call, not once at startup

Related to #2 — if an app runs without mail configured and thousands of users register, the log gets flooded with the same warning. A @PostConstruct approach resolves both issues cleanly:

@PostConstruct
private void init() {
    this.resolvedSender = mailSenderProvider.getIfAvailable();
    if (resolvedSender == null) {
        log.warn("JavaMailSender is not configured — email sending is disabled. Set 'spring.mail.host' to enable.");
    }
}

Then sendSimpleMessage/sendTemplateMessage just check if (resolvedSender == null) return; — no per-call provider lookup, no repeated logging, no PII.

---

🔵 Minor / Nice-to-Have

4. Tests don't verify the warning was logged

The two new tests verify the no-op behavior correctly, but neither asserts that a warning was actually emitted. This is non-critical (the no-op is what matters for behavior), but if the @PostConstruct approach is adopted, a startup-log test would confirm misconfiguration is surfaced to operators.

5. spring-retry version pin in README is a fragile user-side workaround

Pinning 2.0.12 in the consumer's build file works today, but when Spring Boot's BOM eventually starts managing spring-retry, consumers will have a redundant explicit version that may drift. A cleaner long-term fix is for the library itself to express the version constraint via a published BOM or Gradle platform dependency rather than pushing version pins into consumer build files. Not a blocker for this PR, but worth a follow-up issue.

6. @Retryable on a no-op path

When the sender is null, the method returns before any MailException can be thrown, so @Retryable never activates — that's correct. Just confirming the early-return placement (before MimeMessagePreparator construction) is intentional and right.

---

Summary

#	Severity	Item
1	🔴 Must	Remove/correct spring.security.oauth2.enabled=false — property doesn't exist
2	🟡 Should	Remove email address from WARN-level log (PII/GDPR concern)
3	🟡 Should	Log warning once at startup, not per-call (log noise + ties into #2)
4	🔵 Minor	Add log-verification assertion to the two new tests
5	🔵 Minor	Track spring-retry version pin as a follow-up issue for a BOM solution

Fix #1 before merge; #2+#3 are strongly recommended given this library's GDPR positioning. The rest can be follow-ups.

[Copilot]

Pull request overview

This PR smooths out quick-start integration by documenting required dependencies and making outbound email sending degrade gracefully when mail isn’t configured, aligning runtime behavior with the README.

Changes:

• Refactor MailService to lazily resolve JavaMailSender via ObjectProvider and no-op (with a warning) when absent.
• Update MailServiceTest to use the new constructor and add coverage for the “no sender configured” path.
• Clarify README quick-start dependencies (OAuth2 client + pinned spring-retry) and correct guidance around user-facing templates.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/main/java/com/digitalsanctuary/spring/user/mail/MailService.java	Make mail sender optional via ObjectProvider; skip sends when unavailable.
src/test/java/com/digitalsanctuary/spring/user/mail/MailServiceTest.java	Update wiring for new constructor; add tests for missing JavaMailSender.
README.md	Document OAuth2 and spring-retry version; clarify optional mail behavior and template expectations.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.