Move comment creation out of post_bug.cgi and into Bugzilla::Bug

RESOLVED FIXED in Bugzilla 3.0

Status

()

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

People

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

Tracking

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

Details

Attachments

(1 attachment, 2 obsolete attachments)

v2
9.62 KB, patch
Frédéric Buclin
: review+
Details | Diff | Splinter Review
(Assignee)

Description

12 years ago
This is the final step of Bug->create--have it insert the comment into the longdescs table.

This will also include any cleanup code for the completion of Bug->create.
(Assignee)

Comment 1

12 years ago
Created attachment 237422 [details] [diff] [review]
v1

Okay. This puts the finishing touches on Bug->create.
Assignee: create-and-change → mkanat
Status: NEW → ASSIGNED
Attachment #237422 - Flags: review?(bugzilla-mozilla)

Comment 2

12 years ago
Comment on attachment 237422 [details] [diff] [review]
v1

You shouldn't lock tables from a Perl module. You have no idea from where create() will be called. And if there is already a lock running, you will crash. That's a sufficient reason to deny review, IMO.
(Assignee)

Comment 3

12 years ago
(In reply to comment #2)
> (From update of attachment 237422 [details] [diff] [review] [edit])
> You shouldn't lock tables from a Perl module. You have no idea from where
> create() will be called. And if there is already a lock running, you will
> crash. That's a sufficient reason to deny review, IMO.

  Nah, I disagree. I think it's fine to lock tables inside of create(). Just like we'd lock tables inside a remove_from_db or something like that.
(Assignee)

Comment 4

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

Fixed bitrot.
Attachment #237422 - Attachment is obsolete: true
Attachment #237564 - Flags: review?(LpSolit)
Attachment #237422 - Flags: review?(bugzilla-mozilla)

Comment 5

12 years ago
Comment on attachment 237564 [details] [diff] [review]
v1.1

>diff -u post_bug.cgi post_bug.cgi

>-$template->process($format->{'template'}, $vars, \$comment)
>-  || ThrowTemplateError($template->error());

>+$template->process($format->{'template'}, $vars, \$comment);

Do not remove ThrowTemplateError() as we have to throw an error if this template is missing.



>diff -u Bugzilla/Bug.pm Bugzilla/Bug.pm

>+    # XXX Totally empty comments suppress email.
>+    my ($comment, $privacy) = ($params->{comment} || ' ', 
>+                               $params->{commentprivacy});

Do not fall back to ' ' here. This is the job of _check_comment() to return the correct string.


> sub _check_comment {

>+    $comment ||= '';

'0' is a valid comment and shouldn't be replaced by an empty string. Write:
$comment = (defined $comment && $comment ne '') ? $comment : ' ';

This way, you avoid both undefined comments and empty strings.


The rest of the patch looks good and my initial test was successful.
Attachment #237564 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 6

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

Okay, I fixed all your comments.
Attachment #237564 - Attachment is obsolete: true
Attachment #238019 - Flags: review?(LpSolit)

Comment 7

12 years ago
Comment on attachment 238019 [details] [diff] [review]
v2

I opened bug 352695, bug 352699 and bug 352702 to catch regressions/bugs I found while testing the new Bugzilla::Bug->create(). This patch is working fine though, r=LpSolit
Attachment #238019 - Flags: review?(LpSolit) → review+

Updated

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

Comment 8

12 years ago
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.179; previous revision: 1.178
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.155; previous revision: 1.154
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.73; previous revision: 1.72
done
Checking in Bugzilla/Install/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v  <--  DB.pm
new revision: 1.20; previous revision: 1.19
done
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.