hypothesis / h

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

Replace `Group.members` and `User.members` with `memberships` #9047

Closed seanh closed 1 week ago

seanh commented 3 weeks ago

Now that GroupMembership's are going to have roles via the (not-yet-merged) GroupMembership.roles column it's no longer sufficient for code to speak in terms of "members", for example a group having a list of users as group.members. Code instead needs to speak of "memberships" so that it can refer to the role of the membership.

For example when adding a user to a group, this won't work because there's no way to specify the membership role:

group.members.append(user)

Instead code now needs to do this:

group.memberships.append(GroupMembership(group=group, user=user, roles=roles))

And similarly when reading a group's memberships, we need to read a list of GroupMembership's with their roles rather than reading a list of User's. We need Group.memberships not Group.members.

So this PR replaces the SQLAlchemy relationships Group.members and User.groups with new Group.memberships and User.memberships relationships and updates lots of code and tests to work with memberships instead of working with members and groups directly.

The approach that we're taking here where we have three ORM classes User, Group and GroupMembership for the association table is called the "association object" pattern and has its own section in the SQLAlchemy docs. It's the pattern that you're supposed to use when the association class has its own extra attributes like the upcoming GroupMembership.roles attribute.

Testing

  1. GroupCreateService was changed so go to http://localhost:5000/admin/features and enable the group_type feature flag then go to http://localhost:5000/groups/new and test creating each of the three types of group. The group's creator should be added to the group as a member.
  2. GroupMembersService.member_join() was changed so test visiting a private group's page as a user who isn't a member of the group and joining the group.
  3. GroupMembersService.member_leave() was changed so test visiting a private group's page and clicking the Leave this group link.
seanh commented 1 week ago

@marcospri Could you re-review? I've pushed some new commits to avoid some unnecessary accesses to the Group.memberships and User.memberships relationships (which trigger SQL queries). See Slack thread