303393 - Deleting a user omits some bugs_activity logging

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Marc Schumann [:Wurblzap]]

When deleting a user, editusers.cgi does some clean-up work. When doing so, it
should log the changes on bugs in bugs_activity. It fails to do so for CC list
removals and flags requestee clearings.

Comment 1

[Marc Schumann [:Wurblzap]]

As soon as bug 302723 has landed, assignee and qa_contact changes because of
user deletion will be missing in bugs_activity, too ;)

Comment 2

[Marc Schumann [:Wurblzap]]

Attachment: Patch

Log all bug changes to bugs_activity that may currently happen on user
deletion:
- when removing the user as a flag requestee
- when removing the user from cc lists
- when falling back to default assignee or QA contact

Comment 3

[Frédéric Buclin]

Comment on attachment 191638 [details] [diff] [review]
Patch

>Index: editusers.cgi

>+    # Get the timestamp for LogActivityEntry.
>+    my $timestamp =
>+        $dbh->selectrow_array('SELECT NOW() FROM profiles WHERE userid = ?',
>+                              undef, $user->id);

huh??? my $timestamp = $dbh->selectrow_array('SELECT NOW()'); looks *much*
better!


>+    # Simple reference removals.
>+    # <none>

Nit: looks like we will never have "Simple reference removals". I would remove
this two lines.


>+    my $sth_flagupdate_bug =
>+        $dbh->prepare('UPDATE flags set requestee_id = NULL
>+                       WHERE bug_id = ? AND requestee_id = ?');

This won't work, because this SQL statement will also include attachments. You
should add "AND attach_id IS NULL".



>+    foreach (@{$dbh->selectall_arrayref('SELECT bug_id, attach_id FROM flags
>+                                          WHERE requestee_id = ?',
>+                                        undef, $otherUserID)}) {
>+        my ($bug_id, $attach_id) = @$_;

Nit: do you really like having such a compact code?? I mean having the SQL
statement inside the FOREACH statement. At first glance, this makes me think
the SQL statement is the first execution of the loop. :(


>+        # Let update_activity do all the dirty work, including setting
>+        # the bug timestamp.
>+        Bugzilla::Flag::update_activity($bug_id, $attach_id, "'$timestamp'",
>+                                        \@old_summaries, \@new_summaries,
>+                                        $userid);

$userid is useless. Bugzilla::Flag can already access the user ID by using
Bugzilla->user->id.


>-    # More complex deletions in referred tables.
>+    # Deletions in referred tables which need LogActivityEntry.
>+    foreach (@{$dbh->selectcol_arrayref('SELECT bug_id FROM cc WHERE who = ?',
>+                                        {'Slice' => {}}, $otherUserID)}) {

Same remark as above + selectcol_arrayref doesn't require 'Slice'.



>Index: Bugzilla/Flag.pm

> sub update_activity {
>-    my ($bug_id, $attach_id, $timestamp, $old_summaries, $new_summaries) = @_;
>+    my ($bug_id, $attach_id, $timestamp, $old_summaries, $new_summaries, $userid) = @_;

I'm against this change, which isn't justified here.

Comment 4

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.1

(In reply to comment #3)
> huh??? my $timestamp = $dbh->selectrow_array('SELECT NOW()'); looks *much*
> better!

Good to know SELECTs work without FROM on MySQL. Changed.
Afaik this does not work on Oracle -- does it work on PgSQL?

> Nit: looks like we will never have "Simple reference removals". I would
remove
> this two lines.

Gone.

> >+	my $sth_flagupdate_bug =
> >+	    $dbh->prepare('UPDATE flags set requestee_id = NULL
> >+			   WHERE bug_id = ? AND requestee_id = ?');
> 
> This won't work, because this SQL statement will also include attachments.
You
> should add "AND attach_id IS NULL".

Good catch, thanks. Corrected.

> >+	foreach (@{$dbh->selectall_arrayref('SELECT bug_id, attach_id FROM
flags
> >+					      WHERE requestee_id = ?',
> >+					    undef, $otherUserID)}) {
> >+	    my ($bug_id, $attach_id) = @$_;
> 
> Nit: do you really like having such a compact code?? I mean having the SQL
> statement inside the FOREACH statement. At first glance, this makes me think
> the SQL statement is the first execution of the loop. :(

My mind seems to work differently ;)
Moved it out of the FOREACH; I like it better the way it was before.

> >+	    # Let update_activity do all the dirty work, including setting
> >+	    # the bug timestamp.
> >+	    Bugzilla::Flag::update_activity($bug_id, $attach_id,
"'$timestamp'",
> >+					    \@old_summaries, \@new_summaries,
> >+					    $userid);
> 
> $userid is useless. Bugzilla::Flag can already access the user ID by using
> Bugzilla->user->id.

Well, it doesn't, which is the reason I changed it in the first place.
Bugzilla::Flag uses $::userid, and I'm not sure we can move to
Bugzilla->user->id without breaking anything. (If we can, let's address it in
another bug perhaps.)
In our case, we have a global $userid in editusers.cgi, so Flag.pm can access
it, and I'm leaving it alone now.

> >+	foreach (@{$dbh->selectcol_arrayref('SELECT bug_id FROM cc WHERE who =
?',
> >+					    {'Slice' => {}}, $otherUserID)}) {
> 
> Same remark as above + selectcol_arrayref doesn't require 'Slice'.

Addressed.

Comment 5

[Frédéric Buclin]

Comment on attachment 191675 [details] [diff] [review]
Patch 1.1

>     Param('allowuserdeletion')
>         || ThrowUserError('users_deletion_disabled');

This is another bug, but it doesn't make sense to me to lock all these tables
before checking if the user has enough privs to delete the user account. I
would do the locking only when we are sure the user account can be deleted.


>+    my $timestamp = $dbh->selectrow_array('SELECT NOW()');

FYI, both MySQL and PostgreSQL support this syntax, yes.


>+    # Reference removals which need LogActivityEntry.
>+    my $statement_flagupdate = 'UPDATE flags set requestee_id = NULL
>+                                 WHERE bug_id = ?
>+                                   AND attach_id %s
>+                                   AND requestee_id = ?';
>+    my $sth_flagupdate_attachment =
>+        $dbh->prepare(sprintf($statement_flagupdate, '= ?'));
>+    my $sth_flagupdate_bug =
>+        $dbh->prepare(sprintf($statement_flagupdate, 'IS NULL'));
>+    my $buglist = $dbh->selectall_arrayref('SELECT bug_id, attach_id FROM flags
>+                                             WHERE requestee_id = ?',
>+                                           undef, $otherUserID);

Nit: could you add a new line before my $buglist?

More important: per $sth_flagupdate_attachment and $sth_flagupdate_bug above,
all requests set on the same bug/attachment for a given requestee will be
requested from the wind at once. So $buglist should only consider DISTINCT
(bug_id, attach_id).

(With you actual patch, considering flags.id would avoid having two distinct
$sth_flagupdate_attachment and $sth_flagupdate_bug SQL statements. ;))


>+    foreach (@$buglist) {
>+        LogActivityEntry($_, 'cc', $otherUser->login, '', $userid, $timestamp);
>+        $sth_set_bug_timestamp->execute($timestamp, $_);
>+    }
>+    $dbh->do('DELETE FROM cc WHERE who = ?', undef, $otherUserID);

Nit: it sounds more logical to me (from a conceptual point of view) to
effectively remove the user from the cc tables and then say you removed him
instead of doing the opposite as you do here.


>+        my ($bug_id, $default_qa_contact_id) = @$bug;
>+        $usercache{$default_qa_contact_id} ||=
>+            new Bugzilla::User($default_qa_contact_id);

The default QA contact is not mandatory. In this case, $default_qa_contact_id
is undefined and you get $usercache{undef}:

editusers.cgi: Use of uninitialized value in hash element at
/var/www/html/bugzilla/editusers.cgi line 652.

>+        $sth_updateQAcontact->execute($default_qa_contact_id,
>+                                      $timestamp, $bug_id);
>+        LogActivityEntry($bug_id, 'qa_contact', $otherUser->login,
>+                         $usercache{$default_qa_contact_id}->login,
>+                         $userid, $timestamp);

Here again, you get the same error about $usercache{undef}->login. What about
$default_qa_contact_id || 0 ?


Now we have a more serious problem to fix: your patch reports changes in the
activity table correctly, *but* Bugzilla now crashes when you edit a bug! The
reason is that BugMail checks whether the user removed from the CC list or
another role is allowed to see the bug and whether he wants to receive emails.
But as the user account has already been deleted, you get an error:

The name [e-mail] is not a valid username. Either you misspelled it,
or the person has not registered for a Bugzilla account.

To fix that, you should send emails *before* effectively deleting the user
account. (Note that your patch doesn't send any email at all!). And now you
have another problem: sending emails while tables are locked!

Comment 6

[Frédéric Buclin]

I have an idea: in order to deal with deleted user accounts, BugMail.pm should
ignore such accounts instead of returning an error saying that this account
doesn't exist; 'ignore' meaning 'do not send any email to that account'. This
way, Marc's patch (with the few comments addressed) would do the right thing and
there wouldn't be any need to send emails while tables are locked.

I suggest to open another bug to fix the BugMail.pm issue, which will block this
bug.

Comment 7

[Frédéric Buclin]

(In reply to comment #6)
> I suggest to open another bug to fix the BugMail.pm issue

Done! See bug 303824.

Comment 8

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.2

(In reply to comment #5)
> >	Param('allowuserdeletion')
> >	    || ThrowUserError('users_deletion_disabled');
> 
> This is another bug, but it doesn't make sense to me to lock all these tables

> before checking if the user has enough privs to delete the user account. I
> would do the locking only when we are sure the user account can be deleted.

I agree.

> Nit: could you add a new line before my $buglist?

Ok.

> More important: per $sth_flagupdate_attachment and $sth_flagupdate_bug above,

> all requests set on the same bug/attachment for a given requestee will be
> requested from the wind at once. So $buglist should only consider DISTINCT
> (bug_id, attach_id).

That's not necessary; Bugzilla::Flag::snapshot and
Bugzilla::Flag::update_activity take care of this for us, so we can modify all
flags on a given bug/attachment at once.

> >+	foreach (@$buglist) {
> >+	    LogActivityEntry($_, 'cc', $otherUser->login, '', $userid,
$timestamp);
> >+	    $sth_set_bug_timestamp->execute($timestamp, $_);
> >+	}
> >+	$dbh->do('DELETE FROM cc WHERE who = ?', undef, $otherUserID);
> 
> Nit: it sounds more logical to me (from a conceptual point of view) to
> effectively remove the user from the cc tables and then say you removed him
> instead of doing the opposite as you do here.

I don't mind. Switched them around.

> Here again, you get the same error about $usercache{undef}->login. What about

> $default_qa_contact_id || 0 ?

Done. Good you caught this.
I chose '' instead of 0 so that it's not a numeric value.

Comment 9

[Frédéric Buclin]

Comment on attachment 192181 [details] [diff] [review]
Patch 1.2

>+    # Cache for user accounts.
>+    my %usercache = ('' => new Bugzilla::User());

When no QA contact is given, %usercache should use 0 instead of ''. This way,
tests such as
if ($default_qa_contact_id == $user->id) would always work (note the numerical
comparison). Else you would get an error in your apache error log file.


>+    my $buglist = $dbh->selectall_arrayref('SELECT bug_id, attach_id FROM flags
>+                                             WHERE requestee_id = ?',
>+                                           undef, $otherUserID);

You delete flags on a per attachment/bug basis. Then you should look for
*DISTINCT* (bug_id, attach_id) tuples. This avoid us many useless calls to
Flag::snapshot() and Flag::update_activity().


>+        Bugzilla::Flag::update_activity($bug_id, $attach_id, "'$timestamp'",
>+                                        \@old_summaries, \@new_summaries);

Please use $dbh->quote($timestamp) instead of "'$timestamp'".


With the actual patch, bug changes are sent by email only when someone changes
something to affected bugs, and emails say that this user is responsible for
these changes, which is clearly wrong. We could fix that in a separate bug (it
looks straightforward to fix).

Comment 10

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.3

Fixed all comments. Sends bugmail, too (but no flagmail).

Comment 11

[Frédéric Buclin]

Comment on attachment 193546 [details] [diff] [review]
Patch 1.3

>     $template->process('admin/users/search.html.tmpl', $vars)
>        || ThrowTemplateError($template->error());
> 
>+    # Send mail about what we've done to bugs.
>+    # The deleted user is not notified of the changes.
>+    foreach (keys(%updatedbugs)) {
>+        Bugzilla::BugMail::Send($_);
>+    }

Tested, works well!

Nit: Maybe should we send emails before displaying
admin/users/search.html.tmpl. The user may be tempted to go to another page
while emails are being sent and would stop the job. After all, there is nothing
in the UI which warns the user to wait, such as "Sending emails for changed
bugs, please wait... emails sent!". As deleting user accounts is already risky
in itself, I won't deny review for that.

r=LpSolit

Comment 12

[Frédéric Buclin]

r=LpSolit for 2.20 too. Tested, works well. Note that LogActivityEntry() has not
yet been moved into Bug.pm in 2.20 (it's still in CGI.pl), meaning that 'use
Bugzilla::Bug;' is useless for this branch (I tested your patch with this line
removed). I will remove it on checkin (note to myself: for the branch only!)

Comment 13

[Frédéric Buclin]

tip:

Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.100; previous revision: 1.99
done

2.20rc2:

Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.90.2.4; previous revision: 1.90.2.3
done