Add Grails 8 email guide (v8 prose, snippets, guides.yml) - #524
Conversation
jamesfredley
left a comment
There was a problem hiding this comment.
Thanks @sanjana2505006! Reviewed the v8 email guide prose plus conf/guides.yml. The good news first: all 12 TOC keys map to chapter files (no orphans), and every include:: snippet path resolves to a vendored snippet. A few things to address before merge. The publication date is fine, please ignore that one.
Inline comments below cover: the guides.yml TOC syntax and author name, the SMTP config filename mismatch, and the injected == Verify CI heading.
One cross-cutting item that is not line-specific:
-
Typography: please remove em/en dashes (the
—character) throughout. They appear in the subtitle (guides.yml),introduction.adoc(line 1),gettingStarted.adoc(line 17), andsummary.adoc(line 7). Replace them with commas, colons, parentheses, or regular hyphens per project style. -
SMTP config filename (cross-PR): the biggest item is that the mail-config filename must be identical in three places: the sample app (companion PR grails-guides/grails-email#1 currently names it
application-development.yml), the vendored snippet here (application-email.yml), and the prose/caption (application.yml). Please reconcile to a single name across both PRs. Details in the inline comment onsmtpMail.adoc.
|
@jamesfredley I've updated the code |
|
Latest-head re-review: the original feedback is addressed, and I resolved those prior threads. A new blocking cross-PR drift remains: the vendored snippets no longer match the companion sample-app head. In particular:
Please fix the remaining companion-PR blockers first, then re-vendor these files from that corrected head and update the AWS filename/include to |
|
@jamesfredley, addressed the inline notes. |
| } | ||
| this.sesClient = SesClient.builder().region(Region.of(awsRegion)).build() | ||
|
|
||
| this.sourceEmail = co.getProperty('aws.ses.source', String, '') |
There was a problem hiding this comment.
This snippet is still the pre-b07505d implementation: it warns and continues when aws.ses.source is missing, and bodyOfEmail still drops the text alternative whenever HTML is present. Please re-vendor the final companion implementation after the sample PR is green; it must fail fast on missing source and preserve both text and HTML.
| respond cmd.errors, view: '/application/errors', status: UNPROCESSABLE_ENTITY | ||
| return | ||
| } | ||
| log.info '{}', cmd.toString() |
There was a problem hiding this comment.
The guide still publishes cmd.toString() at INFO, which includes recipient, CC/BCC, subject, and both message bodies. Please re-vendor the final privacy-safe controller from the companion app after its logging fix is complete.
|
|
||
| then: | ||
| ex != null | ||
| ex.statusCode == HttpStatus.UNPROCESSABLE_ENTITY |
There was a problem hiding this comment.
This vendored test remains behind the companion head: it still compares against UNPROCESSABLE_ENTITY and only checks that path/links are truthy. Please re-vendor the final green test after the sample app correctly returns and asserts exact /mail/send path and self-link values.
|
|
||
| [source,bash] | ||
| ---- | ||
| cd complete |
There was a problem hiding this comment.
The guide previously leaves the reader in grails-email/initial, so this command targets a nonexistent initial/complete directory. Please use cd ../complete, consistent with the corrected Getting Started chapter.
jamesfredley
left a comment
There was a problem hiding this comment.
Thanks @sanjana2505006. I diffed every vendored snippet in this PR against the companion sample app at its current head (12a0680) and re-checked all prose items from the first two rounds. Everything is addressed except one stale snippet.
Blocker: MailController.groovy snippet is still the pre-fix version
guides/grails-email/v8/snippets/grails-app/controllers/example/MailController.groovy still contains:
log.info 'Sending mail to {} with subject {}', cmd.recipient, cmd.subjectThe companion app replaced that with a generic log.info 'Sending mail' in d3d070a, which landed after the b07505d you vendored from. As published, the guide teaches readers to write recipient addresses and subject lines into application logs, which is the exact pattern the last round asked to remove. Please re-vendor this one file from the final companion head.
Every other snippet (resources.groovy, EmailCmd.groovy, SmtpEmailService.groovy, MailControllerSpec.groovy, AwsSesEmailService.groovy, Email.groovy, EmailService.groovy, SendGridEmailService.groovy) is now byte-identical to the companion, and application.yml differs only by the expected tag markers and surrounding config.
Also blocking, on the companion side
The companion PR grails-guides/grails-email#1 is currently red: ./gradlew test fails at :compileGsonViews because _errors.gson calls g.createLink(...), which does not exist on GrailsJsonViewHelper. Details are in my review there. Since this PR vendors that tree, please re-vendor once the companion is green so the snippets and the guide match a build that actually passes, and I will merge both together.
Separately, the Java CI run on 7c086fd has been sitting queued (fork approval) since it was pushed, so this PR has no green build of its own yet either. That resolves itself on the next push.
Verified as addressed (no action needed)
- Typography: no em or en dashes anywhere in the diff (I scanned every non-ASCII character in it), including the
guides.ymlsubtitle,introduction.adoc,gettingStarted.adoc, andsummary.adoc. guides.ymlTOC now uses the object form withtitle:for every entry, includingrequirements.- Author name: mononym accepted, no change needed.
- SMTP config filename now agrees across the sample app, the vendored snippet, the listing caption, and the prose, all
grails-app/conf/application.yml. - The injected
== Verify CIheading is gone; it is normal prose now. - The
application.ymlsnippet includes the requiredgrails:parent, so the fragment can be copied as shown. cd ../completecorrected in bothgettingStarted.adocandtesting.adoc.controller.adoccarries a prominent demonstration-only IMPORTANT block covering authentication, authorization, recipient controls, and abuse or rate limiting before the curl example.- All 12 TOC keys still map to chapter files, and every
include::resolves to a vendored snippet.
The guide asserted a validation-error path of /mail/send but never showed the template that produces it, so a reader following the chapters kept the generated errors view and got /send instead. Vendor grails-app/views/errors/_errors.gson and explain the convention that resolves it, including why a view: argument is ignored on that path. Re-vendor MailController.groovy from the companion sample application so the guide no longer publishes recipient addresses and subjects in an INFO log, and no longer teaches the view: argument that is discarded. Companion: grails-guides/grails-email#1 Assisted-by: claude-code:claude-opus-5
jamesfredley
left a comment
There was a problem hiding this comment.
All feedback rounds are addressed. I pushed one commit to finish the last blocker rather than send another round.
The stale MailController.groovy snippet is re-vendored from the companion head, so the guide no longer publishes recipient addresses and subject lines in an INFO log. All ten vendored snippets are now byte-identical to the sample app.
Reviewing the pair together also surfaced a gap worth closing before publication: the guide asserted a validation-error path of /mail/send but never showed the template that produces it, so a reader following the chapters kept the generated errors view and would have got /send and a failing spec. The chapter now vendors grails-app/views/errors/_errors.gson and explains the convention that resolves it, including that a view: argument is ignored on that path.
Companion grails-guides/grails-email#1 is green and merged alongside this.
Adds the Grails 8 email guide under
guides/grails-email/v8/(AsciiDoc chapters + vendored snippets) and registers the v8 version inconf/guides.ymlwith TOC.Companion to grails-guides/grails-email#1