Product disallownew should be converted to isactive

RESOLVED FIXED in Bugzilla 3.6

Status

()

Bugzilla
Bugzilla-General
--
enhancement
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Greg Hendricks, Assigned: Greg Hendricks)

Tracking

Bugzilla 3.6
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 3 obsolete attachments)

V3
14.19 KB, patch
Max Kanat-Alexander
: review+
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
In keeping with convention, all other fields in bugzilla contain is_active and product should be no different.
(Assignee)

Updated

9 years ago
Depends on: 493091

Comment 1

9 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

9 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

9 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

9 years ago
Created attachment 378630 [details] [diff] [review]
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.
Assignee: general → ghendricks
Attachment #378630 - Flags: review?(mkanat)

Comment 5

9 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

9 years ago
Created attachment 378699 [details] [diff] [review]
V2

Also forgot to make new products default to TRUE.
Attachment #378630 - Attachment is obsolete: true
Attachment #378699 - Flags: review?(mkanat)

Comment 7

9 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

9 years ago
Created attachment 378708 [details] [diff] [review]
V2 (For real this time)

sheesh... need to get my glasses checked
Attachment #378699 - Attachment is obsolete: true
Attachment #378708 - Flags: review?(mkanat)

Comment 9

9 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

9 years ago
Created attachment 378713 [details] [diff] [review]
V3
Attachment #378708 - Attachment is obsolete: true
Attachment #378713 - Flags: review?(mkanat)

Comment 11

9 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

9 years ago
Flags: approval+
(Assignee)

Comment 12

9 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
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Blocks: 494628
You need to log in before you can comment on or make changes to this bug.