253604 - When committing a bug, errors in the validation must appear before user match.

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Tiago Mello [:timello]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a2) Gecko/20040612 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a2) Gecko/20040612 Firefox/0.8.0+

It makes more sense if errors comes before the user match.

Reproducible: Always
Steps to Reproduce:
1. Add a new cc
2. Type a invalid data in time tracking box
3. Add a comment
4. Press Commit
5. Choose the user
6. Press continue

Comment 1

[Tiago Mello [:timello]]

Attachment: it just invert the order of validation

Comment 2

[Tiago Mello [:timello]]

Attachment: it just invert the order of validation

Comment 3

[Christian Reis]

Comment on attachment 154692 [details] [diff] [review]
it just invert the order of validation

You want to do *only* validation up there. The appendcomment and SQL building
bits should stay where they are.

I recommend validation to be done right after we validate dependson/blocked,
which comes before the comment marked

# End Data/Security Validation

Comment 4

[Christian Reis]

It *is* a bug however :-)

Comment 5

[Tiago Mello [:timello]]

Attachment: all the timetracking validation comes first.

Comment 6

[Christian Reis]

Comment on attachment 154798 [details] [diff] [review]
all the timetracking validation comes first.

Looks good.

Comment 7

[Christian Reis]

Pretty trivial fix. I'd love to see this go into 2.18 as well since it makes the
error message for timetracking so much less annoying.

Comment 8

[Dave Miller [:justdave]]

Sure, there's no changes to the templates, and it's a useful fix, so I don't see
why not.

Comment 9

[Vlad Dascalu]

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

This patch does not apply cleanly to the 2_18 BRANCH, therefore I can't commit
it there. A new patch is needed :)

Comment 10

[Christian Reis]

Attachment: 2.18 backport v1

A couple of modifications.

Comment 11

[Christian Reis]

Comment on attachment 155115 [details] [diff] [review]
2.18 backport v1

Can you guys take a look at this? It's a trivial backport, but it's good to
have someone else apply and test one of your fixes for a stable branch.

Comment 12

[Christian Reis]

Okay, there's a tiny problem there: you need &::ThrowUserError (in lieu of
straight ThrowUserError) in the change in Bug.pm -- I can reattach a patch as
soon as you guys take a look at this one, to minimize attachment and flag games.

Comment 13

[Christian Reis]

Argh. We also need to push up the comment_required check to avoid that silly
message up there. (Yeah, there are a bunch of other potential errors, but ..) 

Comment 14

[Joel Peshkin]

Comment on attachment 155115 [details] [diff] [review]
2.18 backport v1

I suspect this problem is there in 2.19 as well.

invalid bug attribute ThrowUserError at Bugzilla/Bug.pm line 505
       
Bugzilla::Bug::AUTOLOAD('need_positive_number','HASH(0x86ee320)','abort')
called at Bugzilla/Bug.pm line 495
	Bugzilla::Bug::ValidateTime('0ku','work_time') called at
/home/bugzilla/bz3/process_bug.cgi line 99

Comment 15

[Joel Peshkin]

Oh, aside from that, it would be r=joel

Comment 16

[Christian Reis]

This problem isn't there in 2.19, rest assured -- we *do* test our patches (just
not in *cough* obsolete branches).

Comment 17

[Christian Reis]

Attachment: kiko_v2: fix

Comment 18

[Christian Reis]

Comment on attachment 155166 [details] [diff] [review]
kiko_v2: fix

Moving forward Joel's r+

Comment 19

[Vlad Dascalu]

Comment on attachment 155166 [details] [diff] [review]
kiko_v2: fix

sub ValidateTime { uses 2-space ident, which should be 4 space ident. Therefore
this is not commitable.

I could be a bitch and say about the $::FORM to $cgi->param replacement, but
this is 2.18 BRANCH anyway so I won't. However the identation problem must be
solved.

I'd also prefer to have 99999.99 defined as a constant somewhere and not
hard-coded in the middle of the code, but I'll shut up about that one as well.

Comment 20

[Christian Reis]

That whole file uses 2-space indent. There is an indent problem, though, so I'll
fix that before asking for approval.

Comment 21

[Vlad Dascalu]

Comment on attachment 155166 [details] [diff] [review]
kiko_v2: fix

Apparently that whole file is 2-space idented.

r=vladd, but this code is such a mess that I'm going to puke. DoComma()?
Sheesh.

Comment 22

[Christian Reis]

Attachment: kiko_v3: fix indentation

Comment 23

[Christian Reis]

Vlad: about the 99999.99, right now, it's at least kept in a single place. This
is 2.18, remember :-)

Fixed on branch: 

/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.205.2.3; previous revision: 1.205.2.2
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.37.2.2; previous revision: 1.37.2.1

Filed follow-up bug 254498 for the comment change (that was damned silly).