Check for comment required for time validation comes too late.

RESOLVED FIXED in Bugzilla 2.18

Status

()

RESOLVED FIXED
14 years ago
6 years ago

People

(Reporter: kiko, Assigned: timello)

Tracking

unspecified
Bugzilla 2.18
Bug Flags:
approval +
approval2.18 +
blocking2.18 +

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

14 years ago
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.
(Assignee)

Comment 1

14 years ago
Created attachment 158804 [details] [diff] [review]
Bring some validation before of the user match
(Assignee)

Updated

14 years ago
Attachment #158804 - Flags: review?(kiko)
(Reporter)

Comment 2

14 years ago
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.
Attachment #158804 - Flags: review?(kiko) → review+
(Reporter)

Comment 3

14 years ago
The originating bug went into 2.18 as well.
Status: NEW → ASSIGNED
Flags: approval?
Flags: approval2.18?
(Assignee)

Comment 4

14 years ago
Created attachment 158808 [details] [diff] [review]
Simple fix.

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

Updated

14 years ago
Attachment #158804 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #158808 - Flags: review+
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.
Flags: blocking2.18+
Flags: approval?
Flags: approval2.18?
Flags: approval2.18+
Flags: approval+
Target Milestone: --- → Bugzilla 2.18
(Reporter)

Comment 6

14 years ago
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.
(Assignee)

Comment 7

14 years ago
Created attachment 159283 [details] [diff] [review]
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?
(Assignee)

Updated

14 years ago
Attachment #158808 - Attachment is obsolete: true
(Reporter)

Comment 8

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

This should do the job.
Attachment #159283 - Flags: review+
Whiteboard: patch awaiting checkin

Updated

14 years ago
OS: Linux → All
Hardware: PC → All

Comment 9

14 years ago
The patch does not apply cleanly to the 2.18 branch.

Comment 10

14 years ago
kiko, tiago, any news on this 2.18 patch that is already approved before been
written? :)
Created attachment 163000 [details] [diff] [review]
Last patch ported to branch

Works for me
Attachment #163000 - Flags: review?
(Reporter)

Comment 12

14 years ago
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.
Attachment #163000 - Flags: review? → review+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Updated

14 years ago
Whiteboard: patch awaiting checkin
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.