95923 - Don't let user change fields they aren't allowed to change.

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement, P2)

Description

[Neil Nelson]

After being very thorough in filling out the bug form I was able to be blinded
by the bright red banner several times saying I could not do such and such to
certain fields because I was not the owner etc.  If I cannot do certain things
to certain fields, you should not allow me to do them in the first place, or you
might just list the fields on the next page that were not changed for the
indicated reasons instead of beating me up for trying to be thorough.

Regards,

Neil Nelson

Comment 1

[Dave Miller [:justdave]]

Hmm, I know this has come up before, but I can't find another bug on it.

This will be difficult to enforce because currently you don't have to log in
before making changes to a bug.  It asks you for your password after you hit
submit.  So the system really has no way to know what you're allowed to change
if you're not logged in yet.

Although I think the best way to do this is to not allow editing capability for
fields if the user doesn't have access to them, this would require making people
log in before they can make changes to a bug (which means they'll have to log in
twice if they have cookies disabled), so it may be easier in the long run to do
like you said, and just put a list of fields that are being ignored because you
don't have access to change them.

On the other side of that issue, just ignoring those changes may make the
comment you just put on that bug completely irellevant, since you may have made
justifications in your comment for changing some things that you wound up not
being able to change.  Maybe the best way to handle this is the same way mid-air
collisions are handled, with an intermediate page with an option to go back to
the bug or apply the remaining changes anyway.

Comment 2

[Jacob Steenhagen]

If I recall correctly from the days before I got EditBugs, there is already a 
precident for this.  The radio buttons below this comment box don't show up if 
bugzilla can confirm that you don't have permission to use them (other 
than "leave as".  If you aren't logged in or if you do have permissions, then 
they all show up.

As to if this is the correct approach or not, I'm sure.  I do remember not 
really knowing why sometimes there would be more options displayed than 
others...

Comment 3

[Dave Miller [:justdave]]

correcting version field lost in product move

Comment 4

[Dave Miller [:justdave]]

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

Comment 5

[Bradley Baetz (:bbaetz)]

*** Bug 142484 has been marked as a duplicate of this bug. ***

Comment 6

[James Henstridge]

At Bradley's suggestion, I am pasting some of my comments from bug 142484.  They
look at this change from the point of view of making bugzilla less complex for
casual users (eg. people who might just submit the occasional bug):

One of the complaints I hear about bugzilla is that it looks to complex.  Some
even say they like the Debian bug tracker's web interface better (probably
because the debbugs web interface doesn't actually let you do anything).

When an ordinary user goes to a bug page, it has many entries and list boxes for
fields they are not allowed to change.  By changing these fields to straight
text, the bug info page would look a lot simpler, and it would be more obvious
about what they can and can't do.
...

I think this would be a win for bug reporters:  When they go through the
bugzilla helper, it asks them to search for similar bugs.  Any matches it turns
up will belong to other people.  It would be nice if they can judge whether
their problem is the same one without being overwhelmed by the interface.

If they are not the owner of that bug, I guess the only options open to them
would be to add themselves to the CC list, create an attachment or add a
comment, which is a big improvement.

Getting rid of needless UI complexity is a good thing, and should help get rid
of bugzilla's image of being difficult to use.

Comment 7

[Gervase Markham [:gerv]]

Template-wise, this is somewhat problematic. You could either have a single
template, with forks for "edit anything (not logged in or has editbugs)" and
"edit only a few things", or two templates - but then they have to be kept in sync.

Either way, the implementation is reasonably easy - we just need to be sure we
want to live with whichever set of maintenance headaches we choose.

Gerv

Comment 8

[James Henstridge]

would it be more convenient/easier to set the disabled attribute on the elements
that the user can't modify?  That would give the same element structure for the
"can edit" vs "can't edit" cases, which might be easier from a template writer's
point of view.

This would still leave the show_bug page looking fairly complex for casual
users, but would solve the problem of people not knowing which fields they can edit.

Changing the non-editable fields to straight text would still be my first
preference.  Can the template system do macros?  Maybe that would be an easy way
to create the <input> elements for the fields (or plain text if the user can't
change the field).

Comment 9

[Gervase Markham [:gerv]]

> would it be more convenient/easier to set the disabled attribute on the 
> elements that the user can't modify?

Yes, that would be easier in the template. (Although we'd have to have an extra
parameter to the generic "dropdown creation" block.)

<select name="foo" [% "disabled" if !user.groups.editbugs %]>

or some shorter form, like
<select name="foo" [% needprivs %]>

Gerv

Comment 10

[Matthew Tuck [:CodeMachine]]

I think it would be best to pass in a list of editable fields and such. 
Eventually the security policy will probably be customisable, and not using
editbugs in the template would a good step towards that.

Comment 11

[Gervase Markham [:gerv]]

> I think it would be best to pass in a list of editable fields and such. 
> Eventually the security policy will probably be customisable, and not using
> editbugs in the template would a good step towards that.

This has the disadvantage that you have to change Perl to change the editable
fields (or break the mechanism by doing it in the template anyway)

The security policy is not customisable now and, as I keep being told in other
bugs, we should do what's necessary today. And today, we use editbugs for this,
which is cunningly available as user.group.editbugs in the template.

Having investigated the implementation of this, it can be done, but it makes the
template somewhat messy. Also, disabled dropdowns are such a light shade of grey
in Mozilla as to be almost unreadable; replacing with text would be the better
option.

Gerv

Comment 12

[James Henstridge]

I agree that having plain text would be the preferable option here (for user
interface simplicity).

However, if the disabled form widget approach is taken, it is fairly easy to get
disabled form elements looking a bit better by writing CSS rules that match
INPUT[disabled] or SELECT[disabled] that set the foreground and background to
something more appropriate.

Comment 13

[Matthew Tuck [:CodeMachine]]

> This has the disadvantage that you have to change Perl to change the
> editable fields (or break the mechanism by doing it in the template anyway)

How is this a disadvantage?  This decision is already made in Perl by
process_bug, allowing it to be changed by the template is misleading.  Passing a
list of fields would be cleaner even if we never allow customisable security.

> This has the disadvantage that you have to change Perl to change the editable
> fields (or break the mechanism by doing it in the template anyway)

Besides the above, this would avoid a template interface change we know will
happen, and isn't particularly hard besides.

Comment 14

[Dave Miller [:justdave]]

*** Bug 216256 has been marked as a duplicate of this bug. ***

Comment 15

[RK]

I strongly recommend to provide permissions for the state transitions too. For 
example a person belonging to QA group should be able to "Close" a bug.

Comment 16

[Asbjørn Ulsberg]

This isn't related to this bug, exactly, but it's related to making the input
form more user friendly. Could you please add <label> tags to *all* the <input>
fields, like this:

  <input type="radio" name="knob" id="knobNew" value="none" checked="checked">
  <label for="knobNew">Leave as <strong>new</strong></label>

This makes the text clickable as well as the tiny radio-button. The same goes
for all other type of fields, but it is especially needed and noticable when
used with radiobuttons and checkboxes.

Comment 17

[GavinS]

Using <label> is bug#157573 and/or bug#14887

Comment 18

[Dave Miller [:justdave]]

Enhancements which don't currently have patches on them which are targetted at
2.18 are being retargetted to 2.20 because we're about to freeze for 2.18. 
Consideration will be taken for moving items back to 2.18 on a case-by-case
basis (but is unlikely for enhancements)

Comment 19

[Dave Miller [:justdave]]

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.

Comment 20

[Frédéric Buclin]

*** Bug 232723 has been marked as a duplicate of this bug. ***

Comment 21

[Frédéric Buclin]

*** Bug 274715 has been marked as a duplicate of this bug. ***

Comment 22

[Dave Miller [:justdave]]

Reassigning bugs that I'm not actively working on to the default component owner
in order to try to make some sanity out of my personal buglist.  This doesn't
mean the bug isn't being dealt with, just that I'm not the one doing it.  If you
are dealing with this bug, please assign it to yourself.

Comment 23

[Jason Chambers]

Couldn't this be temporarily, although not preferibly, fixed by putting some
elements into the template files that will either disable or hide functions the
user cannot change in the edit bugs form?

This would at least clear up the eye strain you get just from looking at
bugzilla. Well, at least a little. :\

Comment 24

[Jason Chambers]

Attachment: Example of a cut down view

Here is a template file that I have been using at my work. We are using
bugzilla as a computer problem trouble tracking system that seems to work well,
except for when it came to clueless users who didn't know what they were
looking at when they were editing a bug. So, I hidden off some of the features
that I didn't want them seeing, and set them to what was already been set in
the hidden elements.

Comment 25

[Frédéric Buclin]

*** Bug 223439 has been marked as a duplicate of this bug. ***

Comment 26

[Frédéric Buclin]

The trunk is now frozen to prepare Bugzilla 2.22. Enhancement bugs are retargetted to 2.24.

Comment 27

[Olav Vitters]

Attachment: Patch v1

Changes existing [% BLOCK select %] to use bug.check_can_change_field to determine if either just plain text should be down or a real <select>.

Changes the <input> fields to use the newly created [% BLOCK input %]. For truncation I decided to use a <span title="$fulltext"> to allow a logged out user to still see the full text of a field.

Note that I need to use '|| !user.id' to not change the interface for logged out users (changing that is bug 121576).

Comment 28

[Frédéric Buclin]

Comment on attachment 228426 [details] [diff] [review]
Patch v1

>Index: template/en/default/bug/edit.html.tmpl

The QA contact field must be read only if the user cannot edit it.


>+              [% PROCESS input inputname => "keywords" value => bug.keywords.join(', ') size => 60 colspan => 5 %]

Nit: avoid too long lines.


>+  [% IF bug.check_can_change_field(dep.fieldname, 0, 1) || !user.id %]
>+    <td>
>+      <input name="[% dep.fieldname %]" id="[% dep.fieldname %]"
>+             value="[% bug.${dep.fieldname}.join(', ') %]">
>+    </td>
>+  [% END %]

Two things:

1) If the user cannot change the dependency lists, you have to add hidden fields anyway, else process_bug.cgi considers that you are trying to clear the list:

" You tried to change the Blocks field , but only the assignee or reporter of the bug, or a sufficiently empowered user may change that field."

2) In all cases, add <td></td> so that we always have the same number of columns in the table (some UA behaves incorrectly if the number of columns is inconsistent).


> [% BLOCK select %]
>+  [% IF bug.check_can_change_field(selname, 0, 1) || !user.id %]
>+    <td>
>+      <select id="[% selname %]" name="[% selname %]">
>+        [% FOREACH x = bug.choices.${selname} %]
>+          <option value="[% x FILTER html %]"
>+            [% " selected" IF x == bug.${selname} %]>[% x FILTER html %]
>+          </option>
>+        [% END %]
>+      </select>
>+    </td>
>+  [% ELSE %]
>+    <td>
>+      <input type="hidden" name="[% selname %]" value="[% bug.${selname} FILTER html %]">
>+      [% bug.${selname} FILTER html %]
>+    </td>
>+  [% END %]
>+[% END %]

Nit: move <td></td> out of the IF-ELSE-END block as we want them in all cases.


>+[% BLOCK input %]

>+    [% IF size %]
>+      <span title="[% val FILTER html %]">[% val.truncate(size, '...') FILTER html %]</span>

Write FILTER truncate(size) instead, see http://www.template-toolkit.org/docs/plain/Manual/Filters.html#truncate_length_


>+  [% accesskey = "" %]

We never use accesskey! But you have to clear $colspan.

Comment 29

[Olav Vitters]

Attachment: Patch v2: Bugzilla HEAD 2006-07-16

Address all comments (including shortening some stuff to try and fit 80 columns).

Small change:
- Do not add a <span title> if the value is not truncated.
- Indenting fix in BLOCK input (after <td)

Comment 30

[Frédéric Buclin]

Comment on attachment 229401 [details] [diff] [review]
Patch v2: Bugzilla HEAD 2006-07-16

>Index: template/en/default/bug/edit.html.tmpl

>+                 [% IF bug.qa_contact.login.length > 60 %]

If there is no QA contact for this bug, bug.qa_contact.login is undefined and you get the following error in your web server log:

show_bug.cgi: Argument "" isn't numeric in numeric gt (>) at data/template/template/en/default/bug/edit.html.tmpl line 355.

It should make sure there is a QA contact first.


>+  [% maxlength = "" %]
>+  [% colspan = "" %]
>+  [% size = "" %]

Nit: all 3 are integers. You should reset them to 0 instead of "".

Please fix these comments in an updated patch and carry forward my r+. r=LpSolit

Comment 31

[Olav Vitters]

Attachment: Patch v2.5: Fix comments

Fix comments from review.

Carrying forward r=LpSolit.

Comment 32

[Dave Miller [:justdave]]

ooh! ooh! ooh! :)

I'm surprised this bug has so few votes on it.

Comment 33

[Frédéric Buclin]

Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.73; previous revision: 1.72
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.78; previous revision: 1.77
done

Comment 34

[Max Kanat-Alexander]

Added to relnotes in bug 349423. Please let me know if I missed any critical information about this bug in the release notes.