364177 - On attachment and bug creation, if *one* requestee cannot see the bug, *all* requests are cancelled

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Frédéric Buclin]

The bug summary says it all: on attachment and bug creation, if *one* requestee cannot see the bug, *all* requests are cancelled, including those being perfectly valid. This is because Flag::validate() dies and all remaining validations are skipped and no flag is created.

If a requestee cannot see the bug, then the requestee should be dropped but the request itself (as well as all other valid requests) should stay. This is going to make developers crazy if we don't fix it.

Comment 1

[Frédéric Buclin]

Thanks Selenium for finding this bug. :)

Comment 2

[Frédéric Buclin]

Attachment: patch, v1

ERROR_MODE_WARN is an intermediate state between ERROR_MODE_WEBPAGE and ERROR_MODE_DIE. Throw*Error(), when called, stores error messages in Bugzilla->error_queue and returns, so that you can still process further. This is useful for minor notifications which you want to display later, such as requestees who cannot see the bug or the attachment. I keep ERROR_MODE_DIE before calling Bugzilla::Flag::validate(), because all other errors must be fatal, i.e. something really unexpected happened and the safest is to ignore all changes made to flags.

Comment 3

[Frédéric Buclin]

Comment on attachment 248952 [details] [diff] [review]
patch, v1

I think we should have two reviews on this one to be sure we do things correctly and cleanly.

Comment 4

[Max Kanat-Alexander]

Comment on attachment 248952 [details] [diff] [review]
patch, v1

Instead of error_queue, please use the code I wrote in bug 350105.

Comment 5

[Frédéric Buclin]

I will commit bug 364216 first, which is more critical than this one. The dependency is because they conflict.

Comment 6

[Frédéric Buclin]

Attachment: patch, v2

I updated Bugzilla->errors so that it can easily be called by the not yet existing Error::rethrow_error() method.

Comment 7

[Max Kanat-Alexander]

Comment on attachment 249460 [details] [diff] [review]
patch, v2

>+    # If we want to throw the error later, we have to keep these information.
>+    Bugzilla->errors->{'type'} = ($name =~ /code/i) ? 'ThrowCodeError' : 'ThrowUserError';
>+    Bugzilla->errors->{'name'} = $error;
>+    Bugzilla->errors->{'vars'} = $vars;

  If this really is an error queue, then these should be stored as part of a hash inside errors, which should be returning an array, yes?

  See, what I'm thinking is that Bugzilla->errors should always be an array, and it should just contain a lot of "error objects", which represent details.

  In fact, we could make Bugzilla::Error into a real object, and just push those into the array. I think that'd be even better. Then we could call ->throw or ->message or whatever, on those objects. I think that would handle all the rest of my comments on this code, too.

>+sub errors {
> [snip]
>+    if ($message) {
>+        push(@{$errors->{queue}}, $message);
>+    }

  You never made queue into an arrayref there, so that will die.

Comment 8

[Frédéric Buclin]

Attachment: patch, v3

I know use a real array for the error queue.

Comment 9

[Max Kanat-Alexander]

Comment on attachment 252374 [details] [diff] [review]
patch, v3

  Now that I've actually tested the code, I think that this is too much code to solve a problem that isn't really that bad.

  That is, this only triggers when one requestee isn't in the *group* that can see the bug.

  We could eventually fix this by doing requestee validation as part of Bugzilla::Bug->create, which is where it should be done anyhow. Or we could do the validation before filing the bug.

  But all of this code just to work around a problem that won't happen that often...I don't think we should do that.

  I also don't really think this is a blocker anymore.

  Here are the comments that I had on the patch in general:

>+sub errors {
>+    foreach my $error (@error_queue) {
>+        # Some unexpected system errors such as "No such file or directory"
>+        # may appear when checking the existence of a file. That's definitely
>+        # something we don't want to display. So keeping Perl errors for now.

  In that case should we even queue system errors?

>+    my $msg;
>+    if (Bugzilla->usage_mode == USAGE_MODE_BROWSER && scalar(@msgs)) {
>+        $msg = '<ul><li>' . join('</li><li>', @msgs) . '</li></ul>';
>+    }

  I really think that we should always return the array and we should format the output in TT, but if you really think this is the best way to go, then OK.

Comment 10

[Max Kanat-Alexander]

As I said above, I no longer think this is a blocker. We haven't had anybody actually run into this badly on bmo, and I don't think that it will badly affect most, if any, installations.

I think that we should live with it in 3.0, and fix it in a decent way on the trunk for 3.2.

We could possibly check in this fix on the 3.0 *branch* when we have that, but I don't want it on the trunk.

Comment 11

[Frédéric Buclin]

I still want it fixed in 3.0. A minimal fix would be to silently skip unauthorized requestees, which would be non-invasive.

Comment 12

[Max Kanat-Alexander]

Okay, let's do that, then, for now. We can do a better fix with more architecture, later.

Comment 13

[Frédéric Buclin]

Attachment: patch, v4

Silently skip unauthorized requestees.

Comment 14

[Max Kanat-Alexander]

Comment on attachment 253944 [details] [diff] [review]
patch, v4

Okay, I trust that this works.

Comment 15

[Frédéric Buclin]

Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.183; previous revision: 1.182
done
Checking in Bugzilla/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.45; previous revision: 1.44
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.81; previous revision: 1.80
done