Closed
Bug 254498
Opened 20 years ago
Closed 20 years ago
Check for comment required for time validation comes too late.
Categories
(Bugzilla :: Creating/Changing Bugs, defect)
Bugzilla
Creating/Changing Bugs
Tracking
()
RESOLVED
FIXED
Bugzilla 2.18
People
(Reporter: kiko, Assigned: timello)
Details
Attachments
(2 files, 2 obsolete files)
4.24 KB,
patch
|
kiko
:
review+
|
Details | Diff | Splinter Review |
4.83 KB,
patch
|
kiko
:
review+
|
Details | Diff | Splinter Review |
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•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #158804 -
Flags: review?(kiko)
Reporter | ||
Comment 2•20 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•20 years ago
|
||
The originating bug went into 2.18 as well.
Status: NEW → ASSIGNED
Flags: approval?
Flags: approval2.18?
Assignee | ||
Comment 4•20 years ago
|
||
>+if (UserInGroup(Param('timetrackinggroup'))) {
>+ my $work_time = $::FORM{'work_time'};
We don't that.
Assignee | ||
Updated•20 years ago
|
Attachment #158804 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #158808 -
Flags: review+
Comment 5•20 years ago
|
||
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.
Updated•20 years ago
|
Flags: blocking2.18+
Flags: approval?
Flags: approval2.18?
Flags: approval2.18+
Flags: approval+
Target Milestone: --- → Bugzilla 2.18
Reporter | ||
Comment 6•20 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•20 years ago
|
||
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•20 years ago
|
Attachment #158808 -
Attachment is obsolete: true
Reporter | ||
Comment 8•20 years ago
|
||
Comment on attachment 159283 [details] [diff] [review] Fix the last patch This should do the job.
Attachment #159283 -
Flags: review+
Updated•20 years ago
|
Whiteboard: patch awaiting checkin
Updated•20 years ago
|
OS: Linux → All
Hardware: PC → All
Comment 9•20 years ago
|
||
The patch does not apply cleanly to the 2.18 branch.
Comment 10•20 years ago
|
||
kiko, tiago, any news on this 2.18 patch that is already approved before been written? :)
Comment 11•20 years ago
|
||
Works for me
Updated•20 years ago
|
Attachment #163000 -
Flags: review?
Reporter | ||
Comment 12•20 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+
Comment 13•20 years ago
|
||
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
Closed: 20 years ago
Resolution: --- → FIXED
Updated•20 years ago
|
Whiteboard: patch awaiting checkin
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•