186994 - Unable to accept a new bug that has been assigned.

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P1)

Description

[Matthew Buckett]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2.1) Gecko/20021130
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2.1) Gecko/20021130

I have been assigned a bug and the status of this bug it still "NEW". I am
unable to accept the bug so it becomes "ASSIGNED". Currently I have no
permissions in bugzilla, the only options I get are to leave as new, resolve the
bug (x2) and re-assign the bug (x2).

Reproducible: Didn't try

Steps to Reproduce:
I'm unable to try to reproduce it, due to my permissions in bugzilla.

Comment 1

[Dave Miller [:justdave]]

Confirming.  visual examination of the template indicates it would indeed show
this behaviour.  The template is erroneously excluding the assignee of the
ability to change the bug to "assigned" if they don't have canconfirm.

Comment 2

[Jacob Steenhagen]

We talked about this in IRC.  The issue is that edit.html.tmpl will not show the
ASSIGNED option unless you are in canconfirm.  It should also show if you are
the owner of the bug.  process_bug.cgi already has the code in it to allow the
owner to change anything a user with editbugs can.

This appears to be a regression from bug 151714.


http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl&rev=1.28&mark=484#481

Comment 3

[Gervase Markham [:gerv]]

Attachment: Patch v.1

This should fix all occurrences of this problem in the file.

Note: one artefact of the current (unchanged) logic is that if someone has
editbugs but not canconfirm (reasonably unlikely), and isn't the assignee, they
currently can't accept the bug at all, even if it wouldn't involve confirming
it.

Gerv

Comment 4

[Gervase Markham [:gerv]]

Taking.

Gerv

Comment 5

[Gervase Markham [:gerv]]

Comment on attachment 110737 [details] [diff] [review]
Patch v.1

Pretty trivial, this, but important...

Gerv

Comment 6

[Bradley Baetz (:bbaetz)]

Comment on attachment 110737 [details] [diff] [review]
Patch v.1

This is wrong. Its wrong because includes a check for assigned_to in |sub user|

If you want someone to be able to assign a bug to themselves, then you need to:

a) check the backend allows it

b) change line 469 of the template to have an additional test specifically for
user==assignee. Actually, rewriting that block entirly may be better, to make
it match what the backend says. The user being teh assignee doesn't give them
carte blanche to do anything they want, just most things. (Well, evrything
except confirm, I think)

Comment 7

[Gervase Markham [:gerv]]

bbaetz: I can't parse your first sentence as English.

And this is not about someone being able to assign a bug to themselves; it's
about giving the current assignee full powers to change the bug, even if they
don't have editbugs and/or canconfirm. I've done this at the top of the template
to make sure all cases are covered, and to minimise maintenance headaches.

They can assign it to themselves if they have editbugs - in which case, they
have full change powers anyway.

So I'm very confused :-|

Gerv

Comment 8

[Bradley Baetz (:bbaetz)]

Bah, forgot to cc myself.

Anyway, the problem is that if I have a bug assigned to me, then I cannot
confirm it if I do not have canconfirm.

Also, consider this block:

      [% IF bug.bug_status != "ASSIGNED" && bug.user.canconfirm %]
        <input type="radio" name="knob" value="accept">
        Accept bug (
        [% "confirm bug, " IF bug.isunconfirmed %]change
        status to <b>ASSIGNED</b>)
        <br>
        [% knum = knum + 1 %]
      [% END %]

This needs to be redone to allow someone to ASSIGN a bug if they are the
assignee, but not confirm it. IOW, the conditional needs to move the canconfirm
user check from the outer IF to the inner one.

Note that bug.canconfirm is set to true by Bug.pm if they only have editbugs,
since thats what DoConfirm checks.

Comment 9

[Gervase Markham [:gerv]]

Again, I'm having parsing problems.

> Anyway, the problem is that if I have a bug assigned to me, then I cannot
> confirm it if I do not have canconfirm.

Are you saying this is a problem with my patch (doesn't look like it to me), or
are you restating the problem that this bug is about?

> This needs to be redone to allow someone to ASSIGN a bug if they are the
> assignee, but not confirm it. 

I don't agree. If someone is the assignee, they should be able to do anything
they like to their bug, including confirm it. Hence my fix, which makes it so
that it looks like the assignee has canconfirm and editbugs, which is what the
permission-checking code thinks they have already.

> Note that bug.canconfirm is set to true by Bug.pm if they only have editbugs,
> since thats what DoConfirm checks.

So you are saying that part of my patch is redundant?

Gerv

Comment 10

[Bradley Baetz (:bbaetz)]

You're entitled to not agree :) However, you'd also be disagreing with the code,
which isn't a good idea.

Don't forget that if the bug is ASSIGNED, then you're not going to know that
everconfirmed isn't set (unless its reassigned/reopened) so this isn't a problem
from that point of view.

My point is that your patch gives the impressions that the assignee can confirm
the bug (either via that block I mentioned, or just be selecting the 'confirm'
button, or evne reassign + confirm), whilst the code doesn't appear to agree.
Note that I havne't tested this, and that code is a bit ugly, so ICBW.

Comment 11

[Gervase Markham [:gerv]]

> You're entitled to not agree :) However, you'd also be disagreing with the code,
> which isn't a good idea.

There must be something about this bug number...

When you say "disagreeing with the code", do you mean the code as it is now in
the template, the code as it would be after the patch went in, the code in the
CGI which validates the template results, or some other code?

As I remember it, when we cleaned up the permissions model and did the comment
thing in CheckCanChangeField(), we said the owner should be able to change
anything (see around line 411 of process_bug.cgi.) My patch to the template
merely makes the template reflect what the user is able to do.

Gerv

Comment 12

[Bradley Baetz (:bbaetz)]

The code in DoConfirm is diffrent to the code in CheckCanChangeField

Comment 13

[Gervase Markham [:gerv]]

OK... so I'd say what we do in CheckCanChangeField is the sane thing, and any
code which doesn't do that should be changed to match it or, better still, use
the same function.

Gerv

Comment 14

[Bradley Baetz (:bbaetz)]

Well, should I be able to confirm my own bug? I don't think so - every so often
people assign a bug to themselves accidentally (although possibly not now that
we have the helper), and when soemone resets it to the default it shouldn't stay
confirmed.

Comment 15

[Gervase Markham [:gerv]]

This does create the "loophole" that someone could hack the form submission from
format=guided to assign a newly-created bug to themselves, and thereby get
editbugs on that bug. Is this really a problem? They aren't really bothering
anyone if their bug's bogus. 

Worse, IMO, is the current problem where a new contributor (without editbugs)
has a bug assigned to him (because he asks for it) and then can't do things with
it that he wants to do.

Gerv

Comment 16

[Bradley Baetz (:bbaetz)]

Sure, but our bug-change checking stuff hasn't ever tried to be a fully secure
situation. We fixed some of that (reporter can no longer set target_milestone)
but its certainly not iron-clad.

Anyway, I think we're talking past each other - the assignee should be able to
do anything with the bug except confirm it. Thats what the code has. Currently
the UI matches neither reality, or what you want to he code to do, so lets make
it match relatiy, and you can file a separate bug to allow the assignee to
confirm the bug, if you want.

Comment 17

[Gervase Markham [:gerv]]

Attachment: Patch v.2

OK, this is what we want.

A while back, when we did the CheckCanChangeField changes, we went through a
process of deciding exactly what each role should be able to do. This patch
fixes some places where either the UI or the code differs from that scheme. To
be precise:

- Makes assignees and qa contacts able to see the controls for doing anything
- Makes them able to confirm bugs for which they are the assignee or QA
contact, 
  by rewriting DoConfirm() in terms of CheckCanChangeField().

This seems very sane to me. Confirmation was excluded (in the scheme we
decided) for reporters on their own bugs, but not for anyone else.

Gerv

Comment 18

[Gervase Markham [:gerv]]

Comment on attachment 111646 [details] [diff] [review]
Patch v.2

bbaetz: how does this grab you?

Gerv

Comment 19

[Bradley Baetz (:bbaetz)]

Comment on attachment 111646 [details] [diff] [review]
Patch v.2

Well, this is fine assuming that we do want to change these semantics.

However, wouldn't it be better to make these changes to Bug.pm::user, where teh
rest of the stuff is set?

Comment 20

[Gervase Markham [:gerv]]

Attachment: Patch v.3

Good point. How's this, then?

Gerv

Comment 21

[Gervase Markham [:gerv]]

Comment on attachment 111686 [details] [diff] [review]
Patch v.3

Dave: could you confirm that this change is desireable, before I do any more
work? :-)

Gerv

Comment 22

[Bradley Baetz (:bbaetz)]

Comment on attachment 111686 [details] [diff] [review]
Patch v.3

This looks fine, but dave should comment

Comment 23

[Dave Miller [:justdave]]

makes sense to me.

Comment 24

[Bradley Baetz (:bbaetz)]

Comment on attachment 111686 [details] [diff] [review]
Patch v.3

Hang on a sec. CheckCanChangeField lets thorugh bug_status changes for the
user-has-canconfirm case, but not 'canconfirm'. so this will be false, and so
everconfirmed won't be set.

Or am I missing something?

Comment 25

[Bradley Baetz (:bbaetz)]

Comment on attachment 111686 [details] [diff] [review]
Patch v.3

grr. You didn't test this. I know you didn't test this, because the code
as-written doesn't work - see bug 188161.

_Please_ test your patches before attaching and asking for review...

Comment 26

[Gervase Markham [:gerv]]

I did test this - I filed two UNCONFIRMED bugs, and made a person with no privs
qa_contact and assignee respectively. Before the fix, that person couldn't alter
those bugs, and after it, he could. That seemed like the right behaviour, so I
attached the patch.

However, if you say it's wrong, I will look more closely at it.

Gerv

Comment 27

[Bradley Baetz (:bbaetz)]

They should have always been able to alter the bug, just not confirm it.

Comment 28

[Gervase Markham [:gerv]]

Attachment: Patch v.4

How about this, then? Sorry for the delay...

Gerv

Comment 29

[Bradley Baetz (:bbaetz)]

Comment on attachment 114583 [details] [diff] [review]
Patch v.4

This looks fine. r=bbaetz if you tested/etc

Comment 30

[Gervase Markham [:gerv]]

Dave: we need this for any release :-)

Gerv

Comment 31

[Gervase Markham [:gerv]]

Checked in.

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.178; previous revision: 1.177
done
Checking in Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.29; previous revision: 1.28
done

Gerv