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

Add option to auto create user_library_import_dir directories upon user login #5138

Merged
merged 7 commits into from Jan 22, 2018

Conversation

Projects
None yet
8 participants
@scholtalbers
Contributor

scholtalbers commented Dec 6, 2017

# If user_library_import_dir is set, this option will auto create a library
# import directory for every user (based on their email) upon login.
#user_library_import_dir_auto_creation = False

@galaxybot galaxybot added the triage label Dec 6, 2017

@galaxybot galaxybot added this to the 18.01 milestone Dec 6, 2017

@@ -6,6 +6,7 @@
import random
import socket
import urllib
import os

This comment has been minimized.

@nsoranzo

nsoranzo Dec 6, 2017

Member

This should go after import logging

@martenson

This comment has been minimized.

Member

martenson commented Dec 6, 2017

@galaxybot test this

@@ -570,12 +572,30 @@ def __validate_login(self, trans, **kwd):
status = 'warning'
return (message, status, user, success)
def check_user_library_import_dir(self, user):
if self.app.config.user_library_import_dir_auto_creation:

This comment has been minimized.

@jmchilton

jmchilton Dec 7, 2017

Member

Thanks for the contribution!

I think this PR is causing the Tool Shed tests to fail because it reuses this same controller with a different app and config structure. Can you move this logic into trans.handle_user_login (lib/galaxy/web/framework/webapp.py) instead of placing it in the controller. For many reasons we like to keep controllers as slim as possible so I think it would be a better place for it anyway. If that doesn't fix the TS tests you could then use getattr(app.config, "user_library_import_dir_auto_creation", False) to access the config in a way that would harmlessly cause a no-op from within the tool shed.

@martenson

This comment has been minimized.

Member

martenson commented Jan 17, 2018

Should we expand this to cover ftp_upload_dir too?

edit: On second thought that would enlarge the scope of this PR too much, we should do it some other time.

@mvdbeek

This comment has been minimized.

Member

mvdbeek commented Jan 17, 2018

@martenson I think the ftp folder is created when connecting the first time by ftp, I think that's good enough, no ?

@mvdbeek

This comment has been minimized.

Member

mvdbeek commented Jan 17, 2018

@galaxybot test this

@mvdbeek

This comment has been minimized.

Member

mvdbeek commented Jan 18, 2018

The tests are failing here:

Create necessary user accounts and login as an admin user. ... Traceback (most recent call last):
  File "/galaxy_venv/local/lib/python2.7/site-packages/paste/httpserver.py", line 1085, in process_request_in_thread
    self.finish_request(request, client_address)
  File "/usr/lib/python2.7/SocketServer.py", line 331, in finish_request
    self.RequestHandlerClass(request, client_address, self)
  File "/usr/lib/python2.7/SocketServer.py", line 652, in __init__
    self.handle()
  File "/galaxy_venv/local/lib/python2.7/site-packages/paste/httpserver.py", line 459, in handle
    BaseHTTPRequestHandler.handle(self)
  File "/usr/lib/python2.7/BaseHTTPServer.py", line 340, in handle
    self.handle_one_request()
  File "/galaxy_venv/local/lib/python2.7/site-packages/paste/httpserver.py", line 454, in handle_one_request
    self.wsgi_execute()
  File "/galaxy_venv/local/lib/python2.7/site-packages/paste/httpserver.py", line 304, in wsgi_execute
    self.wsgi_start_response)
  File "/galaxy_venv/local/lib/python2.7/site-packages/paste/urlmap.py", line 216, in __call__
    return app(environ, start_response)
  File "/galaxy/lib/galaxy/web/framework/middleware/xforwardedhost.py", line 23, in __call__
    return self.app(environ, start_response)
  File "/galaxy_venv/local/lib/python2.7/site-packages/paste/recursive.py", line 85, in __call__
    return self.application(environ, start_response)
  File "/galaxy_venv/local/lib/python2.7/site-packages/routes/middleware.py", line 141, in __call__
    response = self.app(environ, start_response)
  File "/galaxy_venv/local/lib/python2.7/site-packages/paste/httpexceptions.py", line 640, in __call__
    return self.application(environ, start_response)
  File "/galaxy/lib/galaxy/web/framework/base.py", line 136, in __call__
    return self.handle_request(environ, start_response)
  File "/galaxy/lib/galaxy/web/framework/base.py", line 215, in handle_request
    body = method(trans, **kwargs)
  File "/galaxy/lib/galaxy/webapps/galaxy/controllers/user.py", line 725, in create
    trans.handle_user_login(user)
  File "/galaxy/lib/galaxy/web/framework/webapp.py", line 660, in handle_user_login
    self.user_checks(user)
  File "/galaxy/lib/galaxy/web/framework/webapp.py", line 648, in user_checks
    self.check_user_library_import_dir(user)
  File "/galaxy/lib/galaxy/web/framework/webapp.py", line 637, in check_user_library_import_dir
    if self.app.config.user_library_import_dir_auto_creation:
AttributeError: 'Configuration' object has no attribute 'user_library_import_dir_auto_creation'

I think those toolshed tests setup their own Configuration object. IMO it would be OK to do if getattr(self.app.config, "user_library_import_dir_auto_creation", False):

@mvdbeek

This comment has been minimized.

Member

mvdbeek commented Jan 18, 2018

@galaxybot test this

@mvdbeek

This comment has been minimized.

Member

mvdbeek commented Jan 18, 2018

@galaxybot test this

if getattr(self.app.config, "user_library_import_dir_auto_creation", False):
# try to create a user library import directory
try:
self.app.config._ensure_directory(os.path.join(self.app.config.user_library_import_dir, user.email))

This comment has been minimized.

@natefoo

natefoo Jan 18, 2018

Member

Calling this from outside of the config object seems strange, maybe this should use galaxy.util.path.safe_makedirs?

@natefoo

This comment has been minimized.

Member

natefoo commented Jan 19, 2018

I went ahead and did the merge to resolve the conflict since that was essentially my doing in the first place.

+1 from me, just going to wait for tests.

@martenson

This comment has been minimized.

Member

martenson commented Jan 19, 2018

@galaxybot test this

@natefoo

This comment has been minimized.

Member

natefoo commented Jan 22, 2018

Thanks @scholtalbers!

@natefoo natefoo merged commit ebafd19 into galaxyproject:dev Jan 22, 2018

4 of 6 checks passed

api test Build finished. 343 tests run, 4 skipped, 1 failed.
Details
framework test Build finished. 171 tests run, 0 skipped, 2 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
integration test Build finished. 79 tests run, 4 skipped, 0 failed.
Details
selenium test Build finished. 118 tests run, 3 skipped, 0 failed.
Details
toolshed test Build finished. 577 tests run, 0 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment