254498 - Check for comment required for time validation comes too late.

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Christian Reis]

Follow-up from bug 253604. The check for comment required for time validation
comes too late -- after the user match, which is silly since you click though
the user match only to be confronted with a user error. In certain versions of
IE, this also causes a history problem.

Comment 1

[Tiago Mello [:timello]]

Attachment: Bring some validation before of the user match

Comment 2

[Christian Reis]

Comment on attachment 158804 [details] [diff] [review]
Bring some validation before of the user match

This patch is tricky because you appear to be doing more than you are, but it's
good.

Comment 3

[Christian Reis]

The originating bug went into 2.18 as well.

Comment 4

[Tiago Mello [:timello]]

Attachment: Simple fix.

>+if (UserInGroup(Param('timetrackinggroup'))) {
>+    my $work_time = $::FORM{'work_time'};
   We don't that.

Comment 5

[Dave Miller [:justdave]]

Comment on attachment 158808 [details] [diff] [review]
Simple fix.

I didn't see any mention from kiko that you could pull the review forward with
that change, but it looks okay to me, so signing off on it.

Comment 6

[Christian Reis]

Comment on attachment 158808 [details] [diff] [review]
Simple fix.

>Index: process_bug.cgi
>+if (UserInGroup(Param('timetrackinggroup'))) {
>+    if ($::FORM{'comment'} =~ /^\s*$/ && $::FORM{'work_time'}) {

Hum, hum. I suspect this needs to be:

  my $wk_time = $::FORM{'work_time'};
  if ($wk_time && $wk_time != 0) {
     ThrowUserError('comment_required', undef, "abort");
  } 

Why? I just tested and this surprised me:

  $a = "0.0";		    
  if ($a) { print "AIIII" };
  AIIII

It appears to me that this will break things up because we will require
comments if work_time is 0.0, and we shouldn't.

Comment 7

[Tiago Mello [:timello]]

Attachment: Fix the last patch

kiko, you are certain. Take a look, here it is a interdiff.

diff -u process_bug.cgi process_bug.cgi
--- process_bug.cgi  13 Sep 2004 23:16:12 -0000
+++ process_bug.cgi  18 Sep 2004 03:22:48 -0000
@@ -101,7 +101,8 @@
 }

 if (UserInGroup(Param('timetrackinggroup'))) {
-    if ($::FORM{'comment'} =~ /^\s*$/ && $::FORM{'work_time'}) {
+    my $wk_time = $::FORM{'work_time'};
+    if ($::FORM{'comment'} =~ /^\s*$/ && $wk_time && $wk_time != 0) {
	 ThrowUserError('comment_required', undef, "abort");
     }
 }

But we still need the $::FORM{'comment'} ... I think that you forgot in your
last comment. right?

Comment 8

[Christian Reis]

Comment on attachment 159283 [details] [diff] [review]
Fix the last patch

This should do the job.

Comment 9

[Vlad Dascalu]

The patch does not apply cleanly to the 2.18 branch.

Comment 10

[Vlad Dascalu]

kiko, tiago, any news on this 2.18 patch that is already approved before been
written? :)

Comment 11

[Marc Schumann [:Wurblzap]]

Attachment: Last patch ported to branch

Works for me

Comment 12

[Christian Reis]

Comment on attachment 163000 [details] [diff] [review]
Last patch ported to branch

>+if (UserInGroup(Param('timetrackinggroup'))) {
>+    my $wk_time = $::FORM{'work_time'};
>+    if ($::FORM{'comment'} =~ /^\s*$/ && $wk_time && $wk_time != 0) {
>+        ThrowUserError('comment_required', undef, "abort");

Interesting that this bit is really a pre-catch and we ensure this is so by
raising a CodeError in AppendComment. It's correct -- to the gist of this patch
anyway.

Comment 13

[Dave Miller [:justdave]]

checked in on trunk:

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.214; previous revision: 1.213
done

And 2.18 branch:

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.205.2.4; previous revision: 1.205.2.3
done