947758 - Delete a users whine if they don't have privileges to whine.

Status: NEW

(Bugzilla :: Whining, defect)

Description

[Simon Green]

Attachment: nowhine-v1.patch

(from the brc bug)

While investigating Bug 949532, I discovered that my database snapshot included at least four disabled user accounts that still had whines scheduled.

The disabled users were not in the bz_canusewhines group (presumably they were removed from the group when their accounts were disabled), but the whine_schedules table still had rows for those users' whines.

Furthermore, when I ran whine.pl, the run_next field for those whine schedules was updated, suggesting that whine.pl was still trying to run the whines, though results were not emailed due to the disabled accounts also having bugmail disabled.

I would recommend that disabled users should have whine schedules cleared [...] by whine.pl itself [..., this] would also cleanup any disabled users' whines that are already in the production database.

Version-Release number of selected component (if applicable):
4.4.1011

How reproducible:
Always

Steps to Reproduce:
1. Query the database for whine_schedules related to whine_events owned by disabled users, e.g.

    SELECT whine_schedules.id,owner_userid
    FROM whine_schedules
    LEFT JOIN whine_events ON whine_events.id = whine_schedules.eventid
    LEFT JOIN profiles ON userid=whine_events.owner_userid
    WHERE owner_userid IN (SELECT userid FROM profiles WHERE is_enabled=0);

Actual results:
Disabled users have active whines.

Expected results:
No disabled users have active whines.

Additional info:
none.

Comment 1

[Frédéric Buclin]

Comment on attachment 8344406 [details] [diff] [review]
nowhine-v1.patch

>+# Remove whines that belong to use that cannot whine

s/to use that/to users who/ ?


>+_remove_invalid_whines();

They are not invalid; they are rather obsolete, IMO.


>+    my $group = Bugzilla::Group->new({name => 'bz_canusewhines'}) || return;

If the bz_canusewhines group doesn't exist, we are in trouble. So "|| return" is useless.


>+    # Get the whines that need to be deleted
>+    my $query = q{
>+        SELECT id
>+        FROM whine_events
>+        WHERE owner_userid NOT IN (
>+            SELECT user_id
>+            FROM user_group_map
>+            WHERE group_id IN ( }. join(',', @$group_ids) . '))';

IMO, this would be more efficient with a LEFT JOIN, else the subselect could return a huge list. I'm not sure how the DB optimizer would work in this case. Did you do some benchmarking?


>+        my $in = 'IN (' . join(',', @$del_whines) . ')';

Oracle cannot have a huge list with IN(), so you should use sql_in() instead.


>+        $dbh->do("DELETE FROM whine_schedules WHERE eventid $in");
>+        $dbh->do("DELETE FROM whine_queries WHERE eventid $in");

These two SQL statements are useless. Foreign keys will automatically delete these entries once they are removed from whine_events.

Comment 2

[Simon Green]

Attachment: nowhine-v2.patch

Regarding the database, I didn't do any specific benchmarking. It didn't take an unreasonable amount of time on my development machine (running Pg with a full copy of the brc database).

Comment 3

[Simon Green]

Attachment: nowhine-v3.patch

One character change from the last patch

s/eho/who/

Comment 4

[Frédéric Buclin]

Thinking about this bug again, I think the code should be moved into sanitycheck.cgi, which does this kind of checks. I don't think there is a need to check if there are obsolete whines every 15 minutes, especially because it should be pretty rare that someone with bz_canusewhines privileges gets them removed. And sanitycheck.cgi has the advantage to also display obsolete whines, in case this information is useful to admins.