300952 - change the editmilestones.cgi code to use the Milestone.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: 

The editmilestones.cgi code has to be changed to use Milestone.pm.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Tiago Mello [:timello]]

Attachment: v1: cleaning up editmilestones.cgi

Comment 2

[Frédéric Buclin]

Comment on attachment 189478 [details] [diff] [review]
v1: cleaning up editmilestones.cgi

>Index: editmilestones.cgi

> #
> # often used variables
> #
>+my $product_name   = trim($cgi->param('product')     || '');
>+my $milestone_name = trim($cgi->param('milestone')   || '');
>+my $sortkey        = trim($cgi->param('sortkey')     || '0');

Nit: $sortkey is an integer ->	|| 0 (without quotes).


>+my $action         = trim($cgi->param('action')      || '');
>+
>+my $product = Bugzilla::Product::check_product($product_name);

Give the user a chance to choose a product. Move it after the UNLESS
($product_name) block below.


> 
> #
> # product = '' -> Show nice list of milestones
> #

Show nice list of *products*, not milestones.


> if ($action eq 'new') {

>+    unless ($milestone_name) {
>         ThrowUserError('milestone_blank_name',
>-                       {'name' => $milestone});
>+                       {'name' => $milestone_name});

milestone_blank_name takes no parameter, especially something which is
undefined ($milestone_name).


>+    my $milestone = new Bugzilla::Milestone($product->id,
>+                                            $milestone_name);

Pass the product object, not only its ID. This way, we avoid to recreate this
object in Milestone.pm.


>+    if ($milestone) {
>         ThrowUserError('milestone_already_exists',
>-                       {'name' => $milestone,
>-                        'product' => $product});
>+                       {'name' => $milestone_name,
>+                        'product' => $product->name});

Nit: $milestone_name -> $milestone->name.


>     $dbh->do('INSERT INTO milestones ( value, product_id, sortkey )
>               VALUES ( ?, ?, ? )',
>              undef,
>+             $milestone_name,
>+             $product->id,
>              $sortkey);

Nit: could we write it as undef, ($milestone_name, $product->id, $sortkey),
i.e. on the same line? ;)


> if ($action eq 'del') {

>+    my $milestone = Bugzilla::Milestone::check_milestone($product->name,
>+                                                         $milestone_name);
>     # The default milestone cannot be deleted.
>-    if ($vars->{'default_milestone'} eq $milestone) {
>+    if ($vars->{'default_milestone'} eq $milestone->name) {

$vars->{'default_milestone'} is not defined yet. Write:
if ($product->default_milestone eq $milestone_name) and update $vars
accordingly, or pass the required parameters directly to ThrowUserError, which
is even better.


>+    $vars->{'default_milestone'} = $product->default_milestone;

See, you define $vars->{'default_milestone'} here only!


> if ($action eq 'delete') {

>     my $dbh = Bugzilla->dbh;

Nit: can you remove these useless $dbh definition? We have already one defined
at the top of the file and this block is not a routine.


>+    my $milestone =
>+        Bugzilla::Milestone::check_milestone($product->name,
>+                                             $milestone_name);
>+    $vars->{'name'} = $milestone_name;

Nit: $vars->{'name'} = $milestone->name;


> if ($action eq 'edit') {

>+    $vars->{'sortkey'} = $milestone->sortkey || 0;

Huh?? $milestone->sortkey is *always* defined. Look at the 'milestones' table
definition: sortkey NOT NULL DEFAULT 0. So || 0 is useless!


if ($action eq 'update') {
>     my $milestoneold = trim($cgi->param('milestoneold') || '');
>     my $sortkeyold = trim($cgi->param('sortkeyold')     || '0');

For consistency: $milestone_old_name and $sortkey_old. Moreover, $sortkey_old
is an integer -> || 0.


if ($action eq 'update') {


>Index: Bugzilla/Milestone.pm

>+sub check_milestone ($$) {
>+    my ($product_name, $milestone_name) = @_;

This routine should take a product object. This way, we avoid to recreate it
again.


>+    my $product = Bugzilla::Product::check_product($product_name);

This becomes useless with my previous comment.


>  Params:      $product_id - Integer with a Bugzilla product id.

Nit: why a *Buzilla* product?? ;)


>+ Description: Returns all Bugzilla product milestones that belong
>+              to the supplied product.
>+
>+ Params:      $product_name - String with a Bugzilla product name.
>+              $milestone_name - String with a Bugzilla milestone name.

Nits: same comment here.


>Index: Bugzilla/Product.pm

This part is no longer required due to a previous checkin (bug 300532).

Comment 3

[Tiago Mello [:timello]]

Attachment: v2: fixes

Comment 4

[Frédéric Buclin]

Comment on attachment 189926 [details] [diff] [review]
v2: fixes

>Index: editmilestones.cgi

> use vars qw($template $vars);
> 
> my $cgi = Bugzilla->cgi;

Please define 'my $dbh = Bugzilla->dbh' here and remove *all* other occurences
of this definition (there is one per block!). This way, we are sure it's
defined and we don't have to redefine it everytime!


> #
> # Preliminary checks:
> #

Nit: Just below this title, there is a "print Bugzilla->cgi->header()". Could
you please convert it to $cgi->header().


>-unless ($product) {

>       || ThrowTemplateError($template->error());
>-
>+    
>     exit;
> }

Please remove these useless whitespaces.



> if ($action eq 'del') {

>     # The default milestone cannot be deleted.
>+    if ($product->default_milestone eq $milestone_name) {

Nit: $milestone->name.


>+        ThrowUserError("milestone_is_default",
>+            {name => $milestone_name, product => $product->name});

Hum... $vars->{'name'} and $vars->{'product'} are required below anyway, so it
doesn't make sense to duplicate the code. Please define them before calling
ThrowUserError and pass $vars to the routine.


>+    $vars->{'default_milestone'} = $product->default_milestone;
>+    $vars->{'name'}              = $milestone->name;
>+    $vars->{'product'}           = $product->name;

$vars->{'default_milestone'} is not required by
admin/milestones/confirm-delete.html.tmpl so there is no need to define it.
Please remove it as well as the comment in this template (old code, but the
comment is still here, which is confusing).


> if ($action eq 'delete') {

>+    my $milestone =
>+        Bugzilla::Milestone::check_milestone($product,
>+                                             $milestone_name);
>+    $vars->{'name'} = $milestone_name;

Nit: $milestone->name.


> if ($action eq 'update') {
> 
>-    my $milestoneold = trim($cgi->param('milestoneold') || '');
>-    my $sortkeyold = trim($cgi->param('sortkeyold')     || '0');

You don't use $sortkeyold anymore, so its corresponding hidden field in
admin/milestones/edit.html.tmpl has to be removed completely as well. Else it
would appear confusing to have a field which is never used.


>+    if ($milestone_name ne $milestone_old->name) {
>+        my $milestone = 
>+            new Bugzilla::Milestone($product->id, $milestone_name);
>+        if ($milestone) {
>             ThrowUserError('milestone_already_exists',
>+                           {'name' => $milestone_name,
>+                            'product' => $product->name});

Nit: $milestone->name.



>Index: Bugzilla/Milestone.pm

>+sub bug_count {

>+        $self->{'bug_count'} = $dbh->selectrow_array(q{
>+            SELECT COUNT(*) FROM bugs
>+            WHERE product_id = ? AND target_milestone = ?}, undef,
>+            $self->product_id, $self->name);

If there is no bug with this target milestone, undef is returned, which is not
a number. So add || 0.


>+sub check_milestone ($$) {

>+    unless ($milestone) {
>+        ThrowUserError('milestone_not_valid',
>+                       {'product' => $product->name,
>+                        'milestone' => $milestone_name});

Nit: $milestone->name.


>+=item C<check_milestone($product_name, $milestone_name)>

check_milestone takes a product object, not a product name.


>+
>+ Description: Returns all product milestones that belong
>+              to the supplied product.

Huh??? This is the description of another function.


>+ Params:      $product_name - String with a product name.

As said above, it takes a product object as first parameter.

Comment 5

[Tiago Mello [:timello]]

Attachment: v3: some cleanups

Comment 6

[Frédéric Buclin]

Comment on attachment 190406 [details] [diff] [review]
v3: some cleanups

>Index: editmilestones.cgi

> if ($action eq 'del') {

>     # The default milestone cannot be deleted.
>-    if ($vars->{'default_milestone'} eq $milestone) {
>+    if ($product->default_milestone eq $milestone_name) {

Nit: $milestone->name


>+    $vars->{'bug_count'}         = $milestone->bug_count;

Nit: why so many whitespaces??



>Index: Bugzilla/Milestone.pm

>+sub check_milestone ($$) {

>+    unless ($milestone) {
>+        ThrowUserError('milestone_not_valid',
>+                       {'product' => $product->name,
>+                        'milestone' => $milestone->name});
>+    }

Shame on me! You wrote it right and I told you to change it... :(  Of course,
if the milestone object doesn't exist, we cannot use its name. So it should be
$milestone_name. Sorry!


With this error fixed, your patch works fine. As this last error is my fault, I
suggest to fix it on checkin. Feel free to submit a new patch with this bug and
the two nits fixed and carry forward my r+.

r=LpSolit

Comment 7

[Frédéric Buclin]

Checking in editmilestones.cgi;
/cvsroot/mozilla/webtools/bugzilla/editmilestones.cgi,v  <--  editmilestones.cgi
new revision: 1.40; previous revision: 1.39
done
Checking in Bugzilla/Milestone.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Milestone.pm,v  <--  Milestone.pm
new revision: 1.2; previous revision: 1.1
done
Checking in template/en/default/admin/milestones/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/milestones/confirm-delete.html.tmpl,v
 <--  confirm-delete.html.tmpl
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/admin/milestones/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/milestones/edit.html.tmpl,v
 <--  edit.html.tmpl
new revision: 1.2; previous revision: 1.1
done

Comment 8

[Frédéric Buclin]

Attachment: checked in patch (with bug + nits fixed)

I had to fix the bug and these nits for the checkin, so the updated patch is
already here. :)