matrei commented on PR #16452: URL: https://github.com/apache/grails-core/pull/16452#issuecomment-5949523593
Third round, reviewed at head `2af4f3d978`. The base is `8.0.x` at `d13aa33282`. The merge-base is still `626c81dca3`, and the branch merges cleanly. What I ran locally: - `:grails-mail:test` with `cleanTest --no-build-cache`: all 22 `MailMessageBuilderSpec` features pass, and so do the other specs in the module. - `:grails-mail:codenarcMain`, `codenarcTest` and `checkstyleMain` are clean. - My local spec that sends through `MailService.sendMail()` with only `overrideAddress` set. As expected, it still shows that an explicit `from` is replaced. That is now the intended and documented behaviour, so nothing more to do there. Thanks, this round resolves the main point. The code, the tests and the documentation now agree that `overrideAddress` also replaces an explicit `from`, and that applications that want to keep their real sender should switch to `overrideToAddress`. What's left is about how the documentation is written. ## Findings ### 1. `mailTesting.adoc`: the closing paragraph now refers to the wrong example The original closing paragraph is still there: "All `to`, `cc` and `bcc` addresses will be replaced by this value if set. The advantage of this mechanism is…". It now follows the `overrideFromAddress` example, so "this value" reads as `[email protected]`, and `overrideFromAddress` never touches recipients. "Overriding All Addresses" already says what `overrideAddress` does to `to`, `cc` and `bcc`, so I'd drop that first sentence. The advantage and disadvantage sentences apply to all three properties, so they can stay where they are. ### 2. The guide pages describe the change, not the behaviour `mailConfiguration.adoc` and `mailTesting.adoc` are reference pages for 8.x, but they use wording that only makes sense right after the upgrade: "Two new properties give finer-grained control…" and "Note that `overrideAddress` **now** also replaces a `from`…". After 8.0.0 ships, a reader sees "new" and "now" with no indication of what they're compared to. The comparison with Grails 7 belongs in the upgrade note only. In `mailConfiguration.adoc`, the sentence that said what `overrideAddress` does to the sender was removed in this round instead of corrected. The page now says it only in the "now also" note. I'd say it directly, right after the `overrideAddress` example, for example: > When `overrideAddress` is set, every `to`, `cc` and `bcc` address and the sender are replaced by this value. That includes a `from` set in the `sendMail` closure. `replyTo` and `envelopeFrom` are not changed. > > To override only the recipients or only the sender, use `overrideToAddress` or `overrideFromAddress`: > > * `grails.mail.overrideToAddress` replaces every `to`, `cc` and `bcc` address. It's also used as the `to` address when the `sendMail` closure doesn't set one. > * `grails.mail.overrideFromAddress` replaces the sender, including a `from` set in the `sendMail` closure. It's also used as the `from` address when the closure doesn't set one. > > When `overrideAddress` is set as well, `overrideToAddress` and `overrideFromAddress` take precedence over it. The same paragraphs are currently pasted into four places: `mailConfiguration.adoc`, `mailTesting.adoc`, the upgrade note and the skill. If the behaviour changes again, all four will need to change together. I'd keep the full description in `mailConfiguration.adoc` only. In `mailTesting.adoc`, a short sentence per property with a link back is enough, for example `See <<mailConfiguration,Configuration>> for how the three properties interact.` ### 3. Upgrade note (section 81): put the behaviour change first Thanks for moving the section to 81. Two things about its content: - **Heading:** "`overrideAddress` Split Into Separate To and From Overrides" suggests that `overrideAddress` was replaced or removed. It still exists. What affects existing applications is that it now also replaces an explicit sender. - **Order:** the note opens with the two new properties, and the change that affects existing applications comes in the last paragraph, after "Note that…". An upgrade note is read by someone checking whether their application is affected, so that should come first, followed by what to do about it. Something along these lines: > ==== 81. Mail Plugin: `overrideAddress` Also Replaces an Explicit Sender > > `grails.mail.overrideAddress` now also replaces a `from` set in the `sendMail` closure. In Grails 7, it replaced every `to`, `cc` and `bcc` address and the default sender, and an explicit `from` was kept. > > To keep the sender your application sets, while still redirecting every recipient, use the new `grails.mail.overrideToAddress` in place of `overrideAddress`: > > *(the existing `overrideToAddress` YAML example)* > > To set a fixed sender without redirecting recipients, use the new `grails.mail.overrideFromAddress`. See <<mailConfiguration,Mail Configuration>> for how the three properties interact. The second YAML example (`overrideFromAddress` only) can then go, since it isn't an upgrade action. ### 4. Skill entry: shorter, and under "Configuration Changes" The entry is a new top-level section between "Spring Boot 4 and Spring Framework 7 Code Changes" and "Configuration Changes", and it repeats the full guide text. The other entries in the skill are short instructions for the agent, grouped by area. A single bullet under "Configuration Changes" is enough: > - If `grails.mail.overrideAddress` is set, it now also replaces a `from` set in `sendMail`. To keep the application's sender while still redirecting every recipient, use `grails.mail.overrideToAddress` in its place. The commented-out `overrideFromAddress` line in the YAML example can go along with it. ### 5. Smaller points - **Formatting:** in the "Note that…" sentence, `sendMail` is missing its backticks. The sentence appears in all four places. - **PR description:** the "Documentation Changes" list leaves out `mailConfiguration.adoc`, and it gives the skill path as `.agents/skills/grails-8-upgrade/SKILL.md`. The file the PR changes is `grails-skills/upgrade-guide-8/skills/grails-8-upgrade/SKILL.md`. ## Resolved since the second round - Code, tests and documentation agree that `overrideAddress` also replaces an explicit `from`, and the upgrade note and the skill no longer say that nothing changes. - The duplicate `overrideAddress` feature is gone. The one that's left checks both the sender and the recipients, and "backward-compatibility" is gone from its name. - `mailTesting.adoc` keeps the original disadvantage again: "makes it difficult to test address determination logic". - The upgrade note is appended as section 81, so section 80 keeps its number, and the paragraph about custom `MailMessageBuilder` workarounds is gone. ## Verified as correct - `MailMessageBuilder` and `MailConfigurationProperties` haven't changed since the second round, and what I verified there still holds. A specific override wins over `overrideAddress` in both directions, for explicit and for default addresses. `overrideToAddress` alone leaves the sender alone, and `overrideFromAddress` alone leaves `to`, `cc` and `bcc` alone. - Profile-specific files such as `application-test.yml` and `application-development.yml`, used in the new examples, work, because Grails environments map to Spring profiles. The logging guide already relies on the same thing. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
