Last Comment Bug 818890 - Bugzilla doesn't obey the "Comment required on status transition" for {Start}-> transition (for new bugs)
: Bugzilla doesn't obey the "Comment required on status transition" for {Start}...
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Creating/Changing Bugs (show other bugs)
: 4.2.4
: All All
: -- normal with 1 vote (vote)
: Bugzilla 4.2
Assigned To: Alexander Tereschenko
: default-qa
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-06 06:43 PST by Alexander Tereschenko
Modified: 2012-12-17 14:41 PST (History)
1 user (show)
LpSolit: approval+
LpSolit: approval4.4+
LpSolit: approval4.2+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch for 4.2, v1 (561 bytes, patch)
2012-12-06 06:51 PST, Alexander Tereschenko
LpSolit: review+
Details | Diff | Splinter Review
patch for 4.4 and trunk, v1 (745 bytes, patch)
2012-12-17 14:18 PST, Alexander Tereschenko
no flags Details | Diff | Splinter Review
patch for 4.4 and trunk, v2 (719 bytes, patch)
2012-12-17 14:26 PST, Alexander Tereschenko
LpSolit: review+
Details | Diff | Splinter Review

Description Alexander Tereschenko 2012-12-06 06:43:47 PST
Reproducible: Always (on 4.2 only, 4.0 and trunk are unaffected, as far as I've tested)

Steps to Reproduce:
1) Mark the checkbox for {Start}->UNCONFIRMED (or whatever the second status is, it doesn't matter, the only thing is that it must be available for new bugs) on the Comments Required on Status Transitions config page. The idea is to enforce people to provide long description for new bugs;

2) Try to submit the bug with empty Description;

Actual Results:  
Bug is successfully created

Expected Results:  
Bugzilla should throw an error saying that I need to provide the description

Additinal info:
Transitions between states for existing bugs work as intended (throwing an error in case of an empty comment)

Potential root cause:
As far as I have found out, the reason is a combination of two things:

1) Check in Bugzilla/Bug.pm:_check_bug_status() - it tests is $comment evaluates to TRUE and throws an error if not:

1371 if ($new_status->comment_required_on_change_from($old_status) && !$comment)

2) The fact that $comment (for the case of a new bug creation) will evaluate to TRUE even if it doesn't contain any text. This is because Bugzilla/Bug.pm:_check_comment() changed since 4.0 and now $comment returned by it will contain not only the text itself, but also other key/value pairs necessary for the object creation:

1434    # Create the new comment so we can check it
1435    my $comment = {
1436        thetext  => $comment_txt,
1437        bug_when => $timestamp,
1438    };

Looks like the code in _check_bug_status() simply wasn't adjusted upon changes in _check_comment(), which seemingly happened upon 4.0->4.2 transition.

Proposed fix:
Change the mentioned check to

if ($new_status->comment_required_on_change_from($old_status) && !$comment->{'thetext'}). I'll attach a proposed patch in a second.
Comment 1 Alexander Tereschenko 2012-12-06 06:51:25 PST
Created attachment 689196 [details] [diff] [review]
patch for 4.2, v1

Setting LpSolit as reviewer, please please correct me if that should be someone else.
Comment 2 Frédéric Buclin 2012-12-16 05:59:02 PST
Comment on attachment 689196 [details] [diff] [review]
patch for 4.2, v1

This patch only applies to 4.2.x. The code changed in 4.4 in the error message right below your change. Moreover, if we apply your change manually, the error message triggered here make Bugzilla crash:

 Can't call method "name" on an undefined value at Bugzilla/Bug.pm line 1328.

The reason is that $old_status->name doesn't exist as there is no initial bug status when creating a new bug. This is a regression due to bug 622203.

So r=LpSolit for 4.2, but a new patch is needed for 4.4.
Comment 3 Frédéric Buclin 2012-12-16 06:00:17 PST
Alexander, do you have a 4.4rc1 installation to play with to give us a patch for 4.4?
Comment 4 Frédéric Buclin 2012-12-16 06:02:53 PST
Note that if JavaScript is enabled, the missing description is correctly caught. This is only a problem with JS turned off.
Comment 5 Alexander Tereschenko 2012-12-17 02:10:05 PST
Thanks for the review, Frédéric. Yes, that's a patch for 4.2, as far as I've initially tested out (both on 4.0 and 4.4rc1), that was the only version affected, just like I mentioned in the bug description.

Though I see your point about 4.4rc1 with JS off (I've tested on 4.4rc1 with JS on and that worked, so I haven't thought of trying with JS off). Sure, I'll look into that, I have the 4.4rc1 instance.
Comment 6 Alexander Tereschenko 2012-12-17 14:18:49 PST
Created attachment 693110 [details] [diff] [review]
patch for 4.4 and trunk, v1

Here's the one against 4.4, also tested to apply well to trunk.
Checked to solve the problem on both 4.4rc1 and trunk.

I've made some additional line breaks to fit into 80 symbols line width, but let me know if you want the one with less (but longer) lines.
Comment 7 Alexander Tereschenko 2012-12-17 14:26:01 PST
Created attachment 693115 [details] [diff] [review]
patch for 4.4 and trunk, v2

Oops, looks like the indentation of the ThrowUserError arguments was slightly off in v1, here's v2 which fixes that.
Comment 8 Frédéric Buclin 2012-12-17 14:32:26 PST
Comment on attachment 693115 [details] [diff] [review]
patch for 4.4 and trunk, v2

Works fine. Thanks! r=LpSolit
Comment 9 Frédéric Buclin 2012-12-17 14:41:57 PST
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Bug.pm
Committed revision 8517.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.4/
modified Bugzilla/Bug.pm
Committed revision 8483.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.2/
modified Bugzilla/Bug.pm
Committed revision 8179.

Note You need to log in before you can comment on or make changes to this bug.