Bug 1883428 - Improve the needinfo email situation - #2698
Open
Xzzz wants to merge 3 commits into
Open
Conversation
…necessary emails) Remove Flag::notify()'s separate text-only flag emails and instead add flag requestee/requester and per-flagtype cc_list addresses to the recipient list built by BugMail.pm, so flag grant/deny/needinfo notices ride along with the normal bug-change email instead of arriving as a second, plaintext-only message (also fixes bug 1410772). - Add REL_FLAG_REQUESTEE/REL_FLAG_REQUESTER/REL_FLAG_TYPE_CC relationships; requestee/requester respect the existing EVT_FLAG_REQUESTED/EVT_REQUESTED_FLAG opt-in, type_cc is unconditional (matches notify()'s prior behavior) - Add a flag-events section to bugmail.txt.tmpl/bugmail.html.tmpl, with more descriptive wording for the needinfo requestee - Preserve cc_list addresses with no Bugzilla account via a small side path (bugmail-flagtype-cc.txt.tmpl) sent directly through MessageToMTA
…templates
The previous commit added a flag-event section to the core bugmail templates, but
extensions/BMO/template/.../email/bugmail.{txt,html}.tmpl fully overrides those
core templates, so the new section was never executed. Also fixes a real gap the
core-only version missed: needinfo's normal resolution (auto-clear to flag status 'X'
when the requestee replies) wasn't handled (only +/- were) so the single most common
needinfo outcome silently produced no "your request was answered" notice. notify()
covered this case; now BugMail.pm does too, with matching "cleared" wording alongside
"granted" and "denied".
- Port the flag-event section into the BMO override templates (txt + html), with
a Hook.process('flag_event', ...) extension point mirroring the old request/email.txt.tmpl
hook mechanism
- Add Needinfo and Splinter hook fragments preserving their previous content
(reporter-aware wording + wiki guide link; Splinter review-tool link, now gated
on attachment.can_review so it also covers GitHub PR/Phabricator attachments,
not just ispatch)
- Inline BMO's own external-redirect (GitHub PR) attachment link directly in the
BMO templates rather than hooking it: hooking it caused two fragments (BMO + Splinter)
to concatenate with no separator when both fire for the same event, since TT's TRIM
strips whitespace at each independently compiled hook fragment's own edges.
The original design avoided this by having the parent template render that piece
directly instead of through the shared print-hook slot
- Resolve the attachment object in _get_flag_mail_events() so hooks can check
can_review/external_redirect
- Remove the three hook fragments orphaned by the previous commit's deletion of
request/email.txt.tmpl
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Needinfo and other flag notifications (request/grant/deny/clear) were previously sent as a separate email via
Flag::notify(). This decouples that: flag events are now rendered as a section inside the normal bugmail, sent throughBugzilla/BugMail.pm, instead of a standalone notify() email.Bugzilla/BugMail.pm:_get_flag_mail_events()gathers flag activity (requested/answered, including needinfo auto-clear on reply - statusX, which the old notify()-based flow handled but was initially missed here) and resolves the related attachment so templates can checkcan_review/external_redirecttemplate/en/default/email/bugmail.{txt,html}.tmpland the BMO overrideextensions/BMO/template/en/default/email/bugmail.{txt,html}.tmplrender aflag_eventssection, with aHook.process('flag_event', ...)extension pointextensions/BMOfully overrides the core bugmail templates on this instance, so the flag-event content is ported into bothflag_eventhook, replacing the ones orphaned by the earlier removal ofrequest/email.txt.tmpl. Splinter's review link now gates onattachment.can_reviewinstead ofispatch, so it also covers GitHub PR/Phabricator attachments, matching other call sites in the codebaseTRIMstrips each hook fragment's edges independently and concatenating BMO's + Splinter's fragments with no separator produced garbled text when both fire on the same eventTest plan
perl -con modified Perl filesFlag::notify/hook fragmentsReferences