Repository: hasgeek/coaster

Issue:
MultiModelView required
In a URL structure like `/<parent>/<child>`, ModelView is designed to load the child joined with the parent. However, sometimes we have the relationship inverted in the model design. Funnel's `Commentset`, for instance, has no parent relationship. This means in a URL like `/path/to/<proposal>/comments/<comment>`, we can't load the comment without loading the proposal (or other "parent") first, nor can we discover permissions on the comment without asking the proposal for assistance.

The older `load_models` loader solved this by running a chain of queries and adding up permissions. We will have to rebuild this functionality in ModelView. Proposal:

1. `loader` can return a tuple (or namedtuple) of results that is placed in `self.obj`.
2. `ModelView.after_loader` should recognise the presence of a tuple and iterate through them to find permissions, using the `inherited` parameter.

Unfortunately, there is no solution for `UrlForView` since the model is still blind to its parentage. We can hack this by sticking the parent as an attribute into the loaded instance. Or we can write code in the model to discover parentage, thereby making this ticket irrelevant for this use case.

Discussion:

Patch:
diff --git a/coaster/sqlalchemy/mixins.py b/coaster/sqlalchemy/mixins.py
index bd2edb3..df9a7a2 100644
--- a/coaster/sqlalchemy/mixins.py
+++ b/coaster/sqlalchemy/mixins.py
@@ -201,7 +201,7 @@ class UrlForMixin(object):
     """
     Provides a :meth:`url_for` method used by BaseMixin-derived classes
     """
-    #: Mapping of {app: {action: (endpoint, {param: attr})}}, where attr is a string or tuple of strings.
+    #: Mapping of {app: {action: (endpoint, {param: attr}, _external)}}, where attr is a string or tuple of strings.
     #: The same action can point to different endpoints in different apps. The app may also be None as fallback.
     #: Each subclass will get its own dictionary. This particular dictionary is only used as an inherited fallback.
     url_for_endpoints = {None: {}}
@@ -221,7 +221,14 @@ class UrlForMixin(object):
         params = {}
         for param, attr in list(paramattrs.items()):
             if isinstance(attr, tuple):
-                item = self
+                # attr is a tuple containing:
+                # 1. ('parent', 'name') --> self.parent.name
+                # 2. ('**entity', 'name') --> kwargs['entity'].name
+                if attr[0].startswith('**'):
+                    item = kwargs.pop(attr[0][2:])
+                    attr = attr[1:]
+                else:
+                    item = self
                 for subattr in attr:
                     item = getattr(item, subattr)
                 params[param] = item
diff --git a/coaster/views/classview.py b/coaster/views/classview.py
index 6e71f1e..8fd5d89 100644
--- a/coaster/views/classview.py
+++ b/coaster/views/classview.py
@@ -17,6 +17,7 @@ from werkzeug.routing import parse_rule
 from werkzeug.local import LocalProxy
 from flask import _request_ctx_stack, has_request_context, request, redirect, make_response, Blueprint
 from ..auth import current_auth, add_auth_attribute
+from ..utils import InspectableSet
 
 __all__ = [
     'rulejoin', 'current_view',  # Functions
@@ -507,9 +508,18 @@ class ModelView(ClassView):
     def after_loader(self):
         # Determine permissions available on the object for the current actor,
         # but only if the view method has a requires_permission decorator
-        if (hasattr(self.current_handler.wrapped_func, 'requires_permission') and
-                hasattr(self.obj, 'current_permissions')):
-            perms = self.obj.current_permissions
+        if hasattr(self.current_handler.wrapped_func, 'requires_permission'):
+            if isinstance(self.obj, tuple):
+                perms = None
+                for subobj in self.obj:
+                    if hasattr(subobj, 'permissions'):
+                        perms = subobj.permissions(current_auth.actor, perms)
+                perms = InspectableSet(perms or set())
+            elif hasattr(self.obj, 'current_permissions'):
+                # current_permissions always returns an InspectableSet
+                perms = self.obj.current_permissions
+            else:
+                perms = InspectableSet()
             if hasattr(current_auth, 'permissions'):
                 perms = perms | current_auth.permissions
             add_auth_attribute('permissions', perms)


Test patch:
diff --git a/tests/test_url_for.py b/tests/test_url_for.py
index f770de3..b5a7e3c 100644
--- a/tests/test_url_for.py
+++ b/tests/test_url_for.py
@@ -40,6 +40,15 @@ def doc_upper(doc):
     return u'{} {}'.format('upper', doc)
 
 
+# The unusual parameter `other='@other.name'` requires an explanation.
+# The first `other` refers to `<other>` in the URL. The second refers
+# to the parameter given to `NamedDocument.url_for` in the test below.
+@app1.route('/<doc>/with/<other>')
+@NamedDocument.is_url_for('with', doc='name', other='**other.name')
+def doc_with(doc, other):
+    return u'{} {} {}'.format(doc, 'with', other)
+
+
 @app1.route('/<container>/<doc>')
 @ScopedNamedDocument.is_url_for('view', container='parent.id', doc='name')
 def sdoc_view(container, doc):
@@ -174,6 +183,16 @@ class TestUrlFor(TestUrlForBase):
         # This action is only available in this app
         assert doc1.url_for('app_only') == '/document1/app_only'
 
+    def test_linked_doc(self):
+        """URLs linking two unrelated models are possible"""
+        doc1 = NamedDocument(name=u'document1', title=u"Document 1")
+        doc2 = NamedDocument(name=u'document2', title=u"Document 2")
+        self.session.add_all([doc1, doc2])
+        self.session.commit()
+
+        # url_for is given an object and extracts an attribute from it
+        assert doc1.url_for('with', other=doc2) == '/document1/with/document2'
+
 
 class TestUrlFor2(TestUrlForBase):
     app = app2
diff --git a/tests/test_views_classview.py b/tests/test_views_classview.py
index 3863d19..764fb20 100644
--- a/tests/test_views_classview.py
+++ b/tests/test_views_classview.py
@@ -31,6 +31,7 @@ class ViewDocument(BaseNameMixin, db.Model):
 
     def permissions(self, actor, inherited=()):
         perms = super(ViewDocument, self).permissions(actor, inherited)
+        perms.add('view')
         if actor == 'this-is-the-owner':  # Our hack of a user object, for testing
             perms.add('edit')
             perms.add('delete')
@@ -239,6 +240,28 @@ class RenameableDocumentView(UrlChangeCheck, InstanceLoader, ModelView):
 RenameableDocumentView.init_app(app)
 
 
+@route('/multi/<doc1>/<doc2>')
+class MultiDocumentView(UrlForView, ModelView):
+    model = ViewDocument
+    route_model_map = {
+        'doc1': 'name',
+        'doc2': '**doc2.url_name'
+    }
+
+    def loader(self, doc1, doc2):
+        doc1 = ViewDocument.query.filter_by(name=doc1).first_or_404()
+        doc2 = RenameableDocument.query.filter_by(url_name=doc2).first_or_404()
+        return (doc1, doc2)
+
+    @route('')
+    @requires_permission('view')
+    def linked_view(self):
+        return self.obj[0].url_for('linked_view', doc2=self.obj[1])
+
+
+MultiDocumentView.init_app(app)
+
+
 # --- Tests -------------------------------------------------------------------
 
 class TestClassView(unittest.TestCase):
@@ -493,3 +516,17 @@ class TestClassView(unittest.TestCase):
         assert rv.status_code == 200
         data = json.loads(rv.data)
         assert data['name'] == 'renamed'
+
+    def test_multi_view(self):
+        """
+        A ModelView view can handle multiple objects and also construct URLs
+        for objects that do not have a well defined relationship between each other.
+        """
+        doc1 = ViewDocument(name='test1', title="Test 1")
+        doc2 = RenameableDocument(name='test2', title="Test 2")
+        self.session.add_all([doc1, doc2])
+        self.session.commit()
+
+        rv = self.client.get('/multi/test1/1-test2')
+        assert rv.status_code == 200
+        assert rv.data == b'/multi/test1/1-test2'