Simplify GroupFactory: 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). 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

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.

The feature probably can be fixed one way or another but I think the
simplest fix seems to be to change this factory to *not* automatically
add creators as group members, and require any tests that need the
creator to be a member to do that themselves.

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.

It's also probably best not to have lots of tests that depend implicitly
on the factory adding group creators as members.

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,18 @@ 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),
             ]
         )