349741 - Make Bugzilla::Bug able to do basic bug creation, and have post_bug.cgi use it

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Max Kanat-Alexander]

Okay, now with all the patches that I've posted, we're in a place where Bugzilla::Bug ought to be able to insert a bug into the bugs table. (It won't be able yet to affect any other tables, but we're doing this all incrementally, so this is what we're doing first.)

This will be the beginning of Bugzilla::Bug->create.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

Okay, now everybody should be able to see why I've been making all these validators. :-)

This already considerably simplifies post_bug.

I've tested this fairly well, using a user with editbugs and a user without editbugs, and it seems to work fine.

Comment 2

[Max Kanat-Alexander]

Attachment: v1.1

REQUIRED_CREATE_FIELDS was missing rep_platform.

Comment 3

[Max Kanat-Alexander]

Attachment: v1.2

bkor pointed out on IRC that I had left some debugging code in the last patch.

Comment 4

[Frédéric Buclin]

Comment on attachment 235009 [details] [diff] [review]
v1.2

>Index: Bugzilla/Object.pm

>     my $required   = $class->REQUIRED_CREATE_FIELDS;

LpSolit also pointed out on IRC that this line should be removed, because 1) $required should be an array, not a ref or a scalar, and 2) this variable is never used. ;)

Comment 5

[Olav Vitters]

Comment on attachment 235009 [details] [diff] [review]
v1.2

What LpSolit said and:

>Index: post_bug.cgi

> # Add the bug report to the DB.
> $dbh->bz_lock_tables('bugs WRITE', 'bug_group_map WRITE', 'longdescs WRITE',
>                      'cc WRITE', 'keywords WRITE', 'dependencies WRITE',
>@@ -345,10 +259,10 @@
>                      'user_group_map READ', 'group_group_map READ',
>                      'keyworddefs READ', 'fielddefs READ');
> 
>-$dbh->do($query, undef, @fields_values);
>+my $bug = Bugzilla::Bug->create(\%bug_params);

Gives lots of tables that haven't been locked. Not sure if this is all, but at least a READ lock on: products, versions, milestones, components, profiles, bug_severity, op_sys, priority, rep_platform.

>Index: Bugzilla/Object.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Object.pm,v
>retrieving revision 1.3
>diff -u -r1.3 Object.pm
>--- Bugzilla/Object.pm	10 Aug 2006 23:44:49 -0000	1.3
>+++ Bugzilla/Object.pm	22 Aug 2006 23:27:24 -0000
>@@ -144,18 +159,12 @@
>         else {
>             $value = $params->{$field};
>         }
>-        trick_taint($value);
>+        trick_taint($value) if defined $value;
>         push(@field_names, $field);
>         push(@values, $value);
>     }
> 
>-    my $qmarks = '?,' x @values;
>-    chop($qmarks);
>-    $dbh->do("INSERT INTO $table (" . join(', ', @field_names) 
>-             . ") VALUES ($qmarks)", undef, @values);
>-    my $id = $dbh->bz_last_key($table, 'id');

Slight bitrot here.

>-
>-    return $class->new($id);
>+    return (\@field_names, \@values);
> }
> 
> sub get_all {

Comment 6

[Max Kanat-Alexander]

Attachment: v2

Okay, this can successfully file a bug on MySQL, and I fixed the bitrot.

Comment 7

[Olav Vitters]

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

>Index: post_bug.cgi

>-my $query = qq{INSERT INTO bugs ($sql_used_fields, reporter, delta_ts,
>-                                 estimated_time, remaining_time, deadline)
>-               VALUES ($sql_placeholders ?, ?, ?, ?, ?)};
>-
>-push (@fields_values, $user->id);
>-push (@fields_values, $timestamp);
[..]
>-push (@fields_values, $est_time, $est_time);

This sets remaining_time to the same value as estimated_time, but your patch doesn't do that anymore. See comment below.

>@@ -339,17 +223,50 @@
>     }
> }
> 
>+my @bug_fields = map {$_->name} Bugzilla->get_fields(
>+    { custom => 1, obsolete => 0, enter_bug => 1});
>+push(@bug_fields, qw(
>+    product
>+    component
[snip]

Would be great if all these fields could be retrieved the same as get_fields does for the custom fields (for another bug).


>Index: Bugzilla/Bug.pm

>@@ -157,6 +184,78 @@
[..]
>+# C<estimated_time> - For time-tracking. Will be ignored if 
>+#                     C<timetrackinggroup> is not set, or if the current
>+#                     user is not a member of the timetrackinggroup.
>+# C<deadline>       - For time-tracking. Will be ignored for the same
>+#                     reasons as C<estimated_time>.

remaining_time should be set to the same value as estimated_time (see quoted code at the beginning). I do not see where this is done. From my testing this is not done.

>@@ -293,7 +390,7 @@
>     shift @valid_statuses if !$product->votes_to_confirm;
> 
>     check_field('bug_status', $status, \@valid_statuses);
>-    return $status;
>+    return ($status, $status eq 'UNCONFIRMED' ? 0 : 1);

Nit: I'm not sure if I like this suddenly validating two fields. Wouldn't having something like _check_everconfirmed be more consistent?

> }
> 
> sub _check_cc {

>@@ -341,6 +438,16 @@
>     return $obj;
> }
> 
>+sub _check_deadline {
>+    my ($date) = @_;

Must check if user is part of timetrackingroup just like _check_estimated_time.

>@@ -435,6 +551,8 @@
>     return $short_desc;
> }
> 
>+sub _check_status_whiteboard { return defined $_[0] ? $_[0] : ''; }

Nit: I know it was not done before, but perhaps check for max length/size of the field?

Comment 8

[Max Kanat-Alexander]

Attachment: v3

(In reply to comment #7)
> Would be great if all these fields could be retrieved the same as get_fields
> does for the custom fields (for another bug).

  Agreed. :-) I've wanted to do that for a long time. We'll do it as part of another bug. We just have to set enter_bug on all of the actual enter_bug fields.

> >-    return $status;
> >+    return ($status, $status eq 'UNCONFIRMED' ? 0 : 1);
> 
> Nit: I'm not sure if I like this suddenly validating two fields. Wouldn't
> having something like _check_everconfirmed be more consistent?

  everconfirmed isn't a separate field. Perhaps we'll update this when we have to also use it for process_bug.cgi, but for now it works all right.

> Nit: I know it was not done before, but perhaps check for max length/size of
> the field?

  It's a mediumtext--its max size is ridiculous. I don't think this is too important for now.


  I fixed everything else.

Comment 9

[Olav Vitters]

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

Works fine.

Comment 10

[Max Kanat-Alexander]

Yay! :-)

Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.170; previous revision: 1.169
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.145; previous revision: 1.144
done
Checking in Bugzilla/Object.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Object.pm,v  <--  Object.pm
new revision: 1.5; previous revision: 1.4
done