848567 - Shipit should validate branch/revisions as you enter them

Status: RESOLVED FIXED

(Release Engineering :: Applications: Shipit, defect, P3)

Description

[Chris AtLee [:catlee]]

When I enter in a revision to ship-it, I'd love it if it could do a query in the background to make sure that the repo/revision exists, and provide a link to it if it does.

If it doesn't exist, it should complain in large blinking red letters.

Comment 1

[hwine]

An alternate version of this is to just validate that what is entered _could_ be a revision. 

I lost track of what was on my clipboard, pasted a URL into the field, and didn't notice until I was reviewing it.

Comment 2

[Justin Wood (:Callek)]

(In reply to Hal Wine [:hwine] from comment #1)
> An alternate version of this is to just validate that what is entered
> _could_ be a revision. 
> 
> I lost track of what was on my clipboard, pasted a URL into the field, and
> didn't notice until I was reviewing it.

Which boils down to "don't use ':'" since just about anything can be a tag, and a valid revision includes tags:

Justin@ORION /d/Sources/temp-repo
$ hg tag "http\:\/\/hg\.mozilla\.org\/releases\/mozilla\-release\/FIREFOX_24_0_1_RELEASE"
abort: ':' cannot be used in a tag name

Justin@ORION /d/Sources/temp-repo
$ hg tag "http\/\/hg\.mozilla\.org\/releases\/mozilla\-release\/FIREFOX_24_0_1_RELEASE"

Comment 3

[hwine]

Clarification -- by "could be" I meant 12 or 40 hex digits.

I can't think of a use case for tags in shipit, afaik our practice has always been to use hashes.

Comment 4

[Justin Wood (:Callek)]

(In reply to Hal Wine [:hwine] from comment #3)
> Clarification -- by "could be" I meant 12 or 40 hex digits.
> 
> I can't think of a use case for tags in shipit, afaik our practice has
> always been to use hashes.

SeaMonkey does this with "FIREFOX_XX_BUILD1" etc., and I thought TB did until I just looked in ship-it and saw it doesn't.

However I do see at least one case of using branch names: GECKO210_2013050704_RELBRANCH (Firefox 21.0 build2)

Comment 5

[Lawrence Mandel [:lmandel] (use needinfo)]

Ship-it now auto-populates branch and revision. I don't know if there is any validation for manual entries. I think sanity checks like this are a good idea. We should probably have a way to override in case the system breaks for some reason.

Comment 6

[Massimo Gervasini [:massimo]]

Attachment: Bug 848567 - Shipit should validate branch, revisions as you enter them.patch

First version, probably needs some clean up for some use cases.

This patch adds javascript validation for branch/revision. The validations is performed against:

* https://hg.mozilla.org/<branch> for the branch
* https://hg.mozilla.org/<branch>/rev/<revision> for branch + revision

+ if the branch/revision are ok, the submit button is forced to enabled

+ if the one of the branch/revision fields are not valid, the input becomes red-bold and the submit button is disabled.

+ the submit button is re-enable deleting branch/revision or set them to a valid value.
the use case where we have an empty branch/revision is already managed by the backend

+ the validation is performed only when we leave the input field (blur event): so if the user enters a wrong revision and hits submit without leaving the revision field, the check is not performed.

+ at the moment, this validation is available only for fennec/firefo

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Kudo for this change!

Comment 8

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8506224 [details] [diff] [review]
Bug 848567 - Shipit should validate branch, revisions as you enter them.patch

Review of attachment 8506224 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the great notes on the patch. The behaviour with blur() is a bit suboptimal, say if I modify revision then don't focus elsewhere, and wonder why the button is still disabled. Maybe onKeyUp on the input box instead ? I'm not really a js guy.

::: kickoff/static/kickoff.js
@@ +127,5 @@
> +    // checks if  branch + revision exist on the hg server
> +    "use strict"
> +    var branch = $( '#' + product + '-branch' ).val();
> +    var revision = $( '#' + product + '-mozillaRevision' ).val();
> +    var valid = true;

unused ?

@@ +143,5 @@
> +            },
> +        });
> +    } else {
> +        // branch/revision is not set, flask will take care of this use case
> +        enableSubmitButton(product);

What if we have the submit button disabled on page load, then only enable it when branch and revision are valid ? 

Bigger picture, as we go along we could add validation for other parameters (eg relbranch), which would need a general mechanism for marking values as 'ok', and button state that depends on all of them.

Comment 9

[Massimo Gervasini [:massimo]]

Attachment: Bug 848567 - Shipit should validate branch, revisions as you enter them.patch

Thanks for the feedback, Nick.

here's a new patch changes from last version:

... for the user:
* the submit button is disabled by default. If branch and revision are ok, the button enabled again.
* using keyup() instead of blur()

... in the code.
* removed unused code 
* using the new functions, isValid/isNotValid/checkErrors *should* be easier to extend other checks on different input fields. 


Open problems:

* if the user updates the version (e.g from 40.0b1 to 40.0), the branch changes automatically but no checks are performed. Working on this.

* In theory, if the user enters a good revision and replaces it with a bad one, there are a couple of seconds where the submit button is still active. Do we have another hg url we can use to test if a revision exists?

* Do we need to extend the same mechanism to Thunderbird branch/revision/comm-branch?

Comment 10

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8508139 [details] [diff] [review]
Bug 848567 - Shipit should validate branch, revisions as you enter them.patch

Review of attachment 8508139 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay here. I had a play around with this in a local instance and it works pretty well for the case where you're pasting in a revision from tbpl. Does a lot of requests if you're typing it in, but hopefully that's not an actual use case. It can also swallow keystrokes if you're typing, while the XHR is going on. Almost need to wait for the user to stop typing, then make a query. Alternatively the checks could be done in ReleaseForm.validate() on the server side (assuming it can see hg.m.o). I don't know of any faster way to talk to the hg server.

The submit button was enabled early in one check I made - Firefox release, enter Version, tab to Build and enter, tab to Branch and it's enabled early. Also can't tab-tab to get from Build Number to Mozilla Revision, the check on Branch blocks the second tab. Clicking on Mozilla Revision seems ok though.

The enabled/disabled status of the Submit button are quite hard to tell apart, a little css magic would be good.

We should try to extend to include Thunderbird, and by extension Seamonkey (their own instance), if it's not too complicated.

::: kickoff/static/kickoff.js
@@ +113,5 @@
> +            success: function(data) {
> +                isValid(product, '-branch');
> +                // branch is ok, let's check the revision
> +                checkRevision(product);
> +                // checkErrors calls checkErrors, no need to call it here.

1st of these checkErrors should be checkRevision, by the looks.

Comment 11

[[e-mail] (:bhearsum)]

Mass component change for ship it bugs.

Comment 12

[Rail Aliiev [:rail]]

Implemented in the new UI