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.
There was a problem hiding this comment.
Fixed in "Bug 1883428 - Notify flag type cc_list of every flag change as notify() did"
Changes
- Every flag change now produces an event: a
?with no requestee, a direct+/-, or a clear not preceded by?becomes a genericsetevent. It adds no requestee or requester as a recipient, but still reaches the flag type'scc_list, likenotify()did cc_listusers get neutral wording. If acc_listuser isn't the person asked or the person who asked, they get thesetwording. Before, they got "from you" or "your request" wording or the needinfo text.- Templates: each of the 5 templates has a
setline for it: "<setter>set<flag><status>[for attachment N]". A clear shows "(cleared)" instead.
On addresses that aren't real accounts: we went with the option to add them as-is. Each cc_list entry is looked up. If it's a real account, it's added as a normal recipient. If not, it gets a separate plain-text notice sent straight to that address (_send_flag_type_cc_raw_mail / email/bugmail-flagtype-cc.txt.tmpl), because the normal mail pipeline only handles account ids.
=> Nothing is dropped, except that on group-restricted bugs or private attachments these addresses are skipped, same as notify() did.
|
|
||
| # 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?
There was a problem hiding this comment.
Checked: it was only partially queued, so the concern was valid ;)
With use_mailer_queue on, only the final render and delivery went to the job queue (sendMail() inserts one bug_mail job per recipient). Everything before that in Send() ran inline in the admin request for every affected bug: bug load, activity/comment/flag queries, recipient and watcher resolution, per-user permission and preference checks, and the bugmail_recipients hook.
The previous commit made it heavier on this path: every flag cleared by the edit now produces a set event, so Send() no longer returns early for those bugs.
Changes
FlagType::update: after the commit, it inserts a singlebug_mail_sendjob with the affected bug ids and the admin as changer, instead of callingSend()per bug. Whenuse_mailer_queueis off, it still sends inline as before, since there's no queue to use.- New
Bugzilla::Job::BugMailSend: runsSend()for each bug in the worker, as the admin. Bugs deleted since the job was queued are skipped. It reusesBugzilla::Job::Mailer's retry settings. A retry doesn't re-mail bugs already done, becauseSend()advanceslastdiffed. JobQueue.pm:bug_mail_sendadded toJOB_MAP. Workers register every mapped job, so nothing else is needed.
If someone changes one of these bugs before the job runs, that change's own Send() mails the pending flag events, and the job then finds nothing left to send for that bug.
=> Fixed in "Bug 1883428 - Queue bugmail for bugs affected by a flag type edit"
| 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
There was a problem hiding this comment.
Added DB-backed coverage in a new t/bmo/bugmail-flag-events.t. It needs real flag_activity rows, so it runs in test_bmo against MySQL, not in test_sanity like t/bugmail-flag-events.t (whose header now points to the new file). Passes locally
Each scenario uses a fresh bug with lastdiffed pinned, and inserts flag_activity rows at fixed times before, inside and after the mail window, so the window is deterministic. A dedicated flag type has a cc_list with one real account and one address with no account. Mail delivery is set to None, so Send() builds every recipient's mail and returns who it went to without sending anything.
Changes
_get_flag_mail_events(): one event per in-window row, classified asrequested/answered/set, rows outside the window ignored. It covers:- an answer finding its requester from a
?before the window (prev?check) - an
Xauto-clear in the same second as its?counting as an answer (tie broken on id) - clearing an already-granted flag becoming a
setwith no requester
- an answer finding its requester from a
- Recipient selection in
Send():- request: the requestee (no role on the bug) and the
cc_listaccount are mailed; the requester and an unrelated user aren't - answer: the requester is mailed, the person who answered isn't
- requester cancelling their own request: they aren't mailed,
cc_liststill is ?with no requestee: onlycc_listis mailed, and_get_flag_type_cc()puts the address with no account on the raw mail path
- request: the requestee (no role on the bug) and the
=> Fixed in "Bug 1883428 - Add DB-backed tests for flag mail events and recipients"
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