261181 - the inclusions/exclusions code should be independent of (factored out of) flag types

Status: NEW

(Bugzilla :: Bugzilla-General, enhancement, P2)

Description

[Myk Melez [:myk] [@mykmelez]]

Product/component inclusions/exclusions are built into the flag types
administration interface, but they could be useful for other things, like
product/component watching.  The code that implements them should be made
independent of flag types.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: factors out clusions code

This may look like a large change, but it's virtually all refactoring of code
with very little code modification.  The patch moves clusions updating from
editflagtypes.cgi to Bugzilla/Clusion.pm, and it moves the clusions UI from
admin/flag-type/edit.html.tmpl to global/clusion.html.tmpl.

Along the way it makes the following minor modifications:

* renames "category" (i.e. product/component) to "clusion" on the backend (the
UI continues to call it "category," however);
* switches from StudlyCaps to underscore_separated per recent trends;
* changes the product/component ID validation functions inside Clusion.pm to
return the valid values from the validation functions and pass the product ID
as an argument to the component ID validation function to obviate the need to
store the product and component IDs in global variables;
* factors out the two custom menus (product and component) in the clusions UI
by adding "blank" value support to global/select-menu.html.tmpl and then using
that template in lieu of the custom menus (we already use that template for the
other two menus in the clusions UI; this just means we use it for all four);
* switches from PROCESS to INCLUDE when using select-menu.html.tmpl in the
clusions UI to prevent menu-specific values like "multiple" from messing up the
display of menus that appear later in a page (this isn't currently a problem,
but this move guards against it becoming one);
* elucidates and corrects the grammar of an interface comment in
select-menu.html.tmpl.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 159845 [details] [diff] [review]
patch v1: factors out clusions code

Joel, does this look good to you?  Note the detailed description of changes in
comment 1--this is almost all factoring of code, with very little code
modification.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: additional refactoring

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: updated to work with today's checkins

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 159923 [details] [diff] [review]
patch v3: updated to work with today's checkins

Vladd, is this something you can handle?  It's almost entirely refactoring of
code (there are also a couple of very minor bug fixes), and I can go over it
step by step if that would help.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 159923 [details] [diff] [review]
patch v3: updated to work with today's checkins

Jouni, any chance you can tackle this review?  Although the patch looks large,
it's virtually all factoring out code into a separate module file and template
file.  As such, it doesn't generally need precise character-by-character review
but rather just verification that the structure of the code in the new files
matches its structure in the old files, so it should be a much simpler review
than the size of the patch implies.

Comment 7

[Jouni Heikniemi]

Comment on attachment 159923 [details] [diff] [review]
patch v3: updated to work with today's checkins

With this patch applied on the tip, editflagtypes.cgi no longer works properly.
When I pick a product and a component and click "Exclude" (or "Include", for
that matter), nothing happens. My Included list has Any/Any and my Excluded
list is empty. 


>Index: editflagtypes.cgi
>===================================================================

>+sub reflect_clusion_change {

Nit: "reflect" is a pretty uncommon verb for a method name. You should add a
comment describing what this method actually does and when it's called.


>Index: request.cgi
>===================================================================
>-                else { ThrowCodeError("unknown_component", { component => $cgi->param('component') }) }
>+                else { ThrowCodeError("unknown_component",
>+                                      { acomponent => $cgi->param('component'),
>+                                        product => $cgi->param('product')}) }

Please explain this change?


>Index: Bugzilla/Clusion.pm
>===================================================================

You're missing POD for several of the functions.

>+use constant ALL_ITEMS_LABEL => "__Any__";

Hmmmm... The role of this constant eludes me. I believe it's used both as a
visual and as a codebehind (value) indicator of "any". Is that right? What's
our support for localizing this? Apart from that, "ALL_ITEMS_LABEL" is too
generic a name when exported to the global namespace. Could we come up with
something more clusion-specific? (not making a suggestions as I'm still a bit
uncertain on the role of this).

If some of the above turns into a major issue, let's split it to a separate
bug.

>+sub change_clusions {

Perhaps "parse_clusions_from_form" or something similar; I think we should
specifically indicate the attachment to the form here. It adds clarity and
makes future backend separation work easier (from a naming perspective).

>+        my $product_id = validate_product();
>+        my $component_id = validate_component($product_id);
>+        my $clusion = ($::FORM{'product'} || ALL_ITEMS_LABEL) . ":"
>+                      . ($::FORM{'component'} || ALL_ITEMS_LABEL);
>+        push(@inclusions, $clusion) unless grep($_ eq $clusion, @inclusions);

Remind me why you're assigning product_id and component_id since you're not
using them anyway?

>+    elsif ($clusion_action eq "exclude") {

We should standardize on "addInclusion" and "removeInclusion" instead of
"include" and "removeInclusion", just for clarity.


>+    elsif ($clusion_action eq "removeInclusion") {
>+        @inclusions = map(($_ eq $::FORM{'inclusion_to_remove'} ? () : $_), @inclusions);
>+    }
>+    elsif ($clusion_action eq "removeExclusion") {
>+        @exclusions = map(($_ eq $::FORM{'exclusion_to_remove'} ? () : $_), @exclusions);
>+    }

<scratches his head> Err... Why aren't we grepping here? 

Nit: Line length.

>+sub update_clusions {

Again, "from_form" perhaps.

>+=item C<update_clusions>
>+Processes clusion action and returns updated inclusion/exclusion lists.

You could be a bit more descriptive; which form fields this methods parses, how
are the lists returned etc.

>Index: template/en/default/global/clusion.html.tmpl
>===================================================================

Hmm... Perhaps "clusionselector"? Just "clusion" sounds a bit generic to me.

>+  # default_product: string; the default selected item in the product menu.
>+  # default_component: string; the default selected item in the component menu.
>+  # products: array or hash; products to display in products menu.
>+  # components: array or hash; components to display in components menu.
>+  # inclusions: hash; inclusions to display in inclusions menu.
>+  # exclusions: hash; exclusions to display in exclusions menu.

Nit: For uniformness, use "in the XXX menu" for all of these.

>+  # default_all_label: string; the label of an item representing "all values".

Nit: "the item"

Comment 8

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.