350307 - Split out the create and update functionality of Bugzilla::Field::create_or_update

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Max Kanat-Alexander]

Right now we have Bugzilla::Field::create_or_update. Instead, we should split out the functionality so that we can use Bugzilla::Object->create for the create, and some other method for the updating of field definitions.

Comment 1

[Max Kanat-Alexander]

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

Comment 2

[Max Kanat-Alexander]

Attachment: v1

Okay, here we go. I've tested this lightly, and it works.

You don't have to review the tiny line I added to checksetup.pl--that's just something I've been wanting to do forever.

Comment 3

[Max Kanat-Alexander]

Attachment: v2

Okay, here it is. This has been un-bitrotten and generally updated.

Comment 4

[Frédéric Buclin]

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

>Index: Bugzilla/Field.pm

>+sub _check_description {

>+    $desc = trim($desc);

The original code uses clean_text() instead of trim(). I see no reason to relax this condition.


>+sub _check_name {

>+    $name = trim($name);

Same comment here. clean_text() should be used.


>+    # Custom fields have more restrictive name requirements than
>+    # standard fields.
>+    $name_regex = qr/^\w+$/ if $is_custom;

The name cannot be 'cf_' only (see another bug).


>+    # Assure the name is unique.
>+    if (!ref($invocant) || $invocant->name ne $name) {

Is it intentional that we allow $invocant->name ne $name? At least not for custom fields.


>+sub _check_type {
>+    detaint_natural($type)
>+      || ThrowCodeError('invalid_customfield_type', { type => $saved_type });

This is not enough as I could pass an invalid positive type ID (one which is not in Constants.pm).


>+sub create {

>+        if ($type == FIELD_TYPE_SINGLE_SELECT) {
>+            # Create the table that holds the legal values for this field.
>+            $dbh->bz_add_field_table($name);

How does bz_add_field_table() behave if for some reason the "$name" table already exists? The existing code makes sure the table doesn't exist.



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

>+  [% ELSIF error == "field_invalid_sortkey" %]
>+    [% title = "Invalid Sortkey for Field" %]
>+    The sortkey [% sortkey FILTER html %] that you have provided for
>+    the this field is not a valid positive integer.

s/for the this/for this/


>+  [% ELSIF error == "field_missing_description" %]
>+    [% title = "Missing Description for Field" %]
>+    You must enter a description for this field.

Nit: what a pity the name of the field is not displayed anymore...


>+  [% ELSIF error == "field_missing_name" %]
>+    [% title = "Missing Name for Field" %]
>+    You must enter a name for this field.

Nit: same comment here.


Else your patch looks good. I haven't tested it yet, though.

Comment 5

[Max Kanat-Alexander]

Attachment: v3

Okay, I fixed all the comments above (although I didn't add the field name back in--that's not really possible, and not really necessary anyway, since people know what field they were editing, hopefully). It passes runtests.

Comment 6

[Frédéric Buclin]

Comment on attachment 245044 [details] [diff] [review]
v3

>Index: Bugzilla/Field.pm

You forgot to fix the POD at line 46:
  my $field = Bugzilla::Field::create_or_update(
    {name => 'hilarity', desc => 'Hilarity', custom => 1});

This method no longer exists. Also, checksetup.pl still mentions this method in a comment. Please fix this comment as well.


>+sub _check_type {

I suppose it would be fine to add: $type ||= FIELD_TYPE_UNKNOWN in case $type is undefined or empty. Else detaint_natural($type) will probably fail.


>+sub run_create_validators {

>+    if (!exists $params->{sortkey}) {
>+        $params->{sortkey} = $dbh->selectrow_array(
>+            "SELECT MAX(sortkey) + 100 FROM fielddefs") || 100;
>     }

I'm not really happy with this check. That's the job of _check_sortkey. I will show below how to avoid this duplicated code.


>+    foreach my $def (DEFAULT_FIELDS) {

>+        else {

>+            Bugzilla::Field->create($def);
>+        }

Before calling create(), first write: $def->{sortkey} = "";. This will force _check_sortkey to validate it and give it an appropriate sortkey. This will avoid the duplicated code above.


I tested your patch a bit, and it seems to work fine. r=LpSolit with my comments above fixed.

Comment 7

[Dave Miller [:justdave]]

Please address the stuff in comment 6 before checkin.

Comment 8

[Max Kanat-Alexander]

(In reply to comment #6)
> I suppose it would be fine to add: $type ||= FIELD_TYPE_UNKNOWN in case $type
> is undefined or empty. Else detaint_natural($type) will probably fail.

  That would only happen if somebody explicitly passes an undefined or empty type, in which case we *should* fail. If they don't pass the type parameter at all, it defaults to FIELD_TYPE_UNKNOWN in the database.

> I'm not really happy with this check. That's the job of _check_sortkey. I will
> show below how to avoid this duplicated code.
> [snip]

  Basically what you're saying is that REQUIRED_CREATE_FIELDS should contain sortkey (since that's the only way to enforce the key getting correctly set, in your model). However, that just seems like a pain for callers. This makes life easy for callers. I could have created a utility function called _get_next_sortkey, but I decided not to, since this was just one line of code.

Comment 9

[Max Kanat-Alexander]

  I fixed the POD and the comment. I didn't change anything in relation to the above two comments that I replied to. We can talk about them in IRC if you feel strongly about it, and possibly do a post-checkin change if it makes sense.

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.546; previous revision: 1.545
done
Checking in editfields.cgi;
/cvsroot/mozilla/webtools/bugzilla/editfields.cgi,v  <--  editfields.cgi
new revision: 1.7; previous revision: 1.6
done
Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.90; previous revision: 1.89
done
Checking in Bugzilla/Field.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
new revision: 1.23; previous revision: 1.22
done
Checking in template/en/default/admin/custom_fields/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/custom_fields/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v  <--  messages.html.tmpl
new revision: 1.46; previous revision: 1.45
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.198; previous revision: 1.197
done

Comment 10

[Frédéric Buclin]

(In reply to comment #8)
>   That would only happen if somebody explicitly passes an undefined or empty
> type, in which case we *should* fail. If they don't pass the type parameter at
> all, it defaults to FIELD_TYPE_UNKNOWN in the database.

There is a difference between throwing an error using ThrowCodeError() and having detaint_natural() to complain in the error log of your web server. My goal is to prevent the latter.


>   Basically what you're saying is that REQUIRED_CREATE_FIELDS should contain
> sortkey (since that's the only way to enforce the key getting correctly set, in
> your model). However, that just seems like a pain for callers. This makes life
> easy for callers.

Having duplicated code to make the caller happy doesn't sound right to me. And using my suggestion from comment 6 would make all interactions from the UI correct. You could add the same "trick" in a new Webservice/Field.pm if you want to.

Comment 11

[Max Kanat-Alexander]

(In reply to comment #10)
> There is a difference between throwing an error using ThrowCodeError() and
> having detaint_natural() to complain in the error log of your web server. My
> goal is to prevent the latter.

  I'm pretty sure detaint natural doesn't throw a warning if passed undef. I could be wrong, though.

> Having duplicated code to make the caller happy doesn't sound right to me.

  Well, I'll add _get_next_sortkey, if you want.

> You could add the same "trick" in a new Webservice/Field.pm if you want to.

  Right, and everywhere else that anybody ever calls the function? You want me to also stand over the shoulder of every developer and every customizer in the world and make sure they always add that line?

  I'd rather enforce it in the code, so that's why I did it the way I did it.