Deleting Test connection button and changing Api Keys in UI to Access Keys #2
Conversation
| <f:textbox/> | ||
| </f:entry> | ||
| <f:validateButton title="${%Test Connection}" with="username,apiKey" method="validate"/> |
There was a problem hiding this comment.
now that the button is gone, can you delete the associated function as well?
(I think its called doValidate)
There was a problem hiding this comment.
I still think it is used for automatic validation
There was a problem hiding this comment.
you are awesome @halkeye! removed some code
|
I think doCheckApiKey is used for checking if it works, I think doValidate is no longer used because the button is removed. You should be able to check really easily though. |
| public FormValidation doValidate(@QueryParameter String username, @QueryParameter String apiKey) { | ||
| if (actualValidate(username, apiKey)) | ||
| SauceREST rest = new JenkinsSauceREST(username, value); | ||
| if (!rest.getUser().equals("")) { |
There was a problem hiding this comment.
Personal style here, I find that ! equals("") confusing to read.
I think with java 7, its safe to compare strings using operators, so do you want to try rest.getUser() != "" which might be clearer. Otherwise do you want to make it its own simple function called "isValidCredentials" or something so the conditional is easier to read?
There was a problem hiding this comment.
I agree, i was thinking to refactor some more to actually check 401 unauthorized, as that would be the actual correct way to deal with this.
|
Thats actually one of the ugly features in saucerest. Any error returns a blank string. saucerest isn't likely to be fixed any time soon. |
|
Made it the other way so it is more readable and also added a comment |
|
I think in this case its fine because sauceRest is crazy, but generally comments about statements are considered a code smell so its something to watch out for. |
|
I totally agree 👍 |
|
yea, looks like everything passes, so assuming everyone else is cool, |
ECO-1169
@halkeye please review