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
…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
- 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
| ? Bugzilla::Attachment->new({id => $_->{attachment_id}, cache => 1}) | ||
| : undef, | ||
| } | ||
| } @{$vars->{flag_events}} |
There was a problem hiding this comment.
unguarded deref of $vars->{flag_events}. any bug_mail job already sitting in TheSchwartz when this deploys was enqueued without a flag_events key, so dequeue dies with "Can't use an undefined value as an ARRAY reference" and those mails are lost across retries. use @{$vars->{flag_events} || []}
There was a problem hiding this comment.
Fixed: guarded with @{$vars->{flag_events} || []}
| # that's now been granted/denied, becomes a recipient even without any | ||
| # other role on the bug. Flag-type cc_list addresses (admin-configured, | ||
| # bypasses per-user opt-in) are added the same way notify() used to. | ||
| my @flag_events |
There was a problem hiding this comment.
the guard you fixed in sendMail and in dequeue is still missing from the third copy at line 147, which runs before this block:
return {'sent' => []} unless scalar(@diffs) || scalar(@$comments);flag changes normally write a flagtypes.name bugs_activity row, so @diffs is non-empty and i can't hand you a repro through the ui for this one on its own. still worth || @flag_events for consistency with the other two, and because this return also skips _send_flag_type_cc_raw_mail at line 464. would need @flag_events hoisted above the return, $start/$end are already in scope there
|
|
||
| [% FOREACH event = flag_events %] | ||
| [% IF event.action == 'requested' %] | ||
| [% IF event.type.name == 'needinfo' %] |
There was a problem hiding this comment.
branching on event.type.name == 'needinfo' puts a bmo-only flag type into an upstream core template.
the flag_event hook this pr adds is the right place for it, and extensions/Needinfo/template/en/default/hook/email/bugmail-flag_event.txt.tmpl already fires on exactly this condition, it just contributes the follow-up sentences rather than the headline.
suggest moving the headline into the needinfo hook and letting the generic ELSE wording stand in core. same applies to bugmail.html.tmpl and to both bmo overrides, where the special case is duplicated again
There was a problem hiding this comment.
Removed the needinfo special case from the core text/HTML templates and from both BMO overrides, so every flag request uses the generic "has requested from you" line. The "needs more information from you to work on this bug" sentence now opens the Needinfo flag_event hook, followed by the existing reporter-aware wording and the user-guide link.
=> Fixed in "Bug 1883428 - Move the needinfo headline into the Needinfo flag_event hook"
| $user_cache->{$row->{setter_id}} | ||
| ||= Bugzilla::User->new({id => $row->{setter_id}, cache => 1}); | ||
|
|
||
| if ($row->{status} eq '?' && $row->{requestee_id}) { |
There was a problem hiding this comment.
flag type cc_list no longer gets mail for ? with no requestee or a direct +/- (e.g. approval-* requests), which notify() used to send, so release lists lose these notices
Do you remember what we discussed about how to handle the arbitrary plain text accounts in cc_list since they may or may not be real bmo accounts?
We can either manually add them to the cc of the emails as-is or look up each one to see if a real account exists and then add it normally.
I have an (old) bug that is suppose to make the text field a mapping table of real bmo accounts but it was never done.
|
|
||
| # Bugzilla::BugMail uses this module, so load it lazily. | ||
| require Bugzilla::BugMail; | ||
| Bugzilla::BugMail::Send($_, {changer => Bugzilla->user}) |
There was a problem hiding this comment.
this runs a full Send() for every affected bug inside the admin web request, so an inclusion/exclusion edit that touches many bugs can hang or time out, could this be queued or batched instead
There was a problem hiding this comment.
This may be queued in jobqueue anyway and claude messed up. Can you verify?
| package main; | ||
|
|
||
| ## no critic (Variables::ProtectPrivateVars) | ||
| my $visible = \&Bugzilla::BugMail::_flag_event_visible_to; |
There was a problem hiding this comment.
only _flag_event_visible_to() is tested, please add coverage for _get_flag_mail_events() (prev ? check, X auto-clear) and recipient selection
This branch has not been deployed
2 participants
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