313125 - implement validations and database persistence functions for Versions.pm and clean-up editversions.cgi

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[André Batosti]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050725 Firefox/1.0.6 (Ubuntu package 1.0.6)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050725 Firefox/1.0.6 (Ubuntu package 1.0.6)

We need methods and/or subroutines for validations and database persistence.
All related routines about inserts, updates, deletes and validations for those
routines have to be implemented at this bug.

Reproducible: Always

Comment 1

[André Batosti]

Attachment: batosti_v1

Comment 2

[Frédéric Buclin]

Comment on attachment 200916 [details] [diff] [review]
batosti_v1

This patch no longer applies due to the checkin of bug 311258.

Comment 3

[André Batosti]

Attachment: batosti_v2

Comment 4

[Frédéric Buclin]

Comment on attachment 213887 [details] [diff] [review]
batosti_v2

>Index: editversions.cgi

> if ($action eq 'new') {
> 
>+    my $version = Bugzilla::Version::create_version($version_name,
>+                                                    $product);

Nit: you could write it on one line.



>Index: Bugzilla/Version.pm

>+sub change_name {
>+    my $self = shift;
>+    my ($name) = (@_);

Two things:
1) @_ is already an array, so write either: "my $name = shift;" or "my ($name) = @_;".
2) I'm not sure "change_name" is a good method name. If you have more than one attribute to update, you would need another name. why not "update"? This way, you would call it using $version->update(), which sounds better IMO.


>+    $dbh->bz_lock_tables('bugs WRITE',
>+                         'versions WRITE',
>+                         'products READ');

Do not lock tables in Version.pm. You have no idea from where this method will be called, and maybe there is already a "lock tables" in use. Keep this lock in editversions.cgi.
Also, note that "products" no longer needs to be locked (which is the reason why your patch bitrots).


>+        if ($version) {
>+            my $product = new Bugzilla::Product($version->product_id);

Nit: I'm not sure this is a good idea to call Product.pm from Version.pm (I'm thinking about dependency loops). Have you tested it?


>+            ThrowUserError('version_already_exists',
>+                           {'name' => $self->name,
>+                            'product' => $product->name});

Wrong! The version which already exists is $version->name (or $name), not $self->name.


>+        $self->{'name'} = $name;

$self->{'name'} doesn't exist. It is $self->{'value'}.


>+        $dbh->bz_unlock_tables();
>+        return 1;
>+    }
>+    $dbh->bz_unlock_tables();
>+    return 0;

Instead of this big IF block, write:
return 0 if ($name eq $self->name);
earlier in the method.



>Index: template/en/default/admin/versions/updated.html.tmpl

>+[% IF updated %]
>   <p>Updated Version name to: '[% version.name FILTER html %]'.</p>
> [% END %]
> 
>+[% UNLESS updated %]
>   <p>Nothing changed for version '[% version.name FILTER html %]'.
> [% END %]

Nit: please update this code using IF ELSE END.

Comment 5

[Frédéric Buclin]

One more thing: add POD in Version.pm.

Comment 6

[André Batosti]

Attachment: batosti_v2_fix

Comment 7

[Frédéric Buclin]

Comment on attachment 217848 [details] [diff] [review]
batosti_v2_fix

>Index: Bugzilla/Version.pm

>+sub update {

>+        my $product = new Bugzilla::Product($version->product_id);

Pass the product object as argument to update().


Else this looks good. Next patch should be the final one. ;)

Comment 8

[André Batosti]

Attachment: batosti_v3

Comment 9

[Frédéric Buclin]

Comment on attachment 228085 [details] [diff] [review]
batosti_v3

>Index: editversions.cgi

>-    # Remove unprintable characters
>-    $version_name = clean_text($version_name);

clean_text() must still be called from Version.pm. Do not remove this call!!


>-    # Remove unprintable characters
>-    $version_name = clean_text($version_name);

Same comment here.



>Index: Bugzilla/Version.pm

>+sub update {

>+    $name || ThrowUserError('version_not_specified');
>+
>+    return 0 if ($name eq $self->name);

As I said above, call clean_text *before* comparing $name with $self->name.


>+sub create_version {

>+    # Cleanups and validity checks
>+    $name || ThrowUserError('version_blank_name');
>+
>+    my $version = new Bugzilla::Version($product->id, $name);

Same comment here.


>+    my $updated = $version->update($version_name);

update() takes two arguments, not one.


>+=item C<update($name)>

Same comment.


>+ Params:      $name - String with version value.

With the *new* version value.


>+ Returns:     A integer.

*An* integer. That's a bit vague. Explain what kind of integer and under which conditions.



>Index: template/en/default/admin/versions/updated.html.tmpl

>-[% IF updated_name %]
>+[% IF updated %]

The INTERFACE must be updated. It still says "updated_name".

Comment 10

[André Batosti]

Attachment: batosti_v3_fix

Comment 11

[Frédéric Buclin]

Comment on attachment 229536 [details] [diff] [review]
batosti_v3_fix

>Index: Bugzilla/Version.pm

>+sub update {

>+    # Remove unprintable characters
>+    $name = clean_text($name);
>+
>+    $name || ThrowUserError('version_not_specified');

No and no! Could you please keep the same check order as in editversions.cgi?! If $name is undefined, clean_text() will generate errors. That precisely why we first make sure $name is defined before cleaning its content.


>+sub create_version {

>+    # Remove unprintable characters
>+    $name = clean_text($name);
>+
>+    # Cleanups and validity checks
>+    $name || ThrowUserError('version_blank_name');

Same comment here!


There are also some nits in POD. I will fix them and attach an updated patch myself which will be the final one. r=LpSolit

Comment 12

[Frédéric Buclin]

Attachment: final patch, v4

OK, my previous comments have been fixed. I also renamed create_version() as create() for consistency with update() and remove_from_db().

Comment 13

[Frédéric Buclin]

Checking in editversions.cgi;
/cvsroot/mozilla/webtools/bugzilla/editversions.cgi,v  <--  editversions.cgi
new revision: 1.51; previous revision: 1.50
done
Checking in Bugzilla/Version.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Version.pm,v  <--  Version.pm
new revision: 1.10; previous revision: 1.9
done
Checking in template/en/default/admin/versions/updated.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/versions/updated.html.tmpl,v  <--  updated.html.tmpl
new revision: 1.3; previous revision: 1.2
done