300532 - change the editversions.cgi code to use the Version.pm and Product.pm.

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Tiago Mello [:timello]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.6) Gecko/20050524 Firefox/1.0 (Ubuntu package 1.0.2 MFSA2005-44)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.6) Gecko/20050524 Firefox/1.0 (Ubuntu package 1.0.2 MFSA2005-44)

We can start the code migration that will use the new classes: Version.pm,
Product.pm, Component.pm and Classification.pm.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Tiago Mello [:timello]]

Attachment: v1: cleaning up editversions.cgi

Comment 2

[Christian Reis]

The TestProduct and TestVersion functions seem to have become unnecessary now,
given that all you need to do is instantiate a Product or a Version -- how about
getting rid of them?

Comment 3

[Frédéric Buclin]

Comment on attachment 189113 [details] [diff] [review]
v1: cleaning up editversions.cgi

> sub TestProduct ($)
> {
>-    my $prod = shift;
>-
>-    # does the product exist?
>-    SendSQL("SELECT name
>-             FROM products
>-             WHERE name = " . SqlQuote($prod));
>-    return FetchOneColumn();
>+    my $product_name = shift;
>+    my $product = new Bugzilla::Product({name => $product_name});
>+    return $product;
> }

kiko is right. These Test* routines must go away: TestProduct() is now
equivalent to "new Bugzilla::Product". We don't need a routine to do that.


> sub CheckProduct ($)
> {
>-    my $prod = shift;
>+    my $product_name = shift;
> 
>     # do we have a product?
>-    unless ($prod) {
>+    unless ($product_name) {
>         ThrowUserError('product_not_specified');    
>     }
> 
>-    unless (TestProduct $prod) {
>+    unless (TestProduct $product_name) {
>         ThrowUserError('product_doesnt_exist',
>-                       {'product' => $prod});
>+                       {'product' => $product_name});
>     }
> }

editproducts.cgi and some other edit*.cgi files will require this routine too.
It should go into Product.pm.


> sub TestVersion ($$)
> {
>-    my ($prod,$ver) = @_;
>-
>-    # does the product exist?
>-    SendSQL("SELECT products.name, value
>-             FROM versions, products
>-             WHERE versions.product_id = products.id
>-               AND products.name = " . SqlQuote($prod) . "
>-               AND value = " . SqlQuote($ver));
>-    return FetchOneColumn();
>+    my ($product_id, $version_name) = @_;
>+    my $version = new Bugzilla::Version($product_id, $version_name);
>+    return $version;
> }

Same comment as for TestProduct. Remove this one too.


> sub CheckVersion ($$)
> {
>-    my ($prod, $ver) = @_;
>+    my ($product_name, $version_name) = @_;
> 
>     # do we have the version?
>-    unless ($ver) {
>+    unless ($version_name) {
>         ThrowUserError('version_not_specified');
>     }
> 
>-    CheckProduct($prod);
>+    CheckProduct($product_name);
>+    my $product = new Bugzilla::Product({name => $product_name});
> 
>-    unless (TestVersion $prod, $ver) {
>+    unless (TestVersion $product->id, $version_name) {
>         ThrowUserError('version_not_valid',
>-                       {'product' => $prod,
>-                        'version' => $ver});
>+                       {'product' => $product->name,
>+                        'version' => $version_name});
>     }
> }

Has to go into Version.pm.


>+    CheckProduct($product_name);
>+    my $product = new Bugzilla::Product({name => $product_name});

This doesn't make sense. CheckProduct already creates a product object (by
calling the now obsolete TestProduct routine). Use this object instead of
creating a new one again. This remark applies all over the file.


>+    CheckVersion($product_name, $version_name);
>+    my $product = new Bugzilla::Product({name => $product_name});

Even worse! Now a product object is created three times! TestProduct,
CheckVersion and here directly. :(


>+    CheckVersion($product_name, $version_name);
>+    my $product = new Bugzilla::Product({name => $product_name});
>+    my $version = new Bugzilla::Version($product->id, $version_name);

Both objects are already created. This remark applies later in the code too.


>+sub get_all_products () {
>+    my $dbh = Bugzilla->dbh;
>+
>+    my $ids = $dbh->selectcol_arrayref(q{
>+        SELECT id FROM products
>+        ORDER BY products.name}, undef, ());

Even better: my $ids = $dbh->selectcol_arrayref("SELECT id FROM products ORDER
BY name");


>-sub value      { return $_[0]->{'value'};      }
>+sub name       { return $_[0]->{'value'};      }

Do we really want this change?

Comment 4

[Tiago Mello [:timello]]

Attachment: v2: removing the useless routines from editversions.cgi

Comment 5

[Frédéric Buclin]

Comment on attachment 189227 [details] [diff] [review]
v2: removing the useless routines from editversions.cgi

> # often used variables
> #
>-my $product = trim($cgi->param('product') || '');
>-my $version = trim($cgi->param('version') || '');
>-my $action  = trim($cgi->param('action')  || '');
>+my $product_name = trim($cgi->param('product') || '');
>+my $version_name = trim($cgi->param('version') || '');
>+my $action       = trim($cgi->param('action')  || '');
>+my $product      = Bugzilla::Product::check_product($product_name);

This is not the right place for
Bugzilla::Product::check_product($product_name). Give a chance to the user to
choose a product first.


>-    unless ($version) {
>+    unless ($version_name) {
>         ThrowUserError('version_blank_name',
>-                       {'name' => $version});
>+                       {'name' => $version_name});
>     }

$version_name does not exist. This doesn't make sense to pass it as a param.
Moreover, let Version::check_version() do it for you.


>     SendSQL("INSERT INTO versions ( " .
>             "value, product_id" .
>             " ) VALUES ( " .
>-            SqlQuote($version) . ", $product_id)");
>+            SqlQuote($version_name) . ", " . $product->id . ")");

Please convert SQL statements you change to the new DBI stuff.


>-    $vars->{'name'} = $version;
>-    $vars->{'product'} = $product;
>+    $vars->{'name'} = $version_name;
>+    $vars->{'product'} = $product->name;

Use $version->name.


>-    if ($version ne $versionold) {
>-        unless ($version) {
>+    if ($version_name ne $version_old->name) {
>+        unless ($version_name) {
>             ThrowUserError('version_blank_name');
>         }
>-        if (TestVersion($product,$version)) {
>+        my $version = new Bugzilla::Version($product->id,
>+                                            $version_name);

This is Version::check_version()'s job!


>+sub get_all_products () {
>+    my $dbh = Bugzilla->dbh;
>+
>+    my $ids = $dbh->selectcol_arrayref(q{
>+        SELECT id FROM products ORDER BY products.name});

Nit: ORDER BY name.


>+sub check_version ($$) {
>+    my ($product_name, $version_name) = @_;
>+
>+    unless ($version_name) {
>+        ThrowUserError('version_not_specified');
>+    }
>+
>+    my $product = Bugzilla::Product::check_product($product_name);

Do not create a product object again.

Comment 6

[Tiago Mello [:timello]]

Attachment: v3: some cleanups

Comment 7

[Tiago Mello [:timello]]

Attachment: v4: fixes.

Comment 8

[Frédéric Buclin]

Comment on attachment 189714 [details] [diff] [review]
v4: fixes.

Works great! This is a nice cleanup. r=LpSolit

Comment 9

[Frédéric Buclin]

Checking in editversions.cgi;
/cvsroot/mozilla/webtools/bugzilla/editversions.cgi,v  <--  editversions.cgi
new revision: 1.35; previous revision: 1.34
done
Checking in Bugzilla/Product.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Product.pm,v  <--  Product.pm
new revision: 1.2; previous revision: 1.1
done
Checking in Bugzilla/Version.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Version.pm,v  <--  Version.pm
new revision: 1.2; previous revision: 1.1
done