303709 - Eliminate deprecated Bugzilla::DB routines from sendbugmail.pl, sendunsentbugmail.pl and syncLDAP.pl scripts in contrib

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Teemu Mannermaa (:wicked)]

These lines need to rewritten to use DBI:

contrib/sendbugmail.pl:44:SendSQL("SELECT bug_id FROM bugs WHERE bug_id = $bugnum");
contrib/sendbugmail.pl:46:if (!FetchOneColumn()) {

contrib/sendunsentbugmail.pl:33:SendSQL("SELECT bug_id FROM bugs
contrib/sendunsentbugmail.pl:39:while (MoreSQLData()) {
contrib/sendunsentbugmail.pl:40:    push (@list, FetchOneColumn());

contrib/syncLDAP.pl:84:SendSQL("SELECT login_name, realname, disabledtext " .
contrib/syncLDAP.pl:86:while (MoreSQLData()) {
contrib/syncLDAP.pl:88:        = FetchSQLData();
contrib/syncLDAP.pl:241:        SendSQL("UPDATE profiles SET disabledtext =
'auto-disabled by ldap " .
contrib/syncLDAP.pl:255:          SendSQL("UPDATE profiles SET login_name = '" .
contrib/syncLDAP.pl:259:          SendSQL("UPDATE profiles SET realname = '" .
@$value{'realname'} .
contrib/syncLDAP.pl:272:        SendSQL("INSERT INTO profiles VALUES ('',

Comment 1

[Olav Vitters]

Attachment: Patch v1

I could NOT test contrib/syncLDAP.pl due to lack of LDAP install. That script was broken anyway. The part that created a new user in Bugzilla used 'INSERT INTO profiles VALUES'. This relied on a old order of the columns (also had that old emailsettings in there). I replaced it with insert_new_user as that is much cleaner.

contrib/sendbugmail.pl:
 * Adds 'use Bugzilla' (much better than assuming globals.pl uses it)

contrib/syncLDAP.pl:
 * Script could use a rewrite
 * Indentation is off
 * Seems to made for Oracle ('sysdate()', 'sql_istrcmp' for login_name)
 * Perhaps move this into a seperate bug?
 * See part about it being broken

Comment 2

[Teemu Mannermaa (:wicked)]

Comment on attachment 201559 [details] [diff] [review]
Patch v1

>Index: contrib/sendunsentbugmail.pl
>===================================================================

This is sligthly bitrotten but otherwise seems good.

>Index: contrib/syncLDAP.pl
>===================================================================
>+my $bugzilla_users = $dbh->selectall_hashref(
>+    'SELECT login_name AS new_login_name, realname, disabledtext ' .
>+    'FROM profiles', 'login_name');

This doesn't work:

syncLDAP.pl: DBD::mysql::db selectall_hashref failed: Field 'login_name' does not exist (not one of realname disabledtext new_login_name) [for Statement "SELECT login_name AS new_login_name, realname, disabledtext FROM profiles"] at ./contrib/syncLDAP.pl line 85

>+foreach my $login_name (keys %$bugzilla_users) {
>     # remove whitespaces
>+    $bugzilla_users->{'realname'} =~ s/^\s+|\s+$//g;
> }

This doesn't look like it's going to work.. Shouldn't it use $login_name somehow?

Comment 3

[Frédéric Buclin]

bkor, are you still working on this bug? Else I'm sure gbel would be interested in working on it. ;)

Comment 4

[Olav Vitters]

Attachment: Patch v2

(In reply to comment #2)
> (From update of attachment 201559 [details] [diff] [review] [edit])
> >Index: contrib/sendunsentbugmail.pl
> >===================================================================
> 
> This is sligthly bitrotten but otherwise seems good.

Fixed the bitrot (one SQL)

> >Index: contrib/syncLDAP.pl
> >===================================================================
> >+my $bugzilla_users = $dbh->selectall_hashref(
> >+    'SELECT login_name AS new_login_name, realname, disabledtext ' .
> >+    'FROM profiles', 'login_name');
> 
> This doesn't work:
> 
> syncLDAP.pl: DBD::mysql::db selectall_hashref failed: Field 'login_name' does
> not exist (not one of realname disabledtext new_login_name) [for Statement
> "SELECT login_name AS new_login_name, realname, disabledtext FROM profiles"] at
> ./contrib/syncLDAP.pl line 85

Fixed.

> >+foreach my $login_name (keys %$bugzilla_users) {
> >     # remove whitespaces
> >+    $bugzilla_users->{'realname'} =~ s/^\s+|\s+$//g;
> > }
> 
> This doesn't look like it's going to work.. Shouldn't it use $login_name
> somehow?

Indeed. Fixed and tested until that part. Hopefully no more errors in the LDAP part as I cannot test it here :-( :-(

Comment 5

[Frédéric Buclin]

Comment on attachment 219427 [details] [diff] [review]
Patch v2

>Index: contrib/sendunsentbugmail.pl

Nit: explicitly "use Bugzilla;" as globals.pl will die very soon now.


> my @list;
> while (MoreSQLData()) {
>     push (@list, FetchOneColumn());
> }

Unused code. Remove these lines.



>Index: contrib/syncLDAP.pl

>       while( my ($key, $value) = each(%disable_users) ) {
>+        $dbh->do('UPDATE profiles SET disabledtext = ? WHERE ' .
>+                 $dbh->sql_istrcmp('login_name', '?'), undef, 
>+                 ('auto-disabled by ldap sync', $key));
>       }

Prepare the SQL query outside the while() loop, and use ->execute() inside the loop.


>       while( my ($key, $value) = each(%update_users) ) {
>         if(defined @$value{'new_login_name'}) {
>+          $dbh->do('UPDATE profiles SET login_name = ? WHERE ' .
>+                   $dbh->sql_istrcmp('login_name', '?'), undef,
>+                   (@$value{'new_login_name'}, $key));
>         } else {
>+          $dbh->do('UPDATE profiles SET realname = ? WHERE ' .
>+                   $dbh->sql_istrcmp('login_name', '?'), undef,
>+                   (@$value{'realname'}, $key));
>         }
>       }

Same comment here. Moreover, I prefer @$value{'realname'}[0] as we really want to return a scalar, not an array.

Comment 6

[Olav Vitters]

Attachment: Patch v3: Bugzilla HEAD 2005-05-14

> Nit: explicitly "use Bugzilla;" as globals.pl will die very soon now.

done

> Unused code. Remove these lines.

whoops! removed.

> Prepare the SQL query outside the while() loop, and use ->execute() inside the
> loop.

done

> Same comment here. Moreover, I prefer @$value{'realname'}[0] as we really want
> to return a scalar, not an array.

Done and made same change for @$value{'new_login_name'}.

Comment 7

[Olav Vitters]

Attachment: Patch v4: Use @$values

Comment 8

[Frédéric Buclin]

Comment on attachment 221986 [details] [diff] [review]
Patch v4: Use @$values

>Index: contrib/syncLDAP.pl

>+          $sth_update_login->execute(@$value{'new_login_name'}[0], $key);

>+          $sth_update_realname->execute(@$value{'realname'}[0], $key);

>+        insert_new_user($key, @$value{'realname'}[0]);

Next time I should look at the code more carefully. I thought $value{'foo'} was a reference to an array, but it's a reference to an hash and so appending [0] is wrong here. To make more sense, we should write $value->{'foo'} instead as $value is really a ref to an hash. This file really needs some cleanup, so let's use the $value->{'foo'} notation in another bug. Meanwhile, I r+ this patch and the one doing the checkin should remove these [0] on checkin. Sorry for the confusion, bkor. r=LpSolit

Comment 9

[Frédéric Buclin]

Attachment: patch, v4.1

[0] removed

Comment 10

[Dave Miller [:justdave]]

Comment on attachment 221992 [details] [diff] [review]
patch, v4.1

carrying forward r+ from previous patch based on interdiff exactly matching the previous reviewer's comments.

Comment 11

[Frédéric Buclin]

Checking in contrib/sendbugmail.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/sendbugmail.pl,v  <--  sendbugmail.pl
new revision: 1.4; previous revision: 1.3
done
Checking in contrib/sendunsentbugmail.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/sendunsentbugmail.pl,v  <--  sendunsentbugmail.pl
new revision: 1.8; previous revision: 1.7
done
Checking in contrib/syncLDAP.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/syncLDAP.pl,v  <--  syncLDAP.pl
new revision: 1.5; previous revision: 1.4
done