Closed
Bug 493090
Opened 16 years ago
Closed 16 years ago
Product disallownew should be converted to isactive
Categories
(Bugzilla :: Bugzilla-General, enhancement)
Tracking
()
RESOLVED
FIXED
Bugzilla 3.6
People
(Reporter: gregaryh, Assigned: gregaryh)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
mkanat
:
review+
|
Details | Diff | Splinter Review |
In keeping with convention, all other fields in bugzilla contain is_active and product should be no different.
Comment 1•16 years ago
|
||
is_active means that you can use this product or not, IMO, while disallownew means you cannot *enter* new bugs in this product, but you can still edit bugs being in this product. IMO, is_active less clear than disallownew.
Severity: normal → enhancement
OS: Linux → All
Hardware: x86 → All
Version: unspecified → 3.5
Assignee | ||
Comment 2•16 years ago
|
||
(In reply to comment #1) > is_active means that you can use this product or not, IMO, while disallownew > means you cannot *enter* new bugs in this product, but you can still edit bugs > being in this product. IMO, is_active less clear than disallownew. This seems to be in direct opposition to previous statements as per bug 77193 comment 36 - comment 43 I don't think it is wise to have both a disallownew and is_active on product fields. I need consensus on which to use to move forward. As pointed out, they have essentially opposite meanings. The question is, should product fields (version, milestone, component) behave the same as product in this regard? Meaning, it is still possible to set then on existing bugs even if "disallownew" is set. Or should it once disabled be disabled in that no bugs (new or existing) should be able to select it?
Comment 3•16 years ago
|
||
All isactive fields work the same. If they are already set on the bug, you can keep them set at that value. You cannot set bugs to that value if they do not already have that value. This is the same as how disallownew works, as far as I know.
Summary: Product disallownew should be converted to is_active → Product disallownew should be converted to isactive
Assignee | ||
Comment 4•16 years ago
|
||
This patch does not fix bug 493826 as promised as that will actually be part of the fix to bug 456743 which this blocks.
Assignee: general → ghendricks
Attachment #378630 -
Flags: review?(mkanat)
Comment 5•16 years ago
|
||
Comment on attachment 378630 [details] [diff] [review] V1 >Index: template/en/default/admin/products/confirm-delete.html.tmpl >- [% IF product.disallownew %] >+ [% IF NOT product.isactive %] > closed > [% ELSE %] > open Generally, booleans should be positive. (I'm pretty sure the Developers Guide mentions this.) So just reverse the contents of the IF and ELSE instead of making it IF NOT. >Index: template/en/default/admin/products/updated.html.tmpl >+ [% IF NOT product.is_active %] > closed to > [% ELSE %] > open for Same there. >+ <th align="right">Open for [% terms.bug %] entry:</th> >+ <td><input type="checkbox" name="isactive" value="1" >+ [% IF product.is_active == "1" %] Get rid of the "1" while we're here. Also, we usually do this as [% ' checked="checked"' IF product.is_active %] now. >Index: Bugzilla/Install/DB.pm >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v >retrieving revision 1.63 >diff -u -r1.63 DB.pm >--- Bugzilla/Install/DB.pm 6 Apr 2009 20:57:25 -0000 1.63 >+++ Bugzilla/Install/DB.pm 20 May 2009 16:19:34 -0000 >@@ -459,8 +459,6 @@ > # The products table lacked sensible defaults. > $dbh->bz_alter_column('products', 'milestoneurl', > {TYPE => 'TINYTEXT', NOTNULL => 1, DEFAULT => "''"}); >- $dbh->bz_alter_column('products', 'disallownew', >- {TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 0}); Would it make sense to wrap that in a bz_column_info instead? >@@ -567,6 +565,11 @@ > # 2009-01-16 oreomike@gmail.com - Bug 302420 > $dbh->bz_add_column('whine_events', 'mailifnobugs', > { TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'FALSE'}); >+ >+ # 2009-05-14 ghendricks@novell.com - Bug 493090 >+ $dbh->bz_add_column('products', 'isactive', >+ { TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'TRUE'}); >+ _convert_disallownew_to_isactive(); Everything should be in _convert_disallownew_to_isactive, and no comment is really needed. >@@ -591,8 +594,6 @@ > $dbh->bz_add_column('bugs', 'qa_contact', {TYPE => 'INT3'}); > $dbh->bz_add_column('bugs', 'status_whiteboard', > {TYPE => 'MEDIUMTEXT', NOTNULL => 1, DEFAULT => "''"}); >- $dbh->bz_add_column('products', 'disallownew', >- {TYPE => 'BOOLEAN', NOTNULL => 1}, 0); Wrap it in bz_column_info instead of deleting the line. >Index: contrib/gnatsparse/gnatsparse.py >- print >>outfile, " product, description, milestoneurl, disallownew," >+ print >>outfile, " product, description, milestoneurl, isactive," > print >>outfile, " defaultmilestone, votestoconfirm) values (" > print >>outfile, " '%s', '%s', '%s', 0, '%s', 1);" % (product, Not that this really matters, but shouldn't there be some sense reversal there (from 0 to 1)? >Index: contrib/gnats2bz.pl >- " product, description, milestoneurl, disallownew\n"; >+ " product, description, milestoneurl, isactive\n"; And there. >Index: Bugzilla/DB/Schema/Mysql.pm >@@ -66,7 +66,7 @@ > canedit => 1}, > group_group_map => {isbless => 1}, > user_group_map => {isbless => 1, isderived => 1}, >- products => {disallownew => 1}, >+ products => {isactive => 1}, No, that stays as disallownew. This hash is only historical. See the comment above it.
Attachment #378630 -
Flags: review?(mkanat) → review-
Assignee | ||
Comment 6•16 years ago
|
||
Also forgot to make new products default to TRUE.
Attachment #378630 -
Attachment is obsolete: true
Attachment #378699 -
Flags: review?(mkanat)
Comment 7•16 years ago
|
||
Comment on attachment 378699 [details] [diff] [review] V2 This is some old version of the patch.
Attachment #378699 -
Flags: review?(mkanat) → review-
Assignee | ||
Comment 8•16 years ago
|
||
sheesh... need to get my glasses checked
Attachment #378699 -
Attachment is obsolete: true
Attachment #378708 -
Flags: review?(mkanat)
Comment 9•16 years ago
|
||
Comment on attachment 378708 [details] [diff] [review] V2 (For real this time) >Index: template/en/default/admin/products/confirm-delete.html.tmpl >+ [% IF NOT product.isactive %] Ummm.... >+ <td><input type="checkbox" name="isactive" value="1" >+ [% 'checked="checked"' IF product.is_active %]> Nit: Put a space after that first ' or it will render funny. >Index: template/en/default/admin/products/list.html.tmpl > }, > { >- name => "disallow_new" >+ name => "isactive" Shouldn't that be is_active? >+ if (!$dbh->bz_column_info('products', 'isactive')){ >+ $dbh->bz_add_column('products', 'disallownew', > {TYPE => 'BOOLEAN', NOTNULL => 1}, 0); >+ } Why not just if disallownew exists? >Index: editproducts.cgi >@@ -285,7 +285,7 @@ > $product->set_description(scalar $cgi->param('description')); > $product->set_default_milestone(scalar $cgi->param('defaultmilestone')); > $product->set_milestone_url(scalar $cgi->param('milestoneurl')); >- $product->set_disallow_new(scalar $cgi->param('disallownew')); >+ $product->set_is_active(scalar $cgi->param('isactive')); You know, I think we should call the UI parameter is_active, for consistency.
Attachment #378708 -
Flags: review?(mkanat) → review-
Assignee | ||
Comment 10•16 years ago
|
||
Attachment #378708 -
Attachment is obsolete: true
Attachment #378713 -
Flags: review?(mkanat)
Comment 11•16 years ago
|
||
Comment on attachment 378713 [details] [diff] [review] V3 >Index: template/en/default/admin/products/confirm-delete.html.tmpl >+ [% IF product.isactive %] is_active >Index: Bugzilla/Install/DB.pm >+ if ($dbh->bz_column_info('products', 'disallownew')){ >+ $dbh->bz_alter_column('products', 'disallownew', > {TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 0}); >+ } Fix the indentation of the arguments so that they are aligned similarly (as long as you don't go past 80 characters). This is true in other places in this file, also. You can fix all that on checkin.
Attachment #378713 -
Flags: review?(mkanat) → review+
Updated•16 years ago
|
Flags: approval+
Assignee | ||
Comment 12•16 years ago
|
||
cvs ci -m " Bug 493090 - Product disallownew should be converted to isactive patch by ghendricks r=mkanat a=mkanat" -l "/bmo-tip-493090/template/en/default/admin/products/confirm-delete.html.tmpl" "/bmo-tip-493090/Bugzilla/Product.pm" "/bmo-tip-493090/Bugzilla/Install/DB.pm" "/bmo-tip-493090/Bugzilla/DB/Schema.pm" "/bmo-tip-493090/contrib/gnats2bz.pl" "/bmo-tip-493090/Bugzilla/DB/Schema/Mysql.pm" "/bmo-tip-493090/template/en/default/admin/products/create.html.tmpl" "/bmo-tip-493090/contrib/gnatsparse/gnatsparse.py" "/bmo-tip-493090/editproducts.cgi" "/bmo-tip-493090/template/en/default/admin/products/updated.html.tmpl" "/bmo-tip-493090/template/en/default/admin/products/edit-common.html.tmpl" "/bmo-tip-493090/Bugzilla/User.pm" "/bmo-tip-493090/template/en/default/admin/products/list.html.tmpl" Checking in contrib/gnats2bz.pl; /cvsroot/mozilla/webtools/bugzilla/contrib/gnats2bz.pl,v <-- gnats2bz.pl new revision: 1.9; previous revision: 1.8 done Checking in Bugzilla/DB/Schema.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v <-- Schema.pm new revision: 1.117; previous revision: 1.116 done Checking in Bugzilla/Product.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Product.pm,v <-- Product.pm new revision: 1.37; previous revision: 1.36 done Checking in Bugzilla/User.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v <-- User.pm new revision: 1.186; previous revision: 1.185 done Checking in template/en/default/admin/products/edit-common.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/edit-common.html.tmpl,v <-- edit-common.html.tmpl new revision: 1.10; previous revision: 1.9 done Checking in template/en/default/admin/products/create.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/create.html.tmpl,v <-- create.html.tmpl new revision: 1.8; previous revision: 1.7 done Checking in template/en/default/admin/products/list.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/list.html.tmpl,v <-- list.html.tmpl new revision: 1.7; previous revision: 1.6 done Checking in template/en/default/admin/products/updated.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/updated.html.tmpl,v <-- updated.html.tmpl new revision: 1.9; previous revision: 1.8 done Checking in template/en/default/admin/products/confirm-delete.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/confirm-delete.html.tmpl,v <-- confirm-delete.html.tmpl new revision: 1.12; previous revision: 1.11 done Checking in Bugzilla/Install/DB.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v <-- DB.pm new revision: 1.64; previous revision: 1.63 done Checking in editproducts.cgi; /cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v <-- editproducts.cgi new revision: 1.148; previous revision: 1.147 done Checking in contrib/gnatsparse/gnatsparse.py; /cvsroot/mozilla/webtools/bugzilla/contrib/gnatsparse/gnatsparse.py,v <-- gnatsparse.py new revision: 1.5; previous revision: 1.4 done ok (took 0:30.503)
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•