hypothesis / h

Annotate with anyone, anywhere.
https://hypothes.is/
BSD 2-Clause "Simplified" License
2.96k stars 427 forks source link

Simplify `factories.Group`: don't add creators as members #9045

Closed seanh closed 2 weeks ago

seanh commented 3 weeks ago

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 https://github.com/hypothesis/h/pull/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.