818890 - Bugzilla doesn't obey the "Comment required on status transition" for {Start}-> transition (for new bugs)

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Alexander Tereschenko]

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]

Attachment: 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]

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]

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]

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]

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]

Attachment: 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]

Attachment: 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]

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]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Bug.pm
Committed revision 8517.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.4/
modified Bugzilla/Bug.pm
Committed revision 8483.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/
modified Bugzilla/Bug.pm
Committed revision 8179.