Bug 1883428 - Improve the needinfo email situation - #2698
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} || []}
| requestee_id => $_->{requestee_id}, | ||
| requester_id => $_->{requester_id}, | ||
| status => $_->{status}, | ||
| type => _flatten_object($_->{type}), |
There was a problem hiding this comment.
flatten_to_hash reads Bugzilla::Object::_serialization_keys, a per-request cache only filled by new_from_hash / new_from_list / _do_list_select. _get_flag_mail_events builds the flag type with Bugzilla::FlagType->new({id => ...}), which does not fill it, so in a process that never list-selects flag types this hits @{undef} and dies. reachable via scripts/sendunsentbugmail.pl -> BugMail::Send -> enqueue. Bugzilla::FlagType->new_from_list([$row->{type_id}])->[0] in _get_flag_mail_events fixes it, or carry type_id and name as plain scalars instead of flattening the object
| } | ||
|
|
||
| if (!scalar(@display_diffs) && !scalar(@send_comments)) { | ||
| if (!scalar(@display_diffs) && !scalar(@send_comments) && !scalar(@$flag_events)) { |
There was a problem hiding this comment.
sendMail can now emit a flag-only mail (no diffs, no comments), but dequeue's "if nothing remains to send" bail near line 784 still tests only diffs and new_comments, so on the mailer-queue path that mail is silently dropped. that guard needs || @{$vars->{flag_events}} too
| )) | ||
| { | ||
| my $wants_mail; | ||
| if ($relationship == REL_FLAG_REQUESTEE) { |
There was a problem hiding this comment.
flag recipients now run through the generic recipient loop, which skips on $user->is_bug_ignored($id). the old notify() only checked email_enabled, so a needinfo request aimed at someone who has the bug on their ignore list now disappears with no mail at all. if that is intentional it is worth a note, otherwise the flag rels should bypass the ignore check the way they bypass wants_bug_mail
|
|
||
| # Bug 1883428: recipients added because of a flag change, independently of | ||
| # any other role they may hold on the bug. | ||
| use constant REL_FLAG_REQUESTEE => 6; # A flag was requested of them |
There was a problem hiding this comment.
putting these in RELATIONSHIPS is needed for the X-Bugzilla-Reason header, but it also feeds Bugzilla::User::create and userprefs.cgi, which both iterate relationships(). every new account now gets email_setting rows for rel 6/7/8 across all POS/NEG events, and userprefs deletes them again on the next save because the prefs template renders no checkboxes for them. these rels bypass wants_bug_mail entirely so the rows never do anything, worth skipping them in those two loops
| To: [% to %] | ||
| Subject: [[% terms.Bug %] [%+ bug.id %]] [% bug.short_desc %] | ||
| Date: [% date %] | ||
| X-Bugzilla-Type: request |
There was a problem hiding this comment.
no threadingmarker here, so flag-type cc_list mail stops threading with the rest of the bug's mail. the removed request/email.txt.tmpl emitted one
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