Bug 305136 (bz-postbugpm)

Make Bug.pm able to create bugs--move most post_bug.cgi code into Bug.pm

RESOLVED FIXED in Bugzilla 3.0

Status

()

Bugzilla
Creating/Changing Bugs
P2
enhancement
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: André Batosti, Assigned: Max Kanat-Alexander)

Tracking

({meta})

2.21
Bugzilla 3.0
Dependency tree / graph

Details

Attachments

(1 attachment, 3 obsolete attachments)

15.64 KB, patch
Max Kanat-Alexander
: review-
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050725 Firefox/1.0.6 (Ubuntu package 1.0.6)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050725 Firefox/1.0.6 (Ubuntu package 1.0.6)

move the code of the post_bug.cgi to Bugzilla::Bug.pm to clean the post bug and
in the future combine it on process_bug.cgi. this bug must be a blocker of bug
174988.

Reproducible: Always
(Assignee)

Updated

13 years ago
Blocks: 174988
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: PC → All
Version: unspecified → 2.21
(Reporter)

Comment 1

13 years ago
Created attachment 194381 [details] [diff] [review]
batosti_v1

moving the code and changing the older SendSQL functions to dbh functions.
(Reporter)

Updated

13 years ago
Attachment #194381 - Flags: review?
(Assignee)

Updated

13 years ago
Assignee: create-and-change → batosti
(Assignee)

Comment 2

13 years ago
Comment on attachment 194381 [details] [diff] [review]
batosti_v1

Before I review this -- could you explain why it's OK to change the order of
things in post_bug? That is, you remove code in one place, and add back similar
code in another place.

But in general, just looking at the patch, this patch looks great! :-)
(Reporter)

Comment 3

13 years ago
it's a part of bug 174988 to join bugs functions in Bug.pm and clean up post_bug
and process_bug and then use the this functions in the future.
(Assignee)

Comment 4

13 years ago
(In reply to comment #3)
> it's a part of bug 174988 to join bugs functions in Bug.pm and clean up post_bug
> and process_bug and then use the this functions in the future.

  Yes, I know that. My question is:

  Why did you change the *order* of the code?
(Reporter)

Comment 5

13 years ago
(In reply to comment #4)
>   Why did you change the *order* of the code?

some new methods of Bug.pm only can be called if you have a bug object and i
change the code to be called after the object is created.

Comment 6

13 years ago
Comment on attachment 194381 [details] [diff] [review]
batosti_v1

>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',

>-# Add the group restrictions
>-foreach my $grouptoadd (@groupstoadd) {
>-    SendSQL("INSERT INTO bug_group_map (bug_id, group_id)
>-             VALUES ($id, $grouptoadd)");
>-}

> # All fields related to the newly created bug are set.
> # The bug can now be made accessible.

>+# Groups
>+my @groupstoadd = ();
>+foreach my $b (grep(/^bit-\d*$/, $cgi->param())) {
>+    if ($cgi->param($b)) {
>+        my $v = substr($b, 4);
>+        push @groupstoadd, $v;
>+    }
>+}
>+
>+$bug->add_groups(@groupstoadd);


The order of the different checks/validations/insertions into the DB in
post_bug.cgi is important and I ask you to respect it. Two important things
about your patch:

1) *all* validations must have been done *before* inserting the bug into the
DB. You moved such validations after the insertion of the bug for at least
groups, dependencies and users on the CC list, which all contain a
ThrowUserError(), meaning that the implementation of the bug into the DB may be
interrupted before being complete.

2) the part of your patch which I show here has security implications.
Actually, groups are validated *before* the bug is being inserted into the DB
and required group restrictions are inserted into the bug_group_map table
*before* the bug is being *unlocked*, see the security bug 292544! Here, you
move these security checks after the bug is being released, making the bug
publicly accessible!
Attachment #194381 - Flags: review? → review-
(Reporter)

Comment 7

13 years ago
Created attachment 194682 [details] [diff] [review]
batosti_v1_fix

i splited the functions of add groups and keywords in two. one for checking
before put bug in DB and the add function after have a bug.
Attachment #194381 - Attachment is obsolete: true
Attachment #194682 - Flags: review?(LpSolit)

Comment 8

13 years ago
Comment on attachment 194682 [details] [diff] [review]
batosti_v1_fix


I'm just reviewing the Bug.pm additions for now, but there are a couple of
fixes needed.

All of these routines need POD documents




>+    my $dbh = Bugzilla->dbh;
>+
>+    foreach my $person (@add_cc) {
>+        if (lsearch($self->cc, $person)<0) {
>+            my $ccid = &::DBNameToIdAndCheck($person);

There is a user_to_id function in User.pm for this.

>+
>+sub check_groups {
>+    my ($user, $product_id, $cgi) = (@_);
>+
>+    my $dbh = Bugzilla->dbh;
>+
>+    my @groups = ();
>+    foreach my $b (grep(/^bit-\d*$/, $cgi->param())) {
>+        if ($cgi->param($b)) {
>+            my $v = substr($b, 4);
>+            push @groups, $v;
>+        }

Let's not pass cgi objects to Bug.pm or expect form names to be part of the
interface.  How about passing @groups to check_groups instead?
Attachment #194682 - Flags: review-

Comment 9

13 years ago
Comment on attachment 194682 [details] [diff] [review]
batosti_v1_fix

I'll wait for an updated patch
Attachment #194682 - Flags: review?(LpSolit)
(Reporter)

Comment 10

13 years ago
Created attachment 194939 [details] [diff] [review]
batosti_v2

no $cgi to the Bug functions and with POD docs.
Attachment #194682 - Attachment is obsolete: true
Attachment #194939 - Flags: review?(LpSolit)
(Reporter)

Updated

13 years ago
Attachment #194939 - Flags: review?(mkanat)

Updated

13 years ago
Attachment #194939 - Flags: review?(LpSolit)

Updated

13 years ago
Blocks: 306624
(Reporter)

Updated

13 years ago
No longer blocks: 306624
Depends on: 306624
(Assignee)

Comment 11

13 years ago
Comment on attachment 194939 [details] [diff] [review]
batosti_v2

  OK, I don't know enough about post_bug to review that part, but I have some comments on the Bug.pm part.

>Index: Bugzilla/Bug.pm
>@@ -687,6 +689,84 @@ sub choices {
>     return $self->{'choices'};
> }
> 
>+sub add_cc_list {
>+    my $self = shift;
>+    my (@add_cc) = @_;

  I think the functions should take a *reference* to an array instead of taking the array directly. That way we can add more params later if we want. That goes for all the functions.

>+    return if !@add_cc;

  This isn't really necessary, because the function doesn't do anything if @add_cc is empty...

>+    my $dbh = Bugzilla->dbh;
>+
>+    foreach my $person (@add_cc) {
>+        if (lsearch($self->cc, $person)<0) {

  How is $self->cc going to work when the bug doesn't even exist yet? Or does the bug somehow already exist?

>+            my $ccid = login_to_id($person);
>+            if ($ccid) {

  That looks like it'll just hide errors -- any $person passed into this function should always be a valid $person, since we will have already done the user-matching, right?

>+        $dbh->do("INSERT INTO bug_group_map (bug_id, group_id)
>+                 VALUES (?, ?)", undef, $self->{'bug_id'}, $group);

  Isn't there a $self->bug_id instead of {'bug_id'}? Or $self->id or something?

>+sub add_keywords {
>+    my $self = shift;
>+    my @keywords = (@_); 
>+
>+    my $dbh = Bugzilla->dbh;
>+
>+    if (@keywords) {
>+        foreach my $keyword (@keywords) {
>+            $dbh->do("INSERT INTO keywords (bug_id, keywordid) 
>+                     VALUES ( ?, ?)", undef, $self->{'bug_id'}, 
>+                     $keyword);
>+        }

  This is going to throw taint errors all over the place. That's probably true of all these functions, actually.

>+sub check_groups {
>+    my ($user, $product_id, $groups) = (@_);
>+
>+    my $dbh = Bugzilla->dbh;
>+
>+    my $sth = $dbh->prepare("SELECT DISTINCT groups.id, 
>+                                             groups.name,
>+                                             membercontrol, 
>+                                             othercontrol, 
>+                                             description
>+                            FROM groups 
>+                            LEFT JOIN group_control_map 
>+                            ON group_id = id AND 
>+                               product_id = ?
>+                            WHERE isbuggroup != 0 AND 
>+                                  isactive != 0 
>+                            ORDER BY description");

  Isn't there some Group.pm function that will do that for you??

>+    $sth->execute($product_id);
>+    while (my ($id, $groupname, $membercontrol, $othercontrol) =
>+           $sth->fetchrow_array) {
>+        $membercontrol ||= 0;
>+        $othercontrol ||= 0;
>+        # Add groups required
>+        if (($membercontrol == CONTROLMAPMANDATORY)
>+           || (($othercontrol == CONTROLMAPMANDATORY) 
>+                && (!UserInGroup($groupname)))) {
>+            # User had no option, bug needs to be in this group.
>+            push(@$groups, $id)
>+        }
>+    }

  This whole section needs some comments, I have almost no idea what's going on.


  Otherwise, I have no general architectural problems with this. I think LpSolit would probably have better comments on it than I do, since he probably knows more about post_bug.

  Your POD docs need to describe *why* the check_ functions return arrays.
Attachment #194939 - Flags: review?(mkanat) → review-
(Reporter)

Comment 12

13 years ago
Created attachment 203533 [details] [diff] [review]
batosti_v3
Attachment #194939 - Attachment is obsolete: true
Attachment #203533 - Flags: review?
(Assignee)

Comment 13

13 years ago
This bug needs to be split into multiple bugs, one for each function that we're moving. It's too overwhelming to review the whole patch at once.
(Assignee)

Updated

13 years ago
Priority: -- → P2
Summary: move the code of post_bug.cgi to the Bug.pm → Make Bug.pm able to create bugs--move most post_bug.cgi code into Bug.pm
Target Milestone: --- → Bugzilla 2.24
(Assignee)

Comment 14

13 years ago
*** Bug 306624 has been marked as a duplicate of this bug. ***
No longer depends on: 306624
(Assignee)

Updated

13 years ago
Depends on: 122922
(Assignee)

Updated

13 years ago
Blocks: 122922
Depends on: 283926
No longer depends on: 122922
(Assignee)

Updated

13 years ago
Alias: bz-postbugpm
(Assignee)

Updated

13 years ago
Depends on: 323239
(Assignee)

Updated

13 years ago
Depends on: 323240
(Assignee)

Updated

13 years ago
Depends on: 323241
(Assignee)

Updated

13 years ago
Depends on: 323243
(Assignee)

Updated

13 years ago
Depends on: 323244
(Assignee)

Comment 15

13 years ago
Comment on attachment 203533 [details] [diff] [review]
batosti_v3

Okay, this needs to be split into the bugs that are now blocking this bug.

Also, that check_groups function does too many things. Some of its functions should be in Bugzilla::Group, and it shouldn't have to take an $user argument -- the Bugzilla::Bug already has a built-in $self->user.
Attachment #203533 - Flags: review? → review-
(Assignee)

Updated

12 years ago
Keywords: meta
(Assignee)

Comment 16

12 years ago
I'm actually going to take this over. I have some ideas as to how to do it, and I want it to get in for Bugzilla 3.0.
Assignee: batosti → mkanat
(Assignee)

Comment 17

12 years ago
Okay, here's how we're going to do this, actually:

1) We're going to use Bugzilla::Object->create.

2) We're going to implement the validators one at a time, in the blocking
   bugs, by moving code from post_bug into Bugzilla::Bug.

3) Once all the validators are implemented, we'll change all the DB-updating 
   code from post_bug into a call to Bugzilla::Bug->create.
Depends on: 339386
(Assignee)

Updated

12 years ago
Depends on: 348477
(Assignee)

Updated

12 years ago
Depends on: 348537
(Assignee)

Updated

12 years ago
Depends on: 348539
(Assignee)

Updated

12 years ago
Depends on: 348542
(Assignee)

Updated

12 years ago
Depends on: 349555
(Assignee)

Updated

12 years ago
Depends on: 349558
(Assignee)

Updated

12 years ago
Depends on: 349561
(Assignee)

Updated

12 years ago
Depends on: 349741
(Assignee)

Updated

12 years ago
Depends on: 351312
(Assignee)

Updated

12 years ago
Depends on: 351332
(Assignee)

Updated

12 years ago
Depends on: 351345
(Assignee)

Updated

12 years ago
Depends on: 351877
(Assignee)

Updated

12 years ago
Depends on: 351888

Updated

12 years ago
No longer depends on: 283926

Comment 18

12 years ago
Done! The next step is process_bug.cgi. :)
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
No longer blocks: 174988

Updated

12 years ago
Blocks: 330700

Updated

12 years ago
Blocks: 335437
You need to log in before you can comment on or make changes to this bug.