Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix VCF download feature #818

Merged
merged 4 commits into from
Sep 1, 2015
Merged

Fix VCF download feature #818

merged 4 commits into from
Sep 1, 2015

Conversation

armish
Copy link
Member

@armish armish commented Aug 27, 2015

See #738 for more context.

When clicked on the download, we were getting this error logged:

----------------------------------------
Exception happened during processing of request from ('127.0.0.1', 59979)
Traceback (most recent call last):
  File "/usr/local/Cellar/python/2.7.10/Frameworks/Python.framework/Versions/2.7/lib/python2.7/SocketServer.py", line 295, in _handle_request_noblock
    self.process_request(request, client_address)
  File "/usr/local/Cellar/python/2.7.10/Frameworks/Python.framework/Versions/2.7/lib/python2.7/SocketServer.py", line 321, in process_request
    self.finish_request(request, client_address)
  File "/usr/local/Cellar/python/2.7.10/Frameworks/Python.framework/Versions/2.7/lib/python2.7/SocketServer.py", line 334, in finish_request
    self.RequestHandlerClass(request, client_address, self)
  File "/usr/local/Cellar/python/2.7.10/Frameworks/Python.framework/Versions/2.7/lib/python2.7/SocketServer.py", line 657, in __init__
    self.finish()
  File "/usr/local/Cellar/python/2.7.10/Frameworks/Python.framework/Versions/2.7/lib/python2.7/SocketServer.py", line 716, in finish
    self.wfile.close()
  File "/usr/local/Cellar/python/2.7.10/Frameworks/Python.framework/Versions/2.7/lib/python2.7/socket.py", line 283, in close
    self.flush()
  File "/usr/local/Cellar/python/2.7.10/Frameworks/Python.framework/Versions/2.7/lib/python2.7/socket.py", line 307, in flush
    self._sock.sendall(view[write_offset:write_offset+buffer_size])
error: [Errno 32] Broken pipe
----------------------------------------

which was stemming from a simple coding mistake.

Review on Reviewable

@hammer
Copy link
Member

hammer commented Aug 27, 2015

is there a test you could add that would have caught this mistake?

@armish
Copy link
Member Author

armish commented Aug 27, 2015

yeah, it should do be doable using one of the test files we have. Let me add that and update the PR.

@armish
Copy link
Member Author

armish commented Aug 28, 2015

Adding a download test was much harder than I expected (due to some magic happening with authentication), but this is done now. On the bright side: we now have support for basic authentication for all pages within Cycledash.


def load_user_from_request(request):
"""Support for basic authorization."""
# first, try to login using the api_key url arg
Copy link
Member

Choose a reason for hiding this comment

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

Mind dropping support for this? It's not the most secure (namely because URLs are stored in logs), and I'm not sure it buys us anything?

Copy link
Member

Choose a reason for hiding this comment

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

Not a strong opinion, though, and if we need it in the future for some reason, then just go for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ihodes: without this one, my requests within helpers.ResourceTest were not being authorized and being forwarded to the login page automatically. I realized that all the api pages work fine with ResourceTest's get and post requests, but none of our pages with @login_required decorator.

I implemented this out of my ignorance about a better way; happy to hear your thoughts on this.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I just meant the url param version of this; keeping the BasicAuth version is 👍

Also, I'm confused a bit: I don't see an api_key urlparam in the download URL--how are you passing authorization though?

@ihodes
Copy link
Member

ihodes commented Aug 31, 2015

Looks great! One minor comment.


def test_download(self):
# match the URL with the default download button href
downUrl = ("/runs/" + str(self.run['id']) + "/download?query="
Copy link
Member

Choose a reason for hiding this comment

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

This might be clearer if you write the query-string as JSON and then URL encode it in code--must easier to edit that way, too.

@ihodes
Copy link
Member

ihodes commented Sep 1, 2015

Looks good!

ihodes added a commit that referenced this pull request Sep 1, 2015
@ihodes ihodes merged commit 4d56d8f into master Sep 1, 2015
@armish
Copy link
Member Author

armish commented Sep 1, 2015

Thanks!

@armish armish deleted the fix-vcf-download branch September 1, 2015 15:59
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.

None yet

3 participants