Skip to content

Fix the encode problem#650

Merged
kytrinyx merged 4 commits intoexercism:masterfrom
williandrade:development
Aug 5, 2018
Merged

Fix the encode problem#650
kytrinyx merged 4 commits intoexercism:masterfrom
williandrade:development

Conversation

@williandrade
Copy link
Contributor

Now that the file are download by a get request, if some character do not be encoded will try to get the wrong one.
Follow the evidence

https://api.exercism.io/v1/solutions/latest?exercise_id=hello-world&track_id=plsql
Response:

{
    "solution": {
        "id": "7f887ffb489c40c8ae1c1481eb2e1165",
        "url": "https://exercism.io/my/solutions/7f887ffb489c40c8ae1c1481eb2e1165",
        "team": null,
        "user": {
            "handle": "williandrade",
            "is_requester": true
        },
        "exercise": {
            "id": "hello-world",
            "instructions_url": "https://exercism.io/my/solutions/7f887ffb489c40c8ae1c1481eb2e1165",
            "auto_approve": true,
            "track": {
                "id": "plsql",
                "language": "PL/SQL"
            }
        },
        "file_download_base_url": "https://api.exercism.io/v1/solutions/7f887ffb489c40c8ae1c1481eb2e1165/files/",
        "files": [
            "README.md",
            "hello_world#.plsql",
            "ut_hello_world#.plsql"
        ],
        "iteration": null
    }
}

Response when do the GET request

========================= BEGIN DumpResponse =========================
HTTP/1.1 404 Not Found
Transfer-Encoding: chunked
Cache-Control: no-cache
Connection: keep-alive
Content-Type: application/json; charset=utf-8
Date: Tue, 17 Jul 2018 14:58:29 GMT
Referrer-Policy: strict-origin-when-cross-origin
Server: nginx/1.10.3 (Ubuntu)
Set-Cookie: site_context=normal; domain=.exercism.io; path=/
Set-Cookie: _exercism_session=MTjAmlcMmkeVLWfioqW5PvGvqFSypvhLk3Kb1zPESLkCbpep5XOD68gVDnUS1wX9DwcSe7nSi%2ByBqhsJBYOGccYUl4LC%2BpCLcDCFHRb%2BGeO22NleSGhHSk9avFom%2BJIGOBtyXv0o9FGmT5fKRtNdV61LDoT65jI0jz%2BqmqmPFhSHusAhy1XPYlxYZliLi6kKhjdPf3wVKkleBHfyI6A5sT7jBQf68BIOzqgeDRxcRjIEYba3CWurtxiTUwpIyV%2FNfw9kS63qvvTo--x0xwl%2BdZYyy1tYlD--KCuaNutWr0d%2Fl%2F9bSp5%2FVQ%3D%3D; domain=.exercism.io; path=/; HttpOnly
X-Content-Type-Options: nosniff
X-Download-Options: noopen
X-Frame-Options: SAMEORIGIN
X-Permitted-Cross-Domain-Policies: none
X-Request-Id: 4783f37e-c921-4203-a92f-e56b2df8fba1
X-Runtime: 0.088371
X-Xss-Protection: 1; mode=block


========================= END DumpResponse =========================

Copy link
Member

@kytrinyx kytrinyx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great debugging find! Would you mind adding a test for this? The download_test.go file has some tests that should be fairly straight forward to copy/paste and tweak.

@nywilken
Copy link
Contributor

@williandrade any chance you have time to add a test for this change? I would like to get it in. Let us know if you need help or if you don’t have the time.

Thanks again for your contributions

@williandrade
Copy link
Contributor Author

Sorry for take to long to answer you guys back, I had some things to do at my job, anyway, I added the change into the test file to embrace the new change. I'm new at this thing of contribute, hope I did it right. =D

@nywilken
Copy link
Contributor

Thanks for adding the test case. Would you mind rebasing your changes into the latest master https://github.com/exercism/docs/blob/master/contributing/git-basics.md#rebasing

If you need help or run into issues give us a holler and we can help you resolve any git issues.

@williandrade
Copy link
Contributor Author

I did the rebase, however I haven't sure if I did it right 😊

@nywilken
Copy link
Contributor

@williandrade it looks like you might've merged and rebased which threw the history out of whack. Please follow the steps below to resolve the rebasing issue. Let me know if you have questions.

Bring your fork up to date with upstream

git checkout master
git pull upstream master
git push origin master

Rebase your branch onto latest master
git checkout development

Undo last commit to avoid another conflict resolution

git reset e1ee85824c40ef0226d53b794954922d73332804
git stash

Rebase onto your updated master

git rebase master

Retrieve last commit which was stashed

git stash pop
git commit -a -m "change the test because of the commit 8572e980e811aff3249d980fe04644a51ee5944c"

Force push your changes back to your development branch.

@williandrade
Copy link
Contributor Author

omg, I just did what you said and looks that it works kkkkk. Hope that it works properly.

"files": [
"/file-1.txt",
"/subdir/file-2.txt",
"/subdir/file-2#.txt",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Directionally, this is great! Would you add a comment in the test setup to explain why the hash is part of the name? That's something that would easily be confusing a few weeks or months down the line, and it would be a bit tricky to figure out why it's there.

@nywilken
Copy link
Contributor

nywilken commented Aug 2, 2018

omg, I just did what you said and looks that it works kkkkk. Hope that it works properly.

Happy to hear that worked out. The history looks good. @kytrinyx left feedback for you.

@nywilken
Copy link
Contributor

nywilken commented Aug 5, 2018

@williandrade I was trying to push this through for you by addressing @kytrinyx last comment but I was unable to update your PR. @kytrinyx can you help out here, here is the change I wanted to add nywilken@75a9c50

@kytrinyx kytrinyx merged commit 11b04d8 into exercism:master Aug 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants