302723 - deleting a user account when this user is the assignee or QA contact of a bug should reassign this bug to the default ones

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

[Frédéric Buclin]

Actually, deleting the account of a user who is the assignee or the QA contact
of a bug makes the bug to be with no assignee or QA contact. And the result is
that you cannot edit the bug anymore:

undef error - Can't call method "can_see_bug" on an undefined value at
Bugzilla/BugMail.pm line 407.

Sure, the assignee doesn't exist so it's a bit hard to determine if he can see
the bug or not. :-p

Either we definitely forbid to delete this category of users and ask to reassign
these bugs first, *or* we reassign these bugs to the default assignee and/or QA
contact.

process_bug.cgi crashes and sanitycheck.cgi complains:

Bad value 21 found in bugs.assigned_to (bug 265)

Comment 1

[Frédéric Buclin]

process_bug now crashes, so it's the definition of a blocker to me.

I can easily fix this bug myself if needed.

Comment 2

[Frédéric Buclin]

Attachment: patch for tip and 2.20, v1

reassign bugs to their default assignee or QA contact before deleting the user
account.

Comment 3

[Dave Miller [:justdave]]

so we're actually letting them delete a user under those circumstances?

Comment 4

[Max Kanat-Alexander]

I don't think we should block on this, unless we want to change the editparams
text to say "Hey, deleting a User is totally safe, and a very good idea."

That is, we don't recommend User deletion, particularly not in the case where
they're assigned bugs (which is also what makes this major and not critical).

However, I think it would be a worthwhile fix for the 2.20 branch.

Comment 5

[Marc Schumann [:Wurblzap]]

Comment on attachment 191144 [details] [diff] [review]
patch for tip and 2.20, v1

Looks good, and I think it's the right thing to do.

Nit: some semicolons have managed to sneak away from their execute() commands.

Nit: break the very long line a little earlier. Maybe you can even do a better
job than I did and use [%%] to break the URL :)

[Native speaker wanted:] "his role will be reassigned to" sounds awkward to me
and may be wrong; it should perhaps read "the relations will fall back to" (or
something).

Full review as soon as possible...

Comment 6

[Marc Schumann [:Wurblzap]]

Comment on attachment 191144 [details] [diff] [review]
patch for tip and 2.20, v1

Falls back correctly to assignee and QA contact (to empty QA contacts, too).

>+    # 3) Bugs
>+    # 3.1) fall back to the default assignee
>+    my $sth_updateAssignee = $dbh->prepare(
>+        'UPDATE bugs SET assigned_to = ? WHERE bug_id = ?');
>+
>+    foreach my $bug (@$buglist) {
>+        my ($bug_id, $default_assignee) = @$bug;
>+        $sth_updateAssignee->execute($default_assignee, $bug_id)

Trailing semicolon missing.
You need to log your bug updates in bugs_activity.

>+    # 3.2) fall back to the default QA contact
>+    my $sth_updateQAcontact = $dbh->prepare(
>+        'UPDATE bugs SET qa_contact = ? WHERE bug_id = ?');
>+
>+    foreach my $bug (@$buglist) {
>+        my ($bug_id, $default_qa_contact) = @$bug;
>+        $sth_updateQAcontact->execute($default_qa_contact, $bug_id)

Both applies here, too.

>Index: template/en/default/admin/users/confirm-delete.html.tmpl
>+      [% IF assignee_or_qa %]
>+        <li>
>+          [% otheruser.login FILTER html %]
>+          <a href="buglist.cgi?emailassigned_to1=1&amp;emailqa_contact1=1&amp;emailtype1=exact&amp;email1=[% otheruser.login FILTER url_quote %]">is the assignee or the QA contact of

Please break this line after "is".
(Or, break the URL using [%%].)

>+          If you delete the user account, his role in
>+          [% IF assignee_or_qa == 1 %]
>+            this [% terms.bug %]
>+          [% ELSE %]
>+            these [% terms.bugs %]
>+          [% END %]
>+          will be automatically reassigned to the default assignee or default QA contact.

I'd settle for "these roles will fall back to".

Comment 7

[Frédéric Buclin]

Attachment: patch, v2

Per discussion with Marc on IRC, the log activity part will be considered
separately in bug 303393.

Comment 8

[Dave Miller [:justdave]]

For the record, I'm a bit uneasy about letting this land without bug 303393, but
I guess it's always been this way for CCs and so forth, and we still recommend
leaving this off in the params, so...

Comment 9

[Frédéric Buclin]

tip:

Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.94; previous revision: 1.93
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v 
<--  filterexceptions.pl
new revision: 1.47; previous revision: 1.46
done
Checking in template/en/default/admin/users/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/confirm-delete.html.tmpl,v
 <--  confirm-delete.html.tmpl
new revision: 1.5; previous revision: 1.4
done


2.20rc1:

Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.90.2.2; previous revision: 1.90.2.1
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v 
<--  filterexceptions.pl
new revision: 1.43.2.2; previous revision: 1.43.2.1
done
Checking in template/en/default/admin/users/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/users/confirm-delete.html.tmpl,v
 <--  confirm-delete.html.tmpl
new revision: 1.4.2.1; previous revision: 1.4
done

Comment 10

[irina.dalah]

I'm not sure if this is the correct place for my comment...
I'm the bugzilla administrator for our system. We are still using 2.16.4 ( I know, it's not a stable version, but it was installed long before I got to be the admin and my boss wants to wait for the 2.22 realease ;-)

I deleted an old user without any problems. However, bugz that were CCed to him get __UNKOWN__ in the CC list and the bugzilla doesn't let me delete it (when I select the __UNKOWN__ line from the CC list and tick "Remove selected CCs", bugzilla tells me that this is an invalid user name).
I don't know if you already checked what happens to the CC list of bugz for users upon deletion, or if this is fixed in heigher versions as well.
I can't find a bug more close to this problem, so please treat my comment with patience (I know I don't have it for my users ;-)