493090 - Product disallownew should be converted to isactive

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Greg Hendricks]

In keeping with convention, all other fields in bugzilla contain is_active and product should be no different.

Comment 1

[Frédéric Buclin]

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.

Comment 2

[Greg Hendricks]

(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

[Max Kanat-Alexander]

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.

Comment 4

[Greg Hendricks]

Attachment: V1

This patch does not fix bug 493826 as promised as that will actually be part of the fix to bug 456743 which this blocks.

Comment 5

[Max Kanat-Alexander]

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 [e-mail] - Bug 302420
>     $dbh->bz_add_column('whine_events', 'mailifnobugs',
>         { TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'FALSE'});
>+        
>+    # 2009-05-14 [e-mail] - 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.

Comment 6

[Greg Hendricks]

Attachment: V2

Also forgot to make new products default to TRUE.

Comment 7

[Max Kanat-Alexander]

Comment on attachment 378699 [details] [diff] [review]
V2

This is some old version of the patch.

Comment 8

[Greg Hendricks]

Attachment: V2 (For real this time)

sheesh... need to get my glasses checked

Comment 9

[Max Kanat-Alexander]

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.

Comment 10

[Greg Hendricks]

Attachment: V3

Comment 11

[Max Kanat-Alexander]

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.

Comment 12

[Greg Hendricks]

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)