304722 - Edit Flags Menu should have an inclusion/exclusion for classification

Status: NEW

(Bugzilla :: Administration, task)

Description

[Douglas Calvert]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6

Hello,
 The edit flags menu should have the ability to include/exlude flags for
classifications as well as products and components. Currently it only uses
products and components. 

Reproducible: Always

Comment 1

[Max Kanat-Alexander]

Yeah, Classifications should have all the same abilities as Products, generally,
I think. I *think* this isn't a dup, I haven't seen it before.

Comment 2

[Frédéric Buclin]

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

Comment 3

[rojanu]

Attachment: Classification Flag Attempt 1

Here is a partial patch.  I just would like to know if my implementation is acceptable before I do any more work on it?

Comment 4

[Max Kanat-Alexander]

Comment on attachment 527211 [details] [diff] [review]
Classification Flag Attempt 1

This is more appropriately targeted at LpSolit.

Comment 5

[Frédéric Buclin]

Comment on attachment 527211 [details] [diff] [review]
Classification Flag Attempt 1

>=== modified file 'Bugzilla/Install/DB.pm'

>+	_add_classification_to_flagtypes();
>+
>     Bugzilla::Hook::process('install_update_db_fielddefs');

This is not the right place to call _add_classification_to_flagtypes(). The call must happen in update_table_definitions().


>+sub _add_classification_to_flagtypes {
>+    if (!$dbh->bz_column_info('flaginclusions', 'classification_id')) {

Just in case something goes wrong, I would treat both tables separately. So I would move everything related to flagexclusions under
  if (!$dbh->bz_column_info('flagexclusions', 'classification_id'))



>=== modified file 'editflagtypes.cgi'

>+if ($classification) {
>+    # Make sure the user is allowed to view this product name.
>+    # Users with global editcomponents privs can see all product names.

The comment is about products, not classifications. Classification names are not private; we don't care about them (as they are visible when reporting new bugs or when looking for bugs).


>+    $classification || ThrowUserError('classification_access_denied', { name => $cgi->param('classification') });

If a user doesn't have global editcomponents privs, then he shouldn't be allowed to select any classification. Else he would be able to insert flags into products he is not allowed to administer.


>+    if (Bugzilla->params->{'useclassification'}) {
>+        my @classifications;
>+        if ($user->in_group('editcomponents')) {
>+            @classifications = Bugzilla::Classification->get_all;
>+        }
>+        else {
>+            @classifications = $user->get_selectable_classifications;
>+        }
>+        $vars->{'classifications'} = \@classifications;
>+    }

get_selectable_classifications() is not what we want. This method returns all classifications you can view, not edit. As said above, users without global editcomponents privs shouldn't be allowed to play with classifications, and so this field should be hidden from the UI.


I didn't read further as the whole UI will be affected by my comments above.

Comment 6

[rojanu]

Attachment: Classification Flag

Comment 7

[Frédéric Buclin]

Comment on attachment 562447 [details] [diff] [review]
Classification Flag

There are many unrelated changes to this patch. Maybe some whitespace removals? Anyway, they have nothing to do here, especially in the documentation.

Comment 8

[rojanu]

Attachment: Classification_Flag-Notrim

Text editor had "Remove Trailing spaces" corrected now

Comment 9

[Frédéric Buclin]

Comment on attachment 567782 [details] [diff] [review]
Classification_Flag-Notrim

>=== modified file 'Bugzilla/Classification.pm'

>+sub flag_types {

There is a lot of duplicated code here coming from Bugzilla::Product::flag_types(). The reason we added this complexity in Product.pm was for performance reasons, when filing or editing a bug report. I don't think this is useful at all here.



>=== modified file 'Bugzilla/Component.pm'

>+    my $product = new Bugzilla::Product($self->product_id);
>+
>     if (!defined $self->{'flag_types'}) {
>-        my $flagtypes = Bugzilla::FlagType::match({ product_id   => $self->product_id,
>+        my $flagtypes = Bugzilla::FlagType::match({ classification_id => $product->classification_id,
>+                                                    product_id   => $product->id,
>                                                     component_id => $self->id });

It shouldn't be needed to pass the classification ID to match(). As the classification ID can be NULL, passing NULL or $product->classification_id is not the same thing (think Any:Any:Any).



>=== modified file 'Bugzilla/Constants.pm'

>+use constant ANY_ITEMS_LABEL => '__Any__';

Add a comment explaining what this constant is used for.


>=== modified file 'Bugzilla/DB/Schema.pm'

>+            classification_id   => {TYPE => 'INT2',

The comment above these tables must be updated to also mention the classification.



>=== modified file 'Bugzilla/FlagType.pm'

>+    if ($criteria->{classification_id}) {

What bothers me with this patch is that it adds a lot of complexity to some already complex code. We should at least do as much effort as possible to avoid duplicating too much code. I'm pretty sure this code can be refactored to not be duplicated when no classification ID is passed to match(). Also, the logic is very complex in this SQL query, so I would avoid to play with it as much as possible. Did you make sure it really works as expected?



>=== modified file 'Bugzilla/Install/DB.pm'

>+            $dbh->bz_drop_index("flag$category", 'flag'.$category.'_type_id_idx');

Nit: you could simply write "flag${category}_type_id_idx".


>+            my $sth = $dbh->prepare("UPDATE flag$category
>+                                        SET classification_id = ?
>+                                    WHERE product_id = ?");

classification_id can be NULL, so there is no need to waste time populating this column.



>=== modified file 'Bugzilla/User.pm'

>+                next if $e->{$clas_id}->{$id}->{0};

I'm not sure about the logic here. The DB schema says that the classification ID can be NULL. I'm not sure this code works as expected.



>=== modified file 'docs/en/xml/administration.xml'

>+            <quote>[%- constants.ANY_ITEMS_LABEL.':' IF Param('useclassification')%]
>+            [% constants.ANY_ITEMS_LABEL %]:[% constants.ANY_ITEMS_LABEL %]</quote>

XML templates are not Perl templates. You cannot use the Template-Toolkit syntax here.


I didn't read all your code carefully. But I think that we must really make changes as little invasive as possible, else the code is going to become unreadable and unmaintainable pretty quickly. Also, another reason I took so long to review your patch, besides fighting against all the blockers for Bugzilla 4.2 and 4.2.1, is that I'm not really convinced if we want this feature at all or not. This bug has been filed 7 years ago, and I see very few people interested in it based on the CC list and the number of votes. I would like more input before taking something so invasive.