From 6d573a987046cb65e4821cf9ee4abcbd137ec97e Mon Sep 17 00:00:00 2001
From: Greg Von Kuster <greg@bx.psu.edu>
Date: Thu, 1 Oct 2009 15:07:22 -0400
Subject: [PATCH] Only allow 'uload files' option when replacing a dataset with
 a new version, more code merging, a few bugs fixes and additional functional
 tests for libraries.

---
 lib/galaxy/web/controllers/library.py         | 157 ++++++++----------
 lib/galaxy/web/controllers/library_admin.py   |  48 ++----
 lib/galaxy/web/controllers/library_common.py  |  29 ++++
 templates/admin/library/browse_library.mako   |   2 +-
 templates/admin/library/ldda_info.mako        |   2 +-
 templates/admin/library/upload.mako           |  25 +--
 templates/library/browse_library.mako         |   2 +-
 templates/library/ldda_info.mako              |   2 +-
 templates/library/upload.mako                 |  19 ++-
 test/base/twilltestcase.py                    |  16 +-
 .../functional/test_security_and_libraries.py |  46 ++++-
 11 files changed, 183 insertions(+), 165 deletions(-)

diff --git a/lib/galaxy/web/controllers/library.py b/lib/galaxy/web/controllers/library.py
index 60b30d95f2d..e00eda0090d 100644
--- a/lib/galaxy/web/controllers/library.py
+++ b/lib/galaxy/web/controllers/library.py
@@ -568,64 +568,31 @@ class Library( BaseController ):
                                                            msg=util.sanitize_text( msg ),
                                                            messagetype='error' ) )
             lddas.append( ldda )
-            if params.get( 'update_roles_button', False ):
-                if trans.app.security_agent.can_manage_library_item( user, roles, ldda ) and \
-                    trans.app.security_agent.can_manage_dataset( roles, ldda.dataset ):
-                    permissions = {}
-                    for k, v in trans.app.model.Dataset.permitted_actions.items():
-                        in_roles = [ trans.app.model.Role.get( x ) for x in util.listify( params.get( k + '_in', [] ) ) ]
-                        permissions[ trans.app.security_agent.get_action( v.action ) ] = in_roles
-                    for ldda in lddas:
-                        # Set the DATASET permissions on the Dataset
-                        trans.app.security_agent.set_all_dataset_permissions( ldda.dataset, permissions )
-                        ldda.dataset.refresh()
-                    permissions = {}
-                    for k, v in trans.app.model.Library.permitted_actions.items():
-                        in_roles = [ trans.app.model.Role.get( x ) for x in util.listify( kwd.get( k + '_in', [] ) ) ]
-                        permissions[ trans.app.security_agent.get_action( v.action ) ] = in_roles
-                    for ldda in lddas:
-                        # Set the LIBRARY permissions on the LibraryDataset
-                        # NOTE: the LibraryDataset and LibraryDatasetDatasetAssociation will be set with the same permissions
-                        trans.app.security_agent.set_all_library_permissions( ldda.library_dataset, permissions )
-                        ldda.library_dataset.refresh()
-                        # Set the LIBRARY permissions on the LibraryDatasetDatasetAssociation
-                        trans.app.security_agent.set_all_library_permissions( ldda, permissions )
-                        ldda.refresh()
-                    msg = 'Permissions and roles have been updated on %d datasets' % len( lddas )
-                    messagetype = 'done'
-                else:
-                    msg = "You are not authorized to change the permissions of dataset '%s'" % ldda.name
-                    messagetype = 'error'
-                return trans.fill_template( "/library/ldda_permissions.mako",
-                                            ldda=lddas,
-                                            library_id=library_id,
-                                            msg=msg,
-                                            messagetype=messagetype )
+        if params.get( 'update_roles_button', False ):
             if trans.app.security_agent.can_manage_library_item( user, roles, ldda ) and \
                 trans.app.security_agent.can_manage_dataset( roles, ldda.dataset ):
-                # Ensure that the permissions across all library items are identical, otherwise we can't update them together.
-                check_list = []
+                permissions = {}
+                for k, v in trans.app.model.Dataset.permitted_actions.items():
+                    in_roles = [ trans.app.model.Role.get( x ) for x in util.listify( params.get( k + '_in', [] ) ) ]
+                    permissions[ trans.app.security_agent.get_action( v.action ) ] = in_roles
                 for ldda in lddas:
-                    permissions = []
-                    # Check the library level permissions - the permissions on the LibraryDatasetDatasetAssociation
-                    # will always be the same as the permissions on the associated LibraryDataset, so we only need to
-                    # check one Library object
-                    for library_permission in trans.app.security_agent.get_library_dataset_permissions( ldda.library_dataset ):
-                        if library_permission.action not in permissions:
-                            permissions.append( library_permission.action )
-                    for dataset_permission in trans.app.security_agent.get_dataset_permissions( ldda.dataset ):
-                        if dataset_permission.action not in permissions:
-                            permissions.append( dataset_permission.action )
-                    permissions.sort()
-                    if not check_list:
-                        check_list = permissions
-                    if permissions != check_list:
-                        msg = 'The datasets you selected do not have identical permissions, so they can not be updated together'
-                        trans.response.send_redirect( web.url_for( controller='library',
-                                                                   action='browse_library',
-                                                                   obj_id=library_id,
-                                                                   msg=util.sanitize_text( msg ),
-                                                                   messagetype='error' ) )
+                    # Set the DATASET permissions on the Dataset
+                    trans.app.security_agent.set_all_dataset_permissions( ldda.dataset, permissions )
+                    ldda.dataset.refresh()
+                permissions = {}
+                for k, v in trans.app.model.Library.permitted_actions.items():
+                    in_roles = [ trans.app.model.Role.get( x ) for x in util.listify( kwd.get( k + '_in', [] ) ) ]
+                    permissions[ trans.app.security_agent.get_action( v.action ) ] = in_roles
+                for ldda in lddas:
+                    # Set the LIBRARY permissions on the LibraryDataset
+                    # NOTE: the LibraryDataset and LibraryDatasetDatasetAssociation will be set with the same permissions
+                    trans.app.security_agent.set_all_library_permissions( ldda.library_dataset, permissions )
+                    ldda.library_dataset.refresh()
+                    # Set the LIBRARY permissions on the LibraryDatasetDatasetAssociation
+                    trans.app.security_agent.set_all_library_permissions( ldda, permissions )
+                    ldda.refresh()
+                msg = 'Permissions and roles have been updated on %d datasets' % len( lddas )
+                messagetype = 'done'
             else:
                 msg = "You are not authorized to change the permissions of dataset '%s'" % ldda.name
                 messagetype = 'error'
@@ -634,6 +601,39 @@ class Library( BaseController ):
                                         library_id=library_id,
                                         msg=msg,
                                         messagetype=messagetype )
+        if trans.app.security_agent.can_manage_library_item( user, roles, ldda ) and \
+            trans.app.security_agent.can_manage_dataset( roles, ldda.dataset ):
+            # Ensure that the permissions across all library items are identical, otherwise we can't update them together.
+            check_list = []
+            for ldda in lddas:
+                permissions = []
+                # Check the library level permissions - the permissions on the LibraryDatasetDatasetAssociation
+                # will always be the same as the permissions on the associated LibraryDataset, so we only need to
+                # check one Library object
+                for library_permission in trans.app.security_agent.get_library_dataset_permissions( ldda.library_dataset ):
+                    if library_permission.action not in permissions:
+                        permissions.append( library_permission.action )
+                for dataset_permission in trans.app.security_agent.get_dataset_permissions( ldda.dataset ):
+                    if dataset_permission.action not in permissions:
+                        permissions.append( dataset_permission.action )
+                permissions.sort()
+                if not check_list:
+                    check_list = permissions
+                if permissions != check_list:
+                    msg = 'The datasets you selected do not have identical permissions, so they can not be updated together'
+                    trans.response.send_redirect( web.url_for( controller='library',
+                                                               action='browse_library',
+                                                               obj_id=library_id,
+                                                               msg=util.sanitize_text( msg ),
+                                                               messagetype='error' ) )
+        else:
+            msg = "You are not authorized to change the permissions of dataset '%s'" % ldda.name
+            messagetype = 'error'
+        return trans.fill_template( "/library/ldda_permissions.mako",
+                                    ldda=lddas,
+                                    library_id=library_id,
+                                    msg=msg,
+                                    messagetype=messagetype )
     @web.expose
     def upload_library_dataset( self, trans, library_id, folder_id, **kwd ):
         params = util.Params( kwd )
@@ -652,8 +652,11 @@ class Library( BaseController ):
             replace_dataset = trans.app.model.LibraryDataset.get( params.get( 'replace_id', None ) )
             if not last_used_build:
                 last_used_build = replace_dataset.library_dataset_dataset_association.dbkey
+            # Don't allow multiple datasets to be uploaded when replacing a dataset with a new version
+            upload_option = 'upload_file'
         else:
             replace_dataset = None
+            upload_option = params.get( 'upload_option', 'upload_file' )
         user, roles = trans.get_user_and_roles()
         if trans.app.security_agent.can_add_library_item( user, roles, folder ) or \
              ( replace_dataset and trans.app.security_agent.can_modify_library_item( user, roles, replace_dataset ) ):
@@ -666,15 +669,14 @@ class Library( BaseController ):
                 else:
                     template_id = 'None'
                     widgets = []
-                upload_option = params.get( 'upload_option', 'upload_file' )
                 created_outputs = trans.webapp.controllers[ 'library_common' ].upload_dataset( trans,
-                                                                                                controller='library', 
-                                                                                                library_id=library_id,
-                                                                                                folder_id=folder_id,
-                                                                                                template_id=template_id,
-                                                                                                widgets=widgets,
-                                                                                                replace_dataset=replace_dataset,
-                                                                                                **kwd )
+                                                                                               controller='library', 
+                                                                                               library_id=library_id,
+                                                                                               folder_id=folder_id,
+                                                                                               template_id=template_id,
+                                                                                               widgets=widgets,
+                                                                                               replace_dataset=replace_dataset,
+                                                                                               **kwd )
                 if created_outputs:
                     ldda_id_list = [ str( v.id ) for v in created_outputs.values() ]
                     total_added = len( created_outputs.values() )
@@ -860,35 +862,6 @@ class Library( BaseController ):
                                             msg=msg,
                                             messagetype=messagetype )
     @web.expose
-    def download_dataset_from_folder(self, trans, obj_id, library_id=None, **kwd):
-        """Catches the dataset id and displays file contents as directed"""
-        # id must refer to a LibraryDatasetDatasetAssociation object
-        ldda = trans.app.model.LibraryDatasetDatasetAssociation.get( obj_id )
-        if not ldda.dataset:
-            msg = 'Invalid LibraryDatasetDatasetAssociation id %s received for file downlaod' % str( obj_id )
-            return trans.response.send_redirect( web.url_for( controller='library',
-                                                              action='browse_library',
-                                                              obj_id=library_id,
-                                                              msg=msg,
-                                                              messagetype='error' ) )
-        mime = trans.app.datatypes_registry.get_mimetype_by_extension( ldda.extension.lower() )
-        trans.response.set_content_type( mime )
-        fStat = os.stat( ldda.file_name )
-        trans.response.headers[ 'Content-Length' ] = int( fStat.st_size )
-        valid_chars = '.,^_-()[]0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ'
-        fname = ldda.name
-        fname = ''.join( c in valid_chars and c or '_' for c in fname )[ 0:150 ]
-        trans.response.headers[ "Content-Disposition" ] = "attachment; filename=GalaxyLibraryDataset-%s-[%s]" % ( str( obj_id ), fname )
-        try:
-            return open( ldda.file_name )
-        except: 
-            msg = 'This dataset contains no content'
-            return trans.response.send_redirect( web.url_for( controller='library',
-                                                              action='browse_library',
-                                                              obj_id=library_id,
-                                                              msg=msg,
-                                                              messagetype='error' ) )
-    @web.expose
     def datasets( self, trans, library_id, ldda_ids='', **kwd ):
         # This method is used by the select list labeled "Perform action on selected datasets"
         # on the analysis library browser.
diff --git a/lib/galaxy/web/controllers/library_admin.py b/lib/galaxy/web/controllers/library_admin.py
index 86f651172a3..83599d8f1b7 100644
--- a/lib/galaxy/web/controllers/library_admin.py
+++ b/lib/galaxy/web/controllers/library_admin.py
@@ -673,8 +673,11 @@ class LibraryAdmin( BaseController ):
             replace_dataset = trans.app.model.LibraryDataset.get( int( replace_id ) )
             if not last_used_build:
                 last_used_build = replace_dataset.library_dataset_dataset_association.dbkey
+            # Don't allow multiple datasets to be uploaded when replacing a dataset with a new version
+            upload_option = 'upload_file'
         else:
             replace_dataset = None
+            upload_option = params.get( 'upload_option', 'upload_file' )
         if params.get( 'runtool_btn', False ) or params.get( 'ajax_upload', False ):
             # See if we have any inherited templates, but do not inherit contents.
             info_association, inherited = folder.get_info_association( inherited=True )
@@ -684,15 +687,14 @@ class LibraryAdmin( BaseController ):
             else:
                 template_id = 'None'
                 widgets = []
-            upload_option = params.get( 'upload_option', 'upload_file' )
             created_outputs = trans.webapp.controllers[ 'library_common' ].upload_dataset( trans,
-                                                                                            controller='library_admin',
-                                                                                            library_id=library_id,
-                                                                                            folder_id=folder_id,
-                                                                                            template_id=template_id,
-                                                                                            widgets=widgets,
-                                                                                            replace_dataset=replace_dataset,
-                                                                                            **kwd )
+                                                                                           controller='library_admin',
+                                                                                           library_id=library_id,
+                                                                                           folder_id=folder_id,
+                                                                                           template_id=template_id,
+                                                                                           widgets=widgets,
+                                                                                           replace_dataset=replace_dataset,
+                                                                                           **kwd )
             if created_outputs:
                 total_added = len( created_outputs.values() )
                 if replace_dataset:
@@ -851,36 +853,6 @@ class LibraryAdmin( BaseController ):
                                             messagetype=messagetype )
     @web.expose
     @web.require_admin
-    def download_dataset_from_folder(self, trans, obj_id, library_id=None, **kwd):
-        """Catches the dataset id and displays file contents as directed"""
-        # id must refer to a LibraryDatasetDatasetAssociation object
-        ldda = trans.app.model.LibraryDatasetDatasetAssociation.get( obj_id )
-        if not ldda.dataset:
-            msg = 'Invalid LibraryDatasetDatasetAssociation id %s received for file downlaod' % str( obj_id )
-            return trans.response.send_redirect( web.url_for( controller='library_admin',
-                                                              action='browse_library',
-                                                              obj_id=library_id,
-                                                              msg=util.sanitize_text( msg ),
-                                                              messagetype='error' ) )
-        mime = trans.app.datatypes_registry.get_mimetype_by_extension( ldda.extension.lower() )
-        trans.response.set_content_type( mime )
-        fStat = os.stat( ldda.file_name )
-        trans.response.headers[ 'Content-Length' ] = int( fStat.st_size )
-        valid_chars = '.,^_-()[]0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ'
-        fname = ldda.name
-        fname = ''.join( c in valid_chars and c or '_' for c in fname )[ 0:150 ]
-        trans.response.headers[ "Content-Disposition" ] = "attachment; filename=GalaxyLibraryDataset-%s-[%s]" % ( str( obj_id ), fname )
-        try:
-            return open( ldda.file_name )
-        except: 
-            msg = 'This dataset contains no content'
-            return trans.response.send_redirect( web.url_for( controller='library_admin',
-                                                              action='browse_library',
-                                                              obj_id=library_id,
-                                                              msg=util.sanitize_text( msg ),
-                                                              messagetype='error' ) )
-    @web.expose
-    @web.require_admin
     def datasets( self, trans, library_id, **kwd ):
         # This method is used by the select list labeled "Perform action on selected datasets"
         # on the admin library browser.
diff --git a/lib/galaxy/web/controllers/library_common.py b/lib/galaxy/web/controllers/library_common.py
index edcab8e0ab3..bb64efdffcd 100644
--- a/lib/galaxy/web/controllers/library_common.py
+++ b/lib/galaxy/web/controllers/library_common.py
@@ -181,6 +181,35 @@ class LibraryCommon( BaseController ):
             return None, err_redirect, msg
         return uploaded_datasets, None, None
     @web.expose
+    def download_dataset_from_folder( self, trans, cntrller, obj_id, library_id=None, **kwd ):
+        """Catches the dataset id and displays file contents as directed"""
+        # id must refer to a LibraryDatasetDatasetAssociation object
+        ldda = trans.app.model.LibraryDatasetDatasetAssociation.get( obj_id )
+        if not ldda.dataset:
+            msg = 'Invalid LibraryDatasetDatasetAssociation id %s received for file downlaod' % str( obj_id )
+            return trans.response.send_redirect( web.url_for( controller=cntrller,
+                                                              action='browse_library',
+                                                              obj_id=library_id,
+                                                              msg=util.sanitize_text( msg ),
+                                                              messagetype='error' ) )
+        mime = trans.app.datatypes_registry.get_mimetype_by_extension( ldda.extension.lower() )
+        trans.response.set_content_type( mime )
+        fStat = os.stat( ldda.file_name )
+        trans.response.headers[ 'Content-Length' ] = int( fStat.st_size )
+        valid_chars = '.,^_-()[]0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ'
+        fname = ldda.name
+        fname = ''.join( c in valid_chars and c or '_' for c in fname )[ 0:150 ]
+        trans.response.headers[ "Content-Disposition" ] = "attachment; filename=GalaxyLibraryDataset-%s-[%s]" % ( str( obj_id ), fname )
+        try:
+            return open( ldda.file_name )
+        except: 
+            msg = 'This dataset contains no content'
+            return trans.response.send_redirect( web.url_for( controller=cntrller,
+                                                              action='browse_library',
+                                                              obj_id=library_id,
+                                                              msg=util.sanitize_text( msg ),
+                                                              messagetype='error' ) )
+    @web.expose
     def info_template( self, trans, cntrller, library_id, response_action='library', obj_id=None, folder_id=None, ldda_id=None, **kwd ):
         # Only adding a new templAte to a library or folder is currently allowed.  Editing an existing template is
         # a future enhancement.  The response_action param is the name of the method to which this method will redirect
diff --git a/templates/admin/library/browse_library.mako b/templates/admin/library/browse_library.mako
index 4d7ce6a1377..7e051bb0ee4 100644
--- a/templates/admin/library/browse_library.mako
+++ b/templates/admin/library/browse_library.mako
@@ -137,7 +137,7 @@
                                     <a class="action-button" href="${h.url_for( controller='library_admin', action='ldda_manage_permissions', library_id=library.id, folder_id=folder.id, obj_id=ldda.id, permissions=True )}">Edit this dataset's permissions</a>
                                     <a class="action-button" href="${h.url_for( controller='library_admin', action='upload_library_dataset', library_id=library.id, folder_id=folder.id, replace_id=library_dataset.id )}">Upload a new version of this dataset</a>
                                     %if ldda.has_data:
-                                        <a class="action-button" href="${h.url_for( controller='library_admin', action='download_dataset_from_folder', obj_id=ldda.id, library_id=library.id )}">Download this dataset</a>
+                                        <a class="action-button" href="${h.url_for( controller='library_admin', action='download_dataset_from_folder', cntrller='library_admin', obj_id=ldda.id, library_id=library.id )}">Download this dataset</a>
                                     %endif
                                     <a class="action-button" confirm="Click OK to delete dataset '${ldda.name}'." href="${h.url_for( controller='library_admin', action='delete_library_item', library_id=library.id, library_item_id=library_dataset.id, library_item_type='library_dataset' )}">Delete this dataset</a>
                                 </div>
diff --git a/templates/admin/library/ldda_info.mako b/templates/admin/library/ldda_info.mako
index d1a9402404d..a1ea050c15c 100644
--- a/templates/admin/library/ldda_info.mako
+++ b/templates/admin/library/ldda_info.mako
@@ -47,7 +47,7 @@
                     <a class="action-button" href="${h.url_for( controller='library_admin', action='upload_library_dataset', library_id=library_id, folder_id=ldda.library_dataset.folder.id, replace_id=ldda.library_dataset.id )}">Upload a new version of this dataset</a>
                 %endif
                 %if ldda.has_data:
-                    <a class="action-button" href="${h.url_for( controller='library_admin', action='download_dataset_from_folder', obj_id=ldda.id, library_id=library_id )}">Download this dataset</a>
+                    <a class="action-button" href="${h.url_for( controller='library_admin', action='download_dataset_from_folder', cntrller='library_admin', obj_id=ldda.id, library_id=library_id )}">Download this dataset</a>
                 %endif
                 %if not library.deleted and not ldda.library_dataset.folder.deleted and not ldda.library_dataset.deleted:
                     <a class="action-button" confirm="Click OK to remove dataset '${ldda.name}'?" href="${h.url_for( controller='library_admin', action='delete_library_item', library_id=library_id, folder_id=ldda.library_dataset.folder.id, library_item_id=ldda.library_dataset.id, library_item_type='library_dataset' )}">Delete this dataset</a>
diff --git a/templates/admin/library/upload.mako b/templates/admin/library/upload.mako
index 463ac399de7..cea87914686 100644
--- a/templates/admin/library/upload.mako
+++ b/templates/admin/library/upload.mako
@@ -12,17 +12,20 @@
 %>
 
 <b>Create new data library datasets</b>
-<a id="upload-librarydataset--popup" class="popup-arrow" style="display: none;">&#9660;</a>
-<div popupmenu="upload-librarydataset--popup">
-    <a class="action-button" href="${h.url_for( controller='library_admin', action='upload_library_dataset', library_id=library_id, folder_id=folder_id, replace_id=replace_id, upload_option='upload_file' )}">Upload files</a>
-    %if trans.app.config.library_import_dir and os.path.exists( trans.app.config.library_import_dir ):
-        <a class="action-button" href="${h.url_for( controller='library_admin', action='upload_library_dataset', library_id=library_id, folder_id=folder_id, replace_id=replace_id, upload_option='upload_directory' )}">Upload directory of files</a>
-    %endif
-    %if trans.app.config.allow_library_path_paste:
-        <a class="action-button" href="${h.url_for( controller='library_admin', action='upload_library_dataset', library_id=library_id, folder_id=folder_id, replace_id=replace_id, upload_option='upload_paths' )}">Upload files from filesystem paths</a>
-    %endif
-    <a class="action-button" href="${h.url_for( controller='library_admin', action='upload_library_dataset', library_id=library_id, folder_id=folder_id, replace_id=replace_id, upload_option='import_from_history' )}">Import datasets from your current history</a>
-</div>
+%if replace_dataset in [ None, 'None' ]:
+    ## Don't allow multiple datasets to be uploaded when replacing a dataset with a new version
+    <a id="upload-librarydataset--popup" class="popup-arrow" style="display: none;">&#9660;</a>
+    <div popupmenu="upload-librarydataset--popup">
+        <a class="action-button" href="${h.url_for( controller='library_admin', action='upload_library_dataset', library_id=library_id, folder_id=folder_id, replace_id=replace_id, upload_option='upload_file' )}">Upload files</a>
+        %if trans.app.config.library_import_dir and os.path.exists( trans.app.config.library_import_dir ):
+            <a class="action-button" href="${h.url_for( controller='library_admin', action='upload_library_dataset', library_id=library_id, folder_id=folder_id, replace_id=replace_id, upload_option='upload_directory' )}">Upload directory of files</a>
+        %endif
+        %if trans.app.config.allow_library_path_paste:
+            <a class="action-button" href="${h.url_for( controller='library_admin', action='upload_library_dataset', library_id=library_id, folder_id=folder_id, replace_id=replace_id, upload_option='upload_paths' )}">Upload files from filesystem paths</a>
+        %endif
+        <a class="action-button" href="${h.url_for( controller='library_admin', action='upload_library_dataset', library_id=library_id, folder_id=folder_id, replace_id=replace_id, upload_option='import_from_history' )}">Import datasets from your current history</a>
+    </div>
+%endif
 <br/><br/>
 <ul class="manage-table-actions">
     <li>
diff --git a/templates/library/browse_library.mako b/templates/library/browse_library.mako
index ee0de92ad8d..8003877f070 100644
--- a/templates/library/browse_library.mako
+++ b/templates/library/browse_library.mako
@@ -178,7 +178,7 @@ class RowCounter( object ):
                     %endif
                     %if ldda.has_data:
                         <a class="action-button" href="${h.url_for( controller='library', action='datasets', library_id=library.id, ldda_ids=str( ldda.id ), do_action='add' )}">Import this dataset into your current history</a>
-                        <a class="action-button" href="${h.url_for( controller='library', action='download_dataset_from_folder', obj_id=ldda.id, library_id=library.id )}">Download this dataset</a>
+                        <a class="action-button" href="${h.url_for( controller='library', action='download_dataset_from_folder', cntrller='library', obj_id=ldda.id, library_id=library.id )}">Download this dataset</a>
                     %endif
                 </div>
             </td>
diff --git a/templates/library/ldda_info.mako b/templates/library/ldda_info.mako
index 6c2f64b883a..64dcee66e12 100644
--- a/templates/library/ldda_info.mako
+++ b/templates/library/ldda_info.mako
@@ -53,7 +53,7 @@
             %endif
             %if ldda.has_data:
                 <a class="action-button" href="${h.url_for( controller='library', action='datasets', library_id=library_id, ldda_ids=str( ldda.id ), do_action='add' )}">Import this dataset into your current history</a>
-                <a class="action-button" href="${h.url_for( controller='library', action='download_dataset_from_folder', obj_id=ldda.id, library_id=library_id )}">Download this dataset</a>
+                <a class="action-button" href="${h.url_for( controller='library', action='download_dataset_from_folder', cntrller='library', obj_id=ldda.id, library_id=library_id )}">Download this dataset</a>
             %endif
         </div>
     </div>
diff --git a/templates/library/upload.mako b/templates/library/upload.mako
index ea3699b034f..1fe4bf46b8a 100644
--- a/templates/library/upload.mako
+++ b/templates/library/upload.mako
@@ -12,14 +12,17 @@
 %>
 
 <b>Create new data library datasets</b>
-<a id="upload-librarydataset--popup" class="popup-arrow" style="display: none;">&#9660;</a>
-<div popupmenu="upload-librarydataset--popup">
-    <a class="action-button" href="${h.url_for( controller='library', action='upload_library_dataset', library_id=library_id, folder_id=folder_id, replace_id=replace_id, upload_option='upload_file' )}">Upload files</a>
-    %if trans.app.config.user_library_import_dir and os.path.exists( os.path.join( trans.app.config.user_library_import_dir, trans.user.email ) ):
-        <a class="action-button" href="${h.url_for( controller='library', action='upload_library_dataset', library_id=library_id, folder_id=folder_id, replace_id=replace_id, upload_option='upload_directory' )}">Upload directory of files</a>
-    %endif
-    <a class="action-button" href="${h.url_for( controller='library', action='upload_library_dataset', library_id=library_id, folder_id=folder_id, replace_id=replace_id, upload_option='import_from_history' )}">Import datasets from your current history</a>
-</div>
+%if replace_dataset in [ None, 'None' ]:
+    ## Don't allow multiple datasets to be uploaded when replacing a dataset with a new version
+    <a id="upload-librarydataset--popup" class="popup-arrow" style="display: none;">&#9660;</a>
+    <div popupmenu="upload-librarydataset--popup">
+        <a class="action-button" href="${h.url_for( controller='library', action='upload_library_dataset', library_id=library_id, folder_id=folder_id, replace_id=replace_id, upload_option='upload_file' )}">Upload files</a>
+        %if trans.app.config.user_library_import_dir and os.path.exists( os.path.join( trans.app.config.user_library_import_dir, trans.user.email ) ):
+            <a class="action-button" href="${h.url_for( controller='library', action='upload_library_dataset', library_id=library_id, folder_id=folder_id, replace_id=replace_id, upload_option='upload_directory' )}">Upload directory of files</a>
+        %endif
+        <a class="action-button" href="${h.url_for( controller='library', action='upload_library_dataset', library_id=library_id, folder_id=folder_id, replace_id=replace_id, upload_option='import_from_history' )}">Import datasets from your current history</a>
+    </div>
+%endif
 <br/><br/>
 <ul class="manage-table-actions">
     <li>
diff --git a/test/base/twilltestcase.py b/test/base/twilltestcase.py
index fe0d8f9cc68..6ed2be77ba2 100644
--- a/test/base/twilltestcase.py
+++ b/test/base/twilltestcase.py
@@ -1144,14 +1144,15 @@ class TwillTestCase( unittest.TestCase ):
         tc.fv( "1", "2", description ) # form field 1 is the field named name...
         tc.submit( "create_library_button" )
         self.home()
-    def set_library_permissions( self, library_id, library_name, role_id, permissions_in, permissions_out ):
+    def set_library_permissions( self, library_id, library_name, role_ids_str, permissions_in, permissions_out ):
+        # role_ids_str must be a comma-separated string of role ids
         url = "library_admin/library?obj_id=%s&permissions=True&update_roles_button=Save" % ( library_id )
         for po in permissions_out:
             key = '%s_out' % po
-            url ="%s&%s=%s" % ( url, key, str( role_id ) )
+            url ="%s&%s=%s" % ( url, key, role_ids_str )
         for pi in permissions_in:
             key = '%s_in' % pi
-            url ="%s&%s=%s" % ( url, key, str( role_id ) )
+            url ="%s&%s=%s" % ( url, key, role_ids_str )
         self.home()
         self.visit_url( "%s/%s" % ( self.url, url ) )
         check_str = "Permissions updated for library '%s'" % library_name
@@ -1276,17 +1277,16 @@ class TwillTestCase( unittest.TestCase ):
         self.check_page_for_string( check_str )
         self.library_wait( library_id )
         self.home()
-    def set_library_dataset_permissions( self, library_id, folder_id, ldda_id, ldda_name, role_id, permissions_in, permissions_out ):
+    def set_library_dataset_permissions( self, library_id, folder_id, ldda_id, ldda_name, role_ids_str, permissions_in, permissions_out ):
+        # role_ids_str must be a comma-separated string of role ids
         url = "library_admin/ldda_manage_permissions?library_id=%s&folder_id=%s&obj_id=%s&update_roles_button=Save" % \
             ( library_id, folder_id, ldda_id )
-        #role_ids = util.listify( role_ids )
-        #for role_id in role_ids:
         for po in permissions_out:
             key = '%s_out' % po
-            url ="%s&%s=%s" % ( url, key, str( role_id ) )
+            url ="%s&%s=%s" % ( url, key, role_ids_str )
         for pi in permissions_in:
             key = '%s_in' % pi
-            url ="%s&%s=%s" % ( url, key, str( role_id ) )
+            url ="%s&%s=%s" % ( url, key, role_ids_str )
         self.home()
         self.visit_url( "%s/%s" % ( self.url, url ) )
         check_str = "Permissions have been updated on 1 datasets"
diff --git a/test/functional/test_security_and_libraries.py b/test/functional/test_security_and_libraries.py
index 23e95f525f7..441ebb80be4 100644
--- a/test/functional/test_security_and_libraries.py
+++ b/test/functional/test_security_and_libraries.py
@@ -176,6 +176,7 @@ class TestSecurityAndLibraries( TwillTestCase ):
         actions_in = [ 'manage permissions' ]
         permissions_out = [ 'DATASET_ACCESS' ]
         actions_out = [ 'access' ]
+        global regular_user2_private_role
         regular_user2_private_role = None
         for role in regular_user2.all_roles():
             if role.name == regular_user2.email and role.description == 'Private Role for %s' % regular_user2.email:
@@ -849,9 +850,9 @@ class TestSecurityAndLibraries( TwillTestCase ):
         permissions_in = [ k for k, v in galaxy.model.Dataset.permitted_actions.items() ] + \
                          [ k for k, v in galaxy.model.Library.permitted_actions.items() ]
         permissions_out = []
-        role_ids = "%s,%s" % ( str( role_one.id ), str( admin_user_private_role.id ) )
+        role_ids_str = '%s,%s' % ( str( role_one.id ), str( admin_user_private_role.id ) )
         self.set_library_dataset_permissions( str( library_one.id ), str( folder_one.id ), str( ldda_four.id ), ldda_four.name,
-                                              role_ids, permissions_in, permissions_out )
+                                              role_ids_str, permissions_in, permissions_out )
         # admin_user should now be able to see 4.bed from the analysis view's access libraries
         self.home()
         self.visit_url( '%s/library/browse_library?obj_id=%s' % ( self.url, str( library_one.id ) ) )
@@ -1309,6 +1310,7 @@ class TestSecurityAndLibraries( TwillTestCase ):
 
     def test_170_mark_group_deleted( self ):
         """Testing marking a group as deleted"""
+        # Logged in as admin_user
         self.home()
         self.visit_url( '%s/admin/groups' % self.url )
         self.check_page_for_string( group_two.name )
@@ -1323,12 +1325,14 @@ class TestSecurityAndLibraries( TwillTestCase ):
             raise AssertionError( '%s incorrectly lost all role associations when it was marked as deleted.' % group_two.name )
     def test_175_undelete_group( self ):
         """Testing undeleting a deleted group"""
+        # Logged in as admin_user
         self.undelete_group( str( group_two.id ), group_two.name )
         group_two.refresh()
         if group_two.deleted:
             raise AssertionError( '%s was not correctly marked as not deleted.' % group_two.name )
     def test_180_mark_role_deleted( self ):
         """Testing marking a role as deleted"""
+        # Logged in as admin_user
         self.home()
         self.visit_url( '%s/admin/roles' % self.url )
         self.check_page_for_string( role_two.name )
@@ -1343,9 +1347,11 @@ class TestSecurityAndLibraries( TwillTestCase ):
             raise AssertionError( '%s incorrectly lost all group associations when it was marked as deleted.' % role_two.name )
     def test_185_undelete_role( self ):
         """Testing undeleting a deleted role"""
+        # Logged in as admin_user
         self.undelete_role( str( role_two.id ), role_two.name )
     def test_190_mark_dataset_deleted( self ):
         """Testing marking a library dataset as deleted"""
+        # Logged in as admin_user
         self.home()
         self.delete_library_item( str( library_one.id ), str( ldda_two.library_dataset.id ), ldda_two.name, library_item_type='library_dataset' )
         self.home()
@@ -1359,12 +1365,14 @@ class TestSecurityAndLibraries( TwillTestCase ):
         self.home()
     def test_195_display_deleted_dataset( self ):
         """Testing displaying deleted dataset"""
+        # Logged in as admin_user
         self.home()
         self.visit_url( "%s/library_admin/browse_library?obj_id=%s&show_deleted=True" % ( self.url, str( library_one.id ) ) )
         self.check_page_for_string( ldda_two.name )
         self.home()
     def test_200_hide_deleted_dataset( self ):
         """Testing hiding deleted dataset"""
+        # Logged in as admin_user
         self.home()
         self.visit_url( "%s/library_admin/browse_library?obj_id=%s&show_deleted=False" % ( self.url, str( library_one.id ) ) )
         try:
@@ -1375,6 +1383,7 @@ class TestSecurityAndLibraries( TwillTestCase ):
         self.home()
     def test_205_mark_folder_deleted( self ):
         """Testing marking a library folder as deleted"""
+        # Logged in as admin_user
         self.home()
         self.delete_library_item( str( library_one.id ), str( folder_two.id ), folder_two.name, library_item_type='folder' )
         self.home()
@@ -1387,6 +1396,7 @@ class TestSecurityAndLibraries( TwillTestCase ):
         self.home()
     def test_210_mark_folder_undeleted( self ):
         """Testing marking a library folder as undeleted"""
+        # Logged in as admin_user
         self.home()
         self.undelete_library_item( str( library_one.id ), str( folder_two.id ), folder_two.name, library_item_type='folder' )
         self.home()
@@ -1402,6 +1412,7 @@ class TestSecurityAndLibraries( TwillTestCase ):
         self.home()
     def test_215_mark_library_deleted( self ):
         """Testing marking a library as deleted"""
+        # Logged in as admin_user
         self.home()
         # First mark folder_two as deleted to further test state saving when we undelete the library
         self.delete_library_item( str( library_one.id ), str( folder_two.id ), folder_two.name, library_item_type='folder' )
@@ -1412,6 +1423,7 @@ class TestSecurityAndLibraries( TwillTestCase ):
         self.home()
     def test_220_mark_library_undeleted( self ):
         """Testing marking a library as undeleted"""
+        # Logged in as admin_user
         self.home()
         self.undelete_library_item( str( library_one.id ), str( library_one.id ), library_one.name, library_item_type='library' )
         self.home()
@@ -1426,6 +1438,7 @@ class TestSecurityAndLibraries( TwillTestCase ):
         self.home()
     def test_225_purge_user( self ):
         """Testing purging a user account"""
+        # Logged in as admin_user
         self.mark_user_deleted( user_id=self.security.encode_id( regular_user3.id ), email=regular_user3.email )
         regular_user3.refresh()
         self.purge_user( self.security.encode_id( regular_user3.id ), regular_user3.email )
@@ -1458,6 +1471,7 @@ class TestSecurityAndLibraries( TwillTestCase ):
                 raise AssertionError( 'UserRoleAssociations for user %s are not related with the private role.' % regular_user3.email )
     def test_230_manually_unpurge_user( self ):
         """Testing manually un-purging a user account"""
+        # Logged in as admin_user
         # Reset the user for later test runs.  The user's private Role and DefaultUserPermissions for that role
         # should have been preserved, so all we need to do is reset purged and deleted.
         # TODO: If we decide to implement the GUI feature for un-purging a user, replace this with a method call
@@ -1466,6 +1480,7 @@ class TestSecurityAndLibraries( TwillTestCase ):
         regular_user3.flush()
     def test_235_purge_group( self ):
         """Testing purging a group"""
+        # Logged in as admin_user
         group_id = str( group_two.id )
         self.mark_group_deleted( group_id, group_two.name )
         self.purge_group( group_id, group_two.name )
@@ -1481,6 +1496,7 @@ class TestSecurityAndLibraries( TwillTestCase ):
         self.undelete_group( group_id, group_two.name )
     def test_240_purge_role( self ):
         """Testing purging a role"""
+        # Logged in as admin_user
         role_id = str( role_two.id )
         self.mark_role_deleted( role_id, role_two.name )
         self.purge_role( role_id, role_two.name )
@@ -1506,6 +1522,7 @@ class TestSecurityAndLibraries( TwillTestCase ):
             raise AssertionError( "Purging the role did not delete the DatasetPermissionss for role_id '%s'" % role_id )
     def test_245_manually_unpurge_role( self ):
         """Testing manually un-purging a role"""
+        # Logged in as admin_user
         # Manually unpurge, then undelete the role for later test runs
         # TODO: If we decide to implement the GUI feature for un-purging a role, replace this with a method call
         role_two.purged = False
@@ -1513,6 +1530,7 @@ class TestSecurityAndLibraries( TwillTestCase ):
         self.undelete_role( str( role_two.id ), role_two.name )
     def test_250_purge_library( self ):
         """Testing purging a library"""
+        # Logged in as admin_user
         self.home()
         self.delete_library_item( str( library_one.id ), str( library_one.id ), library_one.name, library_item_type='library' )
         self.purge_library( str( library_one.id ), library_one.name )
@@ -1549,6 +1567,7 @@ class TestSecurityAndLibraries( TwillTestCase ):
         check_folder( library_one.root_folder )
     def test_255_no_library_template( self ):
         """Test library features when library has no template"""
+        # Logged in as admin_user
         name = "Library Two"
         description = "This is Library Two"
         # Create a library, adding no template
@@ -1585,6 +1604,7 @@ class TestSecurityAndLibraries( TwillTestCase ):
         self.home()
     def test_260_library_permissions( self ):
         """Test library permissions"""
+        # Logged in as admin_user
         name = "Library Three"
         description = "This is Library Three"
         # Create a library, adding no template
@@ -1596,12 +1616,30 @@ class TestSecurityAndLibraries( TwillTestCase ):
                                                            galaxy.model.Library.table.c.description==description,
                                                            galaxy.model.Library.table.c.deleted==False ) ).first()
         assert library_three is not None, 'Problem retrieving library named "%s" from the database' % name
-        # TODO: add tests here...
-        self.home()
+        # Set library permissions for regular_user1 and regular_user2.  Each of these users will be permitted to
+        # LIBRARY_ADD, LIBRARY_MODIFY, LIBRARY_MANAGE for library items.
+        permissions_in = [ k for k, v in galaxy.model.Library.permitted_actions.items() ]
+        permissions_out = []
+        role_ids_str = '%s,%s' % ( str( regular_user1_private_role.id ), str( regular_user2_private_role.id ) )
+        self.set_library_permissions( str( library_three.id ), library_three.name, role_ids_str, permissions_in, permissions_out )
+        self.logout()
+        # Login as regular_user1 and make sure they can see the library
+        self.login( email=regular_user1.email )
+        self.visit_url( '%s/library/browse_libraries' % self.url )
+        self.check_page_for_string( name )
+        self.logout()
+        # Login as regular_user1 and make sure they can see the library
+        self.login( email=regular_user2.email )
+        self.visit_url( '%s/library/browse_libraries' % self.url )
+        self.check_page_for_string( name )
+        # TODO: add more tests here to cover adding, modifying, managing permissions from the Librarys persoective as a regular user
+        self.logout()
+        self.login( email=admin_user.email )
         self.delete_library_item( str( library_three.id ), str( library_three.id ), library_three.name, library_item_type='library' )
         self.purge_library( str( library_three.id ), library_three.name )
     def test_265_reset_data_for_later_test_runs( self ):
         """Reseting data to enable later test runs to pass"""
+        # Logged in as admin_user
         ##################
         # Eliminate all non-private roles
         ##################