Bug fixes and code cleanup for managing users, groups and roles.

lib/galaxy/web/controllers/admin.py

@@ -159,7 +159,7 @@ class RoleListGrid( grids.Grid ):
     columns = [
         NameColumn( "Name",
                     key="name",
-                    link=( lambda item: dict( controller="admin", action="role", id=item.id ) ),
+                    link=( lambda item: dict( operation="Manage users and groups", id=item.id ) ),
                     model_class=model.Role,
                     attach_popup=True,
                     filterable="advanced" ),
@@ -187,7 +187,8 @@ class RoleListGrid( grids.Grid ):
     global_actions = [
         grids.GridAction( "Add new role", dict( controller='admin', action='roles', operation='create' ) )
     ]
-    operations = [ grids.GridOperation( "Delete", condition=( lambda item: not item.deleted ), allow_multiple=True ),
+    operations = [ grids.GridOperation( "Rename", condition=( lambda item: not item.deleted ), allow_multiple=False ),
+                   grids.GridOperation( "Delete", condition=( lambda item: not item.deleted ), allow_multiple=True ),
                    grids.GridOperation( "Undelete", condition=( lambda item: item.deleted ), allow_multiple=True ),
                    grids.GridOperation( "Purge", condition=( lambda item: item.deleted ), allow_multiple=True ) ]
     standard_filters = [
@@ -243,7 +244,7 @@ class GroupListGrid( grids.Grid ):
     columns = [
         NameColumn( "Name",
                     key="name",
-                    link=( lambda item: dict( controller="admin", action="group", id=item.id ) ),
+                    link=( lambda item: dict( operation="Manage users and roles", id=item.id ) ),
                     model_class=model.Group,
                     attach_popup=True,
                     filterable="advanced" ),
@@ -261,7 +262,8 @@ class GroupListGrid( grids.Grid ):
     global_actions = [
         grids.GridAction( "Add new group", dict( controller='admin', action='groups', operation='create' ) )
     ]
-    operations = [ grids.GridOperation( "Delete", condition=( lambda item: not item.deleted ), allow_multiple=True ),
+    operations = [ grids.GridOperation( "Rename", condition=( lambda item: not item.deleted ), allow_multiple=False ),
+                   grids.GridOperation( "Delete", condition=( lambda item: not item.deleted ), allow_multiple=True ),
                    grids.GridOperation( "Undelete", condition=( lambda item: item.deleted ), allow_multiple=True ),
                    grids.GridOperation( "Purge", condition=( lambda item: item.deleted ), allow_multiple=True ) ]
     standard_filters = [
@@ -327,8 +329,10 @@ class Admin( BaseController ):
                 return self.undelete_role( trans, **kwargs )
             if operation == "purge":
                 return self.purge_role( trans, **kwargs )
-            if operation == "manage users & groups":
-                return self.role( trans, **kwargs )
+            if operation == "manage users and groups":
+                return self.manage_users_and_groups_for_role( trans, **kwargs )
+            if operation == "rename":
+                return self.rename_role( trans, **kwargs )
         # Render the list view
         return self.role_list_grid( trans, **kwargs )
     @web.expose
@@ -389,7 +393,32 @@ class Admin( BaseController ):
                                     messagetype=messagetype )
     @web.expose
     @web.require_admin
-    def role( self, trans, **kwd ):
+    def rename_role( self, trans, **kwd ):
+        params = util.Params( kwd )
+        msg = util.restore_text( params.get( 'msg', ''  ) )
+        messagetype = params.get( 'messagetype', 'done' )
+        role = get_role( trans, params.id )
+        if params.get( 'rename_role_button', False ):
+            old_name = role.name
+            new_name = util.restore_text( params.name )
+            new_description = util.restore_text( params.description )
+            if not new_name:
+                msg = 'Enter a valid name'
+                return trans.fill_template( '/admin/dataset_security/role/role_rename.mako', role=role, msg=msg, messagetype='error' )
+            elif trans.sa_session.query( trans.app.model.Role ).filter( trans.app.model.Role.table.c.name==new_name ).first():
+                msg = 'A role with that name already exists'
+                return trans.fill_template( '/admin/dataset_security/role/role_rename.mako', role=role, msg=msg, messagetype='error' )
+            else:
+                role.name = new_name
+                role.description = new_description
+                trans.sa_session.add( role )
+                trans.sa_session.flush()
+                msg = "Role '%s' has been renamed to '%s'" % ( old_name, new_name )
+                return trans.response.send_redirect( web.url_for( action='roles', message=util.sanitize_text( msg ), status='done' ) )
+        return trans.fill_template( '/admin/dataset_security/role/role_rename.mako', role=role, msg=msg, messagetype=messagetype )
+    @web.expose
+    @web.require_admin
+    def manage_users_and_groups_for_role( self, trans, **kwd ):
         params = util.Params( kwd )
         msg = util.restore_text( params.get( 'msg', ''  ) )
         messagetype = params.get( 'messagetype', 'done' )
@@ -413,26 +442,7 @@ class Admin( BaseController ):
             trans.app.security_agent.set_entity_role_associations( roles=[ role ], users=in_users, groups=in_groups )
             trans.sa_session.refresh( role )
             msg = "Role '%s' has been updated with %d associated users and %d associated groups" % ( role.name, len( in_users ), len( in_groups ) )
-            trans.response.send_redirect( web.url_for( action='roles', message=util.sanitize_text( msg ), status=messagetype ) )
-        elif params.get( 'rename', False ):
-            if params.rename == 'submitted':
-                old_name = role.name
-                new_name = util.restore_text( params.name )
-                new_description = util.restore_text( params.description )
-                if not new_name:
-                    msg = 'Enter a valid name'
-                    return trans.fill_template( '/admin/dataset_security/role/role_rename.mako', role=role, msg=msg, messagetype='error' )
-                elif trans.sa_session.query( trans.app.model.Role ).filter( trans.app.model.Role.table.c.name==new_name ).first():
-                    msg = 'A role with that name already exists'
-                    return trans.fill_template( '/admin/dataset_security/role/role_rename.mako', role=role, msg=msg, messagetype='error' )
-                else:
-                    role.name = new_name
-                    role.description = new_description
-                    trans.sa_session.add( role )
-                    trans.sa_session.flush()
-                    msg = "Role '%s' has been renamed to '%s'" % ( old_name, new_name )
-                    return trans.response.send_redirect( web.url_for( action='roles', message=util.sanitize_text( msg ), status='done' ) )
-            return trans.fill_template( '/admin/dataset_security/role/role_rename.mako', role=role, msg=msg, messagetype=messagetype )
+            trans.response.send_redirect( web.url_for( action='roles', message=util.sanitize_text( msg ), status=messagetype ) )            
         in_users = []
         out_users = []
         in_groups = []
@@ -561,13 +571,38 @@ class Admin( BaseController ):
                 return self.undelete_group( trans, **kwargs )
             if operation == "purge":
                 return self.purge_group( trans, **kwargs )
-            if operation == "manage users & roles":
-                return self.group( trans, **kwargs )
+            if operation == "manage users and roles":
+                return self.manage_users_and_roles_for_group( trans, **kwargs )
+            if operation == "rename":
+                return self.rename_group( trans, **kwargs )
         # Render the list view
         return self.group_list_grid( trans, **kwargs )
     @web.expose
     @web.require_admin
-    def group( self, trans, **kwd ):
+    def rename_group( self, trans, **kwd ):
+        params = util.Params( kwd )
+        msg = util.restore_text( params.get( 'msg', ''  ) )
+        messagetype = params.get( 'messagetype', 'done' )
+        group = get_group( trans, params.id )
+        if params.get( 'rename_group_button', False ):
+            old_name = group.name
+            new_name = util.restore_text( params.name )
+            if not new_name:
+                msg = 'Enter a valid name'
+                return trans.fill_template( '/admin/dataset_security/group/group_rename.mako', group=group, msg=msg, messagetype='error' )
+            elif trans.sa_session.query( trans.app.model.Group ).filter( trans.app.model.Group.table.c.name==new_name ).first():
+                msg = 'A group with that name already exists'
+                return trans.fill_template( '/admin/dataset_security/group/group_rename.mako', group=group, msg=msg, messagetype='error' )
+            else:
+                group.name = new_name
+                trans.sa_session.add( group )
+                trans.sa_session.flush()
+                msg = "Group '%s' has been renamed to '%s'" % ( old_name, new_name )
+                return trans.response.send_redirect( web.url_for( action='groups', msg=util.sanitize_text( msg ), messagetype='done' ) )
+        return trans.fill_template( '/admin/dataset_security/group/group_rename.mako', group=group, msg=msg, messagetype=messagetype )
+    @web.expose
+    @web.require_admin
+    def manage_users_and_roles_for_group( self, trans, **kwd ):
         params = util.Params( kwd )
         msg = util.restore_text( params.get( 'msg', ''  ) )
         messagetype = params.get( 'messagetype', 'done' )
@@ -579,23 +614,6 @@ class Admin( BaseController ):
             trans.sa_session.refresh( group )
             msg += "Group '%s' has been updated with %d associated roles and %d associated users" % ( group.name, len( in_roles ), len( in_users ) )
             trans.response.send_redirect( web.url_for( action='groups', message=util.sanitize_text( msg ), status=messagetype ) )
-        if params.get( 'rename', False ):
-            if params.rename == 'submitted':
-                old_name = group.name
-                new_name = util.restore_text( params.name )
-                if not new_name:
-                    msg = 'Enter a valid name'
-                    return trans.fill_template( '/admin/dataset_security/group/group_rename.mako', group=group, msg=msg, messagetype='error' )
-                elif trans.sa_session.query( trans.app.model.Group ).filter( trans.app.model.Group.table.c.name==new_name ).first():
-                    msg = 'A group with that name already exists'
-                    return trans.fill_template( '/admin/dataset_security/group/group_rename.mako', group=group, msg=msg, messagetype='error' )
-                else:
-                    group.name = new_name
-                    trans.sa_session.add( group )
-                    trans.sa_session.flush()
-                    msg = "Group '%s' has been renamed to '%s'" % ( old_name, new_name )
-                    return trans.response.send_redirect( web.url_for( action='groups', msg=util.sanitize_text( msg ), messagetype='done' ) )
-            return trans.fill_template( '/admin/dataset_security/group/group_rename.mako', group=group, msg=msg, messagetype=messagetype )
         in_roles = []
         out_roles = []
         in_users = []
@@ -943,7 +961,7 @@ class Admin( BaseController ):
             if operation == "information":
                 return self.user_info( trans, **kwargs )
             if operation == "manage roles and groups":
-                return self.user( trans, **kwargs )
+                return self.manage_roles_and_groups_for_user( trans, **kwargs )
         # Render the list view
         return self.user_list_grid( trans, **kwargs )
     @web.expose
@@ -976,7 +994,7 @@ class Admin( BaseController ):
         return ac_data
     @web.expose
     @web.require_admin
-    def user( self, trans, **kwd ):
+    def manage_roles_and_groups_for_user( self, trans, **kwd ):
         user_id = kwd.get( 'id', None )
         message = ''
         status = ''

templates/admin/dataset_security/group/group.mako

@@ -50,7 +50,7 @@ $().ready(function() {
 <div class="toolForm">
     <div class="toolFormTitle">Group '${group.name}'</div>
     <div class="toolFormBody">
-        <form name="associate_group_role_user" id="associate_group_role_user" action="${h.url_for( action='group', group_id=group.id )}" method="post" >
+        <form name="associate_group_role_user" id="associate_group_role_user" action="${h.url_for( action='manage_users_and_roles_for_group', id=trans.security.encode_id( group.id ) )}" method="post" >
             <div class="form-row">
                 <div style="float: left; margin-right: 10px;">
                     <label>Roles associated with '${group.name}'</label>

templates/admin/dataset_security/group/group_rename.mako

@@ -8,7 +8,7 @@
 <div class="toolForm">
     <div class="toolFormTitle">Change group name</div>
     <div class="toolFormBody">
-        <form name="library" action="${h.url_for( controller='admin', action='group' )}" method="post" >
+        <form name="library" action="${h.url_for( controller='admin', action='rename_group' )}" method="post" >
             <div class="form-row">
                 <label>Name:</label>
                 <div style="float: left; width: 250px; margin-right: 10px;">
@@ -28,7 +28,9 @@
                 </div>
                 <div style="clear: both"></div>
             </div>
-            <input type="submit" name="rename_group_button" value="Save"/>
+            <div class="form-row">
+                <input type="submit" name="rename_group_button" value="Save"/>
+            </div>
         </form>
     </div>
 </div>

templates/admin/dataset_security/role/role.mako

@@ -50,7 +50,7 @@ $().ready(function() {
 <div class="toolForm">
     <div class="toolFormTitle">Role '${role.name}'</div>
     <div class="toolFormBody">
-        <form name="associate_role_user_group" id="associate_role_user_group" action="${h.url_for( action='role', role_id=role.id )}" method="post" >
+        <form name="associate_role_user_group" id="associate_role_user_group" action="${h.url_for( action='manage_users_and_groups_for_role', id=trans.security.encode_id( role.id ) )}" method="post" >
             <div class="form-row">
                 <div style="float: left; margin-right: 10px;">
                     <label>Users associated with '${role.name}'</label>

templates/admin/dataset_security/role/role_rename.mako

@@ -8,7 +8,7 @@
 <div class="toolForm">
     <div class="toolFormTitle">Change role name and description</div>
     <div class="toolFormBody">
-        <form name="library" action="${h.url_for( controller='admin', action='role' )}" method="post" >
+        <form name="library" action="${h.url_for( controller='admin', action='rename_role' )}" method="post" >
             <div class="form-row">
                 <label>Name:</label>
                 <div style="float: left; width: 250px; margin-right: 10px;">
@@ -35,7 +35,9 @@
                 </div>
                 <div style="clear: both"></div>
             </div>
-            <input type="submit" name="rename_role_button" value="Save"/>
+            <div class="form-row">
+                <input type="submit" name="rename_role_button" value="Save"/>
+            </div>
         </form>
     </div>
 </div>

templates/admin/user/user.mako

@@ -50,7 +50,7 @@ $().ready(function() {
 <div class="toolForm">
     <div class="toolFormTitle">User '${user.email}'</div>
     <div class="toolFormBody">
-        <form name="associate_user_role_group" id="associate_user_role_group" action="${h.url_for( action='user', id=trans.app.security.encode_id( user.id ) )}" method="post" >
+        <form name="associate_user_role_group" id="associate_user_role_group" action="${h.url_for( action='manage_roles_and_groups_for_user', id=trans.security.encode_id( user.id ) )}" method="post" >
             <div class="form-row">
                 <div style="float: left; margin-right: 10px;">
                     <label>Roles associated with '${user.email}'</label>

test/base/twilltestcase.py

@@ -960,21 +960,21 @@ class TwillTestCase( unittest.TestCase ):
     def mark_user_deleted( self, user_id, email='' ):
         """Mark a user as deleted"""
         self.home()
-        self.visit_url( "%s/admin/mark_user_deleted?id=%s" % ( self.url, user_id ) )
+        self.visit_url( "%s/admin/users?operation=delete&id=%s" % ( self.url, user_id ) )
         check_str = "Deleted 1 users"
         self.check_page_for_string( check_str )
         self.home()
     def undelete_user( self, user_id, email='' ):
         """Undelete a user"""
         self.home()
-        self.visit_url( "%s/admin/undelete_user?id=%s" % ( self.url, user_id ) )
+        self.visit_url( "%s/admin/users?operation=undelete&id=%s" % ( self.url, user_id ) )
         check_str = "Undeleted 1 users"
         self.check_page_for_string( check_str )
         self.home()
     def purge_user( self, user_id, email ):
         """Purge a user account"""
         self.home()
-        self.visit_url( "%s/admin/purge_user?id=%s" % ( self.url, user_id ) )
+        self.visit_url( "%s/admin/users?operation=purge&id=%s" % ( self.url, user_id ) )
         check_str = "Purged 1 users"
         self.check_page_for_string( check_str )
         self.home()
@@ -983,7 +983,7 @@ class TwillTestCase( unittest.TestCase ):
                                               in_group_ids=[], out_group_ids=[],
                                               check_str='' ):
         self.home()
-        url = "%s/admin/user?id=%s&user_roles_groups_edit_button=Save" % ( self.url, user_id )
+        url = "%s/admin/manage_roles_and_groups_for_user?id=%s&user_roles_groups_edit_button=Save" % ( self.url, user_id )
         if in_role_ids:
             url += "&in_roles=%s" % ','.join( in_role_ids )
         if out_role_ids:
@@ -1006,7 +1006,7 @@ class TwillTestCase( unittest.TestCase ):
                      create_group_for_role='no',
                      private_role='' ):
         """Create a new role"""
-        url = "%s/admin/create_role?create_role_button=Save&name=%s&description=%s" % ( self.url, name.replace( ' ', '+' ), description.replace( ' ', '+' ) )
+        url = "%s/admin/roles?operation=create&create_role_button=Save&name=%s&description=%s" % ( self.url, name.replace( ' ', '+' ), description.replace( ' ', '+' ) )
         if in_user_ids:
             url += "&in_users=%s" % ','.join( in_user_ids )
         if in_group_ids:
@@ -1038,7 +1038,7 @@ class TwillTestCase( unittest.TestCase ):
     def rename_role( self, role_id, name='Role One Renamed', description='This is Role One Re-described' ):
         """Rename a role"""
         self.home()
-        self.visit_url( "%s/admin/role?rename=True&id=%s" % ( self.url, role_id ) )
+        self.visit_url( "%s/admin/roles?operation=rename&id=%s" % ( self.url, role_id ) )
         self.check_page_for_string( 'Change role name and description' )
         tc.fv( "1", "name", name )
         tc.fv( "1", "description", description )
@@ -1081,7 +1081,7 @@ class TwillTestCase( unittest.TestCase ):
     # Tests associated with groups
     def create_group( self, name='Group One', in_user_ids=[], in_role_ids=[] ):
         """Create a new group"""
-        url = "%s/admin/create_group?create_group_button=Save&name=%s" % ( self.url, name.replace( ' ', '+' ) )
+        url = "%s/admin/groups?operation=create&create_group_button=Save&name=%s" % ( self.url, name.replace( ' ', '+' ) )
         if in_user_ids:
             url += "&in_users=%s" % ','.join( in_user_ids )
         if in_role_ids:
@@ -1097,14 +1097,14 @@ class TwillTestCase( unittest.TestCase ):
     def rename_group( self, group_id, name='Group One Renamed' ):
         """Rename a group"""
         self.home()
-        self.visit_url( "%s/admin/group?rename=True&id=%s" % ( self.url, group_id ) )
+        self.visit_url( "%s/admin/groups?operation=rename&id=%s" % ( self.url, group_id ) )
         self.check_page_for_string( 'Change group name' )
         tc.fv( "1", "name", name )
         tc.submit( "rename_group_button" )
         self.home()
     def associate_users_and_roles_with_group( self, group_id, group_name, user_ids=[], role_ids=[] ):
         self.home()
-        url = "%s/admin/group?id=%s&group_roles_users_edit_button=Save" % ( self.url, group_id )
+        url = "%s/admin/manage_users_and_roles_for_group?id=%s&group_roles_users_edit_button=Save" % ( self.url, group_id )
         if user_ids:
             url += "&in_users=%s" % ','.join( user_ids )
         if role_ids:

test/functional/test_security_and_libraries.py

@@ -21,7 +21,7 @@ class TestSecurityAndLibraries( TwillTestCase ):
         self.check_page_for_string( not_logged_in_security_msg )
         self.visit_url( "%s/admin/create_role" % self.url )
         self.check_page_for_string( not_logged_in_security_msg )
-        self.visit_url( "%s/admin/role" % self.url )
+        self.visit_url( "%s/admin/manage_users_and_groups_for_role" % self.url )
         self.check_page_for_string( not_logged_in_security_msg )
         self.visit_url( "%s/admin/groups" % self.url )
         self.check_page_for_string( not_logged_in_security_msg )
@@ -76,7 +76,7 @@ class TestSecurityAndLibraries( TwillTestCase ):
             raise AssertionError( 'The DefaultHistoryPermission.action for history id %d is "%s", but it should be "%s"' \
                                   % ( latest_history.id, dhp.action, galaxy.model.Dataset.permitted_actions.DATASET_MANAGE_PERMISSIONS.action ) )
         self.home()
-        self.visit_url( "%s/admin/user?id=%s" % ( self.url, self.security.encode_id( admin_user.id ) ) )
+        self.visit_url( "%s/admin/manage_roles_and_groups_for_user?id=%s" % ( self.url, self.security.encode_id( admin_user.id ) ) )
         self.check_page_for_string( admin_user.email )
         # Try deleting the admin_user's private role
         check_str = "You cannot eliminate a user's private role association."