Repository navigation
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
dklawren
requested changes
Aug 18, 2026
…he immediately preceding requester
…attachment insider access
…g X status wording and missing body-headers - terms was never defined: added `[% PROCESS global/variables.none.tmpl %]` to fix it - status 'X' (cleared) rendered as "denied": now render as "cleared" (like the other templates) - added missing `@@body-headers@@` placeholder: without it, BMO's `_replace_placeholder_in_part` got nothing to substitute and these mails lose the body headers the old request/email.txt.tmpl carried
…achment visibility at dequeue - enqueue(): flatten flag_events before it hits the job queue - dequeue(): inflate flag_events and re-check attachment visibility at send time
dklawren
requested changes
Aug 20, 2026
- don't add the requester as recipient when they cleared their own request - give flag-type cc_list account holders the actual event content - don't drop flag-only bugmail when there are no diffs/comments - restore X-Bugzilla-Flag-Requestee header, carrying requestee/requester through the mailer-queue flatten/inflate cycle - break same-second ties on id when resolving a flag's previous status - don't credit a watcher with a role that was never actually inherited
- Added `package main;` at top of file - Added `## no critic (Variables::ProtectPrivateVars)` before the private-sub reference
dklawren
requested changes
Aug 21, 2026
dklawren
requested changes
Sep 18, 2026
dklawren
requested changes
Sep 25, 2026
dklawren
requested changes
Oct 8, 2026
| [%+ INCLUDE "email/header-common.txt.tmpl" %] | ||
| [% FOREACH event = flag_events %] | ||
| [% NEXT UNLESS event.action == 'requested' %] | ||
| X-Bugzilla-Flag-Requestee: [% event.requestee.email %] |
Collaborator
There was a problem hiding this comment.
request mails used to have their own subject and X-Bugzilla-Type: request, now they come in as changed so existing filters stop matching. granted/denied/cleared events get no marker header at all since this is only written for requested. worth keeping a header that marks all flag events
What if just add 'requested' to the previous X-Bugzilla-Type if a flag is requested. For users who do exact string matches they will need to change their filter rules but for those who substring matches, the old rule will still work.
| = $relationship == REL_FLAG_REQUESTEE | ||
| || $relationship == REL_FLAG_REQUESTER | ||
| || $relationship == REL_FLAG_TYPE_CC; | ||
| next if $is_bug_ignored && !$is_flag_relationship; |
Collaborator
There was a problem hiding this comment.
flag roles skip the ignore list (and BugmailFilter, since wants_bug_mail() isn't called for them) but the person still gets the full bugmail with every diff and comment. maybe send only the flag section when a flag role is their only reason
| # high (below the MEDIUMINT limit) to stay clear of real flags. | ||
| my $window_start = '2000-01-01 00:00:00'; | ||
| my $window_end = '2000-01-01 01:00:00'; | ||
| my $next_flag_id = 8_000_000; |
Collaborator
There was a problem hiding this comment.
fixed flag ids starting at 8_000_000 clash with rows left from an earlier run, since the look-back query in BugMail.pm only filters by flag_id. on a second run the old '+' row becomes the previous row and the grant is classed as set instead of answered. pick ids above MAX(flag_id) or add a bug_id condition
| [% USE Bugzilla %] | ||
| [% RETURN UNLESS event.action == 'requested' | ||
| && (event.type.name == 'review' || event.type.name == 'feedback') | ||
| && event.attachment && event.attachment.can_review %] |
Collaborator
There was a problem hiding this comment.
can_review is also true for phabricator attachments, which splinter can't review, so this sends a broken review link. the old check used ispatch, maybe ispatch || contenttype == 'text/x-github-pull-request' (same in the html template)
This branch has not been deployed
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
2 participants
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