Last Comment Bug 340278 - Move CheckCanChangeField() from process_bug.cgi to Bug.pm
: Move CheckCanChangeField() from process_bug.cgi to Bug.pm
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Creating/Changing Bugs (show other bugs)
: 2.23
: All All
: -- enhancement (vote)
: Bugzilla 3.0
Assigned To: Frédéric Buclin
: default-qa
Mentors:
Depends on:
Blocks: 95923 343364 355841
  Show dependency treegraph
 
Reported: 2006-06-03 18:06 PDT by Frédéric Buclin
Modified: 2007-02-13 22:02 PST (History)
3 users (show)
justdave: approval+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1 (18.48 KB, patch)
2006-06-04 07:17 PDT, Frédéric Buclin
no flags Details | Diff | Review
patch, v1.1 (18.41 KB, patch)
2006-06-04 08:18 PDT, Frédéric Buclin
mkanat: review-
bugzilla-mozilla: review+
Details | Diff | Review
patch, v1.2 (18.40 KB, patch)
2006-06-26 04:18 PDT, Frédéric Buclin
mkanat: review+
Details | Diff | Review

Description Frédéric Buclin 2006-06-03 18:06:54 PDT
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 2006-06-04 07:17:49 PDT
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.
Comment 2 Frédéric Buclin 2006-06-04 08:18:53 PDT
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 3 Olav Vitters 2006-06-18 12:41:48 PDT
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 2006-06-26 03:40:24 PDT
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 2006-06-26 04:18:14 PDT
Created attachment 227056 [details] [diff] [review]
patch, v1.2
Comment 6 Max Kanat-Alexander 2006-06-26 04:21:12 PDT
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] (justdave@bugzilla.org) 2006-06-26 08:11:24 PDT
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 2006-06-30 02:25:02 PDT
(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 2006-06-30 11:27:04 PDT
(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 2006-06-30 11:28:23 PDT
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] (justdave@bugzilla.org) 2006-06-30 18:22:10 PDT
(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 2006-07-01 13:02:08 PDT
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 2007-02-13 22:02:19 PST
Added to the release notes in bug 349423.

Note You need to log in before you can comment on or make changes to this bug.