382978 - Various cleanup in process_bug.cgi

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Frédéric Buclin]

Attachment: patch, v1

We have objects and these objects have methods. Use them instead of doing our own SQL queries in process_bug.cgi again.

Marc, feel confident enough and have time to review it?

Comment 1

[Frédéric Buclin]

Attachment: patch, v1.1

unbitrotten patch

Comment 2

[Marc Schumann [:Wurblzap]]

Comment on attachment 267043 [details] [diff] [review]
patch, v1.1

Tested randomly; works as far as I can tell.

>Index: process_bug.cgi
>@@ -254,19 +251,11 @@ if ($cgi->cookie("BUGLIST") && defined $
> # At this point, the product must be defined, even if set to "dontchange".
> defined($cgi->param('product'))
>   || ThrowCodeError('undefined_field', { field => 'product' });
> 
>-if ((defined $cgi->param('id') && $cgi->param('product') ne $oldproduct)
>+if ((defined $cgi->param('id') && $cgi->param('product') ne $bug->product)
>      || (!$cgi->param('id')
>          && $cgi->param('product') ne $cgi->param('dontchange')))
> {
>@@ -274,6 +263,7 @@ if ((defined $cgi->param('id') && $cgi->
>         ThrowUserError('comment_required');
>     }
>     # Check to make sure they actually have the right to change the product
>+    my $oldproduct = (defined $cgi->param('id')) ? $bug->product : '';

Suggestion: if you move the |my $oldproduct| line back out of the |if| block, you can simplify the |if| condition considerably. No need to check for |defined $cgi->param('id')|, just to check |$cgi->param('product')| against |$oldproduct| and against |$cgi->param('dontchange')|. This would give readability a little boost.

>@@ -284,26 +274,25 @@ if ((defined $cgi->param('id') && $cgi->
>+    my $check_can_enter = 1;
>+    if ($product) {
>+        $check_can_enter =
>+          $dbh->selectrow_array("SELECT 1 FROM bugs
>+                                 WHERE product_id != ?
>+                                 AND bugs.bug_id IN
>+                                 (" . join(',', @idlist) . ") " .
>+                                 $dbh->sql_limit(1),
>+                                 undef, $product->id);
>+    }
>+    if ($check_can_enter) { $user->can_enter_product($product_name, 1) }

Let me make sure I understand this. If process_bug.cgi was unable to generate a product object from $product_name, then you skip the check whether the product changes at all. We assume that there is no product named $product_name, so you have $user->can_enter_product throw the error. Right?

>@@ -953,7 +936,8 @@ Bugzilla::Bug->check_status_change_trigg
>+# DuplicateUserConfirm = 1 only if a single bug is being edited.

$vars->{DuplicateUserConfirm} is true only if...
This is the only reason for r-.

>@@ -1832,27 +1811,22 @@ foreach my $id (@idlist) {
>+        my $dup = new Bugzilla::Bug($duplicate);

Suggestion, although it's rather a separate bug: while you're at it, do you think you can run s/duplicate/duplicate_of/g on process_bug.cgi? The variable naming is a little backwards by calling the original bug (the one we're about to dupe to) the duplicate bug. It's really the original bug, and the bug we're currently modifying is the duplicate bug. I find this confusing.

>             # The reporter is oblivious to the existence of the new bug and is permitted access

s/new bug/original bug/ (not your fault, just in addition to the above)

Comment 3

[Marc Schumann [:Wurblzap]]

Bitten by bug 365490. Sorry for the spam; blame said bug.

Comment 4

[Frédéric Buclin]

(In reply to comment #2)
> >+# DuplicateUserConfirm = 1 only if a single bug is being edited.
> 
> $vars->{DuplicateUserConfirm} is true only if...
> This is the only reason for r-.

Bah, getting a r- for a comment to fix. :(
/me remembers the good old time of "fix that on checkin..." reviews.

Comment 5

[Marc Schumann [:Wurblzap]]

Comment on attachment 267043 [details] [diff] [review]
patch, v1.1

You're right, "fix on check-in" would have been appropriate. I'm sorry; here you go. I seem to have dulled my edge ;)

Comment 6

[Frédéric Buclin]

Cool, thanks Marc.

(In reply to comment #2)
> Suggestion: if you move the |my $oldproduct| line back out of the |if| block,
> you can simplify the |if| condition considerably. No need to check for |defined
> $cgi->param('id')|, just to check |$cgi->param('product')| against
> |$oldproduct| and against |$cgi->param('dontchange')|. This would give
> readability a little boost.

I will have a look. My idea was to define $oldproduct only if we need it, i.e. on product change only.


> Let me make sure I understand this. If process_bug.cgi was unable to generate a
> product object from $product_name, then you skip the check whether the product
> changes at all. We assume that there is no product named $product_name, so you
> have $user->can_enter_product throw the error. Right?

Yes, right.


> $vars->{DuplicateUserConfirm} is true only if...
> This is the only reason for r-.

I will fix that.

Comment 7

[Frédéric Buclin]

I couldn't find a compact way to use $oldproduct in the IF condition without a possible regression. So I left this condition alone. Also, I didn't rename $duplicate as it was used in many places (much more places than what I was thinking about). I didn't want to bitrot existing patches in the review queue with this minor change. I fixed other comments, though.


Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.364; previous revision: 1.363
done