Closed
Bug 313125
Opened 19 years ago
Closed 18 years ago
implement validations and database persistence functions for Versions.pm and clean-up editversions.cgi
Categories
(Bugzilla :: Administration, task)
Bugzilla
Administration
Tracking
()
RESOLVED
FIXED
Bugzilla 3.0
People
(Reporter: batosti, Assigned: batosti)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
LpSolit
:
review+
|
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
Updated•19 years ago
|
Severity: normal → enhancement
Target Milestone: --- → Bugzilla 2.24
Comment 2•19 years ago
|
||
Comment on attachment 200916 [details] [diff] [review] batosti_v1 This patch no longer applies due to the checkin of bug 311258.
Attachment #200916 -
Flags: review?(LpSolit) → review-
Assignee | ||
Comment 3•19 years ago
|
||
Attachment #200916 -
Attachment is obsolete: true
Attachment #213887 -
Flags: review?(LpSolit)
Comment 4•19 years ago
|
||
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.
Attachment #213887 -
Flags: review?(LpSolit) → review-
Assignee | ||
Comment 6•19 years ago
|
||
Attachment #213887 -
Attachment is obsolete: true
Attachment #217848 -
Flags: review?(LpSolit)
Comment 7•19 years ago
|
||
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. ;)
Attachment #217848 -
Flags: review?(LpSolit) → review-
Assignee | ||
Comment 8•19 years ago
|
||
Attachment #217848 -
Attachment is obsolete: true
Attachment #228085 -
Flags: review?(LpSolit)
Comment 9•18 years ago
|
||
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".
Attachment #228085 -
Flags: review?(LpSolit) → review-
Assignee | ||
Comment 10•18 years ago
|
||
Attachment #228085 -
Attachment is obsolete: true
Attachment #229536 -
Flags: review?(LpSolit)
Comment 11•18 years ago
|
||
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
Attachment #229536 -
Flags: review?(LpSolit) → review+
Comment 12•18 years ago
|
||
OK, my previous comments have been fixed. I also renamed create_version() as create() for consistency with update() and remove_from_db().
Attachment #229536 -
Attachment is obsolete: true
Attachment #229743 -
Flags: review+
Updated•18 years ago
|
Flags: approval?
Updated•18 years ago
|
Flags: approval? → approval+
Comment 13•18 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•