448593 - Move code to edit product group settings from editproducts.cgi to Bugzilla/Product.pm

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Frédéric Buclin]

As I said in bug 313122 comment 18, I didn't move the code to edit product group settings into Product.pm yet to avoid a too big patch. This last part will be done separately in this bug.

Comment 1

[Frédéric Buclin]

Attachment: patch, v1

The patch may look confusing about changes in editproducts.cgi because I moved the 'updategroupcontrols' section to its correct place, i.e. after 'editgroupcontrols', which makes the code more logic and more readable. But I didn't change the 'update' section at all. I tested my patch quite a lot and fixed all bugs I could find.

Comment 2

[Max Kanat-Alexander]

Comment on attachment 333347 [details] [diff] [review]
patch, v1

>Index: editproducts.cgi
>+    my @groups = Bugzilla::Group->get_all;
>+    foreach my $group (@groups) {
>+        next unless $group->is_active_bug_group;

  Use Bugzilla::Group->match instead to just get active bug groups.

>+        my $group_id = $group->id;

>+        $product->set_group_controls($group,
>+                                     {entry          => scalar $cgi->param("entry_$group_id") || 0,

  The only thing I don't like about doing things this way is that the UI has to send every single group. You can't use some custom tool to just update one group--you always have to send all data about all groups.

>+                                      membercontrol  => scalar $cgi->param("membercontrol_$group_id") || CONTROLMAPNA,

  These defaults should be in the checkers instead of in the cgi.

>Index: Bugzilla/Product.pm
>+            # If all new settings are 0 for a given group, we delete the entry
>+            # from group_control_map, so we have to track it here.
>+            my $all_zero = 1;

  You could also just check that using something from List::Util, I think.

>+            foreach my $field ('entry', 'membercontrol', 'othercontrol', 'canedit',
>+                               'editcomponents', 'editbugs', 'canconfirm')
>+            {
>+                my $old_value = $old_setting->{$field};
>+                my $new_value = $new_setting->{$field};
>+                $all_zero = 0 if $new_value;
>+                next if (defined $old_value && $old_value == $new_value);

  Why go through this check? Why not just always update all the fields? That would eliminate all this code, and wouldn't have any downside, that I see.

>+                if (scalar @$bug_ids) {
>+                    $dbh->do('UPDATE bugs SET delta_ts = ? WHERE ' .
>+                             $dbh->sql_in('bug_id', $bug_ids), undef, $timestamp);

  Actually, we're not sending email, so we shouldn't hit delta_ts. I wouldn't consider a new mandatory group to actually be a change, at least not in the sense that somebody searching or looking for changes would care about. (It's still good to put it in LogActivityEntry, though.)

>+                    push(@{$changes->{'group_controls'}->{'now_mandatory'}},

  Shouldn't we be pushing this on in some more standardized fashion, like mandatory => [0, 1] or having a hash/array of groups in group_controls?

>+                    $dbh->do('UPDATE bugs SET delta_ts = ? WHERE ' .
>+                              $dbh->sql_in('bug_id', $bug_ids), undef, $timestamp);

  Same comment here about delta_ts.

>@@ -471,6 +590,63 @@ sub set_votes_per_user { $_[0]->set('vot
>+    $group->is_active_bug_group
>+      || ThrowUserError('product_illegal_group', {group => $group});

  Nit: It'd be nice to have a four-space indent there.

>+        #  Legality of control combination is a function of
>+        #  membercontrol\othercontrol
>+        #                 NA SH DE MA
>+        #              NA  +  -  -  -
>+        #              SH  +  +  +  +
>+        #              DE  +  -  +  +
>+        #              MA  -  -  -  +

  Maybe we should just have a data structure that actually represents that table? Would that be simpler to read, code-wise?

>+=item C<group_controls($full_data)>

  Generally, don't put () or params in the =item (it makes it hard to link to, in the HTML).


  Otherwise, this is a really nice cleanup. :-)

Comment 3

[Frédéric Buclin]

(In reply to comment #2)
> >+        $product->set_group_controls($group,
> 
>   The only thing I don't like about doing things this way is that the UI has to
> send every single group. You can't use some custom tool to just update one
> group--you always have to send all data about all groups.

From the current UI, right, you have to pass them all. This code here is specific to our current editproducts.cgi UI. In a separate bug, we could very easily add a hidden field "defined_group_$groupid" in the template to only update those listed in the page. But I don't want to do it now, which is out of the scope of this bug.



> >+                                      membercontrol  => scalar $cgi->param("membercontrol_$group_id") || CONTROLMAPNA,
> 
>   These defaults should be in the checkers instead of in the cgi.

Hum no. The reason is because the checkers ignore undefined values, which allows you to do partial updates. If the checkers were automatically falling back to default values when they get an undefined value, partial updates would become impossible. So I want to do it here. Also, as the page in editproducts.cgi uses checkboxes, unchecked checkboxes are not passed by CGI, and so if we were passing them as is to checkers, we would be unable to update these fields as they would be ignored all the time.



> >+            my $all_zero = 1;
> 
>   You could also just check that using something from List::Util, I think.

Bah, my code is nice as is. :)



> >+                next if (defined $old_value && $old_value == $new_value);
> 
>   Why go through this check? Why not just always update all the fields? That
> would eliminate all this code, and wouldn't have any downside, that I see.

All this code, as you say, is not that big (at all). And I really don't want to update tens of groups which are not even touched by the caller. This also makes the logic below much easier.



>   Actually, we're not sending email, so we shouldn't hit delta_ts. I wouldn't
> consider a new mandatory group to actually be a change, at least not in the
> sense that somebody searching or looking for changes would care about. (It's
> still good to put it in LogActivityEntry, though.)

The original code updates delta_ts, deleting a target milestone used by bugs (which then have to fall back to the default milestone) also updates delta_ts, and I think there is a good reason to update it, unless otherwise proven. Note that we are updating delta_ts, not lastdiffed. I don't want to remove this now. If we realize it's useless, I prefer to remove it in a separate bug (in case we have to back out the change because we realize it's indeed required).



> >+                    push(@{$changes->{'group_controls'}->{'now_mandatory'}},
> 
>   Shouldn't we be pushing this on in some more standardized fashion, like
> mandatory => [0, 1] or having a hash/array of groups in group_controls?

You already have groups in group_controls, thanks to $product->group_controls. The idea here is to store in %changes changes only. I included the number of bugs being affected to be compatible with what we currently display as information to the administrator.



>   Same comment here about delta_ts.

Same answer. :-D



> >+    $group->is_active_bug_group
> >+      || ThrowUserError('product_illegal_group', {group => $group});
> 
>   Nit: It'd be nice to have a four-space indent there.

Why? We don't use four-space indent when splitting long lines.



> >+        #  Legality of control combination is a function of
> >+        #  membercontrol\othercontrol
> >+        #                 NA SH DE MA
> >+        #              NA  +  -  -  -
> >+        #              SH  +  +  +  +
> >+        #              DE  +  -  +  +
> >+        #              MA  -  -  -  +
> 
>   Maybe we should just have a data structure that actually represents that
> table? Would that be simpler to read, code-wise?

No idea what your idea really is, but in a separate bug, please. :)



>   Otherwise, this is a really nice cleanup. :-)

Thanks! :)

Comment 4

[Max Kanat-Alexander]

(In reply to comment #3)
> > >+                                      membercontrol  => scalar $cgi->param("membercontrol_$group_id") || CONTROLMAPNA,
> > 
> >   These defaults should be in the checkers instead of in the cgi.
> 
> Hum no. The reason is because the checkers ignore undefined values, which
> allows you to do partial updates. 

  That is totally inconsistent with how any other set function behaves.

  The checkers shouldn't be called if the key isn't in the hash.

> So I want to do it here.

  And you want us to duplicate that anywhere else that we call this function? In the WebServices? In custom code? (Where people will forget.)

> Also, as the page in
> editproducts.cgi uses checkboxes, unchecked checkboxes are not passed by CGI,

  Then they should have a defined_ thing.

> All this code, as you say, is not that big (at all). And I really don't want to
> update tens of groups which are not even touched by the caller.

  Then you just do a check using List::Utils or map() like I said above. There's no need to update only some columns. There is no other updater that does that.

> The original code updates delta_ts, deleting a target milestone used by bugs
> (which then have to fall back to the default milestone) also updates delta_ts,
> and I think there is a good reason to update it, unless otherwise proven. Note
> that we are updating delta_ts, not lastdiffed. I don't want to remove this now.

  Yes, updating delta_ts and not lastdiffed will make Bugzilla think that you haven't sent a mail. Either you should send mail or you should not update delta_ts.

> You already have groups in group_controls, thanks to $product->group_controls.
> The idea here is to store in %changes changes only. I included the number of
> bugs being affected to be compatible with what we currently display as
> information to the administrator.

  "Compatible with what we currently do" does not make for good software design.

> > >+    $group->is_active_bug_group
> > >+      || ThrowUserError('product_illegal_group', {group => $group});
> > 
> >   Nit: It'd be nice to have a four-space indent there.
> 
> Why? We don't use four-space indent when splitting long lines.

  What? Is that written somewhere?

Comment 5

[Frédéric Buclin]

(In reply to comment #4)
>   That is totally inconsistent with how any other set function behaves.

Because other functions handle one bit at a time. I don't want to implement ->set_entry, ->set_canedit, ->set_membercontrol, etc... And that's a big difference with what we do at other places.



>   And you want us to duplicate that anywhere else that we call this function?
> In the WebServices? In custom code? (Where people will forget.)

You don't have to duplicate anything. This works perfectly well with WebServices. If you don't pass a key, or its value is undefined, it's ignored. The only reason the "default" value appears here is because the UI is supposed to display these fields. There is nothing wrong here.


> There's no need to update only some columns. There is no other updater that
> does that.

Because we don't have ->set_entry, ->set_membership, etc... as I said above.


>   Yes, updating delta_ts and not lastdiffed will make Bugzilla think that you
> haven't sent a mail. Either you should send mail or you should not update
> delta_ts.

We should make sure this doesn't break anything. Also, this prevents sanitycheck.cgi from sending them.


>   "Compatible with what we currently do" does not make for good software
> design.

Maybe, but for now, I'm moving the code to Product.pm, and I don't think that's such a big deal for now.


> > Why? We don't use four-space indent when splitting long lines.
> 
>   What? Is that written somewhere?

We do it everywhere in the code. Or you missed the last 700 patches I wrote (+ including patches written by others).

Comment 6

[Max Kanat-Alexander]

Okay, that's all fine.

However, you should never update lastdiffed if you don't send mail. So either set delta_ts and send mail or don't set it and don't send mail.

Comment 7

[Frédéric Buclin]

Attachment: patch, v2

I no longer update delta_ts in Product.pm, and I call Group->match instead of Group->get_all in editproducts.cgi. This is still working fine.

Comment 8

[Max Kanat-Alexander]

Comment on attachment 333767 [details] [diff] [review]
patch, v2

Okay, sounds fine then.

Comment 9

[Frédéric Buclin]

Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.144; previous revision: 1.143
done
Checking in Bugzilla/Product.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Product.pm,v  <--  Product.pm
new revision: 1.30; previous revision: 1.29
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.115; previous revision: 1.114
done
Checking in template/en/default/admin/products/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.14; previous revision: 1.13
done
Checking in template/en/default/admin/products/groupcontrol/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/groupcontrol/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.12; previous revision: 1.11
done
Checking in template/en/default/admin/products/groupcontrol/updated.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/groupcontrol/updated.html.tmpl,v  <--  updated.html.tmpl
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.106; previous revision: 1.105
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.256; previous revision: 1.255
done