Product disallownew should be converted to isactive

RESOLVED FIXED in Bugzilla 3.6

Status

()

enhancement
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: gregaryh, Assigned: gregaryh)

Tracking

Bugzilla 3.6
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 3 obsolete attachments)

In keeping with convention, all other fields in bugzilla contain is_active and product should be no different.
Depends on: 493091
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
(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?
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
Posted patch V1 (obsolete) — Splinter Review
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 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-
Posted patch V2 (obsolete) — Splinter Review
Also forgot to make new products default to TRUE.
Attachment #378630 - Attachment is obsolete: true
Attachment #378699 - Flags: review?(mkanat)
Comment on attachment 378699 [details] [diff] [review]
V2

This is some old version of the patch.
Attachment #378699 - Flags: review?(mkanat) → review-
Posted patch V2 (For real this time) (obsolete) — Splinter Review
sheesh... need to get my glasses checked
Attachment #378699 - Attachment is obsolete: true
Attachment #378708 - Flags: review?(mkanat)
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-
Posted patch V3Splinter Review
Attachment #378708 - Attachment is obsolete: true
Attachment #378713 - Flags: review?(mkanat)
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+
Flags: approval+
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: 10 years ago
Resolution: --- → FIXED
Blocks: 494628
You need to log in before you can comment on or make changes to this bug.