Merge branch 'release/0.7.3'

README.rst

@@ -195,6 +195,13 @@ DB Models
 Changelog
 =========
 
+v. 0.7.3
+--------
+
+* UX enhancements (`#149 <https://github.com/libris/xl_auth/issues/149>`_,
+  `#146 <https://github.com/libris/xl_auth/issues/146>`_)
+
+
 v. 0.7.2
 --------
 

package.json

@@ -1,6 +1,6 @@
 {
   "name": "xl_auth",
-  "version": "0.7.2",
+  "version": "0.7.3",
   "author": "National Library of Sweden",
   "license": "Apache-2.0",
   "description": "Authorization and OAuth2 provider for LibrisXL",

tests/end2end/test_permission_editing.py

@@ -120,13 +120,14 @@ def test_cataloging_admin_can_edit_permission_from_user_view(user, permission, s
     assert res.status_code == 302
     assert url_for('permission.edit', permission_id=permission.id) in res.location
     other_user = User.get_by_email('[email protected]')
-    # Fills out the form to grant 'other_user' permissions on 'collection'
-    res = res.follow()
+    assert other_user is not None
+    # Saves the form to grant 'other_user' permissions on 'collection'
+    res = res.follow(headers={'Referer': res.request.referrer})  # FIXME: Webtest dropping referer.
     assert res.status_code == 200
     # Fills out the form, by changing to 'other_user''
     form = res.forms['editPermissionForm']
+    # New user is preset, ``form['user_id'] = other_user.id`` is redundant
     # Defaults are kept, setting ``form['collection_id'] = permission.collection.id`` is redundant
-    form['user_id'] = other_user.id
     # Submits
     res = form.submit()
     assert res.status_code == 302

tests/end2end/test_permission_registering.py

@@ -75,11 +75,12 @@ def test_cataloging_admin_can_register_permission_from_collection_view(user, col
     assert res.status_code == 302
     assert url_for('permission.register', collection_id=collection.id) in res.location
     other_user = User.get_by_email('[email protected]')
-    # Fills out the form to grant 'other_user' permissions on 'collection'
-    res = res.follow()
+    assert other_user is not None
+    # Saves the form to grant 'other_user' permissions on 'collection'
+    res = res.follow(headers={'Referer': res.request.referrer})  # FIXME: Webtest dropping referer.
     assert res.status_code == 200
     register_permission_form = res.forms['registerPermissionForm']
-    register_permission_form['user_id'] = other_user.id
+    # New user is preset, ``register_permission_form['user_id'] = other_user.id`` is redundant
     # Defaults are kept, ``register_permission_form['collection_id'] = collection.id`` is redundant
     register_permission_form['registrant'].checked = True
     register_permission_form['cataloger'].checked = True

tests/forms/test_permission_edit.py

@@ -31,6 +31,15 @@ def test_validate_inconsistent_permission_id(permission, superuser):
         form.permission_id.errors
 
 
+def test_validate_with_placeholder_user_id(superuser, permission):
+    """Attempt editing entry with the placeholder ID."""
+    form = EditForm(superuser, permission.id, permission_id=permission.id,
+                    user_id='-1', collection_id=permission.collection.id)
+
+    assert form.validate() is False
+    assert _('A user must be selected.') in form.user_id.errors
+
+
 def test_validate_without_user_id(superuser, permission):
     """Attempt editing entry with empty string for user ID."""
     form = EditForm(superuser, permission.id, permission_id=permission.id,
@@ -40,6 +49,15 @@ def test_validate_without_user_id(superuser, permission):
     assert _('This field is required.') in form.user_id.errors
 
 
+def test_validate_with_placeholder_collection_id(superuser, permission):
+    """Attempt editing entry with the placeholder ID."""
+    form = EditForm(superuser, permission.id, permission_id=permission.id,
+                    user_id=permission.user.id, collection_id='-1')
+
+    assert form.validate() is False
+    assert _('A collection must be selected.') in form.collection_id.errors
+
+
 def test_validate_without_collection_id(superuser, permission):
     """Attempt editing entry with empty string for collection ID."""
     form = EditForm(superuser, permission.id, permission_id=permission.id,

tests/forms/test_permission_register.py

@@ -10,6 +10,14 @@
 from ..factories import PermissionFactory, UserFactory
 
 
+def test_validate_with_placeholder_user_id(superuser, collection):
+    """Attempt registering entry with the placeholder ID."""
+    form = RegisterForm(superuser, user_id='-1', collection_id=collection.id)
+
+    assert form.validate() is False
+    assert _('A user must be selected.') in form.user_id.errors
+
+
 def test_validate_without_user_id(superuser, collection):
     """Attempt registering entry with empty string for user ID."""
     form = RegisterForm(superuser, user_id='', collection_id=collection.id)
@@ -18,6 +26,14 @@ def test_validate_without_user_id(superuser, collection):
     assert _('This field is required.') in form.user_id.errors
 
 
+def test_validate_with_placeholder_collection_id(superuser, user):
+    """Attempt registering entry with the placeholder ID."""
+    form = RegisterForm(superuser, user_id=user.id, collection_id='-1')
+
+    assert form.validate() is False
+    assert _('A collection must be selected.') in form.collection_id.errors
+
+
 def test_validate_without_collection_id(superuser, user):
     """Attempt registering entry with empty string for collection ID."""
     form = RegisterForm(superuser, user_id=user.id, collection_id='')

xl_auth/permission/forms.py

@@ -28,24 +28,27 @@ def __init__(self, current_user, *args, **kwargs):
         """Create instance."""
         super(PermissionForm, self).__init__(*args, **kwargs)
         self.current_user = current_user
-        self.user_id.choices = [(user.id, user.email)
-                                for user in User.query.order_by('email').all()]
+        self.user_id.choices = [(-1, _('--- Select User ---'))] + [
+            (user.id, user.email) for user in User.query.order_by('email').all()]
 
+        self.collection_id.choices = [(-1, _('--- Select Collection ---'))]
         if current_user.is_admin:
-            self.collection_id.choices = [
+            self.collection_id.choices += [
                 (collection.id, collection.code)
                 for collection in Collection.query.filter_by(is_active=True).order_by('code').all()
             ]
         else:
-            self.collection_id.choices = sorted(
+            self.collection_id.choices += sorted(
                 [(permission.collection.id, permission.collection.code)
                  for permission in current_user.get_cataloging_admin_permissions()],
                 key=lambda _: _[1]
             )
 
     # noinspection PyMethodMayBeStatic
     def validate_user_id(self, field):
-        """Validate user ID exists in 'users' table."""
+        """Validate user ID is selected and exists in 'users' table."""
+        if field.data == -1:
+            raise ValidationError(_('A user must be selected.'))
         if not User.get_by_id(field.data):
             raise ValidationError(_('User ID "%(user_id)s" does not exist', user_id=field.data))
 
@@ -61,6 +64,8 @@ def set_defaults(self, user_id, collection_id):
 
     def validate_collection_id(self, field):
         """Validate collection ID exists and current user may register permissions on it."""
+        if field.data == -1:
+            raise ValidationError(_('A collection must be selected.'))
         collection = Collection.get_by_id(field.data)
         if collection:
             if not (self.current_user.is_cataloging_admin_for(collection) or
@@ -105,10 +110,10 @@ def __init__(self, current_user, target_permission_id, *args, **kwargs):
         self.target_permission_id = target_permission_id
         self.permission_id.validators = [AnyOf([target_permission_id])]
 
-    def set_defaults(self, permission):
-        """Apply 'permission' attributes as field defaults."""
+    def set_defaults(self, permission, new_user_id=None):
+        """Apply 'permission' attributes as field defaults, maybe overridden by 'new_user_id'."""
         self.permission_id.default = permission.id
-        self.user_id.default = permission.user_id
+        self.user_id.default = new_user_id or permission.user_id
         self.collection_id.default = permission.collection_id
         self.registrant.default = permission.registrant
         self.cataloger.default = permission.cataloger
@@ -124,18 +129,16 @@ def validate_permission_id(self, field):
                                     permission_id=self.target_permission_id))
         current_collection = target_permission.collection
         form_collection = Collection.get_by_id(self.collection_id.data)
-        if not form_collection:
-            raise ValidationError(_('Collection ID "%(collection_id)s" does not exist',
-                                    collection_id=self.collection_id.data))
-
-        if not (self.current_user.is_cataloging_admin_for(
+        if form_collection and not (self.current_user.is_cataloging_admin_for(
                 current_collection, form_collection) or self.current_user.is_admin):
             raise ValidationError(_('You do not have sufficient privileges '
                                     'for this operation.'))
 
     # noinspection PyMethodMayBeStatic
     def validate_collection_id(self, field):
-        """Validate collection ID exists in 'collections' table."""
+        """Validate collection ID is selected and exists in 'collections' table."""
+        if field.data == -1:
+            raise ValidationError(_('A collection must be selected.'))
         if not Collection.get_by_id(field.data):
             raise ValidationError(_('Collection ID "%(collection_id)s" does not exist',
                                     collection_id=field.data))

xl_auth/permission/views.py

@@ -3,7 +3,7 @@
 
 from __future__ import absolute_import, division, print_function, unicode_literals
 
-from flask import Blueprint, abort, flash, redirect, render_template, request
+from flask import Blueprint, abort, flash, redirect, render_template, request, url_for
 from flask_babel import lazy_gettext as _
 from flask_login import current_user, login_required
 from sqlalchemy.orm import contains_eager
@@ -61,6 +61,8 @@ def register(user_id, collection_id):
         else:
             flash_errors(register_permission_form)
 
+    if request.referrer and url_for('user.register') in request.referrer:
+        user_id = User.query.filter_by(created_by=current_user).order_by(-User.id).all()[0].id
     register_permission_form.set_defaults(user_id, collection_id)
     return render_template('permissions/register.html',
                            register_permission_form=register_permission_form,
@@ -91,7 +93,11 @@ def edit(permission_id):
         else:
             flash_errors(edit_permission_form)
 
-    edit_permission_form.set_defaults(permission)
+    if request.referrer and url_for('user.register') in request.referrer:
+        new_user_id = User.query.filter_by(created_by=current_user).order_by(-User.id).all()[0].id
+        edit_permission_form.set_defaults(permission, new_user_id=new_user_id)
+    else:
+        edit_permission_form.set_defaults(permission)
     return render_template('permissions/edit.html', permission=permission,
                            edit_permission_form=edit_permission_form,
                            full_path_quoted=quote(request.full_path),

xl_auth/templates/users/inspect.html

@@ -24,28 +24,28 @@ <h1>{{ _('Inspect User \'%(email)s\'', email=user.email) }}</h1>
             <dd>{{ _('Yes') if user.is_cataloging_admin else _('No') }}</dd>
 
             <dt>{{ _('Users Created') }}:</dt>
-            <dd>{{ users_created }}</dd>
+            <dd>{{ num_users_created }}</dd>
 
             <dt>{{ _('Users Modified') }}:</dt>
-            <dd>{{ users_modified }}</dd>
+            <dd>{{ num_users_modified }}</dd>
 
             <dt>{{ _('Permissions Created') }}:</dt>
-            <dd>{{ permissions_created }}</dd>
+            <dd>{{ num_permissions_created }}</dd>
 
             <dt>{{ _('Permissions Modified') }}:</dt>
-            <dd>{{ permissions_modified }}</dd>
+            <dd>{{ num_permissions_modified }}</dd>
 
             <dt>{{ _('Collections Created') }}:</dt>
-            <dd>{{ collections_created }}</dd>
+            <dd>{{ num_collections_created }}</dd>
 
             <dt>{{ _('Collections Modified') }}:</dt>
-            <dd>{{ collections_modified }}</dd>
+            <dd>{{ num_collections_modified }}</dd>
 
             <dt>{{ _('Clients Created') }}:</dt>
-            <dd>{{ clients_created }}</dd>
+            <dd>{{ num_clients_created }}</dd>
 
             <dt>{{ _('Clients Modified') }}:</dt>
-            <dd>{{ clients_modified }}</dd>
+            <dd>{{ num_clients_modified }}</dd>
 
             <dt>{{ _('Last Modified') }}:</dt>
             <dd>
@@ -66,7 +66,7 @@ <h1>{{ _('Inspect User \'%(email)s\'', email=user.email) }}</h1>
     </div>
     <div class="panel panel-default">
         <div class="panel-heading">
-            <strong>{{ _('Permissions') }}</strong>
+            <strong>{{ _('Own Permissions') }} ({{ user.permissions | length }})</strong>
         </div>
         <table class="table table-hover">
             <thead>
@@ -99,7 +99,8 @@ <h1>{{ _('Inspect User \'%(email)s\'', email=user.email) }}</h1>
                     <td class="{{ "bool-value-{}".format(permission.collection.is_active).lower() }}">
                         {{ _('Yes') if permission.collection.is_active else _('No') }}
                     </td>
-                    <td>{{ permission.modified_at.strftime('%y-%m-%d') }}, {{  _('by') }}
+                    <td>
+                        {{ permission.modified_at.strftime('%y-%m-%d') }}, {{  _('by') }}
                         <a href="{{ url_for('user.inspect', user_id=permission.modified_by.id) }}">
                             {{ permission.modified_by.full_name }}
                         </a>
@@ -109,6 +110,102 @@ <h1>{{ _('Inspect User \'%(email)s\'', email=user.email) }}</h1>
             </tbody>
         </table>
     </div>
+    <div class="panel panel-default">
+        <div class="panel-heading">
+            <strong>
+                {{ _('Permissions Created or Modified by This User') }}
+                ({{ permissions_created_or_modified | length }})
+            </strong>
+        </div>
+        <table class="table table-hover">
+            <thead>
+            <tr>
+                <th class="col-md-1">{{ _('Collection') }}</th>
+                <th class="col-md-3">{{ _('Email') }}</th>
+                <th class="col-md-3 word-break-all">{{ _('Registrant') }}</th>
+                <th class="col-md-2 word-break-all">{{ _('Cataloger') }}</th>
+                <th class="col-md-1">{{ _('CatAdm') }}</th>
+                <th class="col-md-2">{{ _('Created At') }}</th>
+            </tr>
+            </thead>
+            <tbody>
+            {% for permission in permissions_created_or_modified | sort(attribute='collection.code') %}
+                <tr>
+                    <td>
+                        <a href="{{ url_for('collection.view', collection_code=permission.collection.code) }}">
+                            {{ permission.collection.code }}
+                        </a>
+                    </td>
+                    <td class="word-break-all">
+                        <a href="{{ url_for('user.inspect', user_id=permission.user.id) }}">
+                            {{ permission.user.email }}
+                        </a>
+                    </td>
+                    <td class="{{ "bool-value-{}".format(permission.registrant).lower() }}">
+                        {{ _('Yes') if permission.registrant else _('No') }}
+                    </td>
+                    <td class="{{ "bool-value-{}".format(permission.cataloger).lower() }}">
+                        {{ _('Yes') if permission.cataloger else _('No') }}
+                    </td>
+                    <td class="{{ "bool-value-{}".format(permission.cataloging_admin).lower() }}">
+                        {{ _('Yes') if permission.cataloging_admin else _('No') }}
+                    </td>
+                    <td>
+                        {{ permission.created_at.strftime('%y-%m-%d') }}, {{  _('by') }}
+                        <a href="{{ url_for('user.inspect', user_id=permission.created_by.id) }}">
+                            {{ permission.created_by.full_name }}
+                        </a>
+                    </td>
+                </tr>
+            {% endfor %}
+            </tbody>
+        </table>
+    </div>
+    <div class="panel panel-default">
+        <div class="panel-heading">
+            <strong>
+                {{ _('Users Created or Modified by This One') }}
+                ({{ users_created_or_modified | length }})
+            </strong>
+        </div>
+        <table class="table table-hover">
+            <thead>
+            <tr>
+                <th class="col-md-5">{{ _('Email') }}</th>
+                <th class="col-md-1">{{ _('Collections') }}</th>
+                <th class="col-md-1">{{ _('CatAdm') }}</th>
+                <th class="col-md-1">{{ _('Admin') }}</th>
+                <th class="col-md-2">{{ _('Last Login At') }}</th>
+                <th class="col-md-2">{{ _('Created At') }}</th>
+            </tr>
+            </thead>
+            <tbody>
+            {% for user in users_created_or_modified %}
+                <tr>
+                    <td class="word-break-all">
+                        <a href="{{ url_for('user.inspect', user_id=user.id) }}">
+                            {{ user.email }}
+                        </a>
+                    </td>
+                    <td>{{ user.permissions | length }}</td>
+                    <td class="{{ "bool-value-{}".format(user.is_cataloging_admin).lower() }}">
+                        {{ _('Yes') if user.is_cataloging_admin else _('No') }}
+                    </td>
+                    <td class="{{ "bool-value-{}".format(user.is_admin).lower() }}">
+                        {{ _('Yes') if user.is_admin else _('No') }}
+                    </td>
+                    <td>{{ '-' if not user.last_login_at else user.last_login_at.strftime('%y-%m-%d') }}</td>
+                    <td>
+                        {{ user.created_at.strftime('%y-%m-%d') }}, {{  _('by') }}
+                        <a href="{{ url_for('user.inspect', user_id=user.created_by.id) }}">
+                            {{ user.created_by.full_name }}
+                        </a>
+                    </td>
+                </tr>
+            {% endfor %}
+            </tbody>
+        </table>
+    </div>
     <div class="panel panel-default">
         <div class="panel-heading">
             <strong>{{ _('Password Resets') }}</strong>