André Batosti Reporter
	Description • 19 years ago

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

	Hermann Schwab
	Comment 1 • 19 years ago

*** Bug 313129 has been marked as a duplicate of this bug. ***

	Frédéric Buclin Assignee
	Comment 3 • 19 years ago

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

>Index: editclassifications.cgi

>+    my $classification =
>+        Bugzilla::Classification::create_classification($class_name,
>+                                                        $description);

Nit: do we want to call it create_classification() or create() only? I would tend to create() only.


>+    ($vars->{'updated_classification'},
>+     $vars->{'updated_description'}) = 
>+        $classification->update($class_name, $description);

Question: Do we really want update() to return these information?


>             foreach my $prod ($cgi->param("prodlist")) {
>+                push(@add, $prod);
>             }

You should write the following, which is more efficient:
my @prodlist = $cgi->param("prodlist");
push(@add, @prodlist);


>             foreach my $prod ($cgi->param("myprodlist")) {
>+                push(@remove, $prod);
>             }

Same comment here.



>Index: Bugzilla/Classification.pm

>+use Bugzilla::Config qw($datadir);

I don't see where $datadir is used. Probably a mistake.


>+    # lock the tables before we start to change everything:
>+    $dbh->bz_lock_tables('classifications WRITE', 'products WRITE');

Nit: do we really want to lock tables inside Classification.pm? This could be a problem if this method is called while some tables are already locked. Probably should we lock them outside Classification.pm. You should ask on IRC.


>+    $dbh->do("DELETE FROM classifications WHERE id = ?", undef,
>+             $self->id);

Nit: You could write this on one line.


>+    $dbh->do("UPDATE products SET classification_id = 1
>+              WHERE classification_id = ?", undef, $self->id);

I guess some DBs could complain that we delete a classification before removing all references to it. So I would first update classification_id in products and then remove the classification itself.


>+sub update {
>+    my $self = shift;
>+    my ($class_name, $desc) = (@_);

@_ is already an array. You don't have to put it in parens here. This remark applies in several other places too.


>+    if ($class_name && $class_name ne $self->name) {

Err... wait! You update the classification name without making sure a name has been given. This check *must be* in this method, not in the .cgi which calls it.


>+    if ($desc && $desc ne $self->description) {

You introduce a regression here. I cannot delete a description anymore. '' is a valid description. At the beginning of this method, you should write:
$desc = '' unless defined($desc);


>+sub create_classification {

>+    trick_taint($desc);

If $desc is undefined, trick_taint() will die(). So write
$desc = '' unless defined($desc);
here too.


I didn't test your patch yet, so this is only a review by inspection so far.

	Frédéric Buclin Assignee
	Comment 6 • 19 years ago

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

>Index: editclassifications.cgi

>     # Make versioncache flush
>     unlink "$datadir/versioncache";

versioncache no longer exists.



>Index: Bugzilla/Classification.pm

>+use Bugzilla::Config;

You don't need to use Bugzilla::Config (I see no use of Param() anywhere).


>+sub remove_from_db {

>+    # lock the tables before we start to change everything:
>+    $dbh->bz_lock_tables('classifications WRITE', 'products WRITE');

Do not lock tables inside Classification.pm. Let the caller do it (you have no idea if some other tables are locked already or not).


This is by far not a complete review. But these points have to be addressed already. I will review more carefully your updated patch.

	Frédéric Buclin Assignee
	Comment 8 • 18 years ago

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

Bitrotten due to the checkin of bug 277377 yesterday which introduces a sortkey for classifications.

	André Batosti Reporter
	Comment 9 • 18 years ago

fixed bitrott

	Frédéric Buclin Assignee
	Comment 10 • 18 years ago

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

Please use methods defined in Object.pm. This will make your code easier to write.

	Frédéric Buclin Assignee
	Comment 11 • 18 years ago

We are in "soft freeze" mode to prepare 3.0 RC1.

	Max Kanat-Alexander
	Comment 12 • 17 years ago

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set the "blocking3.2" flag to "?", and either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

	Frédéric Buclin Assignee
	Comment 13 • 16 years ago

This patch is based on bug 339381. Note that I left the "reclassify" part of editclassifications.cgi alone as I think this section should be converted to $product->set_classification($class_id) rather than $classification->reclassify($prod_name). So we will fix that in a separate bug.

	Teemu Mannermaa (:wicked)
	Comment 14 • 16 years ago

Comment on attachment 354553 [details] [diff] [review]
patch, v4

>Index: template/en/default/global/messages.html.tmpl
>===================================================================
>+        [% IF changes.description.defined %]
>+          <li>Description updated to '[% classification.description FILTER html %]'</li>

Nit: Would be nice to say "Description removed" instead of "Description updated to ''" if the description is blanked.

>+        [% IF changes.sortkey.defined %]
>+          <li>Sortkey updated to '[% classification.sortkey FILTER html %]'</li>

Nit: This shows input value (for example, 999999999999) instead of the real value (32767) if the sortkey overflows.

	Frédéric Buclin Assignee
	Comment 15 • 16 years ago

(In reply to comment #14)
> Nit: Would be nice to say "Description removed" instead of "Description updated
> to ''" if the description is blanked.

Good idea. I will fix that on checkin.


> Nit: This shows input value (for example, 999999999999) instead of the real
> value (32767) if the sortkey overflows.

This means we should check that the sortkey is in the expected range. I also noticed that the current code doesn't check the max length of the classification name. As the goal of this bug was to use Classification.pm in editclassifications.cgi only, I didn't want to add missing checks as part of this bug. I will file a separate bug to add missing checks (that's not a major issue, but still good to fix).

Thanks for the review! :)

	Frédéric Buclin Assignee
	Comment 16 • 16 years ago

Checking in editclassifications.cgi;
/cvsroot/mozilla/webtools/bugzilla/editclassifications.cgi,v  <--  editclassifications.cgi
new revision: 1.33; previous revision: 1.32
done
Checking in Bugzilla/Classification.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Classification.pm,v  <--  Classification.pm
new revision: 1.13; previous revision: 1.12
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.177; previous revision: 1.176
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v  <--  messages.html.tmpl
new revision: 1.82; previous revision: 1.81
done

	Frédéric Buclin Assignee
	Comment 17 • 16 years ago

See comments 14/15 about "Description removed".

Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v  <--  messages.html.tmpl
new revision: 1.83; previous revision: 1.82
done