301743 - change the editcomponents.cgi code to use the Component.pm and Product.pm

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Tiago Mello [:timello]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.6) Gecko/20050524 Firefox/1.0 (Ubuntu package 1.0.2 MFSA2005-44)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.6) Gecko/20050524 Firefox/1.0 (Ubuntu package 1.0.2 MFSA2005-44)

Do those things for the editcomponents.cgi too.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Tiago Mello [:timello]]

Attachment: v1: cleaning up editcomponents.cgi

I didn't put some pod docs, I'll leave it for the next patch.

Comment 2

[Frédéric Buclin]

Comment on attachment 190567 [details] [diff] [review]
v1: cleaning up editcomponents.cgi

>Index: editcomponents.cgi

>+my $description  = trim($cgi->param('description') || '');

I see no reason to define $description here. Most of the time, you don't need
it.


> if ($action eq 'new') {
>+    
>+    my $default_assignee      = trim($cgi->param('initialowner')     || '');
>+    my $default_qa_contact    = trim($cgi->param('initialqacontact') || '');
> 
>+    my $default_assignee_id   = login_to_id($default_assignee);
>+    my $default_qa_contact_id = login_to_id($default_qa_contact);

I would first check whether the component name is not already in use, and if
not, *then* check whether the assignee and QA contact exist. In other words,
move this block after the component check.


>+    my $component =
>+        Bugzilla::Component::check_component($product, $comp_name);

You have a problem here. If the component name doesn't exist, which is
expected, check_component will complain, which is not expected! :) What you
have to do is to create a component object using
new Bugzilla::Component() and see if a defined object is returned.

Moreover, don't forget to check first if a component name is given:
$comp_name || ThrowUserError()


>+    if (Param('useqacontact') && !$default_qa_contact_id
>+        && $default_qa_contact ne '') {

Nit: I would first check whether a default QA contact is given before checking
its ID:

if (Param('useqacontact')
    && $default_qa_contact ne ''
    && !$default_qa_contact_id)
{
    ThrowUserError();
}


>+    trick_taint($comp_name);
>+    trick_taint($description);
>+    trick_taint($default_assignee);

trick_taint($default_assignee) is useless as its ID only is sent to the DB.


>+    unless ($default_qa_contact_id) {
>+        $default_qa_contact_id = 'NULL';
>+        trick_taint($default_qa_contact_id);
>     }

What's that?? To insert 'NULL' into the DB, the variable must be 'undef'. Note
that login_to_id returns 0 instead of undef when the login name doesn't exist.
So I would write:

$default_qa_contact_id ||= undef;


> if ($action eq 'delete') {

>+            foreach my $bug_id (@$component->bug_ids) {

@{$component->bug_ids} looks better.


> if ($action eq 'update') {

>+    my $old_comp_name         = trim($cgi->param('componentold')     || '');

Nit: for consistency with other edit*.cgi files, it should be $comp_old_name
and the component object should be $component_old.

Also, as you don't care about the other hidden fields, they should be removed
from admin/components/edit.html.tmpl.


>+    my $default_assignee      = trim($cgi->param('initialowner')     || '');
>+    my $default_qa_contact    = trim($cgi->param('initialqacontact') || '');
>+    my $default_qa_contact_id = login_to_id($default_qa_contact);

Why is $default_qa_contact_id defined here??? Please move it to its right
place.


>+    if (Param('useqacontact')
>+        && $default_qa_contact ne $old_comp->default_qa_contact_login) {
>+        if (!$default_qa_contact_id && $default_qa_contact ne '') {

Nit: here again, I would first check whether a default QA contact is given
before checking its ID:

if ($default_qa_contact ne '' && !$default_qa_contact_id)


>+        unless ($default_qa_contact_id) {
>+            $default_qa_contact_id = 'NULL';
>+            trick_taint($default_qa_contact_id);
>         }

Here again, this is not the right way to do it. Write $default_qa_contact_id
||= undef; instead.


>+    if ($comp_name ne $old_comp->name) {
>+
>+        my $component =
>+            new Bugzilla::Component({product_id => $product->id,
>+                                     name => $comp_name});

You first have to check whether $comp_name is given:

$comp_name || ThrowUserError();



>Index: Bugzilla/Component.pm

>+sub default_assignee_login {
>+    my $self = shift;
>+
>+    if (!defined $self->{'default_assignee_login'}) {
>+        my $user = new Bugzilla::User($self->default_assignee);
>+        $self->{'default_assignee_login'} = $user->login;
>+    }
>+    return $self->{'default_assignee_login'};
>+}

Either we return the full user object and call the method 'default_assignee'
(actually a bad idea due to the reason below), or you use the ugly
DBID_to_name() routine from globals.pl to get the login name only.
Please note that creating a user object involves much more tables than the
'profiles' table only, due to this stupid $user->derive_groups() which may be
called at that time! And we are in the middle of a LOCK TABLES when calling
$component->default_assignee_login.

So till we have a "light" user object creation (which would simply involve not
to call derive_groups() on user creation time but only when we really need to
check user privs), I prefer to use this ugly but unfortunately usefull
DBID_to_name() routine.

An alternative would be to call $component->default_assignee and
$component->default_qa_contact before we lock tables and save their values into
variables.


>+sub default_qa_contact_login {

The same comment applies here.

Comment 3

[Tiago Mello [:timello]]

Attachment: v2: fixes.

Comment 4

[Tiago Mello [:timello]]

Interdiff to help: http://phpfi.com/73197

Comment 5

[Joel Peshkin]

Comment on attachment 191979 [details] [diff] [review]
v2: fixes.

Some bitrot. 

You need to put componentold back in the edit component form. 

 However, even that does not cause it to propely function.  It locks a few
tables and then calls functions that need to access the user object.  Better to
validate all the input first and then lock the tables and do the update.

Comment 6

[Joel Peshkin]

In addition to looking up the userids before locking, you should really check to
make sure the name change doesn't conflict with another component first. 
Otherwise, an update can become half-done before you detect the error.

Comment 7

[Joel Peshkin]

Attachment: Patch fixes issues noted on prior patch

Comment 8

[Frédéric Buclin]

Comment on attachment 192611 [details] [diff] [review]
Patch fixes issues noted on prior patch

>Index: editcomponents.cgi

>@@ -44,70 +46,6 @@
> 
> my $showbugcounts = (defined $cgi->param('showbugcounts'));

This variable should go together with $product_name, $comp_name and $action
below:

>+my $product_name = trim($cgi->param('product')     || '');
>+my $comp_name    = trim($cgi->param('component')   || '');
>+my $action       = trim($cgi->param('action')      || '');


> if ($action eq 'new') {

>+    my $default_assignee_id   = login_to_id($default_assignee);
>+    my $default_qa_contact_id = login_to_id($default_qa_contact);
> 
>+    $description || ThrowUserError('component_blank_description',
>+                                   {name => $comp_name});
> 
>+    $default_assignee || ThrowUserError('component_need_initialowner',
>+                                        {name => $comp_name});

This sequence doesn't make sense. What you should do is:
1) check whether a description is given
2) check whether a default assignee is given
3) get the default assignee and default QA contact IDs

What you have done here is 3 - 1 - 2. So move the login_to_id() calls after
point 1 and 2.

Moreover, where is the test about the length of the component name???


> if ($action eq 'del') {

>+    $vars->{'comp'} =
>+        Bugzilla::Component::check_component($product, $comp_name);
> 
>+    $template->process("admin/components/confirm-delete.html.tmpl", $vars)
>+        || ThrowTemplateError($template->error());

As confirm-delete.html.tmpl requires product information, I would add a
$vars->{'prod'} = $product so that you don't need the component.product.foo
trick, see my comment later about Component::product().


> if ($action eq 'edit') {

>+    $vars->{'comp'} =
>+        Bugzilla::Component::check_component($product, $comp_name);
> 
>     $template->process("admin/components/edit.html.tmpl",
>                        $vars)

Here also, some information about the product are required by edit.html.tmpl.
Having a $vars->{'prod'} = $product would be fine.


> if ($action eq 'update') {

>+    my $component_old =
>+        new Bugzilla::Component({product_id => $product->id,
>+                                 name => $comp_old_name});
>+    my $default_assignee_id = login_to_id($default_assignee);
>+    my $default_qa_contact_id = login_to_id($default_qa_contact);
>+    my $old_default_assignee_login = $component_old->default_assignee_login;
>+    my $old_default_qa_contact_login = $component_old->default_qa_contact_login;

All this part about 'update' is a mess!

First of all, we really don't need to define $old_default_assignee_login and
$old_default_qa_contact_login as we already have
$component_old->default_assignee_login and
$component_old->default_qa_contact_login.

Second, it's not the right place to get the default assignee and default QA
contact IDs as you haven't checked yet if non-empty ones have been given (same
remark as above when adding a new component).


>     $dbh->bz_lock_tables('components WRITE', 'products READ',
>                          'profiles READ');

>+    $comp_name ||= ThrowUserError('component_blank_name');

All these validation tests should be done *before* locking tables. We don't
need to lock them to check whether a component name, a description or a default
assignee have been given!

>+    if ($comp_name ne $component_old->name) {

>+        $dbh->do("UPDATE components SET name = ?
>+                  WHERE id = ?", undef, ($comp_name, $component_old->id));

It's too early to change the name in the DB; maybe another test about e.g. the
description will fail, in which case only a partial update has been made.


>+    if ($description ne $component_old->description) {
>         unless ($description) {
>             ThrowUserError('component_blank_description',
>-                           {'name' => $componentold});
>+                           {'name' => $component_old->name});
>         }

Here is what I meant in my previous comments: this check neither requires to
lock tables nor should happen after we started updating the DB. Move this check
*before* the locking.


>+    if ($default_assignee ne $old_default_assignee_login) {
> 
>+        unless ($default_assignee_id) {
>             ThrowUserError('component_need_valid_initialowner',
>-                           {'name' => $componentold});
>+                           {'name' => $component_old->name});
>         }

Same remarks as for the description!! Move this check *before* the locking.


>+    if (Param('useqacontact')
>+        && $default_qa_contact ne $old_default_qa_contact_login) {
>+        if ($default_qa_contact ne '' && !$default_qa_contact_id) {
>             ThrowUserError('component_need_valid_initialqacontact',
>+                           {'name' => $component_old->name});
>         }

Same remark here! Move this check *before* the locking.

With all these changes, we are now sure that *all* changes will be sent to the
DB.



>Index: Bugzilla/Component.pm

>+sub product {
>+    my $self = shift;
>+
>+    if (!defined $self->{'product'}) {
>+        $self->{'product'} = new Bugzilla::Product($self->product_id);
>+    }
>+    return $self->{'product'};
>+}

We definitely don't need this method! We already have $product in
editcomponents.cgi. Use it! See my comments about $vars->{'prod'} = $product.
And this way, we avoid a loop between Product.pm and Component.pm.


>+# XXX Just to use in admin/table.html.tmpl
>+sub default_assignee_login   { return $_[0]->default_assignee->login;   }
>+sub default_qa_contact_login { return $_[0]->default_qa_contact->login; }

Useless! You can define variables in the templates themselves if required, i.e.
"foo = bar.baz" (you told me on IRC that the dot was a problem; here is a
better workaround).


> ###############################
> ####      Accessors        ####
> ###############################

>-sub default_assignee   { return $_[0]->{'initialowner'};     }
>-sub default_qa_contact { return $_[0]->{'initialqacontact'}; }

Yes, that's nice! :)


>+sub check_component ($$) {

No more prototypes please! All prototypes have been removed from the Bugzilla
code FYI!


>+    $comp_name || ThrowCodeError('component_blank_name');

ThrowCodeError('component_blank_name') doesn't exist. It should be
ThrowUserError('component_blank_name') ('User' instead of 'Code').


>+=item C<product()>
>+
>+ Description: It is used to instantiate a Bugzilla::Product object.
>+
>+ Params:      none.
>+
>+ Returns:     A Bugzilla::Product object.
>+
>+=item C<product()>
>+
>+ Description: It is used to instantiate a Bugzilla::Product object.
>+
>+ Params:      none.
>+
>+ Returns:     A Bugzilla::Product object.
>+

First, there is no need to repeat things twice, especially when it should be
removed. ;)


>Index: template/en/default/admin/components/confirm-delete.html.tmpl

>Index: template/en/default/admin/components/edit.html.tmpl

>Index: template/en/default/admin/components/list.html.tmpl


I didn't review templates yet!

Comment 9

[Tiago Mello [:timello]]

Attachment: v4: removing the mess

Comment 10

[Frédéric Buclin]

Comment on attachment 193480 [details] [diff] [review]
v4: removing the mess

>Index: editcomponents.cgi

> if ($action eq 'delete') {

>+    $dbh->do("DELETE FROM components WHERE id = ?", undef,
>+             $component->id);

Nit: this would be cleaner having 'undef' on a new line:
      $dbh->do("DELETE FROM components WHERE id = ?",
	       undef, $component->id);


> if ($action eq 'update') {

>+    my $component_old =
>+        new Bugzilla::Component({product_id => $product->id,
>+                                 name => $comp_old_name});

We have to make sure the old component exists. $component_old has to be created
using
Bugzilla::Component::check_component().


>+    if (length($comp_name) > 64) {
>         ThrowUserError('component_name_too_long',
>-                       {'name' => $component});
>+                       {'name' => $comp_name});
>     }
> 
>+    $description || ThrowUserError('component_blank_description',
>+                                   {'name' => $component_old->name});

Please don't check the description while checking the component name. As soon
as we know the new component name is valid and not already in use, you can
check the description.


>+        new Bugzilla::Component({product_id => $product->id,
>+                                 name => $comp_name});
>+    if ($component && $comp_name ne $component_old->name) {

Don't try to create the component object if the component name hasn't change.
Only do it if it has changed:

if ($comp_name ne $component_old->name) {
    my $component =
	new Bugzilla::Component({product_id => $product->id,
				 name => $comp_name});
    if ($component) { ThrowUserError('component_already_exists', { ... }) }
}

At this point, we know that the component name is valid. You can know check the
description here. ;)


>+    # Qa contact is not required even if useqacontact is enable.
>+    my $default_qa_contact_id = undef;
>+    if (Param('useqacontact')
>+        && $default_qa_contact ne '') {
>+        $default_qa_contact_id = login_to_id($default_qa_contact);
>+        $default_qa_contact_id
>+            || ThrowUserError('component_need_valid_initialqacontact',
>+                              {'name' => $component_old->name});
>+    }

Nit: That's a nice way to do it. You don't need to specify '= undef':
     my $default_qa_contact_id; will do the same job.
     For consistency, and because the way you do it here is better, could you
please do the same in the "$action = 'new'" section?


>+        $dbh->do("UPDATE components SET name = ?
>+                  WHERE id = ?", undef, ($comp_name, $component_old->id));

Nit: it's cleaner to write:
      $dbh->do("UPDATE components SET name = ? WHERE id = ?",
		undef, ($comp_name, $component_old->id));
     Please do the same for the subsequent SQL statements (when appropriate).


>+    if ($default_assignee ne $component_old->default_assignee_login) {

$component_old->default_assignee_login is actually a workaround for the
admin/table.html.tmpl template and should be removed asap. It's better to use
$component_old->default_assignee->login instead.


>+    if (Param('useqacontact')
>+        && $default_qa_contact
>+        ne $component_old->default_qa_contact_login) {

Same remark here; use $component_old->default_qa_contact->login instead. And
keep the second test on one line (it's not that long).



>Index: Bugzilla/Component.pm

>+sub default_assignee {

>+        my $user = new Bugzilla::User($self->{'initialowner'});
>+        $self->{'default_assignee'} = $user;

Nit: you can write this in one line:
      $self->{'default_assignee'} =
	  new Bugzilla::User($self->{'initialowner'});


>+sub default_qa_contact {

>+        my $user = new Bugzilla::User($self->{'initialqacontact'});
>+        $self->{'default_qa_contact'} = $user;

Nit: same remark here.


>+# XXX Just to use in admin/table.html.tmpl
>+# Remove those methods as soon as possible!
>+sub default_assignee_login   { return $_[0]->default_assignee->login;   }
>+sub default_qa_contact_login { return $_[0]->default_qa_contact->login; }

We will be able to remove this workaround as soon as bug 302955 is checked in.
:)


>+sub check_component {

>+    $comp_name || ThrowCodeError('component_blank_name');

As I said in my previous review, it should be ThrowUserError()!!


>  Params:      $product_id - Integer with a Bugzilla product id.
> 
>- Returns:     A hash with component id as key and Bugzilla::Component
>-              object as value.
>+ Returns:     Bugzilla::Component object list.

bug_count(), bug_ids(), default_assignee() and default_qa_contact() methods, as
well as the check_component() subroutine need their POD too.



>Index: template/en/default/admin/components/confirm-delete.html.tmpl

> [%# INTERFACE:
>+  # comp: object; Bugzilla::Component object.
>   #%]

It also takes a product object!


>+[% IF comp.bug_count %]
>   <a title="List of [% terms.bugs %] for component '[% name FILTER html %]'"

'name' should be 'comp.name'.



>Index: template/en/default/admin/components/edit.html.tmpl

> [%# INTERFACE:
>+  # comp: object; The Bugzilla::Component object.
>   #%]

It also takes a product object.


> [% IF bug_count > 0 %]

It should be 'comp.bug_count'.


I haven't tested your patch yet. I may find other bugs.

Comment 11

[Frédéric Buclin]

timello, when will your updated patch be ready? There are now security
implications in having this bug fixed, see bug 271596.

Comment 12

[Tiago Mello [:timello]]

This patch I'll update today night. Sorry for the later :(.

Comment 13

[Tiago Mello [:timello]]

Attachment: v5: updated.

Fred, here it is. I just want help when using the new feature about the
table.html.tmpl. I don't know how to use it, or maybe because it is too later
and I can't see :).

Comment 14

[Frédéric Buclin]

Comment on attachment 194746 [details] [diff] [review]
v5: updated.

>Index: editcomponents.cgi

> if ($action eq 'new') {

>+    my $default_assignee   = trim($cgi->param('initialowner')     || '');
>+    my $default_qa_contact = trim($cgi->param('initialqacontact') || '');

>     # Do the user matching
>     Bugzilla::User::match_field ($cgi, {
>         'initialowner'     => { 'type' => 'single' },
>         'initialqacontact' => { 'type' => 'single' },
>     });
> 
>+    $default_assignee || ThrowUserError('component_need_initialowner',
>+                                        {name => $comp_name});
> 
>+    my $default_assignee_id   = login_to_id($default_assignee);
> 
>+    if (!$default_assignee_id) {
>         ThrowUserError('component_need_valid_initialowner',
>+                       {'name' => $comp_name});
>     }

It looks like you don't understand how User::match_field works. This routine
autocompletes the login names you enter in the fields mention in the keys of
the hash passed to the routine (in our case initialowner and initialqacontact)
and updates CGI params accordingly. This means three important things:
1) $default_assignee and $default_qa_contact have to be defined *after*
User::match_field() is being called in order to catch autocompleted logins;
2) you only have to check whether a default assignee has been defined, as you
already know that if there is one, it's a valid user account (thanks to
User::match_field() which checks it for you).
3) you don't need to check the QA contact at all as if there is one, you
already know the user account is valid; and you don't need to check if there is
one as this field is optional.
This will simplify your code a lot!

Note: better is to move User::match_field() at the very beginning of this
section.


> if ($action eq 'update') {

>+    if ($comp_name ne $component_old->name) {
>+        my $component =
>+            new Bugzilla::Component({product_id => $product->id,
>+                                     name => $comp_name});
> 
>+        $component || ThrowUserError('component_already_exists',
>+                                     {'name' => $component->name});
>     }

This is wrong! If $component exists, an error should be returned, not the
opposite. So you should have: if ($component) {ThrowUserError()}.


>+    $default_assignee_id
>+        || ThrowUserError('component_need_valid_initialowner',
>+                          {'name' => $component_old->name});

Here again, this test is useless as User::match_field checks it for you.


>+    # Qa contact is not required even if useqacontact is enable.
>+    my $default_qa_contact_id;
>+    if (Param('useqacontact')
>+        && $default_qa_contact ne '') {
>+        $default_qa_contact_id = login_to_id($default_qa_contact);
>+        $default_qa_contact_id
>+            || ThrowUserError('component_need_valid_initialqacontact',
>+                              {'name' => $component_old->name});
>     }

Same remark here. This block reduces to:
$default_qa_contact_id = login_to_id($default_qa_contact) || undef;

No need to check whether a QA contact is given. If there is none, || undef will
do what we want.



>Index: Bugzilla/Component.pm

>+sub bug_count {
>+sub bug_ids {
>+sub check_component {

These three routines should appear in the SYNOPSIS section of the POD too.


>+=item C<bugs_ids()>
>+
>+ Description: Returns all bugs id that belongs to the component.

Nit: all bug IDs that belong to...

>+ 
>+ Params:      none.
>+
>+ Returns:     A list of bugs id.

Nit: A list of bug IDs.


>+=item C<default_assignee()>
>+
>+ Description: When called, caches and returns a Bugzilla::User object.

"When called, caches" is useless as that's what all methods do! Only say that
it returns a user object representing the default assignee for this component.


>+=item C<default_qa_contact()>
>+
>+ Description: When called, caches and returns a Bugzilla::User object.

Same remark here.



>Index: template/en/default/admin/components/confirm-delete.html.tmpl

> [%# INTERFACE:
>+  # comp: object; Bugzilla::Component object.
>+  # prod: object; Bugzilla::Product object.
>   #%]

Be more precise, such as "comp: component object representing the component the
user wants to delete" and "prod: product object representing the product to
which the component belongs".



>Index: template/en/default/admin/components/edit.html.tmpl

> [%# INTERFACE:
>+  # comp: object; The Bugzilla::Component object.
>+  # prod: object; The Bugzilla::Product object.
>   #%]

Same remark as for the other file; be more precise.


>Index: template/en/default/admin/components/list.html.tmpl

>      { 
>-       name => "initialowner"
>+       name => "default_assignee_login"
>        heading => "Default Assignee"
>      },
>    ]

>-       name => 'initialqacontact'
>+       name => 'default_qa_contact_login'
>        heading => 'QA Contact'
>      }) %]

Keep these names as is, and use [% overrides.* %] instead. Have a look at
admin/milestones/list.html.tmpl to see how it works.

Comment 15

[Tiago Mello [:timello]]

Attachment: v6: some fixes about previous reviews

Comment 16

[Tiago Mello [:timello]]

Attachment: v7: missing spelling change.

Sorry for the spam.

Comment 17

[Frédéric Buclin]

Comment on attachment 194977 [details] [diff] [review]
v7: missing spelling change.

>Index: editcomponents.cgi

> if ($action eq 'new') {

>+    my $default_qa_contact_id = login_to_id($default_qa_contact) || undef;

If Param('useqacontact') is off, $default_qa_contact_id should be undefined in
all cases. Something like the following should do it:

   my $default_qa_contact_id = Param('useqacontact') ?
	(login_to_id($default_qa_contact) || undef) : undef;


> if ($action eq 'update') {

>+        $dbh->do("UPDATE components SET name = ?
>+                  WHERE id = ?", undef, ($comp_name, $component_old->id));

Nit: it would be cleaner to move "WHERE id = ?" on the previous line.



>Index: Bugzilla/Component.pm

>+use Bugzilla::User;

We now have a dependency loop User - Product - Component - User. :(


>+sub bug_ids {
>+    my $self = shift;
>+    my $dbh = Bugzilla->dbh;
>+
>+    if (!defined $self->{'bugs_ids'}) {
>+        $self->{'bug_count'} = $dbh->selectcol_arrayref(q{

It should be $self->{'bug_ids'}, not $self->{'bug_count'}!


>+sub default_qa_contact {
>+    if (!defined $self->{'default_qa_contact'}) {
>+        my $user = 
>+        $self->{'default_qa_contact'} =
>+            new Bugzilla::User($self->{'initialqacontact'});
>+    }
>+    return $self->{'default_qa_contact'};
>+}

my " $user = " has nothing to do here.


>+    my $bug_count          = $component->bug_count();
>+    my @bug_ids            = $component->bug_ids();

$component->bug_ids returns a reference to an array, not an array itself. So
you should write $bug_ids.


>+=item C<bugs_ids()>
>+
>+ Description: Returns all bug IDs that belong to the component.
>+ 
>+ Params:      none.
>+
>+ Returns:     A list of bug IDs.

A *reference* to an array of bug IDs; see my previous comment.



>Index: template/en/default/admin/components/list.html.tmpl

You have to update the INTERFACE of this template. 'components' is now an array
of component objects.


>+[%# Overrides the initialowner and the initialqacontact with right values %]
>+[% FOREACH component = components %]
>+  [% overrides.initialowner = [ {
>+       match_value => component.initialowner
>+       match_field => 'initialowner'
>+       override_content => 1
>+       content => component.default_assignee.login
>+     } ]
>+  %]
>+  [% overrides.initialqacontact = [ {
>+       match_value => component.initialqacontact
>+       match_field => 'initialqacontact'
>+       override_content => 1
>+       content => component.default_qa_contact.login
>+     } ]
>+  %]
>+[% END %]

This doesn't work as you override your previous values at each loop. Moreover,
'overrides' should be initialized out of the loop. As it took me a long time to
find the right way to do it (and with the help of justdave), I show you how to
do it:

[%# Overrides the initialowner and the initialqacontact with right values %]
[% overrides.initialowner = [] %]
[% overrides.initialqacontact = [] %]

[% FOREACH component = components %]
  [% overrides.initialowner.push({
       match_value => component.name
       match_field => 'name'
       override_content => 1
       content => component.default_assignee.login
     })
  %]
  [% overrides.initialqacontact.push({
       match_value => component.name
       match_field => 'name'
       override_content => 1
       content => component.default_qa_contact.login
     })
  %]
[% END %]

This solution works fine. :)


>Index: template/en/default/global/user-error.html.tmpl

>-  [% ELSIF error == "component_need_valid_initialowner" %]
>-    [% title = "Component Requires A Valid Default Assignee" %]
>-    You must use an existing [% terms.Bugzilla %] account as the default assignee for
>-    component '[% name FILTER html %]'.
>-
>   [% ELSIF error == "component_need_valid_initialqacontact" %]
>     [% title = "Component Requires A Valid Default QA Contact" %]
>     You must use an existing [% terms.Bugzilla %] account as default QA contact for

component_need_valid_initialqacontact is now useless too.


I did some basic tests with your patch (and all these comments fixed), and it
seems to work well. Let's hope the next one will be the last one. :-/

Comment 18

[Tiago Mello [:timello]]

Attachment: v8: cleanups

Maybe the final one?

Comment 19

[Frédéric Buclin]

Comment on attachment 195030 [details] [diff] [review]
v8: cleanups

tested; works well. Thanks for the work, timello. :)

Comment 20

[Frédéric Buclin]

Checking in editcomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v  <--  editcomponents.cgi
new revision: 1.60; previous revision: 1.59
done
Checking in Bugzilla/Component.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Component.pm,v  <--  Component.pm
new revision: 1.7; previous revision: 1.6
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v 
<--  filterexceptions.pl
new revision: 1.54; previous revision: 1.53
done
Checking in template/en/default/admin/components/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/components/confirm-delete.html.tmpl,v
 <--  confirm-delete.html.tmpl
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/admin/components/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/components/edit.html.tmpl,v
 <--  edit.html.tmpl
new revision: 1.5; previous revision: 1.4
done
Checking in template/en/default/admin/components/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/components/list.html.tmpl,v
 <--  list.html.tmpl
new revision: 1.3; previous revision: 1.2
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v
 <--  user-error.html.tmpl
new revision: 1.125; previous revision: 1.124
done