388557 - Make "Change Several Bugs At Once" appear if the user can edit *any* bugs in the list

Status: NEW

(Bugzilla :: Query/Bug List, enhancement, P3)

Description

[timeless]

Attachment: changes that i believe are appropriate

davefr complains about this. i believe he's right.

i believe he's right

Comment 1

[Frédéric Buclin]

Comment on attachment 272760 [details] [diff] [review]
changes that i believe are appropriate

>Index: mozilla/webtools/bugzilla/buglist.cgi

>+    ++$caneditbugs if Bugzilla->user->in_group('editbugs', $bug->{'product'});

$bug->{'product'} contains the product name while User->in_group() requires the product ID.


>+$vars->{'caneditbugs'} = $caneditbugs;

This won't help as buglist.cgi will throw an error if you are not in the global editbugs group. Look for 'dotweak' at the beginning of this script.



mozilla/webtools/bugzilla/template/en/default/reports/create-chart.html.tmpl

>-[% IF user.in_group('editbugs') %]
>+[% IF user.in_group('editbugs') || user.groups.editbugs %]

Irrevelant as both notation are equivalent.

Comment 2

[timeless]

Attachment: closer

i need help figuring out how to do the last template change. I'm willing to punt on it and get it later. I don't think it's critical.

Comment 3

[Teemu Mannermaa (:wicked)]

Comment on attachment 273387 [details] [diff] [review]
closer

What happens when the change multiple list contains some bugs user can't edit but selects them for change? Should the checkboxes or even bug rows be omitted for bugs user can't change?

>Index: buglist.cgi
>===================================================================

You should really test your patches before submitting them:

#   Failed test 'buglist.cgi --ERROR'
#   at t/001compile.t line 101.
not ok 3 - buglist.cgi --ERROR
syntax error at buglist.cgi line 1049, near ") {"
syntax error at buglist.cgi line 1061, near "}"
buglist.cgi had compilation errors.

>+        && Bugzilla->user->in_group(
>+             'editbugs',
>+             new Bugzilla::Product({name => $bug->{'product'}})) {

Looks to me that this gives the in_group a product object when it expects only a product id.

>+if ($dotweak && $caneditbugs < 2) {
>+    ThrowUserError("auth_failure", {group  => "editbugs",

This could probably do with a comment to say that there has to be atleast one bug the user can edit for the tweak to work. Also, should the error message make it more clear our rules for the user?

>Index: template/en/default/reports/create-chart.html.tmpl
>===================================================================

I can't see how this change related to this bug about "Change Several Bugs At Once". Please, move this change to the bug it belongs to or create a new bug for it.

Besides, your changes don't pass tests:

not ok 245 - (en/default) template/en/default/reports/create-chart.html.tmpl has unfiltered directives:
#   265: \# IF !editbugs
#   266: \# FOREACH product = products
#   267: \# IF user.in_group('editbugs', product.id)
#   268: \# editbugs = 1
#   270: \# END
#   271: \# END
#   272: \# IF editbugs
#   273: \# restriction = "&product=" _ editable_products.join(',')
#   274: \# END
#   275: \# END
# --ERROR

Comment 4

[timeless]

Attachment: less stuff

this bug is really about letting group editbugs work.
there are two places where it's broken:
1. buglist (tweak)
2. charting

I didn't finish fixing charting, but I wanted to checkpoint my work (my mac eats my homework - literally, there's a reaper script that *did* kill my work which means I would have lost it had I not posted it here). editbugs should really be fixed for both cases. But I'm fine with leaving this here so that I can get it fixed in another bug. I guess i needed to use [%# instead of [% #] it doesn't matter.

Comment 5

[Teemu Mannermaa (:wicked)]

(In reply to comment #4)
> this bug is really about letting group editbugs work.

Then you should have said so in summary or description. I have no way knowing what some "davefr" has complained about something somewhere in the world..

Comment 6

[Teemu Mannermaa (:wicked)]

Comment on attachment 274131 [details] [diff] [review]
less stuff

>Index: buglist.cgi
>===================================================================
>+        my $product = new Bugzilla::Product({name => $bug->{'product'}});

This crashes with error "Bad argument param sent to Bugzilla::Product::new function." if I don't have product column selected for my buglist output. This is because product has not been read from the DB and so $bug->{'product'}} will be undefined. You need to make sure product column is read from the DB for your code to work, see buglist.cgi starting from line 736.

Also, I still think something should be done for checkboxes in tweak mode for buglist lines user doesn't have right to change. It will just frustrate users when their multiple change dies with authority errors if they select bugs they don't have access to. Especially when the error doesn't make clear what bug the error is about. :(

Comment 7

[Frédéric Buclin]

(In reply to comment #4)
> this bug is really about letting group editbugs work.
> there are two places where it's broken:
> 1. buglist (tweak)
> 2. charting

No, this bug is about 1. only. Don't mix everything!

Comment 8

[timeless]

Attachment: bugs bugs bugs

Comment 9

[Frédéric Buclin]

Comment on attachment 274400 [details] [diff] [review]
bugs bugs bugs

>-    my $bug = {}; # a record
>+    my $record = {}; # a record

All these changes about renaming a variable irritates me. Not only this makes the patch much bigger and makes us loose the focus on the main part of the patch, but it also unnecessarily bitrots other patches awaiting review or local customizations.


>+    $bugowners->{$bug->{'assigned_to'}} = 1;
>+    my $product = new Bugzilla::Product({name => $bug->{'product'}});

Don't use internal variables of objects. We have methods for this!

Comment 10

[Frédéric Buclin]

I talked about this with justdave on IRC, and we agreed that the condition to accept this RFE is that clicking the "Change several bugs at once" link only redisplays bugs which are in a product where the user has editbugs privs. We will first implement bug 413439.

Comment 11

[Dave Miller [:justdave]]

midaired with LpSolit... here's what I tried to post:

OK, here's my take on how this ought to work...  fix bug 413439 first.  Once
that's done, then this bug can further loosen that to be "if any" instead of
"if all".  When the dotweak form is loaded, any bugs that the user doesn't have
permission to edit will be removed from the buglist, and a message put at the
top saying why they were removed.  This way it preserves the intent without
having to force the user to re-run their query to contain only products they
can edit.

Comment 12

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.