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

RESOLVED FIXED in Bugzilla 3.0

Status

()

Bugzilla
Creating/Changing Bugs
--
enhancement
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Frédéric Buclin, Assigned: Frédéric Buclin)

Tracking

2.23
Bugzilla 3.0
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 2 obsolete attachments)

18.40 KB, patch
Max Kanat-Alexander
: review+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
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

11 years ago
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.
Attachment #224354 - Flags: review?(mkanat)
(Assignee)

Comment 2

11 years ago
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.
Attachment #224354 - Attachment is obsolete: true
Attachment #224356 - Flags: review?(mkanat)
Attachment #224354 - Flags: review?(mkanat)
(Assignee)

Updated

11 years ago
Attachment #224356 - Flags: review?(bugzilla-mozilla)

Comment 3

11 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

11 years ago
Flags: approval?

Comment 4

11 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

11 years ago
Flags: approval?
(Assignee)

Comment 5

11 years ago
Created attachment 227056 [details] [diff] [review]
patch, v1.2
Attachment #224356 - Attachment is obsolete: true
Attachment #227056 - Flags: review?(mkanat)

Comment 6

11 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

11 years ago
Flags: approval?
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?
Flags: approval?
(Assignee)

Comment 8

11 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

11 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

11 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?
(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

11 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
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Keywords: relnote

Updated

11 years ago
Blocks: 343364
(Assignee)

Updated

11 years ago
Blocks: 355841

Comment 13

10 years ago
Added to the release notes in bug 349423.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.