Closed
Bug 340278
Opened 19 years ago
Closed 19 years ago
Move CheckCanChangeField() from process_bug.cgi to Bug.pm
Categories
(Bugzilla :: Creating/Changing Bugs, enhancement)
Tracking
()
RESOLVED
FIXED
Bugzilla 3.0
People
(Reporter: LpSolit, Assigned: LpSolit)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
mkanat
:
review+
|
Details | Diff | Splinter Review |
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").
Assignee | ||
Comment 1•19 years ago
|
||
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.
Attachment #224354 -
Flags: review?(mkanat)
Assignee | ||
Comment 2•19 years ago
|
||
"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.
Attachment #224354 -
Attachment is obsolete: true
Attachment #224356 -
Flags: review?(mkanat)
Attachment #224354 -
Flags: review?(mkanat)
Assignee | ||
Updated•19 years ago
|
Attachment #224356 -
Flags: review?(bugzilla-mozilla)
Comment 3•19 years ago
|
||
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).
Attachment #224356 -
Flags: review?(bugzilla-mozilla) → review+
Updated•19 years ago
|
Flags: approval?
Comment 4•19 years ago
|
||
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.
Attachment #224356 -
Flags: review?(mkanat) → review-
Updated•19 years ago
|
Flags: approval?
Assignee | ||
Comment 5•19 years ago
|
||
Attachment #224356 -
Attachment is obsolete: true
Attachment #227056 -
Flags: review?(mkanat)
Comment 6•19 years ago
|
||
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.
Attachment #227056 -
Flags: review?(mkanat) → review+
Updated•19 years ago
|
Flags: approval?
Comment 7•19 years ago
|
||
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?
Updated•19 years ago
|
Flags: approval?
Assignee | ||
Comment 8•19 years ago
|
||
(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.
Status: NEW → ASSIGNED
Comment 9•19 years ago
|
||
(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•19 years ago
|
||
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.
Flags: approval?
Comment 11•19 years ago
|
||
(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. :)
Flags: approval? → approval+
Assignee | ||
Comment 12•19 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•