These ones involve some interesting checks, but it should still be definitely possible. I'm also going to modify Bugzilla::Object::create and update to accept Bugzilla::Objects as field values, and it will use their "id" as what it stores in the DB.

Oh, no, wait. Because of the strict_isolation checks, I have to implement the product-change code first. Sigh.

This most likely doesn't even compile yet.

Any progress on this bug? I need $bug->set_assignee() to nicely fix bug 398739. As it's blocking a blocker, I would say this one is also a blocker. ;)

Comment on attachment 281747 [details] [diff] [review] WIP >Index: Bugzilla/Bug.pm > sub _check_assigned_to { > return $id; > } >+sub set_assigned_to { $_[0]->set('assigned_to', $_[1]); } That's the irritating part. $bug->{'assigned_to'} must contain a user object, but _check_assigned_to() returns its ID instead. I just got this problem when trying to implement a partial set_assignee() method. Btw, I prefer set_assignee() than set_assigned_to() as a method name.

Okay, I've tested this and it seems to work. I had to get kind of clever with some things and move some things around.

Comment on attachment 286617 [details] [diff] [review] v1 >Index: process_bug.cgi >+my (%usercache, $timestamp); %usercache is no longer in use. >- } elsif ($col eq 'assigned_to' || $col eq 'qa_contact') { >- # Display the assignee or QA contact email address >- $vars->{'oldvalue'} = user_id_to_login($oldhash{$col}); >- $vars->{'newvalue'} = user_id_to_login($formhash{$col}); >- $vars->{'field'} = $col; Not sure if that's the reason of the problem, but trying to change the assignee of a bug when you don't have editbugs privs and you play no role with that bug displays the error message with the assignee ID instead of his login name. >- # save off the old value for passing to Bugzilla::BugMail so >- # the old assignee can be notified >- # >- if ($col eq 'assigned_to') { >- $old = ($old) ? user_id_to_login($old) : ""; >- $new = ($new) ? user_id_to_login($new) : ""; >- $origOwner = $old; >- } >- >- # ditto for the old qa contact >- # >- if ($col eq 'qa_contact') { >- $old = ($old) ? user_id_to_login($old) : ""; >- $new = ($new) ? user_id_to_login($new) : ""; >- $origQaContact = $old; >- } Now both $origOwner and $origQaContact are left empty. Make sure it's correct to remove them, else please restore them. If they are not needed to notify the old assignee and QA contact, then you should remove their occurences from process_bug.cgi entirely. > next if grep($_ eq $col, qw(keywords op_sys rep_platform priority > product_id component_id version >- target_milestone >+ target_milestone assigned_to qa_contact > ... >- status_whiteboard bug_file_loc), >+ status_whiteboard bug_file_loc >+ assigned_to qa_contact), assigned_to and qa_contact are duplicated. >Index: Bugzilla/Bug.pm >- 'assigned_to AS assigned_to_id', > 'reporter AS reporter_id', >- 'qa_contact AS qa_contact_id', Why removing these lines for the assignee and QA contact, but not for the reporter? >+ $class->_check_strict_isolation($params->{cc}, $params->{assigned_to}, >+ $params->{qa_contact}, $product); It's sad that you have to explicitly call _check_strict_isolation() when creating a new bug while it's automatically called by set_assigned_to() and set_qa_contact() via _check_assigned_to() and _check_qa_contact() when editing an existing bug. This makes the logic harder IMO as you have to constantly jump between ref($invocant) and !ref($invocant) in this method. I'm sure there is a way to unify this and I would love to see this being done here. > sub _check_assigned_to { >+ ThrowUserError("reassign_to_empty") if ref $invocant && !trim($assignee); >+ $assignee = Bugzilla::User->check($assignee) if !ref $assignee; Can you trim() an object?? >+ # create() checks this another way, so we don't have to run this >+ # check during create(). >+ $invocant->_check_strict_isolation_for_user($assignee) if ref $invocant; As said above, this bothers me. I really think it's possible to merge the logic when creating and editing a bug as _check_assigned_to() is called on both bug creation and editing. >+sub _check_qa_contact { >+ if (!ref $invocant) { >+ $id = $component->default_qa_contact->id if !ref $invocant; We already know that !ref $invocant is true as that's the block we are in. >+ # create() checks this another way, so we don't have to run this >+ # check during create(). >+ $invocant->_check_strict_isolation_for_user($qa_contact) >+ if ref $invocant; Same comment as above: merge the logic. > sub _check_strict_isolation { >+ @related_users = Bugzilla::User->new_from_list(\@related_users) >+ if !ref $invocant; new_from_list() returns an arrayref. This generates an error on bug creation: Can't call method "id" on unblessed reference at Bugzilla/Bug.pm line 1239. due to this line: >+ my %unique_users = map {$_->id => $_} @related_users; I did a few tests so far, and the main problem I found (besides the crash on bug creation due to new_from_list() above) is that when you turn on strict_isolation *after* some unauthorized users already play a role with some bugs, then you cannot edit these bugs anymore unless you kick these users. A user without editbugs privs would then be unable to kick them and so would be unable to e.g. comment. We shouldn't call $user->can_edit_product() if the user already plays a role. AFAIK, that's the current behavior and the desired one (I fixed it myself a few months ago IIRC). Else this looks great but I will have to do more testing.

(In reply to comment #7) >>- 'assigned_to AS assigned_to_id', >> 'reporter AS reporter_id', >>- 'qa_contact AS qa_contact_id', > > Why removing these lines for the assignee and QA contact, but not for the > reporter? Because I didn't change how the reporter field is used. > I'm sure there is a > way to unify this and I would love to see this being done here. Yeah, we could always check it in _check_assigned_to, but we get a nicer single error message with the create() version. (That is, if you have both a QA Contact and an Assignee who can't see the bug, you get a single error message containing both of them.) Also, assigned_to and qa_contact aren't in REQUIRED_CREATE_FIELDS, so the strict_isolation check might not get called for them, but I suppose that's OK, because editcomponents checks that? (I haven't looked.) I have a version that should fix everything else coming up soon.

Okay, this one fixes everything that you mentioned. I tested this one with strict_isolation, too, and it seems to work just fine.

Comment on attachment 290534 [details] [diff] [review] v2 >Index: process_bug.cgi >+ if ($product_change) { >+ $b-> _check_strict_isolation(); >+ } Remove the whitespace after ->. >Index: Bugzilla/Bug.pm >+ if (grep ($_ eq $field, qw(qa_contact assigned_to))) { >+ $from = $old_bug->$field()->login if $from; >+ $to = $self->$field->login if $to; >+ } Nit: either $field() or $field. > sub _check_assigned_to { >+ if (!ref $assignee) { >+ $assignee = Bugzilla::User->check($assignee) if !ref $assignee; |if !ref $assignee| is useless as you are already in the corresponding block. > sub _set_global_validator { >+ my $current = $self->$field; > my $privs; >+ my $can = $self->check_can_change_field($field, $current, $value, \$privs); >+ if (!$can) { >+ if ($field->name eq 'assigned_to' || $field->name eq 'qa_contact') { >+ $value = user_id_to_login($value); >+ $current = user_id_to_login($current); >+ } This is the reason of my r-. I cannot test your patch at all due to this bug. $self->assigned_to and $self->qa_contact both return a user object while $value is a user ID. check_can_change_field() obviously sees them as always different and users without editbugs privs are unable to edit bugs. Moreover, $field is a string, not a object, so $field->name throws an error: Can't locate object method "name" via package "assigned_to" (perhaps you forgot to load "assigned_to"?) at Bugzilla/Bug.pm line 1407. Writing $field alone will fix the second issue, but the first one is more critical to fix as input and output are not of the same type (user ID/name vs object). I guess your workaround will be to write |if ($field eq 'assigned_to' || $field eq 'qa_contact')| but as you do this test twice, I suppose you have to do something upstream instead, before calling _set_global_validator(), to convert data to the expected format. > sub assigned_to { > sub qa_contact { Now that they are explicitly defined, you should remove them from Bug::fields().

Okay, this fixes it for this instance and for all future instances like this that might come up. I didn't change fields() because it's used by other places in the code (grep for "Bugzilla::Bug->fields").

Comment on attachment 292200 [details] [diff] [review] v3 Do not forget to add: foreach my $col (@editable_bug_fields) { # XXX - Ugly workaround which has to go away before 3.1.3. next if ($col eq 'assigned_to' || $col eq 'qa_contact'); at line 691 of process_bug.cgi as discussed on IRC. r=LpSolit with it.

Hey Max, why not committing this patch?

Forgot, apparently! :-) Checking in process_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v <-- process_bug.cgi new revision: 1.395; previous revision: 1.394 done Checking in Bugzilla/Bug.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v <-- Bug.pm new revision: 1.220; previous revision: 1.219 done