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.

(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)

(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.

(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.

(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.

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()

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.

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.

This would be really useful to MozReview.

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.

(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?

(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. :)

(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

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.

(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.

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.

Comment on attachment 8548289 [details] [diff] [review] minor_update_v2.diff Review of attachment 8548289 [details] [diff] [review]: ----------------------------------------------------------------- r=dylan

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.

(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.

(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.

dylan: please commit

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git 592e6fd..1d96fa1 master -> master

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".

No, it's the former. The idea is that certain updates are never worthy of bugmail.