Repository: hasgeek/coaster

Issue:
In BaseNameMixin, make_name should be called automatically if required
`BaseNameMixin` and related mixins currently require `make_name` to be explicitly called to set the name. Names are not automatically generated when the title is set to avoid breaking existing URLs.

However, this case does not apply for new objects, where no prior name exists and a name is necessary for a database commit.

`BaseNameMixin` (and other classes) should apply the same semantics to calling `make_name` as SQLAlchemy offers for the `default` handler on a column. Since the default handler will not receive the object as a parameter, we can't supply `make_name` itself as the default. Instead, we'll need to listen for changes to the title column and call `make_name` if `name` is currently unset. If there is a hook into the default setting mechanism, we should use it.

Caveat: this should account for cases where the object has been loaded without the `name` field, making it appear unset within SQLAlchemy.

Discussion:

Patch:
diff --git a/coaster/sqlalchemy/mixins.py b/coaster/sqlalchemy/mixins.py
index 98809d5..03a30ae 100644
--- a/coaster/sqlalchemy/mixins.py
+++ b/coaster/sqlalchemy/mixins.py
@@ -162,24 +162,6 @@ class UuidMixin(object):
     suuid = with_roles(suuid, read={'all'})
 
 
-# Setup listeners for UUID-based subclasses
-def __configure_id_listener(mapper, class_):
-    if hasattr(class_, '__uuid_primary_key__') and class_.__uuid_primary_key__:
-        auto_init_default(mapper.column_attrs.id)
-
-
-def __configure_uuid_listener(mapper, class_):
-    if hasattr(class_, '__uuid_primary_key__') and class_.__uuid_primary_key__:
-        return
-    # Only configure this listener if the class doesn't use UUID primary keys,
-    # as the `uuid` column will only be an alias for `id` in that case
-    auto_init_default(mapper.column_attrs.uuid)
-
-
-event.listen(IdMixin, 'mapper_configured', __configure_id_listener, propagate=True)
-event.listen(UuidMixin, 'mapper_configured', __configure_uuid_listener, propagate=True)
-
-
 # Also see functions.make_timestamp_columns
 class TimestampMixin(object):
     """
@@ -730,3 +712,47 @@ class CoordinatesMixin(object):
     @coordinates.setter
     def coordinates(self, value):
         self.latitude, self.longitude = value
+
+
+# --- Auto-populate columns ---------------------------------------------------
+
+# Setup listeners for UUID-based subclasses
+def __configure_id_listener(mapper, class_):
+    if hasattr(class_, '__uuid_primary_key__') and class_.__uuid_primary_key__:
+        auto_init_default(mapper.column_attrs.id)
+
+
+def __configure_uuid_listener(mapper, class_):
+    if hasattr(class_, '__uuid_primary_key__') and class_.__uuid_primary_key__:
+        return
+    # Only configure this listener if the class doesn't use UUID primary keys,
+    # as the `uuid` column will only be an alias for `id` in that case
+    auto_init_default(mapper.column_attrs.uuid)
+
+
+event.listen(IdMixin, 'mapper_configured', __configure_id_listener, propagate=True)
+event.listen(UuidMixin, 'mapper_configured', __configure_uuid_listener, propagate=True)
+
+
+# Populate name and url_id columns
+def __make_name(mapper, connection, target):
+    if target.name is None:
+        target.make_name()
+
+
+def __make_scoped_name(mapper, connection, target):
+    if target.name is None and target.parent is not None:
+        target.make_name()
+
+
+def __make_scoped_id(mapper, connection, target):
+    if target.url_id is None and target.parent is not None:
+        target.make_id()
+
+
+event.listen(BaseNameMixin, 'before_insert', __make_name, propagate=True)
+event.listen(BaseIdNameMixin, 'before_insert', __make_name, propagate=True)
+event.listen(BaseScopedIdMixin, 'before_insert', __make_scoped_id, propagate=True)
+event.listen(BaseScopedNameMixin, 'before_insert', __make_scoped_name, propagate=True)
+event.listen(BaseScopedIdNameMixin, 'before_insert', __make_scoped_id, propagate=True)
+event.listen(BaseScopedIdNameMixin, 'before_insert', __make_name, propagate=True)


Test patch:
diff --git a/tests/test_models.py b/tests/test_models.py
index 0305457..19ee28b 100644
--- a/tests/test_models.py
+++ b/tests/test_models.py
@@ -265,7 +265,6 @@ class TestCoasterModels(unittest.TestCase):
     def test_named(self):
         """Named documents have globally unique names."""
         c1 = self.make_container()
-        # XXX: We don't know why, but this raises a non-Unicode string warning
         d1 = NamedDocument(title="Hello", content="World", container=c1)
         self.session.add(d1)
         self.session.commit()
@@ -511,6 +510,47 @@ class TestCoasterModels(unittest.TestCase):
         d2.make_name(reserved=['new2'])
         self.assertEqual(d2.name, 'new3')
 
+    def test_named_auto(self):
+        """
+        The name attribute is auto-generated on database insertion
+        """
+        c1 = self.make_container()
+        d1 = NamedDocument(container=c1)
+        d2 = ScopedNamedDocument(container=c1)
+        d3 = IdNamedDocument(container=c1)
+        d4 = ScopedIdNamedDocument(container=c1)
+        d1.title = "Auto name"
+        d2.title = "Auto name"
+        d3.title = "Auto name"
+        d4.title = "Auto name"
+        self.session.add_all([d1, d2, d3, d4])
+        assert d1.name is None
+        assert d2.name is None
+        assert d3.name is None
+        assert d4.name is None
+        self.session.commit()
+        assert d1.name == 'auto-name'
+        assert d2.name == 'auto-name'
+        assert d3.name == 'auto-name'
+        assert d4.name == 'auto-name'
+
+    def test_scoped_id_auto(self):
+        """
+        The url_id attribute is auto-generated on database insertion
+        """
+        c1 = self.make_container()
+        d1 = ScopedIdDocument()
+        d1.container = c1
+        d2 = ScopedIdNamedDocument()
+        d2.container = c1
+        d2.title = "Auto name"
+        self.session.add_all([d1, d2])
+        assert d1.url_id is None
+        assert d2.url_id is None
+        self.session.commit()
+        assert d1.url_id == 1
+        assert d2.url_id == 1
+
     def test_has_timestamps(self):
         # Confirm that a model with multiple base classes between it and
         # TimestampMixin still has created_at and updated_at