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").
Created attachment 224354 [details] [diff] [review] 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.
Created attachment 224356 [details] [diff] [review] 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 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 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.
Created attachment 227056 [details] [diff] [review] patch, v1.2
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.
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?
(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.
(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.
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.
(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. :)
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
Added to the release notes in bug 349423.