add the ability to disable sending of mail when updating bugs

RESOLVED FIXED in Bugzilla 6.0

Status

()

Bugzilla
Creating/Changing Bugs
P1
enhancement
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: glob, Assigned: Pami Ketolainen)

Tracking

(Blocks: 3 bugs, {relnote})

unspecified
Bugzilla 6.0
relnote
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
while this has been filed a large number of times, i'm opening a new bug to track work following consensus reached in the last bugzilla meeting that this feature is one that we want to implement now in bugzilla.

in order for a bug changer to tag an update as "minor" to not trigger bugmail we require:

- a checkbox on show_bug under the comment:
  "this is a minor update (do not send email)"
  - i think it's important that the wording here states that it's for
    minor updates, as well as the results of this checkbox

- a similar checkbox on the bulk bug update page

- support for a parameter to the bug.update api call

- an admin parameter for groups allowed to use this feature
  - defaults to editbugs
  - an empty group disables the feature

- a user preference to allow users to receive minor bug updates


due to the way bugmail is generated, this is trickier than it first seems.  because all updates between last-diffed and last-updated are sent as one email, we need to avoid a situation where a major change is somehow unsent at the time the change was made, and then left unsent because a minor update to the bug is made next.

one way of addressing this is to check if last-diffed matches last-updated prior to calculating the set of changes.  if they do not match, then do not honour the "minor update" preference, and treat all changes as triggering bugmail.

Comment 1

4 years ago
(In reply to Byron Jones ‹:glob› from comment #0)
> - support for a parameter to the bug.update api call

And to all other API calls that could send e-mail eg. Bug.add_attachment.

> - a user preference to allow users to receive minor bug updates

Default should be off (i.e. don't receive minor bug updates)
(Reporter)

Updated

4 years ago
Assignee: create-and-change → pami.ketolainen

Comment 2

4 years ago
(In reply to sgreen from comment #1)
> Default should be off (i.e. don't receive minor bug updates)

Default should be ON, to match the current behavior. If users are tired by minor updates (with a very vague definition of "minor"), then they can turn the user pref off. But if it's off by default, users won't even know they are missing something.

Also, I don't think "editbugs" is a good group by default. Too many users have these privs for various reasons. Default should be empty (i.e. disabled) or editcomponents, IMO.

Updated

4 years ago
Severity: normal → enhancement

Comment 3

4 years ago
(In reply to Frédéric Buclin from comment #2)
> (In reply to sgreen from comment #1)
> Default should be ON, to match the current behavior. If users are tired by
> minor updates (with a very vague definition of "minor"), then they can turn

I disagree. That defeats the whole purposes of the change we are going to make.

> Also, I don't think "editbugs" is a good group by default. Too many users
> have these privs for various reasons. Default should be empty (i.e.
> disabled) or editcomponents, IMO.

This I agree with. The default should be empty. I know for brc, we will probably only enable it for users with the 'redhat' group (which is most employees). As alluded to above, Joe Partner and Joe User can't be trusted to use this correctly.
(Reporter)

Comment 4

4 years ago
(In reply to sgreen from comment #3)
> (In reply to Frédéric Buclin from comment #2)
> > (In reply to sgreen from comment #1)
> > Default should be ON, to match the current behavior.
> 
> I disagree. That defeats the whole purposes of the change we are going to
> make.

i also disagree. in the bugzilla meeting we agreed to add an _opt-out_ ability, which means this should be "off" by default.

> > Also, I don't think "editbugs" is a good group by default. Too many users
> > have these privs for various reasons. Default should be empty.
> 
> This I agree with. The default should be empty.

no see issues with defaulting to an empty group here.
(Assignee)

Comment 5

4 years ago
Created attachment 8490264 [details] [diff] [review]
minor_update_v1.diff

The implementation turned out to be not so simple, when taking into account the possible unsent changes and not using any shortcuts for passing the parameters.

I think it could be made in a cleaner way, but it would probably require quite some refactoring around Bug::update() and BugMail::Send()
Attachment #8490264 - Flags: review?(glob)
(Reporter)

Updated

4 years ago
Target Milestone: --- → Bugzilla 6.0
Comment on attachment 8490264 [details] [diff] [review]
minor_update_v1.diff

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

Fwiw, as a user I'd expect a new line in the e-mail prefs matrix, or at the _least_ the pref being IN that same e-mail prefs page.

Specifically I'd expect a "but NOT when (overrides above):" ... "the change author suggests no mail" or some such.
(Reporter)

Comment 7

4 years ago
Comment on attachment 8490264 [details] [diff] [review]
minor_update_v1.diff

after a quick read, this patch looks excellent.

i'm going to pass the full review over to dylan, who has a little more time than me to do it.
Attachment #8490264 - Flags: review?(glob) → review?(dylan)

Comment 8

4 years ago
This would be really useful to MozReview.
Priority: -- → P1

Comment 9

4 years ago
Related to that, we'd want minor updates to be allowed for anyone in the WebService, or at least an option for that.  As glob said, you're tall enough to ride if you use the API; we can keep the restriction on group membership just in the UI.
Comment on attachment 8490264 [details] [diff] [review]
minor_update_v1.diff

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

First, great patch.

There are two problems:

1) It's bit-rotted a bit, so you'll want to rebase it on the current master. This is my fault for taking so long to review it.
2) The method Bugzilla::Bug->has_unsent_changes() needs to be documented -- not just stubbed. 

I'm not overly concerned with the location of the pref, per Callek's review.

If you can resolve those two things it'll be a certain r+ from me.

::: Bugzilla/Bug.pm
@@ +4881,4 @@
>  
>  =item target_milestone
>  
> +=item has_unsent_changes

As this is a new method, it needs to be documented before it can be accepted into the code base.
Attachment #8490264 - Flags: review?(dylan) → review-
(Assignee)

Comment 11

4 years ago
(In reply to Dylan William Hardison [:dylan] from comment #10) 
> I'm not overly concerned with the location of the pref, per Callek's review.

I actually agree, that it would probably make more sense to put it in the "but NOT when"-section of the email preferences matrix.

And as the minor_update parameter is already passed all the way to User->wants_bug_mail() method, it should be fairly easy to change the implementation this way. Only thing that would be lost in this approach is the ability to change the site wide default for the setting (afaik, there is no admin controllable defaults for the email preferences). But I guess that is not so important feature in this case?
Status: NEW → ASSIGNED
(In reply to Pami Ketolainen from comment #11)
> (In reply to Dylan William Hardison [:dylan] from comment #10) 
> > I'm not overly concerned with the location of the pref, per Callek's review.
> 
> I actually agree, that it would probably make more sense to put it in the
> "but NOT when"-section of the email preferences matrix.
> 
> And as the minor_update parameter is already passed all the way to
> User->wants_bug_mail() method, it should be fairly easy to change the
> implementation this way. Only thing that would be lost in this approach is
> the ability to change the site wide default for the setting (afaik, there is
> no admin controllable defaults for the email preferences). But I guess that
> is not so important feature in this case?

I think site administrators may want the ability turn off the minor edits feature. If I'm understanding you correctly. 

Needinfo-ing dkl as he has vastly more experience in the needs/use-cases of bz. :)
Flags: needinfo?(dkl)
(Assignee)

Comment 13

4 years ago
(In reply to Dylan William Hardison [:dylan] from comment #12) 
> I think site administrators may want the ability turn off the minor edits
> feature. If I'm understanding you correctly. 

The feature can be disabled completely by setting the minor_update_group to empty.
But for user email preferences there isn't configurable defaults like there is for "normal" user preferences in the admin interface.
(In reply to Pami Ketolainen from comment #13)
> (In reply to Dylan William Hardison [:dylan] from comment #12) 
> > I think site administrators may want the ability turn off the minor edits
> > feature. If I'm understanding you correctly. 
> 
> The feature can be disabled completely by setting the minor_update_group to
> empty.
> But for user email preferences there isn't configurable defaults like there
> is for "normal" user preferences in the admin interface.

I agree that this would be better suited as a email preference that can be turned off/on 
instead of as a general preference.

dkl
Flags: needinfo?(dkl)
Hi Pami, do you think you'll have time to submit an updated patch for review anytime soon?  The BMO team could use this feature for a few different things, so if you don't have time, we may take it over.
Flags: needinfo?(pami.ketolainen)
(Assignee)

Comment 16

4 years ago
(In reply to Mark Côté [:mcote] from comment #15)
> Hi Pami, do you think you'll have time to submit an updated patch for review
> anytime soon?  The BMO team could use this feature for a few different
> things, so if you don't have time, we may take it over.

I have already the implementation updated to use the email preferences, I'll just need to rebase and test it. So yes, I'll try to get the patch in ASAP.
Flags: needinfo?(pami.ketolainen)
(Assignee)

Comment 17

4 years ago
Created attachment 8548289 [details] [diff] [review]
minor_update_v2.diff

Here is the updated patch.
* Rebased on master
* Added pod entry for the has_unsent_changes() method
* Moved the option to receive minor update mails from general user preferences to user email preferences.
Attachment #8490264 - Attachment is obsolete: true
Attachment #8548289 - Flags: review?(dylan)
Comment on attachment 8548289 [details] [diff] [review]
minor_update_v2.diff

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

r=dylan
Attachment #8548289 - Flags: review?(dylan) → review+
Flags: approval?

Updated

4 years ago
Keywords: relnote

Updated

4 years ago
Flags: approval? → approval+

Comment 19

4 years ago
Can this feature also be added to places in the admin interface that can cause bulk changes to bugs?  For example, if a flag is deleted or excluded from a component, the flag is removed from bugs, causing a flood of notification emails.  The admin making that change should have the option to suppress the emails.

Comment 20

4 years ago
(In reply to Jason McDonald from comment #19)
> Can this feature also be added to places in the admin interface that can
> cause bulk changes to bugs?

File a new bug for that feature please. This bug is ready for release.

Comment 21

4 years ago
(In reply to Simon Green from comment #20)
> (In reply to Jason McDonald from comment #19)
> > Can this feature also be added to places in the admin interface that can
> > cause bulk changes to bugs?
> 
> File a new bug for that feature please. This bug is ready for release.

Done -- Bug 1134528.
Duplicate of this bug: 23924

Comment 23

3 years ago
dylan: please commit

Updated

3 years ago
Blocks: 1092341
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   592e6fd..1d96fa1  master -> master
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Comment 25

3 years ago
Does this mean if a user doesn't want minor updates, the user will never see them?  Or will the user see a summary of all the minor updates once a major update has occurred?  Can't quite tell from the patch. 

I prefer the latter, as it ensures the user will eventually see all changes, including accidental "minor updates".

Updated

3 years ago
Blocks: 1144473
No, it's the former.  The idea is that certain updates are never worthy of bugmail.

Updated

3 years ago
No longer blocks: 1092341

Updated

3 years ago
Blocks: 1233014
You need to log in before you can comment on or make changes to this bug.