Closed
Bug 313126
Opened 19 years ago
Closed 16 years ago
Step 2 (RW): implement validations and database persistence functions for Classification.pm and clean-up editclassifications.cgi.
Categories
(Bugzilla :: Administration, task)
Bugzilla
Administration
Tracking
()
RESOLVED
FIXED
Bugzilla 3.4
People
(Reporter: batosti, Assigned: LpSolit)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 5 obsolete files)
9.84 KB,
patch
|
wicked
:
review+
|
Details | Diff | Splinter Review |
1.03 KB,
patch
|
Details | Diff | Splinter Review |
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
Updated•19 years ago
|
Assignee: administration → batosti
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 1•19 years ago
|
||
*** Bug 313129 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 2•19 years ago
|
||
Attachment #200981 -
Flags: review?(LpSolit)
Assignee | ||
Updated•19 years ago
|
Severity: normal → enhancement
Target Milestone: --- → Bugzilla 2.24
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.
Attachment #200981 -
Flags: review?(LpSolit) → review-
Reporter | ||
Comment 4•19 years ago
|
||
Attachment #200981 -
Attachment is obsolete: true
Attachment #216045 -
Flags: review?(LpSolit)
Reporter | ||
Comment 5•19 years ago
|
||
Attachment #216045 -
Attachment is obsolete: true
Attachment #217905 -
Flags: review?(LpSolit)
Attachment #216045 -
Flags: review?(LpSolit)
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.
Attachment #217905 -
Flags: review?(LpSolit) → review-
Reporter | ||
Comment 7•19 years ago
|
||
Attachment #217905 -
Attachment is obsolete: true
Attachment #228667 -
Flags: review?(LpSolit)
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.
Attachment #228667 -
Flags: review?(LpSolit) → review-
Reporter | ||
Comment 9•18 years ago
|
||
fixed bitrott
Attachment #228667 -
Attachment is obsolete: true
Attachment #229479 -
Flags: review?(LpSolit)
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.
Attachment #229479 -
Flags: review?(LpSolit) → review-
Assignee | ||
Comment 11•18 years ago
|
||
We are in "soft freeze" mode to prepare 3.0 RC1.
Target Milestone: Bugzilla 3.0 → Bugzilla 3.2
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.
Target Milestone: Bugzilla 3.2 → Bugzilla 4.0
Assignee | ||
Updated•16 years ago
|
Assignee: batosti → LpSolit
Assignee | ||
Updated•16 years ago
|
Status: NEW → ASSIGNED
Target Milestone: Bugzilla 4.0 → Bugzilla 3.4
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.
Attachment #229479 -
Attachment is obsolete: true
Attachment #354553 -
Flags: review?(mkanat)
Assignee | ||
Updated•16 years ago
|
Attachment #354553 -
Flags: review?(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.
Attachment #354553 -
Flags: review?(wicked)
Attachment #354553 -
Flags: review?(mkanat)
Attachment #354553 -
Flags: review+
Updated•16 years ago
|
Flags: approval?
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! :)
Flags: approval? → approval+
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
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
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
You need to log in
before you can comment on or make changes to this bug.
Description
•