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

RESOLVED FIXED in Bugzilla 3.0

Status

()

Bugzilla
Creating/Changing Bugs
--
enhancement
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Max Kanat-Alexander, Assigned: Max Kanat-Alexander)

Tracking

({selenium})

2.23
Bugzilla 3.0
selenium
Dependency tree / graph
Bug Flags:
approval +
testcase +

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

12 years ago
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.
(Assignee)

Comment 1

12 years ago
Created attachment 235006 [details] [diff] [review]
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.
Assignee: create-and-change → mkanat
Status: NEW → ASSIGNED
Attachment #235006 - Flags: review?(bugzilla-mozilla)
(Assignee)

Comment 2

12 years ago
Created attachment 235007 [details] [diff] [review]
v1.1

REQUIRED_CREATE_FIELDS was missing rep_platform.
Attachment #235006 - Attachment is obsolete: true
Attachment #235007 - Flags: review?(bugzilla-mozilla)
Attachment #235006 - Flags: review?(bugzilla-mozilla)
(Assignee)

Comment 3

12 years ago
Created attachment 235009 [details] [diff] [review]
v1.2

bkor pointed out on IRC that I had left some debugging code in the last patch.
Attachment #235007 - Attachment is obsolete: true
Attachment #235009 - Flags: review?(bugzilla-mozilla)
Attachment #235007 - Flags: review?(bugzilla-mozilla)

Comment 4

12 years ago
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

12 years ago
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 {
Attachment #235009 - Flags: review?(bugzilla-mozilla) → review-
(Assignee)

Comment 6

12 years ago
Created attachment 235677 [details] [diff] [review]
v2

Okay, this can successfully file a bug on MySQL, and I fixed the bitrot.
Attachment #235009 - Attachment is obsolete: true
Attachment #235677 - Flags: review?(bugzilla-mozilla)

Comment 7

12 years ago
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?
Attachment #235677 - Flags: review?(bugzilla-mozilla) → review-
(Assignee)

Comment 8

12 years ago
Created attachment 236634 [details] [diff] [review]
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.
Attachment #235677 - Attachment is obsolete: true
Attachment #236634 - Flags: review?(bugzilla-mozilla)

Comment 9

12 years ago
Comment on attachment 236634 [details] [diff] [review]
v3

Works fine.
Attachment #236634 - Flags: review?(bugzilla-mozilla) → review+

Updated

12 years ago
Flags: approval?
Flags: approval? → approval+
(Assignee)

Comment 10

12 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Flags: testcase?

Updated

11 years ago
Flags: testcase? → testcase+

Updated

11 years ago
Keywords: selenium
You need to log in before you can comment on or make changes to this bug.