Fix pending issues with Account merger (#1868)

* Fix siteadmin dashboard graph data
* Fix comment role assignment
* Fix Account.validate_name_candidate

funnel/models/account.py

@@ -398,6 +398,11 @@ def _defercols(cls) -> List[sa.orm.interfaces.LoaderOption]:
             defer(cls.timezone),
         ]
 
+    @classmethod
+    def type_filter(cls) -> sa.ColumnElement[bool]:
+        """Return filter for the subclass's type."""
+        return cls.type_ == cls.__mapper_args__.get('polymorphic_identity')
+
     primary_email: Optional[AccountEmail]
     primary_phone: Optional[AccountPhone]
 
@@ -990,9 +995,7 @@ def get(
         else:
             query = cls.query.filter_by(buid=buid)
         if cls is not Account:
-            query = query.filter(
-                cls.type == cls.__mapper_args__['polymorphic_identity']
-            )
+            query = query.filter(cls.type_filter())
         if defercols:
             query = query.options(*cls._defercols())
         account = query.one_or_none()
@@ -1026,9 +1029,7 @@ def all(  # noqa: A003
         else:
             return []
         if cls is not Account:
-            query = query.filter(
-                cls.type == cls.__mapper_args__['polymorphic_identity']
-            )
+            query = query.filter(cls.type_filter())
 
         if defercols:
             query = query.options(*cls._defercols())
@@ -1043,9 +1044,7 @@ def all_public(cls) -> Query:
         """Construct a query filtered by public profile state."""
         query = cls.query.filter(cls.profile_state.PUBLIC)
         if cls is not Account:
-            query = query.filter(
-                cls.type == cls.__mapper_args__['polymorphic_identity']
-            )
+            query = query.filter(cls.type_filter())
         return query
 
     @classmethod
@@ -1069,9 +1068,7 @@ def autocomplete(cls, prefix: str) -> List[Account]:
         )
 
         if cls is not Account:
-            base_users = base_users.filter(
-                cls.type == cls.__mapper_args__['polymorphic_identity']
-            )
+            base_users = base_users.filter(cls.type_filter())
         base_users = (
             base_users.options(*cls._defercols()).order_by(Account.title).limit(20)
         )
@@ -1164,8 +1161,10 @@ def validate_name_candidate(cls, name: str) -> Optional[str]:
             return 'invalid'
         if len(name) > cls.__name_length__:
             return 'long'
+        # Look for existing on the base Account model, not the subclass, as SQLAlchemy
+        # will add a filter condition on subclasses to restrict the query to that type.
         existing = (
-            cls.query.filter(sa.func.lower(cls.name) == sa.func.lower(name))
+            Account.query.filter(sa.func.lower(Account.name) == sa.func.lower(name))
             .options(sa.orm.load_only(cls.id, cls.uuid, cls.type_))
             .one_or_none()
         )

funnel/models/comment.py

@@ -282,7 +282,7 @@ class Comment(UuidMixin, BaseMixin, Model):
             'read': {'created_at', 'urls', 'uuid_b58', 'has_replies'},
             'call': {'state', 'commentset', 'view_for', 'url_for'},
         },
-        'replied_to_commenter': {'granted_via': {'in_reply_to': '_user'}},
+        'replied_to_commenter': {'granted_via': {'in_reply_to': '_posted_by'}},
     }
 
     __datasets__ = {

funnel/views/siteadmin.py

@@ -169,7 +169,9 @@ def dashboard_data_users_by_month(self) -> ReturnView:
                 sa.func.count().label('count'),
             )
             .select_from(Account)
-            .filter(Account.state.ACTIVE)
+            .filter(
+                Account.state.ACTIVE, Account.joined_at.isnot(None), User.type_filter()
+            )
             .group_by('month')
             .order_by('month')
         )

tests/unit/models/account_name_test.py

@@ -1,5 +1,7 @@
 """Tests for Account name."""
 
+from typing import Literal
+
 import pytest
 from sqlalchemy.exc import IntegrityError
 
@@ -19,25 +21,32 @@ def test_is_available_name(db_session, user_rincewind) -> None:
 
 
 @pytest.mark.usefixtures('user_rincewind', 'org_uu')
-def test_validate_name_candidate(db_session) -> None:
+@pytest.mark.parametrize('model', ['A', 'U', 'O', 'P'])
+def test_validate_name_candidate(
+    db_session, model: Literal['A', 'U', 'O', 'P']
+) -> None:
     """The name validator returns error codes as expected."""
-    assert (
-        models.Account.validate_name_candidate(None)  # type: ignore[arg-type]
-        == 'blank'
-    )
-    assert models.Account.validate_name_candidate('') == 'blank'
-    assert models.Account.validate_name_candidate('invalid-name') == 'invalid'
-    assert models.Account.validate_name_candidate('0123456789' * 7) == 'long'
-    assert models.Account.validate_name_candidate('0123456789' * 6) is None
-    assert models.Account.validate_name_candidate('ValidName') is None
-    assert models.Account.validate_name_candidate('test_reserved') is None
+    modelref: dict[str, type[models.Account]] = {
+        'A': models.Account,
+        'U': models.User,
+        'O': models.Organization,
+        'P': models.Placeholder,
+    }
+    cls = modelref[model]
+    assert cls.validate_name_candidate(None) == 'blank'  # type: ignore[arg-type]
+    assert cls.validate_name_candidate('') == 'blank'
+    assert cls.validate_name_candidate('invalid-name') == 'invalid'
+    assert cls.validate_name_candidate('0123456789' * 7) == 'long'
+    assert cls.validate_name_candidate('0123456789' * 6) is None
+    assert cls.validate_name_candidate('ValidName') is None
+    assert cls.validate_name_candidate('test_reserved') is None
     db_session.add(models.Placeholder(name='test_reserved'))
-    assert models.Account.validate_name_candidate('test_reserved') == 'reserved'
-    assert models.Account.validate_name_candidate('Test_Reserved') == 'reserved'
-    assert models.Account.validate_name_candidate('TestReserved') is None
-    assert models.Account.validate_name_candidate('rincewind') == 'user'
-    assert models.Account.validate_name_candidate('uu') == 'org'
-    assert models.Account.validate_name_candidate('UU') == 'org'
+    assert cls.validate_name_candidate('test_reserved') == 'reserved'
+    assert cls.validate_name_candidate('Test_Reserved') == 'reserved'
+    assert cls.validate_name_candidate('TestReserved') is None
+    assert cls.validate_name_candidate('rincewind') == 'user'
+    assert cls.validate_name_candidate('uu') == 'org'
+    assert cls.validate_name_candidate('UU') == 'org'
 
 
 def test_reserved_name(db_session) -> None: