303708 - Eliminate deprecated Bugzilla::DB routines from BugzillaEmail.pm, bug_email.pl and bugzilla_email_append.pl in contrib

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Teemu Mannermaa (:wicked)]

These lines need to rewritten to use DBI:

contrib/BugzillaEmail.pm:52:    SendSQL($stmt);
contrib/BugzillaEmail.pm:53:    my $found_address = FetchOneColumn();
contrib/BugzillaEmail.pm:59:    SendSQL($stmt);
contrib/BugzillaEmail.pm:65:    while ((!$found) && ($found_address =
FetchOneColumn())) {
contrib/BugzillaEmail.pm:77:    SendSQL($stmt);
contrib/BugzillaEmail.pm:78:    my $found_address = FetchOneColumn();

contrib/bug_email.pl:168:        $sql .= "$bugid, now(), " . SqlQuote(
$description ) . ", ";
contrib/bug_email.pl:169:        $sql .= SqlQuote( $mime ) . ", ";
contrib/bug_email.pl:171:        $sql .= SqlQuote( $decoded_file ) . ", ";
contrib/bug_email.pl:172:        $sql .= SqlQuote( $data ) . ", ";
contrib/bug_email.pl:174:        SendSQL( $sql ) unless( $test );
contrib/bug_email.pl:198:#    SendSQL("select login_name from profiles,groups
where groups.name='$GroupName' and profiles.groupset & groups.bit = groups.bit
and profiles.login_name=\'$Name\'");
contrib/bug_email.pl:199:#    my $NewName = FetchOneColumn();
contrib/bug_email.pl:206:#    SendSQL($query);
contrib/bug_email.pl:207:#    my $check_name = FetchOneColumn();
contrib/bug_email.pl:221:    SendSQL("select name from products where name = " .
SqlQuote($Product));
contrib/bug_email.pl:222:    my $Result = FetchOneColumn();
contrib/bug_email.pl:236:    SendSQL("select components.name from components,
products where components.product_id = products.id AND products.name=" .
SqlQuote($Product) . " and components.name=" . SqlQuote($Component));
contrib/bug_email.pl:237:    my $Result = FetchOneColumn();
contrib/bug_email.pl:251:    SendSQL("select value from versions, products where
versions.product_id = products.id AND products.name=" . SqlQuote($Product) . "
and value=" . SqlQuote($Version));
contrib/bug_email.pl:252:    my $Result = FetchOneColumn();
contrib/bug_email.pl:294:    SendSQL( "describe bugs $fieldname" );
contrib/bug_email.pl:295:    my ($f, $type) = FetchSQLData();
contrib/bug_email.pl:320:            SqlQuote( Param('defaultpriority')) . "\n";
contrib/bug_email.pl:344:            SqlQuote( "normal" ) . "\n";
contrib/bug_email.pl:366:            SqlQuote( "BUILD" ) . "\n";
contrib/bug_email.pl:388:            SqlQuote( "All" ) . "\n";
contrib/bug_email.pl:410:            SqlQuote( "Linux" ) . "\n";
contrib/bug_email.pl:428:    while( MoreSQLData() ){
contrib/bug_email.pl:429:        push( @res, FetchOneColumn() );
contrib/bug_email.pl:836:        SendSQL("select initialqacontact from
components, products where components.product_id = products.id AND products.name=" .
contrib/bug_email.pl:837:                SqlQuote($Control{'product'}) .
contrib/bug_email.pl:838:                " and components.name=" .
SqlQuote($Control{'component'}));
contrib/bug_email.pl:839:        $Control{'qa_contact'} = FetchOneColumn();
contrib/bug_email.pl:860:    SendSQL("select name from products ORDER BY name");
contrib/bug_email.pl:900:        SendSQL("SELECT components.name FROM
components, products WHERE components.product_id=products.id AND products.name =
" . SqlQuote($prod));
contrib/bug_email.pl:912:        $Text .= " * You did not send a component, but
a valid product " . SqlQuote( $Product ) . ".\n";
contrib/bug_email.pl:913:        $Text .= " * This product only has one
component ". SqlQuote(  $Component ) .".\n" .
contrib/bug_email.pl:933:    SendSQL("select initialowner from components,
products where " .
contrib/bug_email.pl:935:            SqlQuote($Control{'product'}) .
contrib/bug_email.pl:936:            " and components.name=" .
SqlQuote($Control{'component'}));
contrib/bug_email.pl:937:    $Control{'assigned_to'} = FetchOneColumn();
contrib/bug_email.pl:978:        $Text .= "Valid versions for product " .
SqlQuote( $prod ) . " are: \n\t";
contrib/bug_email.pl:980:        SendSQL("select value from versions, products
where versions.product_id=products.id AND products.name=" . SqlQuote( $prod ));
contrib/bug_email.pl:991:        $Text .= " * You did not send a version, but a
valid product " . SqlQuote( $Product ) . ".\n";
contrib/bug_email.pl:992:        $Text .= " * This product has has only the one
version ". SqlQuote(  $Version) .".\n" .
contrib/bug_email.pl:1016:SendSQL("select id from groups where name=" .
SqlQuote( $DefaultGroup ));
contrib/bug_email.pl:1017:my $default_group = FetchOneColumn();
contrib/bug_email.pl:1038:      SendSQL("select id, Name from groups where
name=" . SqlQuote($_));
contrib/bug_email.pl:1039:        my( $bval, $bname ) = FetchSQLData();
contrib/bug_email.pl:1054:      SendSQL( "select g.name from groups g,
user_group_map u where u.user_id=".$Control{'reporter'}.
contrib/bug_email.pl:1113:        $query .= SqlQuote($Control{$field}) . ",\n";
contrib/bug_email.pl:1138:    SendSQL("SELECT now()");
contrib/bug_email.pl:1139:    my $bug_when = FetchOneColumn();
contrib/bug_email.pl:1142:    my $state = SqlQuote("UNCONFIRMED");
contrib/bug_email.pl:1144:    SendSQL("SELECT votestoconfirm FROM products WHERE
name = " .
contrib/bug_email.pl:1145:            SqlQuote($Control{'product'}));
contrib/bug_email.pl:1146:    if (!FetchOneColumn()) {
contrib/bug_email.pl:1148:      $state = SqlQuote("NEW");
contrib/bug_email.pl:1152:#    $query .=  SqlQuote( "NEW" ) . ", now(), " .
SqlQuote($comment) . " )\n";
contrib/bug_email.pl:1154:    SendSQL("SELECT userid FROM profiles WHERE " .
contrib/bug_email.pl:1156:    my $userid = FetchOneColumn();
contrib/bug_email.pl:900:        SendSQL("SELECT components.name FROM
components, products WHERE components.product_id=products.id AND products.name =
" . SqlQuote($prod));
contrib/bug_email.pl:912:        $Text .= " * You did not send a component, but
a valid product " . SqlQuote( $Product ) . ".\n";
contrib/bug_email.pl:913:        $Text .= " * This product only has one
component ". SqlQuote(  $Component ) .".\n" .
contrib/bug_email.pl:933:    SendSQL("select initialowner from components,
products where " .
contrib/bug_email.pl:935:            SqlQuote($Control{'product'}) .
contrib/bug_email.pl:936:            " and components.name=" .
SqlQuote($Control{'component'}));
contrib/bug_email.pl:937:    $Control{'assigned_to'} = FetchOneColumn();
contrib/bug_email.pl:978:        $Text .= "Valid versions for product " .
SqlQuote( $prod ) . " are: \n\t";
contrib/bug_email.pl:980:        SendSQL("select value from versions, products
where versions.product_id=products.id AND products.name=" . SqlQuote( $prod ));
contrib/bug_email.pl:991:        $Text .= " * You did not send a version, but a
valid product " . SqlQuote( $Product ) . ".\n";
contrib/bug_email.pl:992:        $Text .= " * This product has has only the one
version ". SqlQuote(  $Version) .".\n" .
contrib/bug_email.pl:1016:SendSQL("select id from groups where name=" .
SqlQuote( $DefaultGroup ));
contrib/bug_email.pl:1017:my $default_group = FetchOneColumn();
contrib/bug_email.pl:1038:      SendSQL("select id, Name from groups where
name=" . SqlQuote($_));
contrib/bug_email.pl:1039:        my( $bval, $bname ) = FetchSQLData();
contrib/bug_email.pl:1054:      SendSQL( "select g.name from groups g,
user_group_map u where u.user_id=".$Control{'reporter'}.
contrib/bug_email.pl:1113:        $query .= SqlQuote($Control{$field}) . ",\n";
contrib/bug_email.pl:1138:    SendSQL("SELECT now()");
contrib/bug_email.pl:1139:    my $bug_when = FetchOneColumn();
contrib/bug_email.pl:1142:    my $state = SqlQuote("UNCONFIRMED");
contrib/bug_email.pl:1144:    SendSQL("SELECT votestoconfirm FROM products WHERE
name = " .
contrib/bug_email.pl:1145:            SqlQuote($Control{'product'}));
contrib/bug_email.pl:1146:    if (!FetchOneColumn()) {
contrib/bug_email.pl:1148:      $state = SqlQuote("NEW");
contrib/bug_email.pl:1152:#    $query .=  SqlQuote( "NEW" ) . ", now(), " .
SqlQuote($comment) . " )\n";
contrib/bug_email.pl:1154:    SendSQL("SELECT userid FROM profiles WHERE " .
contrib/bug_email.pl:1156:    my $userid = FetchOneColumn();
contrib/bug_email.pl:900:        SendSQL("SELECT components.name FROM
components, products WHERE components.product_id=products.id AND products.name =
" . SqlQuote($prod));
contrib/bug_email.pl:912:        $Text .= " * You did not send a component, but
a valid product " . SqlQuote( $Product ) . ".\n";
contrib/bug_email.pl:913:        $Text .= " * This product only has one
component ". SqlQuote(  $Component ) .".\n" .
contrib/bug_email.pl:933:    SendSQL("select initialowner from components,
products where " .
contrib/bug_email.pl:935:            SqlQuote($Control{'product'}) .
contrib/bug_email.pl:936:            " and components.name=" .
SqlQuote($Control{'component'}));
contrib/bug_email.pl:937:    $Control{'assigned_to'} = FetchOneColumn();
contrib/bug_email.pl:978:        $Text .= "Valid versions for product " .
SqlQuote( $prod ) . " are: \n\t";
contrib/bug_email.pl:980:        SendSQL("select value from versions, products
where versions.product_id=products.id AND products.name=" . SqlQuote( $prod ));
contrib/bug_email.pl:991:        $Text .= " * You did not send a version, but a
valid product " . SqlQuote( $Product ) . ".\n";
contrib/bug_email.pl:992:        $Text .= " * This product has has only the one
version ". SqlQuote(  $Version) .".\n" .
contrib/bug_email.pl:1016:SendSQL("select id from groups where name=" .
SqlQuote( $DefaultGroup ));
contrib/bug_email.pl:1017:my $default_group = FetchOneColumn();
contrib/bug_email.pl:1038:      SendSQL("select id, Name from groups where
name=" . SqlQuote($_));
contrib/bug_email.pl:1039:        my( $bval, $bname ) = FetchSQLData();
contrib/bug_email.pl:1054:      SendSQL( "select g.name from groups g,
user_group_map u where u.user_id=".$Control{'reporter'}.
contrib/bug_email.pl:1113:        $query .= SqlQuote($Control{$field}) . ",\n";
contrib/bug_email.pl:1138:    SendSQL("SELECT now()");
contrib/bug_email.pl:1139:    my $bug_when = FetchOneColumn();
contrib/bug_email.pl:1142:    my $state = SqlQuote("UNCONFIRMED");
contrib/bug_email.pl:1144:    SendSQL("SELECT votestoconfirm FROM products WHERE
name = " .
contrib/bug_email.pl:1145:            SqlQuote($Control{'product'}));
contrib/bug_email.pl:1146:    if (!FetchOneColumn()) {
contrib/bug_email.pl:1148:      $state = SqlQuote("NEW");
contrib/bug_email.pl:1152:#    $query .=  SqlQuote( "NEW" ) . ", now(), " .
SqlQuote($comment) . " )\n";
contrib/bug_email.pl:1154:    SendSQL("SELECT userid FROM profiles WHERE " .
contrib/bug_email.pl:1156:    my $userid = FetchOneColumn();
contrib/bug_email.pl:900:        SendSQL("SELECT components.name FROM
components, products WHERE components.product_id=products.id AND products.name =
" . SqlQuote($prod));
contrib/bug_email.pl:912:        $Text .= " * You did not send a component, but
a valid product " . SqlQuote( $Product ) . ".\n";
contrib/bug_email.pl:913:        $Text .= " * This product only has one
component ". SqlQuote(  $Component ) .".\n" .
contrib/bug_email.pl:933:    SendSQL("select initialowner from components,
products where " .
contrib/bug_email.pl:935:            SqlQuote($Control{'product'}) .
contrib/bug_email.pl:936:            " and components.name=" .
SqlQuote($Control{'component'}));
contrib/bug_email.pl:937:    $Control{'assigned_to'} = FetchOneColumn();
contrib/bug_email.pl:978:        $Text .= "Valid versions for product " .
SqlQuote( $prod ) . " are: \n\t";
contrib/bug_email.pl:980:        SendSQL("select value from versions, products
where versions.product_id=products.id AND products.name=" . SqlQuote( $prod ));
contrib/bug_email.pl:991:        $Text .= " * You did not send a version, but a
valid product " . SqlQuote( $Product ) . ".\n";
contrib/bug_email.pl:992:        $Text .= " * This product has has only the one
version ". SqlQuote(  $Version) .".\n" .
contrib/bug_email.pl:1016:SendSQL("select id from groups where name=" .
SqlQuote( $DefaultGroup ));
contrib/bug_email.pl:1017:my $default_group = FetchOneColumn();
contrib/bug_email.pl:1038:      SendSQL("select id, Name from groups where
name=" . SqlQuote($_));
contrib/bug_email.pl:1039:        my( $bval, $bname ) = FetchSQLData();
contrib/bug_email.pl:1054:      SendSQL( "select g.name from groups g,
user_group_map u where u.user_id=".$Control{'reporter'}.
contrib/bug_email.pl:1113:        $query .= SqlQuote($Control{$field}) . ",\n";
contrib/bug_email.pl:1138:    SendSQL("SELECT now()");
contrib/bug_email.pl:1139:    my $bug_when = FetchOneColumn();
contrib/bug_email.pl:1142:    my $state = SqlQuote("UNCONFIRMED");
contrib/bug_email.pl:1144:    SendSQL("SELECT votestoconfirm FROM products WHERE
name = " .
contrib/bug_email.pl:1145:            SqlQuote($Control{'product'}));
contrib/bug_email.pl:1146:    if (!FetchOneColumn()) {
contrib/bug_email.pl:1148:      $state = SqlQuote("NEW");
contrib/bug_email.pl:1152:#    $query .=  SqlQuote( "NEW" ) . ", now(), " .
SqlQuote($comment) . " )\n";
contrib/bug_email.pl:1154:    SendSQL("SELECT userid FROM profiles WHERE " .
contrib/bug_email.pl:1156:    my $userid = FetchOneColumn();
contrib/bug_email.pl:1161:        SendSQL($query);
contrib/bug_email.pl:1165:        my $long_desc_query = "INSERT INTO longdescs
SET bug_id=$id, who=$userid, bug_when=\'$bug_when\', thetext=" . SqlQuote($comment);
contrib/bug_email.pl:1166:        SendSQL($long_desc_query);
contrib/bug_email.pl:1181:        SendSQL("INSERT INTO bug_group_map SET
bug_id=$id, group_id=$GroupArr{$grp}");

contrib/bugzilla_email_append.pl:98:SendSQL("SELECT bug_id FROM bugs WHERE
bug_id = $bugid;");
contrib/bugzilla_email_append.pl:99:my $found_id = FetchOneColumn();
contrib/bugzilla_email_append.pl:106:SendSQL("SELECT userid FROM profiles WHERE " .
contrib/bugzilla_email_append.pl:108:my $userid = FetchOneColumn();
contrib/bugzilla_email_append.pl:122:my $long_desc_query = "INSERT INTO
longdescs SET bug_id=$found_id, who=$userid, bug_when=NOW(), thetext=" .
SqlQuote($Body) . ";";
contrib/bugzilla_email_append.pl:123:SendSQL($long_desc_query);

Comment 1

[Gabriel Sales de Oliveira]

Attachment: v1-diff

version 1.

Comment 2

[Frédéric Buclin]

Comment on attachment 218024 [details] [diff] [review]
v1-diff

>Index: contrib/BugzillaEmail.pm

Moreover, $dbh should be defined in findUser().


>+    my $stmt = q{SELECT login_name FROM profiles WHERE } .
>                $dbh->sql_istrcmp('login_name', $dbh->quote($address));

Use placeholders here too for $address:
$dbh->sql_istrcmp('login_name', '?')


>+    my $stmt = q{SELECT login_name FROM profiles WHERE } . $dbh->sql_regexp(
>                $dbh->sql_istring('login_name'), $dbh->sql_istring($dbh->quote($username)));

Same comment.


>+    my $found_address = $dbh->selectrow_array($stmt);

>-    while ((!$found) && ($found_address = FetchOneColumn())) {
>+    while ((!$found)) {

Wrong! You can have several login names which match the regexp, so you should use selectcol_arrayref() instead and use it in the loop in a "clever" way (clever because it's not trivial).


>-    my $stmt = "SELECT login_name FROM profiles WHERE " .$dbh->sql_regexp(
>+    my $stmt = q{SELECT login_name FROM profiles WHERE } .$dbh->sql_regexp(
>                 $dbh->sql_istring('login_name'), $dbh->sql_istring($dbh->quote($username)));

Use a placeholder here too for $username.


>+    my $found_address = $dbh->selectrow_array($stmt);
>     return $found_address;

It looks like several login names may match the regexp, but the original code seems to say that only one can match. Go figure! This means what you wrote seems correct here.



>Index: contrib/bug_email.pl

This script should also explicitly 'use Bugzilla;'.
$dbh should also be defined in each subroutine which require it.


>+        my @values = ($bugid);
>+        push (@values, $timestamp, $description, $mime, $decoded_file, $submitter_id);

write: my @values = ($bugid, $timestamp, ...)


> 
>         # Make SQL-String
>+        $sth_attach->execute(@values) unless( $test );
>+        $sth_data->execute($data) unless( $test );

The comment doesn't make sense anymore. Remove it.


Nit: could you remove the big comment in the CheckPermissions() routine so that we don't catch SendSQL there anymore when doing a grep?


> sub CheckProduct {

>+    my $prod_name = $dbh->selectrow_array(q{SELECT name
>+                                              FROM products
>+                                             WHERE name = ?}, undef, $Product);
>+    if (lc($prod_name) eq lc($Product)) {
>+        return $prod_name;
>     } else {
>         return "";
>     }

Nit: if $prod_name exists, then we already know that |lc($prod_name) eq lc($Product)|. This is a nit as the original code already introduces this useless check. :)


> sub CheckComponent {

>+    my $comp_name = $dbh->selectrow_array(q{SELECT components.name 
>+                                              FROM components, products 
>+                                             WHERE components.product_id = products.id 

Write:
FROM components
INNER JOIN products
ON components.product_id = products.id
WHERE ...


>+    if (lc($comp_name) eq lc($Component)) {
>+        return $comp_name;
>     } else {
>         return "";
>     }

Nit: same remark as above about this useless test.


> sub CheckVersion {

>+    my $version_value = $dbh->selectrow_array(q{SELECT value 
>+                                                  FROM versions, products 
>+                                                 WHERE versions.product_id = products.id 

Same comment as above: use INNER JOIN.


>+    if (lc($version_value) eq lc($Version)) {
>+        return $version_value;
>     } else {
>         return "";
>     }

Nit: same comment.


>+        $Text .= "*  The bug_severity is set to the default value " . 
>+            "normal" . "\n";

You could as well write it on one line. "normal" won't have quotes displayed. If you want to show quotes, you have to write "'normal'".


>+        $Text .= "*  The area is set to the default value " . "BUILD" . "\n";

Same comment here.


>+        $Text .= "*  The rep_platform is set to the default value " . "All" . "\n";

And here.


>+        $Text .= "*  The op_sys is set to the default value " . "Linux" . "\n";

And here.


>+        $Control{'qa_contact'} = $dbh->selectrow_array(q{
>+                                    SELECT initialqacontact 
>+                                      FROM components, products 
>+                                     WHERE components.product_id = products.id

Use INNER JOIN.


>-    @all_products = FetchAllSQLData();
>-    $Text .= join( "\n\t", @all_products ) . "\n\n";
>+    my $all_products = $dbh->selectcol_arrayref(q{SELECT name 
>+                                                    FROM products 
>+                                                ORDER BY name});

@all_products is used later in the code. So you still have to define it: @foo = @$foo.


>     foreach my $prod ( @all_products ) {

>+        $val_components = $dbh->selectcol_arrayref(q{

We are in a FOREACH loop. Prepare it outside the loop.


>+                                SELECT components.name 
>+                                  FROM components, products 
>+                                 WHERE components.product_id=products.id 

Use INNER JOIN.


>-        $Component = $val_components[0];
>+        $Component = @$val_components;

Wrong! You have to write: $Component = @$val_components[0], else the number of elements in the array is returned (1) instead of its content.


>+        $Text .= " * You did not send a component, but a valid product " . $Product . ".\n";
>+        $Text .= " * This product only has one component ". $Component .".\n" .

You can insert variables in the string directly: "... valid product $Product \n".


>+    $Control{'assigned_to'} = $dbh->selectrow_array(q{
>+                                SELECT initialowner 
>+                                  FROM components, products 
>+                                 WHERE components.product_id=products.id 

Here again, use INNER JOIN.


>+    my $sth_versions = $dbh->prepare(q{SELECT value 
>+                                         FROM versions, products 
>+                                        WHERE versions.product_id = products.id 

Same comment.


>     foreach my $prod ( @all_products ) {

See, @all_products is used here. :)


>+        $Text .= "Valid versions for product " . $prod  . " are: \n\t";

$prod can be contained in quotes: "... product $prod are: \n\t";


>+        @all_versions = $dbh->selectrow_array($sth_versions, undef, $prod);

There are several versions per product. Use selectcol_arrayref().


>+        $Text .= " * You did not send a version, but a valid product " . $Product . ".\n";
>+        $Text .= " * This product has has only the one version ". $Version .".\n" .

...


>+my $default_group = $dbh->selectcol_arrayref(q{
>+                                    SELECT id 
>+                                      FROM groups 
>+                                     WHERE name= ?}, undef, $DefaultGroup);

The name of groups is unique. So only one group will match. Use selectrow_array().


>+        my $groups = $dbh->selectcol_arrayref(q{
>+                                SELECT g.name 
>+                                  FROM groups g, user_group_map u 

Use INNER JOIN.

>+                              GROUP BY g.name}, undef, $Control{'reporter'});

This won't work on PostgreSQL. Use $dbh->sql_group_by().


>+    my $v_confirm = $dbh->selectcol_arrayref(q{SELECT votestoconfirm 
>+                                                 FROM products 
>+                                                WHERE name = ?}, undef, $Control{'product'});

There is only one value per product. Use selectrow_array().


>-    SendSQL("SELECT userid FROM profiles WHERE " .
>-            $dbh->sql_istrcmp('login_name', $dbh->quote($reporter)));
>-    my $userid = FetchOneColumn();

>+    my $userid = $dbh->selectcol_arrayref(q{SELECT userid 
>+                                             FROM profiles 
>+                                            WHERE login_name = ?}, undef, $reporter);

First of all, use selectrow_array() as there will be only one match (selectcol_arrayref returns a reference to an array, not a scalar!).
Secondly, $dbh->sql_istrcmp() is required by PostgreSQL. So don't remove it!


>     foreach my $grp (keys %GroupArr) {
>+        if( ! $test) {
>+            $dbh->do(q{INSERT INTO bug_group_map SET bug_id= ?, group_id= ?}, undef, $id, $GroupArr{$grp});

Prepare it outside the FOREACH loop.



>Index: contrib/bugzilla_email_append.pl

This script should 'use Bugzilla;' explicitly.


>+my $found_id = $dbh->selectcol_arrayref(q{SELECT bug_id 
>+                                            FROM bugs 
>+                                           WHERE bug_id = ?}, undef, $bugid);

Use selectrow_array().


>-SendSQL("SELECT userid FROM profiles WHERE " . 
>-        $dbh->sql_istrcmp('login_name', $dbh->quote($SenderShort)));
>-my $userid = FetchOneColumn();
>+my $userid = $dbh->selectcol_arrayref(q{SELECT userid 
>+                                    FROM profiles 
>+                                   WHERE login_name = ?}, undef, $SenderShort);

First of all, use selectrow_array(). Secondly, $dbh->sql_istrcmp() is required by PostgreSQL.


>+$dbh->do(q{INSERT INTO longdescs SET bug_id= ?, who= ?, bug_when= ?,thetext= ? },
>+          undef, $bugid, @$userid, $bug_when, $Body);

The bug ID is $found_id, not $bugid and @$userid will pass an array, not a scalar (?!).


>-Bugzilla::BugMail::Send( $found_id, { changer => $SenderShort } );
>+Bugzilla::BugMail::Send( @$found_id, { changer => $SenderShort } );

If you used selectrow_array(), you wouldn't have to change this line.

Comment 3

[Gabriel Sales de Oliveira]

Attachment: v2-fixed

I change the while loop to a foreach and fixed the other things.

Comment 4

[Frédéric Buclin]

Comment on attachment 219015 [details] [diff] [review]
v2-fixed

As discussed on IRC, some points of my previous review haven't been fixed. Fix them and I will review your updated patch.

Comment 5

[Gabriel Sales de Oliveira]

Attachment: v3-fixed

fixed diff.

Comment 6

[Frédéric Buclin]

Comment on attachment 220606 [details] [diff] [review]
v3-fixed

>Index: contrib/bug_email.pl

>+        $sth_attach->execute(@values) unless( $test );
>+        $sth_data->execute($data) unless( $test );

Nit even better:

unless ($test) {
    $sth_attach->execute(@values);
    $sth_data->execute($data);
}


>+    my $all_products = $dbh->selectcol_arrayref(q{SELECT name 

$all_products has already been defined outside the block. Remove "my" here.


>+        @all_versions = $dbh->selectrow_array($sth_versions, undef, $prod);

Wrong! Use $all_versions and selectcol_arrayref(). A product can have more than one version. Or to avoid changing too much code, write:
@all_versions = @{$dbh->selectcol_arrayref(...)};


>+        my $groups = $dbh->selectcol_arrayref(q{
>+                                SELECT g.name 
>+                                  FROM groups g
>+                            INNER JOIN user_group_map u 
>+                                    ON g.id = u.group_id 
>+                                 WHERE u.user_id = ? 
>+                                   AND g.isbuggroup = 1} .
>+                     $dbh->sql_group_by('g.name'), undef, $Control{'reporter'});

Missing whitespace after "1".



>Index: contrib/bugzilla_email_append.pl

>+my $userid = $dbh->selectrow_array(q{SELECT userid FROM profiles WHERE} .
>+                $dbh->sql_istrcmp('login_name', '?'), undef, $SenderShort);

Missing whitespace after WHERE.



>+my $bug_when = $dbh->selectrow_array("SELECT NOW()");
>+$dbh->do(q{INSERT INTO longdescs SET bug_id= ?, who= ?, bug_when= ?,thetext= ? },
>+          undef, $found_id, $userid, $bug_when, $Body);

Leave NOW() in the query itself => bug_when = NOW().

Comment 7

[Gabriel Sales de Oliveira]

Attachment: v3-fix

Fixed version ;)

Comment 8

[Gabriel Sales de Oliveira]

Attachment: v3-fix-fix

My last patch was with a wrong RCS path sorry about the Spam :(

Comment 9

[Frédéric Buclin]

Comment on attachment 221760 [details] [diff] [review]
v3-fix-fix

The code compiles correctly and the code looks good. r=LpSolit

Comment 10

[Gabriel Sales de Oliveira]

Before commit the patch, add a 'my' before '$all_products' line 827 of bug_email.pl.

I don't know why but without this my i'm getting this error:

[Thu May 11 21:36:09 2006] bug_email.pl: Use of uninitialized value in numeric eq (==) at ./bug_email.pl line 915.
[Thu May 11 21:36:09 2006] bug_email.pl: Use of uninitialized value in pattern match (m//) at ./bug_email.pl line 1060.

:/

Comment 11

[Frédéric Buclin]

(In reply to comment #10)
> Before commit the patch, add a 'my' before '$all_products' line 827 of
> bug_email.pl.

No! Not only that's unrelated to the error message you get, but doing so is WRONG, see one of my previous reviews!

Comment 12

[Frédéric Buclin]

Checking in contrib/BugzillaEmail.pm;
/cvsroot/mozilla/webtools/bugzilla/contrib/BugzillaEmail.pm,v  <--  BugzillaEmail.pm
new revision: 1.5; previous revision: 1.4
done
Checking in contrib/bug_email.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/bug_email.pl,v  <--  bug_email.pl
new revision: 1.34; previous revision: 1.33
done
Checking in contrib/bugzilla_email_append.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/bugzilla_email_append.pl,v  <--  bugzilla_email_append.pl
new revision: 1.11; previous revision: 1.10
done