455632 - Add Bugzilla::Field::Choice->create and have editvalues.cgi use it.

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Max Kanat-Alexander]

Once we have a Bugzilla::Field::Choice object, it should have a create() method that editvalues.cgi uses instead of manual SQL in the script.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

Okay, here we go. I had to modify Bugzilla::Field::Choice to make it subclassable, because Bugzilla::Status has some custom bits to it that are different from a normal "field choice" and instead of hacking them into Field::Choice, I wanted to make Bugzilla::Status a subclass of Field::Choice (which gets us lots of other features for Bugzilla::Status without any work, which is nice).

Comment 2

[Bradley Baetz (:bbaetz)]

Comment on attachment 339004 [details] [diff] [review]
v1

>=== modified file 'Bugzilla/Field/Choice.pm'
>--- Bugzilla/Field/Choice.pm	2008-09-17 01:56:21 +0000
>+++ Bugzilla/Field/Choice.pm	2008-09-17 03:15:42 +0000

> sub _check_field_arg {

>+    # Bugzilla::Object internally calls new() with just an id for $params, so
>+    # we don't do any of this function if we're there.
>+    return if !ref $params;

>+    # Subclasses don't use this logic, because they are specific fields.
>+    return if $class ne __PACKAGE__;

This seems really ugly and fragile. What are you trying to do here?

Comment 3

[Max Kanat-Alexander]

(In reply to comment #2)
> >+    # Subclasses don't use this logic, because they are specific fields.
> >+    return if $class ne __PACKAGE__;
> 
> This seems really ugly and fragile. What are you trying to do here?

  This comment's in the wrong bug, but I know what you're referring to.

  The problem is that some constructors will call Bugzilla::Status without passing a "field" argument (because they don't have to). This will in fact be true for all subclasses of Bugzilla::Field::Choice as I currently imagine them.

  However, I agree that it's fragile. In fact, the whole architecture where I have to override every method of Bugzilla::Object is fragile, and it bugs me. Do you have any better thoughts on it?

Comment 4

[Bradley Baetz (:bbaetz)]

(In reply to comment #3)
> (In reply to comment #2)
> > >+    # Subclasses don't use this logic, because they are specific fields.
> > >+    return if $class ne __PACKAGE__;
> > 
> > This seems really ugly and fragile. What are you trying to do here?
> 
>   This comment's in the wrong bug, but I know what you're referring to.

Its in the attachment on this bug....

>   The problem is that some constructors will call Bugzilla::Status without
> passing a "field" argument (because they don't have to). This will in fact be
> true for all subclasses of Bugzilla::Field::Choice as I currently imagine them.

Yeah, thats a bad sign

>   However, I agree that it's fragile. In fact, the whole architecture where I
> have to override every method of Bugzilla::Object is fragile, and it bugs me.
> Do you have any better thoughts on it?

Not offhand, no.

Comment 5

[Max Kanat-Alexander]

Attachment: v2

Okay, this version is MUCH better! Thanks for the discussion today on IRC--I got an idea of how to fix things and this is working much more nicely.

Comment 6

[Bradley Baetz (:bbaetz)]

Comment on attachment 340084 [details] [diff] [review]
v2

This looks a lot better; I'll have a closer look at it tomorrow.

Comment 7

[Bradley Baetz (:bbaetz)]

Comment on attachment 340084 [details] [diff] [review]
v2

>--- Bugzilla/Field.pm	2008-09-24 07:55:05 +0000
>+++ Bugzilla/Field.pm	2008-09-24 03:54:51 +0000

>@@ -376,7 +375,8 @@
>     my $self = shift;
> 
>     if (!defined $self->{'legal_values'}) {
>-        my @values = Bugzilla::Field::Choice->get_all({ field => $self });
>+        require Bugzilla::Field::Choice;
>+        my @values = Bugzilla::Field::Choice->type($self)->get_all();
>         $self->{'legal_values'} = \@values;
>     }
>     return $self->{'legal_values'};

Can this be done via inheritance now?

>=== modified file 'template/en/default/global/user-error.html.tmpl'
>--- template/en/default/global/user-error.html.tmpl	2008-08-22 07:35:00 +0000
>+++ template/en/default/global/user-error.html.tmpl	2008-09-24 04:14:04 +0000

>@@ -1672,5 +1674,10 @@
>     milestone
>   [% ELSIF class == "Bugzilla::Status" %]
>     status
>+  [% ELSIF class == "Bugzilla::Field" %]
>+    field
>+  [% ELSIF ( matches = class.match('^Bugzilla::Field::Choice::(.+)') ) %]
>+    [% SET field_name = matches.0 %]
>+    [% field_descs.$field_name %]
>   [% END %]
> [% END %]

This block just seems to be lc(last part of class name). I can see this being in a template rather than a class method due to localisation, but then the new addition you've made won't really be localisable.

Comment 8

[Max Kanat-Alexander]

(In reply to comment #7)
> >+        require Bugzilla::Field::Choice;
> >+        my @values = Bugzilla::Field::Choice->type($self)->get_all();
> >         $self->{'legal_values'} = \@values;
> >     }
> >     return $self->{'legal_values'};
> 
> Can this be done via inheritance now?

  No, we don't have subclasses of Bugzilla::Field.

> This block just seems to be lc(last part of class name). I can see this being
> in a template rather than a class method due to localisation, but then the new
> addition you've made won't really be localisable.

  field_descs is localizable.

Comment 9

[Bradley Baetz (:bbaetz)]

> 
> > This block just seems to be lc(last part of class name). I can see this being
> > in a template rather than a class method due to localisation, but then the new
> > addition you've made won't really be localisable.
> 
>   field_descs is localizable.

Yes, but translators won't be able to know what to translate, since they can't use the regexp that you're using. They'll have to go through the code, work out which bits may show up there, and so on

Comment 10

[Max Kanat-Alexander]

(In reply to comment #9)
> Yes, but translators won't be able to know what to translate, since they can't
> use the regexp that you're using. They'll have to go through the code, work out
> which bits may show up there, and so on

  The standard bits that show up there are already in field_descs. That's why this works. That's all that localizers translate. The only other bits that would show up there are custom field descriptions, which localizers don't translate anyhow (though local installs could if they so chose).

Comment 11

[Bradley Baetz (:bbaetz)]

Comment on attachment 340084 [details] [diff] [review]
v2

OK then. r=bbaetz

Comment 12

[Max Kanat-Alexander]

Checking in editvalues.cgi;
/cvsroot/mozilla/webtools/bugzilla/editvalues.cgi,v  <--  editvalues.cgi
new revision: 1.32; previous revision: 1.31
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.262; previous revision: 1.261
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.98; previous revision: 1.97
done
Checking in Bugzilla/Field.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
new revision: 1.34; previous revision: 1.33
done
Checking in Bugzilla/Status.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Status.pm,v  <--  Status.pm
new revision: 1.8; previous revision: 1.7
done
Checking in Bugzilla/Field/Choice.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field/Choice.pm,v  <--  Choice.pm
new revision: 1.2; previous revision: 1.1
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.107; previous revision: 1.106
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.259; previous revision: 1.258
done