Error message returned if the track is locked#871
Error message returned if the track is locked#871kytrinyx merged 3 commits intoexercism:masterfrom Jrank2013:locked_exercise_reporting
Conversation
kytrinyx
left a comment
kytrinyx
left a comment
There was a problem hiding this comment.
Thank you so much @Jrank2013 !
Would you be able to write a test that reproduces the problem and shows the fix?
kytrinyx
left a comment
kytrinyx
left a comment
There was a problem hiding this comment.
Thanks so much for adding the test! I have a couple of suggestions/questions, added below.
cmd/download.go
Outdated
| body, err := ioutil.ReadAll(res.Body) | ||
| res.Body = ioutil.NopCloser(bytes.NewReader(body)) | ||
|
|
||
| if err := json.Unmarshal(body, &d.payload); err != nil || res.StatusCode < 200 || res.StatusCode > 299 { |
There was a problem hiding this comment.
The decodedAPIError already unmarshals the error, right? Can we move all the error handling logic into that method?
There was a problem hiding this comment.
The response body can only be read once. If we unmarshal or decode the body before saving a copy we wouldn't have a way to decode the JSON into the error struct.
|
|
||
| if err := json.NewDecoder(res.Body).Decode(&d.payload); err != nil { | ||
| if res.StatusCode < 200 || res.StatusCode > 299 { | ||
| return nil, decodedAPIError(res) |
There was a problem hiding this comment.
Now we have the decodedAPIError twice, which seems like it shouldn't be necessary.
I'm sorry if I'm being dumb, here. I'll take some time after work to pull this down and play around with it to see if I understand it better.
There was a problem hiding this comment.
Ok, I played with this and I totally see what's going on.
Sorry for being slow!
There was a problem hiding this comment.
No problem, just glad I can help!
kytrinyx
left a comment
kytrinyx
left a comment
There was a problem hiding this comment.
I'm going to merge this because it totally fixes the error.
I'm also going to spend a little bit of time rethinking the decodedAPIError function, because I'm not really happy with it right now (but that's nothing to do with this PR, more about decisions made previously).
Thanks for your patience in walking me through the fix, @Jrank2013 !
2 participants
This fixes #870.
If the user has not unlocked the track that they have requested to download, the cli will return an error.