Closed
Bug 305136
(bz-postbugpm)
Opened 19 years ago
Closed 18 years ago
Make Bug.pm able to create bugs--move most post_bug.cgi code into Bug.pm
Categories
(Bugzilla :: Creating/Changing Bugs, enhancement, P2)
Tracking
()
RESOLVED
FIXED
Bugzilla 3.0
People
(Reporter: batosti, Assigned: mkanat)
References
Details
(Keywords: meta)
Attachments
(1 file, 3 obsolete files)
15.64 KB,
patch
|
mkanat
:
review-
|
Details | Diff | Splinter Review |
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•19 years ago
|
Blocks: 174988
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: PC → All
Version: unspecified → 2.21
Reporter | ||
Comment 1•19 years ago
|
||
moving the code and changing the older SendSQL functions to dbh functions.
Reporter | ||
Updated•19 years ago
|
Attachment #194381 -
Flags: review?
Assignee | ||
Updated•19 years ago
|
Assignee: create-and-change → batosti
Assignee | ||
Comment 2•19 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•19 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•19 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•19 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•19 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•19 years ago
|
||
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•19 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•19 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•19 years ago
|
||
no $cgi to the Bug functions and with POD docs.
Attachment #194682 -
Attachment is obsolete: true
Attachment #194939 -
Flags: review?(LpSolit)
Reporter | ||
Updated•19 years ago
|
Attachment #194939 -
Flags: review?(mkanat)
Updated•19 years ago
|
Attachment #194939 -
Flags: review?(LpSolit)
Reporter | ||
Updated•19 years ago
|
Assignee | ||
Comment 11•19 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•19 years ago
|
||
Attachment #194939 -
Attachment is obsolete: true
Attachment #203533 -
Flags: review?
Assignee | ||
Comment 13•19 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•19 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•19 years ago
|
||
*** Bug 306624 has been marked as a duplicate of this bug. ***
No longer depends on: 306624
Assignee | ||
Updated•19 years ago
|
Depends on: bz-bugwrite
Assignee | ||
Updated•19 years ago
|
Blocks: bz-bugwrite
Assignee | ||
Updated•19 years ago
|
Alias: bz-postbugpm
Assignee | ||
Comment 15•19 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 | ||
Comment 16•18 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•18 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
Comment 18•18 years ago
|
||
Done! The next step is process_bug.cgi. :)
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Updated•18 years ago
|
Blocks: bz-roadmap
You need to log in
before you can comment on or make changes to this bug.
Description
•