990980 - create an extension for server-side filtering of bugmail

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, defect)

Description

[:glob ✱]

spun off bug 978850:

we want the ability to give users the ability to opt out of receiving changes to particular fields.

UI:

add a section to the 'email preferences' tab of user preferences:

--8<-----

** Ignore Bugmail **

You can instruct Bugzilla to never notify you for changes to specific fields.

    Field: [ list of field descs, empty selected as default, but mandatory v]
  Product: [ product list, __Any__ selected as default v]
Component: [ component list, __Any__ selected as default v]
           ( Add )

You are currently filtering on:

    Field         Product  Component
[ ] QA Whiteboard __Any__  __Any__
    ( Remove Selected )

--8<-----

implementation:

add a new table:

  bugmail_filters
    id           primary key autoinc
    user_id      fk --> profiles.userid, not null
    field_id     fk --> fielddefs.id, not null
    product_id   fk --> products.id, nullable
    component_id fk --> components.id, nullable

  product_id and component_id are null if __Any__ is selected

implement logic to update this table when the user preferences change.  a lot of this logic can be lifted from the component watching extension.

filtering needs to happen in the mailer_before_send hook, by inspecting the following headers in the email:
  To
  X-Bugzilla-Changed-Field-Names
  X-Bugzilla-Product
  X-Bugzilla-Component

load the user from the "To" field, load filters from the bugmail_filters table, and cross-check.  emails should only be dropped if a single field is changed.

to drop an email set its "to" header to an empty string.

Comment 1

[:glob ✱]

in bug 1009138 eshan asked for the ability to limit component watching.  this could be implemented here by adding the inclusive/exclusive filtering:

--8<-----

** Filter Bugmail **

You can instruct Bugzilla to filter bugmail based on the field that was changed.

  Product: [ product list, __Any__ selected as default v]
Component: [ component list, __Any__ selected as default v]
    Field: [ list of field descs, empty selected as default, but mandatory v]
   Action: [ Exclude | Include v] (action description)
           ( Add )

You are currently filtering on:

    Field         Product  Component Action
[ ] QA Whiteboard __Any__  __Any__   Exclude
    ( Remove Selected )

--8<-----

changing the action select updates the action description text:
  exclude: Ignore bugmail where just the selected field is changed
  include: Ignore bugmail unless the selected field is changed

  bugmail_filters
    id           primary key autoinc
    user_id      fk --> profiles.userid, not null
    field_id     fk --> fielddefs.id, not null
    product_id   fk --> products.id, nullable
    component_id fk --> components.id, nullable
    action       int    0 = exclude, 1 = include

multiple fields can be entered for the same product/component, so you can do something like "only send me bugmail for the firefox product where the priority or severity is changed".

exclusions always take priority over inclusions.

Comment 3

[:glob ✱]

we'll need to write some documentation on this on the wiki, as some of the fields are not obvious.

for example, if you wanted to only receive bugmail for new bugs, you have to filter on the "bug_id" field.

Comment 4

[(no longer active)]

I almost cried when I read comment 0 and 1!  Thanks so much for doing this!

Comment 5

[Matthew N. [:MattN]]

I think the relationship to the bug is relevant in the filtering but it doesn't seem like that's addressed by the current plan.

My use case:
I want to see changes to the QA whiteboard when I'm the assignee (or perhaps component watcher) of the bug but not if I'm just CC'd.

Comment 6

[:glob ✱]

(In reply to Matthew N. [:MattN] from comment #5)
> I think the relationship to the bug is relevant in the filtering but it
> doesn't seem like that's addressed by the current plan.

thanks - an excellent suggestion; i should be able to integrate that.

Comment 7

[:glob ✱]

Attachment: 990980_1.patch

adds server-side bugmail filtering

Comment 8

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #7)
> Created attachment 8438519 [details] [diff] [review]
> 990980_1.patch
> 
> adds server-side bugmail filtering

I have started reviewing this now.

dkl

Comment 9

[David Lawrence [:dkl]]

Comment on attachment 8438519 [details] [diff] [review]
990980_1.patch

Review of attachment 8438519 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly nit pick comments so and all can be fixed on commit barring any other issues. Finished code review and basic sanity. Just finishing up on thorough manual testing. We really need a test suite for this at some point.

dkl

::: extensions/BugmailFilter/Extension.pm
@@ +108,5 @@
> +    }
> +
> +    # remove all tracking flag fields.  these change too frequently to be of
> +    # value, so they only add noise to the list.
> +    my @tracking_flags = Bugzilla::Extension::TrackingFlags::Flag->get_all();

Not used. remove

::: extensions/BugmailFilter/lib/Constants.pm
@@ +15,5 @@
> +    IGNORE_FIELDS
> +    FIELD_DESCRIPTION_OVERRIDE
> +    FILTER_RELATIONSHIPS
> +    TRACKING_FLAG_TYPES
> +    TRACKING_FLAG_TYPES_SORTED

Do not see these being declared below anywhere so should be removed.

@@ +26,5 @@
> +
> +use constant FAKE_FIELD_NAMES => [
> +    {
> +        name        => 'comment.created',
> +        description => 'Comment Created',

Nit: Change to 'Comment created' for consistency with other field descriptions.

@@ +30,5 @@
> +        description => 'Comment Created',
> +    },
> +    {
> +        name        => 'attachment.created',
> +        description => 'Attachment Created',

Nit: 'Attachment created' (same reason above)

::: extensions/BugmailFilter/lib/Filter.pm
@@ +82,5 @@
> +}
> +
> +sub field_name {
> +    my ($self) = @_;
> +    return $self->{field_name} //= '';

Nit: Can be shortened to 

return $_[0]->{field_name} //= '';

Same with others. Not a blocker.

@@ +104,5 @@
> +}
> +
> +sub relationship {
> +    my ($self) = @_;
> +    return $self->{relationship};

return $_[0]->{relationship};

::: extensions/BugmailFilter/template/en/default/account/prefs/bugmail_filter.html.tmpl
@@ +106,5 @@
> +    </select>
> +    <span id="action_exclude" class="action_desc bz_default_hidden">
> +      Ignore bugmail where just the selected field is changed
> +    </span>
> +    <span id="action_include" class="action_descbz_default_hidden">

missing space between action_desc and bz_default_hidden.

@@ +134,5 @@
> +      <th>Relationship</th>
> +      <th>Action</th>
> +    </tr>
> +    [% FOREACH filter = filters %]
> +      <tr>

Nit: Use alternating row coloring as the text blends together when many rows are present.

Comment 10

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #9)
> ::: extensions/BugmailFilter/lib/Constants.pm
> @@ +15,5 @@
> > +    IGNORE_FIELDS
> > +    FIELD_DESCRIPTION_OVERRIDE
> > +    FILTER_RELATIONSHIPS
> > +    TRACKING_FLAG_TYPES
> > +    TRACKING_FLAG_TYPES_SORTED
> 
> Do not see these being declared below anywhere so should be removed.

odd; all of these constants are declared.

Comment 11

[:glob ✱]

(In reply to Byron Jones ‹:glob› from comment #10)
> (In reply to David Lawrence [:dkl] from comment #9)
> > ::: extensions/BugmailFilter/lib/Constants.pm
> > > +    TRACKING_FLAG_TYPES
> > > +    TRACKING_FLAG_TYPES_SORTED
> > 
> > Do not see these being declared below anywhere so should be removed.

ah, just the TRACKING_FLAG_TYPES ones are missing.

Comment 12

[David Lawrence [:dkl]]

Comment on attachment 8438519 [details] [diff] [review]
990980_1.patch

Review of attachment 8438519 [details] [diff] [review]:
-----------------------------------------------------------------

Functionally works fine. Just a few minor issue to fix with a new revision.

dkl

::: Bugzilla/BugMail.pm
@@ +408,5 @@
>      my @changedfieldnames = uniq map { $_->{field_name} } @display_diffs;
>  
> +    # BMO: Add a field to indicate when a comment was added
> +    if (grep($_->type != CMT_ATTACHMENT_CREATED, @send_comments)) {
> +        push(@changedfields, 'Comment Created');

Comment created

::: extensions/BugmailFilter/Extension.pm
@@ +171,5 @@
> +    return unless $$wants_mail;
> +
> +    my $filters = Bugzilla::Extension::BugmailFilter::Filter->match({
> +        user_id => $user->id
> +    });

Need to request cache this list as it executes the same SQL up to 5 times per recipient depending on the value of $wants_mail.

::: extensions/BugmailFilter/lib/Constants.pm
@@ +60,5 @@
> +
> +# relationship / int mappings
> +# _should_drop() also needs updating when this const is changed
> +
> +use constant FILTER_RELATIONSHIPS => [

How much work would it be to add Mentor, Not Mentor?

::: extensions/BugmailFilter/template/en/default/account/prefs/bugmail_filter.html.tmpl
@@ +28,5 @@
> +  [% FOREACH comp = prod.components %]
> +    [% IF comp.watch_user %]
> +      if (!watch_users['[% prod.name FILTER js %]'])
> +        watch_users['[% prod.name FILTER js %]'] = new Array();
> +      watch_users['[% prod.name FILTER js %]']['[% comp.name FILTER js %]'] = '[% comp.watch_user.login FILTER js %]';

This generates *a lot* of redundant JS code. Instead do:

[% FOREACH prod = selectable_products %]
  cpts['[% n %]'] = [
    [%- FOREACH comp = prod.components %]'[% comp.name FILTER js %]'[% ", " UNLESS loop.last %] [%- END -%] ];
  [% n = n + 1 %]
  [% array_created = 0 %]
  [% FOREACH comp = prod.components %]
    [% IF comp.watch_user %]
      [% IF !array_created %]
        watch_users['[% prod.name FILTER js %]'] = new Array();
        [% array_created = 1 %]
      [% END %] 
      watch_users['[% prod.name FILTER js %]']['[% comp.name FILTER js %]'] = '[% comp.watch_user.login FILTER js %]';
    [% END %]
  [% END %]
[% END %]

But then I looked some more and noticed that watch_users is not used anywhere as I can tell? Should we just remove the above completely?

::: extensions/BugmailFilter/template/en/default/hook/account/prefs/prefs-tabs.html.tmpl
@@ +7,5 @@
> +  #%]
> +
> +[% tabs = tabs.import([{
> +    name      => "bugmail_filter",
> +    label     => "Bugmail Filtering",

Should we call this Advanced Email or something less similar to Email Preferences? Just a thought.

Comment 13

[:glob ✱]

Attachment: 990980_2.patch

- added "Mentoring" and "Not Mentoring"
- removed unused code (watch_user, constants, etc)
- adds alternating background to filter list
- adds descriptions next to filter fields
- s/Created/created/ in fake field descriptions
- per-user filters hidden behind request_cache

> Should we call this Advanced Email or something less similar to Email
> Preferences? Just a thought.

i'd prefer to keep it named "bugmail filtering" because it's unambiguous and won't cause confusion should we add another email-related tab.

> > ::: Bugzilla/BugMail.pm
> > +    # BMO: Add a field to indicate when a comment was added
> > +    if (grep($_->type != CMT_ATTACHMENT_CREATED, @send_comments)) {
> > +        push(@changedfields, 'Comment Created');
> Comment created

i've left this as "Comment Created" to match the existing fake field "Attachment Created".

Comment 14

[David Lawrence [:dkl]]

Comment on attachment 8450832 [details] [diff] [review]
990980_2.patch

Review of attachment 8450832 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work as always. r=dkl

Comment 15

[:glob ✱]

schema only:

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   64d77ab..d741293  master -> master

Comment 16

[:glob ✱]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   d741293..2f3b5dd  master -> master