The error message should be more accurate on who can change fields

RESOLVED FIXED in Bugzilla 2.20

Status

()

Bugzilla
Creating/Changing Bugs
P3
normal
RESOLVED FIXED
15 years ago
6 years ago

People

(Reporter: Usul, Assigned: Frédéric Buclin)

Tracking

unspecified
Bugzilla 2.20
Bug Flags:
approval +

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

15 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.5b) Gecko/20030901 Camino/0.7+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.5b) Gecko/20030901 Camino/0.7+

This bug is filled against the bugzilla version in production on
bugzilla.mozilla.org on the 09th September 2003. I created a bug for the Camino
product and wanted to set teh Target Milestone to 0.8. The bug is unconfirmed. I
get the error message :
" You tried to change the target_milestone field from --- to Camino0.8, but only
the owner or submitter of the bug, or a sufficiently empowered user, may change
that field."
But I am the bug submitter. I think the problem comes from that my bug is
"unconfirmed".

Reproducible: Always

Steps to Reproduce:
1.Create a "unconfirmed" bug.
2.Change Target Milestone.
3.You have a unfriendly/helpfull message :" You tried to change the
target_milestone field from --- to Camino0.8, but only the owner or submitter of
the bug, or a sufficiently empowered user, may change that field."

Actual Results:  
" You tried to change the target_milestone field from --- to Camino0.8, but only
the owner or submitter of the bug, or a sufficiently empowered user, may change
that field."

Expected Results:  
"The bus is not confirmed yet, thus milestone can't be targeted" or some similar
message.
This has been brought up repeatedly as a UI issue with Zippy as well.

We really need to find a way for the "CheckCanChangeField" sub to return more
specific errors than just a pass/fail on being able to change the field.  The
user really needs to be able to know which rule (s)he violated.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.18
Summary: Can't change Traget Milestone, Message repported is not helpfull → Can't change Target Milestone, Message reported is not helpful

Comment 2

15 years ago
Unloved bugs targetted for 2.18 but untouched since 9-15-2003 are being
retargeted to 2.20
If you plan to act on one immediately, go ahead and pull it back to 2.18.
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
(Assignee)

Comment 3

14 years ago
The error message (mainly about the submitter) will be more accurate as soon as
my patch for bug 266579 will be reviewed and checked-in.
(Assignee)

Updated

14 years ago
Assignee: myk → LpSolit
OS: MacOS X → All
Priority: P2 → P3
Hardware: Macintosh → All
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

14 years ago
*Fixed* by my patch in bug 266579 which landed a few minutes ago (I prefered to
merge the patches together; it was easier for me)! :)

Now, the error message will not mention the reporter anymore (or the owner in
some special cases) as being able to do things if he can't do these things!
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Summary: Can't change Target Milestone, Message reported is not helpful → The error message should be more accurate on who can change fields

Comment 5

14 years ago
Re-opened per bug 266579 comment 43 - see that comment for more info.

As far as I can see (couldn't check on landfill, as my account there has too 
many permissions!), the message given to the reporter when he tries to change 
the target milestone would be fixed by the patch on that bug, but if another 
user attempts the same change, she is still informed that the reporter could 
make that change.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 6

14 years ago
Stephen, please read my answer to your comment in bug 266579. As said there, you
convinced me to give a more accurate error message. ;)

My idea was to avoid useless tests if we already know the current user is *not*
the reporter. But as you say, this may be confusing for users. So ok, looks like
it is worth some additional tests... ;)

The patch is trivial, I will submit tonight or tomorrow...
Status: REOPENED → ASSIGNED
(Assignee)

Updated

14 years ago
Depends on: 266579

Comment 7

14 years ago
hmmm.... after reading your replies, I was tempted to suggest changing the 
wording of the message so that it doesn't say that the reporter definitively 
can make that change.

I didn't come up with anything that didn't sound clumsy while still being 
accurate though... and given that the fix is accomplished by moving one source 
line (and changing the indent level on a few others), I guess that requires 
less thought than adjusting the message!
(Assignee)

Comment 8

14 years ago
(In reply to comment #7)
> I guess that requires 
> less thought than adjusting the message!

Indeed! :)
(Assignee)

Comment 9

14 years ago
Created attachment 172099 [details] [diff] [review]
more accurate error message, v1

If the current user is not the reporter (and has no privs), some tests aren't
done in CheckCanChangeField(). By default, the error message says that you need
to be at least the reporter in order to do this action. But if the reporter
tries to change the same field, he may get another error message saying that
only the owner or an empowered user may change that field, because additional
tests are done and some of them may fail.

This patch alter CheckCanChangeField() in a way so that *all* tests are done
even if you are not the reporter, so that any user (reporter or not) without
privs gets *exactly* the same error message, avoiding confusion.
Attachment #172099 - Flags: review?(myk)

Comment 10

14 years ago
Comment on attachment 172099 [details] [diff] [review]
more accurate error message, v1

As far as I can tell, this seems functionally identical to the change I
proposed (although I am not currently in a position to run either), so the
following is just nitpicks.


I'm wondering about:

+    my $isreporter = ($reporterid == $whoid);
[...]
+    # If we haven't returned by this point, then the
+    # reporter *only* is allowed to change this field.
     $PrivilegesRequired = 1;
+    return $isreporter;

Firstly, is it necessary to initialise $isreporter so early? ... or at all?
i.e. Would replacing the final line with "return ($reporterid == $whoid);" be
equivalent in perl, or is there a relevant semantic difference between a
variable and an expression? (I'm not a perl expert - this is a genuine
question)

Secondly, I'd also wonder whether, considering bug 266579 Comment #12 From Myk,
the old condition should be retained, favouring consistency with the Owner and
QA checks over conciseness? i.e. keeping the originals of these lines after the
checks for stuff the reporter may not do:

-    if ($reporterid == $whoid) {
-	 # Allow the reporter to change anything else.
-	 return 1;
     }

-    # If we haven't returned by this point, then the user doesn't have the
-    # necessary permissions to change this field.
     $PrivilegesRequired = 1;
-    return 0;


The latter would also allow for additional checks to be inserted for a
(hypothetical) class of users with slightly fewer privileges than the reporter
without disrupting existing lines of code.
Comment on attachment 172099 [details] [diff] [review]
more accurate error message, v1

This change does what it's supposed to, but it makes the logic even harder to
understand than it already was.

One complexity is that we still check if the user is the owner/assignee or QA
contact and proceed accordingly, but we no longer check if the user is the
reporter, we just proceed accordingly if she is, which means we treat the
reporter role differently without making it clear that we're doing so and why.

Another ambiguity is that $PrivilegesRequired is being set to "2" outside a
block, so it's unclear that it applies to the set of statements with which it
used to be enclosed but which now merely follow it.

A third problem is that PrivilegesRequired is irrelevant when the function
returns a true value, but we still set it in that case.  (This was true before,
but it's even more true now.)

I think Stephen Lee is right that it makes sense to make the reporter check
consistent with the owner/assignee and QA contact checks, i.e. instead of
setting $isreporter, just do the following after checking for fields the
reporter isn't allowed to change:

if ($reporterid == $whoid) {
    # Allow the reporter to change anything else.
    return 1;
}


And it would also make sense to add a comment after the QA contact check
stating that at this point the user is either the reporter or an unprivileged
user and that we have to check for fields the reporter can't edit before
allowing the change for other fields if the user is the reporter.

However, I think we should additionally set PrivilegesRequired to "2" only when
we know we're going to reject the change, i.e.:

# The reporter may not:
# - reassign bugs, unless the bugs are assigned to him;
#   in that case we will have already returned 1 above
#   when checking for the owner of the bug.
if ($field eq "assigned_to") {
    $PrivilegesRequired = 2;
    return 0;
}
# - change the QA contact
if ($field eq "qa_contact") {
    $PrivilegesRequired = 2;
    return 0;
}
# - change the target milestone
if ($field eq "target_milestone") {
    $PrivilegesRequired = 2;
    return 0;
}
# - change the priority (unless he could have set it originally)
if ($field eq "priority"
    && !Param('letsubmitterchoosepriority'))
{
    $PrivilegesRequired = 2;
    return 0;
}

This may be more code, but it's consistent with how we set $PrivilegesRequired
elsewhere, and it's easier to understand, since $PrivilegesRequired is
basically a pseudo-return value implemented as a global variable (a sub-optimal
solution we should fix at some point), and this way associates it with the
returning of the real return value.
Attachment #172099 - Flags: review?(myk) → review-
(Assignee)

Comment 12

14 years ago
Created attachment 172304 [details] [diff] [review]
more accurate error message, v2

Is that better? :)
Attachment #172099 - Attachment is obsolete: true
Attachment #172304 - Flags: review?(myk)
Comment on attachment 172304 [details] [diff] [review]
more accurate error message, v2

Looks good; r=myk
Attachment #172304 - Flags: review?(myk) → review+
(Assignee)

Updated

14 years ago
Flags: approval? → approval+

Comment 14

14 years ago
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.227; previous revision: 1.226
done
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago14 years ago
Resolution: --- → FIXED
(Assignee)

Comment 15

14 years ago
*** Bug 243567 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 16

14 years ago
*** Bug 288347 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 17

13 years ago
*** Bug 310259 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

12 years ago
Duplicate of this bug: 109786
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.