Simplify factories.Group(): don't add creators as members

Change `factories.Group()` to *not* automatically add the group's
creator as a member of the group.

Future commits need to replace the `group.members` relation with a
`group.memberships` relation (which is a list of `GroupMembership`'s
rather than a list of `User`'s). See
<#9047>. This is necessary because
`GroupMembership`'s will in future have additional attributes (e.g.
`roles`) and to add a user to a group with a particular role it'll be
necessary to append a `GroupMembership` with that role to
`group.memberships`, it's not enough to append a `User` to
`group.members` because the role is an attribute of the membership not
an attribute of the user, so we need to actually create a
`GroupMembership` with the desired role and append that.

With this change it'll no longer be possible for `factories.Group`'s
`add_creator_as_member()` to add the creator as a member. For example
this kind of thing won't work:

    @factory.post_generation
    def add_creator_as_member(  # pylint:disable=no-self-argument
        obj, _create, _extracted, **_kwargs
    ):
        if (
            obj.creator
            and obj.creator
            not in obj.members
        ):
            obj.memberships.append(
                models.GroupMembership(
                    group=obj,
                    user=obj.creator,
                    role="owner
                )
            )

The problem is that the `GroupMembership` that's been appended will not
have been added to the DB session, which causes this SQLAlchemy error:
https://docs.sqlalchemy.org/en/20/errors.html#object-is-being-merged-into-a-session-along-the-backref-cascade

Or alternatively you get a `NotNullViolation`, depending.

Nor can `factories.Group.add_creator_as_member()` simply add the
`GroupMembership` to the DB session: it doesn't have access to the DB
session (and this wouldn't necessarily get around `NotNullViolation`'s
anyway).

Removing this feels like a good direction to me because
`add_creator_as_member()` seems too clever for a test factory, and my
experience with test factories is that having them do extra things like
this automatically usually ends up creating problems and it's better to
keep the factories simpler and just make certain tests do more work.

There looks to have been a bunch of tests that were implicitly or
explicitly relying on the fact that the factory adds the group's creator
as a member, even when this concept is irrelevant to the test at hand.
So I think removing this is a good thing.

The current behavior is also potentially confusing when you do something
like `factories.Group(members=[...])` and then it auto-generates a user
to be the group's `creator` and adds them to the group's members even
though that user wasn't in the members list that was passed in.

Author: seanh

tests/common/factories/group.py

@@ -33,17 +33,6 @@ def scopes(  # pylint: disable=method-hidden,unused-argument
 
         self.scopes = scopes or []
 
-    @factory.post_generation
-    def add_creator_as_member(  # pylint:disable=no-self-argument
-        obj, _create, _extracted, **_kwargs
-    ):
-        if (
-            obj.creator
-            and obj.creator
-            not in obj.members  # pylint:disable=unsupported-membership-test
-        ):
-            obj.members.insert(0, obj.creator)  # pylint:disable=no-member
-
 
 class OpenGroup(Group):
     name = factory.Sequence(lambda n: f"Open Group {n}")

tests/functional/api/groups/members_test.py

@@ -2,6 +2,7 @@
 
 import pytest
 
+from h.models import GroupMembership
 from h.models.auth_client import GrantType
 
 
@@ -21,17 +22,16 @@ def test_it_returns_list_of_members_if_user_has_access_to_private_group(
         self, app, factories, db_session, group, user_with_token, token_auth_header
     ):
         user, _ = user_with_token
-        group.members.append(user)
+        group.members.extend([user, factories.User()])
         db_session.commit()
+
         res = app.get(
             "/api/groups/{pubid}/members".format(pubid=group.pubid),
             headers=token_auth_header,
         )
 
         returned_usernames = [member["username"] for member in res.json]
-        assert user.username in returned_usernames
-        assert group.creator.username in returned_usernames
-
+        assert returned_usernames == [member.username for member in group.members]
         assert res.status_code == 200
 
     def test_it_returns_404_if_user_does_not_have_read_access_to_group(

tests/functional/api/groups/read_test.py

@@ -2,6 +2,7 @@
 
 import pytest
 
+from h.models import GroupMembership
 from h.models.auth_client import GrantType
 
 
@@ -20,8 +21,10 @@ def test_it_returns_private_groups_along_with_world_groups(
         self, app, factories, db_session, user_with_token, token_auth_header
     ):
         user, _ = user_with_token
-        group1 = factories.Group(creator=user)
-        group2 = factories.Group(creator=user)
+        group1 = factories.Group()
+        db_session.add(GroupMembership(group_id=group1.id, user_id=user.id))
+        group2 = factories.Group()
+        db_session.add(GroupMembership(group_id=group2.id, user_id=user.id))
         db_session.commit()
 
         res = app.get("/api/groups", headers=token_auth_header)
@@ -35,8 +38,8 @@ def test_it_overrides_authority_param_with_user_authority(
         self, app, factories, db_session, user_with_token, token_auth_header
     ):
         user, _ = user_with_token
-        # This group will be created with the user's authority
-        group1 = factories.Group(creator=user)
+        group1 = factories.Group(authority=user.authority)
+        db_session.add(GroupMembership(group_id=group1.id, user_id=user.id))
         db_session.commit()
 
         res = app.get("/api/groups?authority=whatever.com", headers=token_auth_header)
@@ -92,6 +95,7 @@ def test_it_returns_http_200_for_private_group_with_creator_authentication(
     ):
         user, _ = user_with_token
         group = factories.Group(creator=user)
+        db_session.add(GroupMembership(group_id=group.id, user_id=user.id))
         db_session.commit()
 
         res = app.get(

tests/unit/h/services/group_list_test.py

@@ -4,7 +4,7 @@
 import pytest
 from h_matchers import Any
 
-from h.models.group import Group
+from h.models.group import Group, GroupMembership
 from h.services.group_list import GroupListService, group_list_factory
 from h.services.group_scope import GroupScopeService
 
@@ -331,8 +331,10 @@ def document_uri():
 
 
 @pytest.fixture
-def sample_groups(factories, other_authority, document_uri, default_authority, user):
-    return {
+def sample_groups(
+    factories, other_authority, document_uri, default_authority, user, db_session
+):
+    sample_groups = {
         "open": factories.OpenGroup(
             name="sample open",
             authority=default_authority,
@@ -354,6 +356,13 @@ def sample_groups(factories, other_authority, document_uri, default_authority, u
         "private": factories.Group(creator=user),
     }
 
+    # Make `user` a member of the sample_groups["private"].
+    db_session.add(
+        GroupMembership(user_id=user.id, group_id=sample_groups["private"].id)
+    )
+
+    return sample_groups
+
 
 @pytest.fixture
 def group_scope_service(pyramid_config, sample_groups):

tests/unit/h/services/group_members_test.py

@@ -76,15 +76,13 @@ def test_it_adds_users_in_userids(self, factories, group_members_service):
     def test_it_does_not_remove_existing_members(
         self, factories, group_members_service
     ):
-        creator = factories.User()
-        group = factories.Group(creator=creator)
-        users = [factories.User(), factories.User()]
-        userids = [user.userid for user in users]
+        group = factories.Group()
+        existing_member = factories.User()
+        group.members.append(existing_member)
 
-        group_members_service.add_members(group, userids)
+        group_members_service.add_members(group, [factories.User().userid])
 
-        assert len(group.members) == len(users) + 1  # account for creator user
-        assert creator in group.members
+        assert existing_member in group.members
 
 
 class TestUpdateMembers:
@@ -143,7 +141,6 @@ def test_it_proxies_to_member_join_and_leave(
         )
         assert sorted(group_members_service.member_leave.call_args_list) == sorted(
             [
-                mock.call(group, group.creator.userid),
                 mock.call(group, new_members[0].userid),
             ]
         )

tests/unit/h/views/activity_test.py

@@ -1189,7 +1189,7 @@ def user_search_controller(self, user, pyramid_request):
 @pytest.fixture
 def group(factories):
     group = factories.Group()
-    group.members.extend([factories.User(), factories.User()])
+    group.members.extend([group.creator, factories.User(), factories.User()])
     return group
 
 
@@ -1203,20 +1203,23 @@ def no_creator_group(factories):
 @pytest.fixture
 def no_organization_group(factories):
     group = factories.Group(organization=None)
-    group.members.extend([factories.User(), factories.User()])
+    group.members.extend([group.creator, factories.User(), factories.User()])
     return group
 
 
 @pytest.fixture
 def open_group(factories):
     open_group = factories.OpenGroup()
+    open_group.members.append(open_group.creator)
     return open_group
 
 
 @pytest.fixture
 def restricted_group(factories):
     restricted_group = factories.RestrictedGroup()
-    restricted_group.members.extend([factories.User(), factories.User()])
+    restricted_group.members.extend(
+        [restricted_group.creator, factories.User(), factories.User()]
+    )
     return restricted_group