340278 - Move CheckCanChangeField() from process_bug.cgi to Bug.pm

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Frédéric Buclin]

Both templates and process_bug.cgi need to know which fields can be changed by the logged in user and which ones cannot, in order to have a consistent UI (e.g. displaying the fields the user cannot change as read only text instead of letting him change them and then getting an error from process_bug.cgi telling him he wasn't allowed to do so, see bug 95923).

Probably templates should use this function using bug.user.check_can_change_field("foo").

Comment 1

[Frédéric Buclin]

Attachment: patch, v1

Now templates can easily determine if a user is allowed to change a given field by writing:

[% IF bug.check_can_change_field("op_sys", 0, 1) %]
  <select name="op_sys">
    <option value="foo">foo
    ...
  </select>
[% ELSE %]
  <input type="hidden" name="op_sys" value="foo">foo
[% END %]

This will highly help having the UI to be consistent with what users can do.

Comment 2

[Frédéric Buclin]

Attachment: patch, v1.1

"new Bugzilla::Bug" doesn't like undefined bug ID due to
$bug_id !~ /^0*[1-9][0-9]*$/.
This was filling the apache error log file. Thanks to bkor for catching that.

Comment 3

[Olav Vitters]

Comment on attachment 224356 [details] [diff] [review]
patch, v1.1

It had a small conflict at line 1414 in process_bug.cgi. I reviewed it after fixing the conflict. Passes my tests. Wondered about that error check causing the function to perhaps disallow some changes, but that will only happen if the user has editbugs (otherwise they cannot change multiple bugs at once).

Comment 4

[Max Kanat-Alexander]

Comment on attachment 224356 [details] [diff] [review]
patch, v1.1

>@@ -1125,6 +1137,138 @@
>+# $PrivilegesRequired - reason of the failure, if any (output variable)

  Instead, this should just be the return value, yes?

>+    my ($field, $oldvalue, $newvalue, $PrivilegesRequired, $cgi) = (@_);

  Instead of passing in the CGI, can't we just use Bugzilla->cgi?

>+    my $user = Bugzilla->user;

  Instead we should use the person inspecting the bug.

Comment 5

[Frédéric Buclin]

Attachment: patch, v1.2

Comment 6

[Max Kanat-Alexander]

Comment on attachment 227056 [details] [diff] [review]
patch, v1.2

Okay, I suppose I can accept this.

I don't like having to pass in $PrivilegesRequired. Can we file another bug to make that better? Like, we could return some constant or figure out something.

Comment 7

[Dave Miller [:justdave]]

The primary thrust of this patch appears to be moving the CheckCanChangeField sub.  This is probably one of the most customized functions in Bugzilla (we advertise it's customizability all over the place).  We had mentioned something in the past about moving this entirely to its own file instead of burying it in the middle of some other module somewhere when we moved it out of process_bug.  We should probably do that.  Can we make it Bugzilla::Bug::FieldChangeRules.pm or something like that?

Comment 8

[Frédéric Buclin]

(In reply to comment #7)
> We had mentioned something
> in the past about moving this entirely to its own file instead of burying it in
> the middle of some other module somewhere when we moved it out of process_bug. 
> We should probably do that.  Can we make it Bugzilla::Bug::FieldChangeRules.pm
> or something like that?

I want check_can_change_field() to be a method of bug objects, so that we can easily use it from templates, see comment 1. Having it in a separate module won't work AFAIK. And I definitely don't want to write
check_can_change_field($bug, $field, $old, $new, $privs, $data), i.e. I don't want it to be a routine only but a real method of bug objects.

Comment 9

[Max Kanat-Alexander]

(In reply to comment #8)
> I don't want it to be a routine only but a real method of bug objects.

  I definitely agree with LpSolit on this. I think it would be best to have it as a method of the Bug object.

Comment 10

[Max Kanat-Alexander]

Also, I think the best way forward is to eventually set up an actual admin page that allows people to define who can change what, instead of requiring them to edit this code. That way we don't have to worry about where the code is.

Comment 11

[Dave Miller [:justdave]]

(In reply to comment #10)
> Also, I think the best way forward is to eventually set up an actual admin page
> that allows people to define who can change what, instead of requiring them to
> edit this code. That way we don't have to worry about where the code is.

Hmm, yes, this would be way cool.

OK, fair enough.  It's cool that you even fixed the docs with this patch.  Way to go. :)

Comment 12

[Frédéric Buclin]

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.330; previous revision: 1.329
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.124; previous revision: 1.123
done
Checking in docs/xml/customization.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/customization.xml,v  <--  customization.xml
new revision: 1.35; previous revision: 1.34
done

Comment 13

[Max Kanat-Alexander]

Added to the release notes in bug 349423.