224208 - classification - Add a higher level of categorization (.ie departments, locations, etc.)

Status: RESOLVED FIXED

(Bugzilla :: Administration, task, P2)

Description

[Albert Ting]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.5) Gecko/20031007 Firebird/0.7
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.5) Gecko/20031007 Firebird/0.7

It would be nice if one could group products into categories
(departments, groups, organization, etc.).  In our situation, this is
a benefit as we track many products/workflows across multiple
group/locations.  There is also overlap among the groups (.ie bugs 
get moved around).

I had considered Bug #173133 which offers a third "subcomponent"
level, but it doesn't quite work as the version id is tied to the
top level product level.

I had also considered multiple bugzilla servers but it would make
moving bugs cumbersome.  Maintenance would be an issue as well.

In any case, I've implementated some of the feature based on the
2.17.4 build.  What it currently does:

 - new "categories" table, along with a products.category_id field
 - editcategories.cgi to manage categories.  Product assignment to the 
   category is optional (defaults to an "unassigned" category)
 - checksetup.pl updated
 - the entre category level is optional, based on the "usecategories" parameter

What's missing:
 - updated query, buglist, and reports
 - category based group access, not sure if it's worth doing

I don't understand enough of the code to do the missing parts yet.  And
the javascript code looks like a fair bit of work.

However, I'm happy with what we have right now and have been using it.
Hopefully I'm on the right track.

Will post patches after it's had some use.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Albert Ting]

Attachment: initial 2.17.4 patch

Here's the initial patch, along with two additional files as separate
attachments. 

The query/reports functions have been updated as well to support categories. 
The key issue with this feature is that it still assumes all product names are
unique, rather than unique(product.name,product.category). Making it unique for
the specific category would seem difficult considering bugzilla indexes
everything based on unique product names.

But I hope this is still useful enough to be part of the cvs tree, even with
product name limitation.

Thanks.

Comment 2

[Albert Ting]

Attachment: editcategories.cgi

Comment 3

[Albert Ting]

Attachment: global/choose-category.html.tmpl

Comment 4

[Albert Ting]

Attachment: 2.17.6 patch

I've updated the patch based on the recent 2.17.6.  Since "category"
is now being used for the new graphing features, I've changed the top
level to "classification".

I've enhanced the querying/reports features to be more in line with
what bugzilla already offers.  Also added a "editclassifications"
group.

Product names still has to be unique across all classification.  I
wasn't comfortable yet making it unique per classification as I'd have
to update get_product_id and the graphing/reporting features.

Comment 5

[Albert Ting]

Attachment: editclassifications.cgi

Comment 6

[Albert Ting]

Attachment: global/choose-classification.html.tmpl

Comment 7

[Joel Peshkin]

I think either bug 220794 is a dup of this or vice-versa
Both seem active

Comment 8

[Albert Ting]

Thanks for the pointer to #220794, I didn't see it when I did a query.  

It doesn't matter to me how it's implemented, but would like to see it part of
the standard bugzilla release.  Key features I'm interested in, combined with
the other bug is:

   - hierarchally group the products
   - ability to query bugs based on Classifications (aka ProductGroups)
   - buglist should support classifications as one of the display columns
   - for folks who only have one product, this extra level should be optional.
   - ability to display products hierarchically or flat, where "flat" still 
     groups the product based on classifications/productgroups.

This bug does everything above except the last item (which is easy to implement).
And I think the database schema in this bug does what bug #220794 suggests.

But again, it doesn't matter to me how it's implemented.  I just like to see 
this extra level of catagorization be part of the standard bugzilla release. 

Comment 9

[Joel Peshkin]

I'm confused.

Is it possible for 2 classifications to each contain products with identical names?

Comment 10

[Gervase Markham [:gerv]]

We can only implement one - either a higher categorisation or a lower. But when
we last discussed this, I seem to remember a consensus that both arbitrary
numbers of levels, and arbitrary hierarchies were bad for UI and admin
complexity reasons.

Gerv

Comment 11

[Dave Miller [:justdave]]

I don't see a problem with implementing both a higher and a lower, if the higher
one is implemented only as a way to group the products....  an automated way to
achieve what the custom templates on mozilla.org are doing, for example.  This
seems to be what bug 220794 is aiming for.

It sounds like the main motivation behind this bug is to get the milestones and
versions to belong to the second level instead of the top level.  I think there
are other bugs towards doing that specifically (having those items be
per-component instead of per-product).  I think getting a lot of the current
"per-product" settings to be "per-component" might be a good thing, but we'd
need to come up with an easy way to do inheritance first, otherwise the UI will
be a nightmare for people who like having it per-product.

In answer to Joel's question in comment 9, yes, the patch that's here still
requires product names to be globally unique (can't have products with the same
name in two different categories).

Comment 12

[Albert Ting]

I didn't see bug 220794 before I started implementing this.  But my goals are
similar, to create a higher level lightweight MenuGroup feature.  However, once
started down this path, I realized this level could be a bit more heavyweight
(.ie classification based searches and reports).   At my company,
"classifications" is equated with "departments", so we can now review
bugs/issues on a per department level. 

Note that this extra level is turned off by default, hence the UI for defining
classifications is different from defining products/components.

I've added some more bugs/patches since last year.  After I've upgraded to
2.17.7, will post the newer version.

Comment 13

[Albert Ting]

Attachment: 2.17.7 patch

Updated patch for 2.17.7.  Folded in the new files into the patch as well.

This version cleans up some reporting problems.  This also sorts the products
based on the classification, such as in the "find specific bug" form.

Remaining issue is adding classification support to the new charting routines,
via perhaps a new classcategory field.

Comment 14

[Albert Ting]

I've concluded it would be not be fruitful to update the graph package at this
time.  Partially because I don't quite understand it :)  But I also don't want
to create such a large patch until there is a willingness to have this patch
checked into the main code stream.  That is, I'm concerned this could drag
out, similar to the custom field support in bug #91037.

So, is there anything else I can do to help get this checked in?  Am I going
in the right direction?  This feature has worked out well for my company.

Comment 15

[Joel Peshkin]

Comment on attachment 144097 [details] [diff] [review]
2.17.7 patch

Just from a quick check...

I don't see any code here that keeps all users from seeing all classifications.
 It seems that a user should never see a classification that contains no
products the user can see.

Also, the patch does not seem to take normal bugzilla conventions into account,
such as 4-space indents, centralization of error messages in ThrowUserError()'s
templates, etc...

Comment 16

[Joel Peshkin]

This could actually be very handy on sites where there are a huge number of
products and users have access to many more products than they ordinarily care
about.  This could cut clutter a lot.

If Albert fixes the localization and coding convention issues and adds
GetSelectableClassifications() etc, this should land.  I have not tested this
yet, but we should make sure that users can include the classification list in
their default queries.

Gerv, what does this mean to charting??

Comment 17

[Albert Ting]

Regarding comment #15, I'll look to see if I can hide classifications if
there are no products the user can see.  Will also look into following the 
standard coding conventions.

Regarding comment #16, queries can include the classification list.
But I also need to post a small patch that supports classifications 
in the advanced queries.

Comment 18

[Joel Peshkin]

The way to address comment 15 is to extend the query in GetSelectableProducts()
to  make a version for classifications.  That way you don't have to worry about
first getting classifications and then disqualifying them.  Instead, the code
doesn't even fetch any information about unauthorized classifications from the
database.

Comment 19

[Gervase Markham [:gerv]]

Re: charting. It depends what this patch does. If it's purely an optional
cosmetic extra layer of classification - "product grouping", then I doubt it'll
affect it much. If it goes deeper than that, then maybe it will (and I'd have
reservations about it for other reasons anyway.)

Gerv

Comment 20

[Albert Ting]

re: charting, what's missing is the ability to chart bugs based on a classification.
I found this useful for the text/graphical reports.  I don't konw if it's 
useful to have this for charts, other than being complete. 

Comment 21

[Joel Peshkin]

Actually, it's simpler than that.  As we make it possible to query based on the
classification just like any other criteria, the charting will automaticaly pick
up on it.

Comment 22

[Albert Ting]

Joe just clarified something for me.  I was focused on adding a
"Classification-Category" field, as an extension to "Category" and
"Sub-Category".  That's incorrect, for Category/Sub-Category don't
correlate to Product/Components.

Since queries for charting is already extended to support
classifications, we're all set.  But perhaps what's missing is
auto-filling the Category with the list of classifications?

Comment 23

[Gervase Markham [:gerv]]

Category/Sub-Category does map to Product/Component, at least in the
auto-generated data sets.

This is what I meant when I asked if the new code on top was merely cosmetic. If
that's the case, then charting can ignore it. Products are still categories, and
components are still sub-categories. It's just that the charting doesn't expose
the classification of Products into whatever groups you've got them in.

If, however, your changes go deeper than that, then it's very hard to see how
this could work easily.

Gerv

Comment 24

[Joel Peshkin]

Gerv,

   A bunch of products together can be selected as a Classification in queries
and UI elements can use that to help organize and de-clutter.  The only manner
in which this is not totally cosmetic is that we would be able to search on the
group of products by specifying the Classification as a query criteria.

   I don't see any reason that this would impact the series code.

Comment 25

[Gervase Markham [:gerv]]

Cool. So the right thing for this patch to do to the series code is "nothing". :-)

Gerv

Comment 26

[Albert Ting]

Attachment: updated 2.17.7 patch

This updated version has the following changes:

   - updated GetSelectableProducts to take into account classifications.  
     Query form now lists only those products the user can select.
     - Note that the enter_bug form still lists all products, which is 
       consistent with how things are done now.
   - added classification support to advanced the Advanced Query
     Boolean Chart.
   - cleaned up indentation, per bugzilla coding convention

I'm already using ThrowError where I can, other than perhaps
editclassifications.cgi.  But that file follows the same style as
editproducts.cgi.

Comment 27

[Joel Peshkin]

Comment on attachment 151963 [details] [diff] [review]
updated 2.17.7 patch

This is getting very close, but it needs to be brought up to the tip.

You still need to add a getselectableclassifications() function unless I missed
something.  If there are 10 classifications, but all of my selectable prodycts
are from one of them, I should only be offerred the single classification just
as if the other did not exist.

Similarly, on entry, if all of my enterable products are in the same
classification, enter_bug should not make me visit a page to choose among the
sole clasisification.

This is a really cool feature.

Comment 28

[Albert Ting]

Attachment: updated 2.18RC1 patch

Attached is an updated patch based on the 2.18RC1 release.  Below are
the changes:

- enter_bug and query now uses GetSelectableClassifications().
  Classifications with no accessible products are now skipped.
- If there is only one class, then enter_bug will just list the
  products
- I have a default "Unassigned" classification added into the mysql
  database.  For localization purposes, this word can be changed in
  checksetup.pl
- Added a "showallproducts" parameter, in addition to the current
  "useclassifications" parameter.
- products can only exist in one classification.  I prefer to keep 
  this rule as I hope some day we might support N-level
  classifications. :)

Comment 29

[Joel Peshkin]

Comment on attachment 153825 [details] [diff] [review]
updated 2.18RC1 patch

This is looking quite proimising, but a few notes....

1) The UI for assigning prodycts to classifications (the multi-select) wont
work out well when there are many many products.  It probably should be an
add/drop or something.

2) Check a few of the pages for validation errors.  It looks like you are not
using the right FILTER operations in the templates. Run runtests.sh before
finalizing the patch.  It will catch tabs (taboo) and filter violations.

3) This doesn't work on HEAD.  There are a small number of merge conflicts if
you apply it to 2.18rc1 and then do a cvs update -A 
Please do that and regenerate the patch.

4) I found it a bit hard to find my way through the new edit classification
entry point to edit my list of products. I think that administrators will
expect to be able to dive straight into the product list independently of how
they are classified and the clasify them.

I didn't yet delve into the depths of the code, but everything I tried seems to
work fairly well.

Comment 30

[Albert Ting]

Attachment: updated 2.19.tip patch

This patch is based on the cvs head.  But the selectProduct() calls in the 
report-graph/table is broken due to bug #254545.  

Other changes:
   - filter/TAB issues have been cleaned up
   - "Products" have been added back into useful-links
   - improved the classification UI, there are now three lists: 
       unclassified	 - list of products not classified
       classified	 - list of products already classified
       current products  - list of products in the current classification

     You can add/remove between Unclassified and Current.  And you can migrate
     from Classified to Current.

Comment 31

[Albert Ting]

Attachment: brief release note

Comment 32

[Joel Peshkin]

Comment on attachment 155385 [details] [diff] [review]
updated 2.19.tip patch

This looks very good.  The only major item from my inspection is
editclassifications.cgi.  All the existing administrative CGIs are in the
process of being templatized.  New cgis are required to be templatized.

I'll test this a bit this weekend.  If everything works as advertised, I think
this just needs editcomponents.cgi templatized and it'll be reviewable.

Comment 33

[Albert Ting]

> All the existing administrative CGIs are in the process of being templatized. 

Agreed.  But I chose to wait since I could tell the other admin cgi's have not 
been completed yet.  Let me know what you think.

Comment 34

[Joel Peshkin]

Each CGI gets templatized on its own.  There was an attempt to make a
one-template-fits-all template, but that never panned out.  Once you get over
the intitial shock, it is way easier.

Comment 35

[Joel Peshkin]

OK, ran some tests....

I'm not sure if the javascript that is supposed to change the search interface
to the selected classifications is broken because of the known issue on the
trunk or if there is more to it.  I expected that, when I select a
calssification, I'd see only the products that go with it (query.cgi javascript)
and this did not happen.

All the data security stuff seems to be working well.  

The editclassifications UI seems a bit funny because you edit "unassigned" just
as if it were any other classification except that products are never "in it"
and always show up as "Unclassified" rather than current.  That is a nit and I
would not place a high priority on changing it.

  Checksetup should  add the "admin" group to editclassicifations by default.
(Hint: AddGroup returns true if the group was freshly added, false if it did not
need to add a new group)

  Not meaning to rush you, but this is way cool.  I want it now. :-)

Comment 36

[Joel Peshkin]

Actually, a good way to deal with the "unclassified product" versus migrate
issue might be....

Always treat the products as migrate, but show each product as classification:name

So, if I have Testproduct unclassified and oak and pine both classified as
trees, I would see either

trees:oak
trees:pine
unclassified:Testproduct

or 

Testproduct
trees:oak
trees:pine

Comment 37

[Joel Peshkin]

whoops..  a few more things (you can fix while templatizing).   

Select classification page is not filtering the "&" in the urls.  This should
get fixed when you templatize as long as you use the right filter, but always
run your new pages past the w3c validation service.  (footer will fail if you
have multiple saved queries, you can ignore that)

There are other validation items that also need to be fixed.  The easiest way to
do this is to run the firefox developer toolbar and use the "Validate Local
html" option.

Comment 38

[Joel Peshkin]

Hmm... the javascript work as long as all products are classified.  If there is
one unclassified product, then the javascript fails.

Comment 39

[Joel Peshkin]

After all that, I cant make the javascript thing fail any more.

Comment 40

[Albert Ting]

Attachment: updated 2.19 patch

Updated the patch with the following changes:

   - editclassifications.cgi now templatized
   - UI for reclassifying products changed to just two lists and now
     include the classification name in the list
   - Cleaned up the filtering, no tab/filter errors from runtests.
     Validator from firefox came out clean as well.
   
I also took the liberty of fixing bug #254545 so that all the javascript
classification/product queries works.  It seemed the best option to me was
to move the javascript array code into form.html.tmpl and parameterize the
doOnSelectProduct function.  This way, one just needs to add 2-4 lines of
tmpl code in the three search*.tmpl files.  Seems to work for me.

Comment 41

[Joel Peshkin]

Comment on attachment 155757 [details] [diff] [review]
updated 2.19 patch


I'll start with nits, though I'll need to really dive into this later.	Also,
let's request a review from kiko since I am pretrty bad at javascript.

+	     [% FOREACH p = c.products %]
+	       <option value="[% p FILTER html %]"
+		 [% " selected" IF lsearch(default.product, p) != -1 %]>
+		 [% p FILTER html %]</option>
+	     [% END %]

nit: Align </option> with <option>



+	 [% IF Param('useclassification') %]
+	    [% IF bug.classification_id != "1" %]

Is "1" a special predetermined value for "Unassigned"???  How do we make sure
this is always true?
also nit:  This block has 3 spaces per indent-level instead of 2.


+if ($action eq 'add') {
+    trick_taint($action);
+    print Bugzilla->cgi->header();
+    $template->process("admin/classifications/$action.html.tmpl", $vars)
+      || ThrowTemplateError($template->error());
+    exit;
+}

Putting a forcibly detainted form variable in a place like this is a bad habit.
 While you do check to see if this is 'add' first, you may as well just put the
'add' right in the string.

runtests fails the tabs check due to some tabs in search/form

Testing it out, it works well.	I'll scrub the rest of it later.  If you fix
the items above first, go ahead and request reviews.

Comment 42

[Albert Ting]

Thanks for the pointers.

> +	     [% FOREACH p = c.products %]
> +	       <option value="[% p FILTER html %]"
> +		 [% " selected" IF lsearch(default.product, p) != -1 %]>
> +		 [% p FILTER html %]</option>
> +	     [% END %]
> 
> nit: Align </option> with <option>

I can fix this, but what I have now is similar to how many of the option
constructs are being defined in the CVS tip. Examples are form.html.tmpl,
create.html.tmpl, duplicate.html.tmpl, etc.  Shall I still fix this specific
one?

> +	 [% IF Param('useclassification') %]
> +	    [% IF bug.classification_id != "1" %]
> 
> Is "1" a special predetermined value for "Unassigned"???  How do we make
> sure this is always true? 

Yes, "1" is predetermined and defined in checksetup.pl.  One can change the
name, but you can't delete it.  I'll update checksetup.pl to make it
cleaner (current versions always recreates it).  I'll also update
editclassifications.cgi to prevent someone from bypassing the confirmation
and attempt to delete id "1".

> also nit: This block has 3 spaces per indent-level instead of 2.
Hey, we need an EMACS mode for these types of things.... :)

> +if ($action eq 'add') {
> +    trick_taint($action);
> +    print Bugzilla->cgi->header();
> +    $template->process("admin/classifications/$action.html.tmpl", $vars)
> +      || ThrowTemplateError($template->error());
> +    exit;
> +}
> 
> Putting a forcibly detainted form variable in a place like this is a bad
> habit. While you do check to see if this is 'add' first, you may as well
> just put the 'add' right in the string.

My intention was to trick_taint() only once at the top (which I just
fixed).  Then make all the template->process calls be generic.  In
fact, to cut down redundant code, I should probably just convert this
into a function and have all the action blocks call that function?

> runtests fails the tabs check due to some tabs in search/form

My bad.  I had checked it but then proceeded to fix bug 254545 without
running runtests.sh again.

Comment 43

[Albert Ting]

Attachment: updated 2.19 patch

Updated patch does this:
- templates: cleaned up indentation per Joel
- editclassifications.cgi:
  - aborts if someone attempts to bypass the confirmation form and
    delete the default classification
  - cleaned up how I load the editclassification templates
- long_list.cgi: updated to support classification
- checksetup.pl: changed the order of the classification fielddefs

Comment 44

[Joel Peshkin]

Comment on attachment 155873 [details] [diff] [review]
updated 2.19 patch

This was very close the last time I looked at it.  I'll be unavailable until
Monday.  Kiko - can you review this?

Comment 45

[Albert Ting]

Attachment: updated release note

Comment 46

[Joel Peshkin]

Don't use a trick-taint for that.  Detaint within the function using....

$action =~ /(\w+)/;
$action = $1;

That will make sure that the remaining $action is all alphanumeric.

Comment 47

[Joel Peshkin]

Comment on attachment 155873 [details] [diff] [review]
updated 2.19 patch

r=joel as long as the trick_taint is replaced.

Needs a 2xr.  This is too big a change for 1xr.

Comment 48

[:glob ✱]

Comment on attachment 155873 [details] [diff] [review]
updated 2.19 patch

>Index: editclassifications.cgi
>+    SendSQL("SELECT name
>+             FROM classifications
>+             WHERE name=" . SqlQuote($cl));
>+    return FetchOneColumn();

SendSQL, FetchOneColumn, MoreSQLData, etc are depreciated.
please use $dbh directly instead for all of editclassifications.cgi.

for changes to existing code it's ok to continue to use the old calls, but new
files should use $dbh.

also replace SqlQuote with SQL placeholders or $dbh->quote.


>+    my $description  = trim($::FORM{description}  || '');

$::FORM is also depreciated, use $cgi->param instead, for all instances in
editclassificatoins.cgi.


otherwise this patch looks really good :)

Comment 49

[Albert Ting]

Attachment: updated 2.19 patch

Ok, here's another spin!

- editclassifications.cgi
  - now uses the $dbh sql routines
  - cleaned up the action trick-taint
  - when reclassifying, place all the unclassified products at the
    top.  Sort the remaining by products, then classifications.  
- admin/classifiations/new.html.tmpl: 
  - make the "add products" link point to editproducts.cgi

Comment 50

[:glob ✱]

Comment on attachment 156460 [details] [diff] [review]
updated 2.19 patch


>--- buglist.cgi	6 Aug 2004 03:12:18 -0000	1.1.2.5
>+++ buglist.cgi	10 Aug 2004 02:02:17 -0000	1.8
>@@ -458,6 +458,7 @@
> DefineColumn("status_whiteboard" , "bugs.status_whiteboard"     , "Status Summary"   );
> DefineColumn("component"         , "map_components.name"        , "Component"        );
> DefineColumn("product"           , "map_products.name"          , "Product"          );
>+DefineColumn("classification"          , "map_classifications.name"        , "Classification"          );

nit: it would be nice if the alignment matches the other columns

r=glob, good work :)

Comment 51

[Joel Peshkin]

Comment on attachment 156460 [details] [diff] [review]
updated 2.19 patch

r=joel
noting nits on whitespace.  Those should be fixed on checkin.

Comment 52

[Joel Peshkin]

checked in -- good job Albert

Comment 53

[Albert Ting]

Cool!  Thanks for all the help and pointers.

Comment 54

[Andrew Sobala]

Thanks, this looks like a very fun patch ;)

Comment 55

[Dave Miller [:justdave]]

*** Bug 230893 has been marked as a duplicate of this bug. ***

Comment 56

[Vlad Dascalu]

Looks like we're dealing with the documentation for this in bug 328330.

Comment 57

[Frédéric Buclin]

*** Bug 220794 has been marked as a duplicate of this bug. ***