1156945 - [gui] error checking for the regression range

Status: RESOLVED FIXED

(Testing :: mozregression, defect)

Description

[Julien Pagès (:parkouss)]

We don't check values in the GUI wizard fileds for the dates, and we should.

If "Search fix" is not checked, good date must be < bad date, and this is the opposite if it is checked.

Comment 1

[MikeLing]

Hello~ Julien, I notice something could be add into this error checking function. I think the date user input couldn't bigger than our current date because is impossible. :)

Comment 2

[Julien Pagès (:parkouss)]

(In reply to MikeLing from comment #1)
> Hello~ Julien, I notice something could be add into this error checking
> function. I think the date user input couldn't bigger than our current date
> because is impossible. :)

Sure, you're right! Good catch. :)

Comment 3

[Julien Pagès (:parkouss)]

Hey MikeLing,

In fact you can start to work on this bug, if you'd like.

Do not take in account what I said in comment 0. :)

We should just ensure here that the start_date field is always < to end_date field, and say it to the user if it is not the case (Do not take in account the "Search fix"). That's all.

To validate a field, you can use for example

http://doc.qt.io/qt-4.8/qwizardpage.html#validatePage
or
http://doc.qt.io/qt-4.8/qwizardpage.html#isComplete

For now, we have no standard way of validating a user entry. It could be a message dialog for example (fine for me).
The requirements are that the user must understand the error and that he won't be able to go to the next screen until he fixed it.

Comment 4

[MikeLing]

I see, I will commit first patch ASAP: )

Comment 5

[MikeLing]

(In reply to Julien Pagès from comment #3)

Hi Julien, I will commit a patch for this bug. Should I rebase my commit like tomorrow or git push it directly? ( I wonder it because they are different bug, so if it's necessary to rebase it into last issue. :)

Comment 6

[MikeLing]

:) I had create a pull request, forget the Comment 5. :D

Comment 7

[Julien Pagès (:parkouss)]

Hey Mikeling,

Once you created the PR (as you just did) and that you're happy with it, please ask for a review like we did last time. This way it gets on my reviews list and I can manage it easily.

I just looked at your PR anyway (https://github.com/mozilla/mozregression/pull/212), and I saw that you checked for inbound - this can not be done because inbounds are not dates, we have mercurial changesets.
We have no way here to check if one changeset is before another, so you can simply remove the validate method for the inbound class. :) If you look at the inbound editors, there are simple line edit, not date editors.

I'm pleased to see how it goes!

Comment 8

[MikeLing]

Attachment: fix error checking for the regression range

It's a patch after remove the validate method for the inbound class. :D

Comment 9

[Julien Pagès (:parkouss)]

Comment on attachment 8597602 [details] [review]
fix error checking for the regression range

Thanks a lot MikeLing, this is quite good!

I had hesitations, but finally I put a r- because of the use of good/bad in variables (which would be complex to understand, and we are trying to avoid that in the other bug). So I would not take the patch as it in mozregression - and this is the reason of the r-.

Still no worries, this is a great work! Please just fix this, and the others minor things I noted on the PR, then it will be all good to me. :)

Comment 10

[MikeLing]

Attachment: Second version

Comment 11

[Julien Pagès (:parkouss)]

Comment on attachment 8597903 [details] [review]
Second version

Looks all good to me!

Thanks :)

Comment 12

[Julien Pagès (:parkouss)]

Merged: https://github.com/mozilla/mozregression/commit/b8511e680200491ffbe124177777aab73e65d4fa

Comment 13

[MikeLing]

(In reply to Julien Pagès from comment #11)

You are welcome :)

I will let you know as soon as I fix the other one.