Submit prints out api error messages

[Smarticles101]

Closes #768

I mostly copied this after the code similar to it in cmd/download. It should print out any error message the api returns.

I haven't implemented tests, but I have some thoughts on it. fakeSubmitServer should test for some case where the api should return an error message (like when a duplicate submission happens) and then the test should check that runSubmit returns an error. Not sure how to do this though. I'm a go noob 😛

[jdsutherland]

@Smarticles101 do you intend to continue working on this?

[kytrinyx]

fakeSubmitServer should test for some case where the api should return an error message (like when a duplicate submission happens) and then the test should check that runSubmit returns an error.

I would recommend not using fakeSubmitServer first, just writing all the test code inside your test to get it working. Then we can talk about how to refactor afterwards.

You can set up a mock server and set the status for the response by adding a header (w.WriteHeader(http.StatusInternalServerError)), and also write the JSON response the same as the other mock servers.

[Smarticles101]

@jdsutherland yeah, just a bit busy recently.

Thanks for the comments @kytrinyx, I should have some time today or tomorrow to try and throw a test together

[Smarticles101]

I implemented a test that whatever error message is returned by the server gets printed.

I struggled with some unsupported protocol scheme error for a while that was caused by not having this v.Set("apibaseurl", errts.URL).

For right now is it necessary that we even use fakeSubmitServer? The server I implemented, simple as it is, is unique to the one test. Would refactoring add value for other tests?

[jdsutherland]

Yeah, v.Set("apibaseurl", errts.URL) is required to make runSubmit hit the httptest.Server (rather than the default endpoint).

I don't think fakeSubmitServer is necessary because the CLI doesn't need to concern itself with the API implementation details - the CLI just needs to deal with the response.

[kytrinyx]

I don't think fakeSubmitServer is necessary because the CLI doesn't need to concern itself with the API implementation details - the CLI just needs to deal with the response.

Agreed.

My comment above was more to make sure that you didn't try to shoehorn your thing into fakeSubmitServer than to suggest that eventually we should use it. Sorry about the confusion!

[jdsutherland]

I've been refactoring submit and noticed something I think might be worth pointing out. In thinking about validation for the ID used in the request, if the ID (and resulting URL) is bad and 404's, in master we get output saying submit was successful (even though 404) and with these changes we'd get an error trying to decode the response.

We might want to handle 404's so that we get more appropriate error messages. I'm not sure how likely a bad ID is (I can think of programmer error or someone mangling the metadata file) but we might avoid potential future headache.

[kytrinyx]

if the ID (and resulting URL) is bad and 404's, in master we get output saying submit was successful (even though 404) and with these changes we'd get an error trying to decode the response.

Ouch, yes this is not good at all. @jdsutherland would you mind opening an issue for this?

[kytrinyx]

Other than a couple of small comments with respect to variable naming, this is looking ready to go!

(I'm sorry it took so long to get to this @Smarticles101. It's been a rough few months).

[kytrinyx]

We only have one test server in this test, right? If so, can we name the variable ts for consistency with the other tests? By giving it a different name I find myself reading everything more carefully to see in what way it is special or what the other servers in the test are called to require it to be special.

[kytrinyx]

Let's give this a prefix that reflects the purpose of the test. Maybe something like submit-error

[Smarticles101]

@kytrinyx thanks for reviewing this and getting some of the other pull requests closed out.

I’ll commit those changes when I get a chance in the upcoming days

[Smarticles101]

This should be ready for review now, I've resolved your comments @kytrinyx, resolved merge conflicts and then squashed the changes into one commit. :)

[kytrinyx]

Awesome, @Smarticles101, thanks for your patience and for following up on all the comments!