Last Comment Bug 823153 - Duplicate bugs are created when requestee confirmation is required and SKIP_REQUESTEE_ON_ERROR is disabled
: Duplicate bugs are created when requestee confirmation is required and SKIP_R...
Status: NEW
:
Product: bugzilla.mozilla.org
Classification: Other
Component: General (show other bugs)
: Production
: All All
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
: 833960 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-19 11:23 PST by Jeff Muizelaar [:jrmuizel]
Modified: 2013-01-23 13:44 PST (History)
5 users (show)
See Also:
Due Date:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
screenshot 1 (108.93 KB, image/png)
2013-01-15 15:35 PST, David Lawrence [:dkl]
no flags Details
screenshot 2 (76.62 KB, image/png)
2013-01-15 15:36 PST, David Lawrence [:dkl]
no flags Details

Description Jeff Muizelaar [:jrmuizel] 2012-12-19 11:23:51 PST
This seems to happen when you create a bug with an attachment and request review on someone who doesn't exist. Bugzilla gives you a long complainy message and asks you to reattach the file. Doing so seems to create two bugs.

This has happened to me twice in the past two days with the following bugs:
823149, 823148
822143, 822142
Comment 1 Frédéric Buclin 2012-12-19 11:32:25 PST
Definitely not an upstream bug. If you ask someone who doesn't exist, your requestee is ignored and the bug + attachment submitted anyway.
Comment 2 Byron Jones ‹:glob› [PTO until 2017-01-09] 2012-12-19 17:44:50 PST
(In reply to Frédéric Buclin from comment #1)
> Definitely not an upstream bug. If you ask someone who doesn't exist, your
> requestee is ignored and the bug + attachment submitted anyway.

you forgot about SKIP_REQUESTEE_ON_ERROR.



i can reproduce this on trunk with SKIP_REQUESTEE_ON_ERROR set to 0

if you enter a requestee which matches more than one user, you're presented with two confirmation pages.

the first page is the "select the correct user" page.
the second is the "invalid token" page.
Comment 3 Frédéric Buclin 2012-12-20 04:37:37 PST
This is a constant which is not supposed to be edited. In fact, this constant is not editable. So you hacked the source code in bmo. Not an upstream bug (upstream, I would mark it INVALID).
Comment 4 David Lawrence [:dkl] 2012-12-20 10:10:05 PST
(In reply to Frédéric Buclin from comment #3)
> This is a constant which is not supposed to be edited. In fact, this
> constant is not editable. So you hacked the source code in bmo. Not an
> upstream bug (upstream, I would mark it INVALID).

Then why do we have the constant there at all if setting it to the opposite value causes issues. If what you are saying is true, then we should just remove the constant and have the core code always skip on requestee error. 

At BMO, it was requested that we fail if the requestee does not exist so that the proper requestee can always be set. So we changed the value to 0 and thought it would work as expected by the description of the constant. Apparently it doesn't so we will have to fix it on our end and push the fix upstream. 

I just dont think that INVALID is appropriate here unless you are saying the constant should be removed which I don't think it should. It should work as expected based on the value set by SKIP_REQUESTEE_ON_ERROR.

dkl
Comment 5 Frédéric Buclin 2012-12-20 11:02:04 PST
(In reply to David Lawrence [:dkl] from comment #4)
> Then why do we have the constant there at all if setting it to the opposite
> value causes issues. If what you are saying is true, then we should just
> remove the constant and have the core code always skip on requestee error. 

We have a constant because we don't want to throw an error when attaching a new file. For existing attachments, it's fine to throw an error, because we are not loosing the attachment being uploaded. That's why this code is written like this:

    my ($flags, $new_flags) = Bugzilla::Flag->extract_flags_from_cgi(
                                  $bug, $attachment, $vars, SKIP_REQUESTEE_ON_ERROR);

which is clearer than written like that:

    my ($flags, $new_flags) = Bugzilla::Flag->extract_flags_from_cgi(
                                  $bug, $attachment, $vars, 1);

where one wouldn't understand what 1 means. But in no way is this constant supposed to be removed nor changed to 0.


> At BMO, it was requested that we fail if the requestee does not exist so

Upstream, it was a clear decision that we didn't want to stop the upload process just for that. The requestee can be set again once the attachment is fully uploaded.


> that the proper requestee can always be set. So we changed the value to 0
> and thought it would work as expected by the description of the constant.

There is no description for this constant which says that you can change it to 0 to force errors to be thrown.


> I just dont think that INVALID is appropriate here unless you are saying the
> constant should be removed which I don't think it should. It should work as
> expected based on the value set by SKIP_REQUESTEE_ON_ERROR.

You are misinterpreting what you can do with this constant. It's like editing USER_MATCH_SUCCESS => 1 in User.pm to 0 and expecting that everything will work as expected. The code would fail miserably, because this constant is not supposed to be set to a false value. So yes, I'm really saying that this bug is invalid despite I say the constant should not be removed.
Comment 6 David Lawrence [:dkl] 2012-12-20 12:38:02 PST
Thanks. I understand better now the reasoning for the constant. As for BMO, we either need 

1) revert back to allowing the attachment to be created even with an incorrect requestee user and make it more obvious what needs to be done to fix it by the user, or
2) figure out a way to upload the attachment to a staging area on the filesystem, throw the error asking for another requestee value or to drop the flag, then resubmitting the bug using the attachment previously uploaded. 

1) will obviously be easier but 2) should probably be done eventually for other reasons but will be more work involved.

dkl
Comment 7 Byron Jones ‹:glob› [PTO until 2017-01-09] 2013-01-07 09:49:52 PST
looks like the cure is worse than the disease here.
let's revert SKIP_REQUESTEE_ON_ERROR to 1 before this week's push.
Comment 8 David Lawrence [:dkl] 2013-01-07 13:17:26 PST
Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bmo/4.0
modified Bugzilla/Flag.pm
Committed revision 8431.

Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bmo/4.2
modified Bugzilla/Flag.pm
Committed revision 8480. 

Leaving this bug open as we still have an issue that needs to be addressed here.
dkl
Comment 9 David Lawrence [:dkl] 2013-01-15 15:35:57 PST
Created attachment 702556 [details]
screenshot 1
Comment 10 David Lawrence [:dkl] 2013-01-15 15:36:23 PST
Created attachment 702557 [details]
screenshot 2
Comment 11 David Lawrence [:dkl] 2013-01-23 13:44:49 PST
*** Bug 833960 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.