Replace Group.members and User.members with memberships

h/models/group.py

@@ -4,6 +4,7 @@
 
 import slugify
 import sqlalchemy as sa
+from sqlalchemy.orm import relationship
 
 from h import pubid
 from h.db import Base, mixins
@@ -40,13 +41,16 @@ class GroupMembership(Base):
 
     id = sa.Column("id", sa.Integer, autoincrement=True, primary_key=True)
     user_id = sa.Column("user_id", sa.Integer, sa.ForeignKey("user.id"), nullable=False)
+    user = relationship("User", back_populates="memberships")
+
     group_id = sa.Column(
         "group_id",
         sa.Integer,
         sa.ForeignKey("group.id", ondelete="cascade"),
         nullable=False,
         index=True,
     )
+    group = relationship("Group", back_populates="memberships")
 
 
 class Group(Base, mixins.Timestamps):
@@ -138,12 +142,11 @@ def groupid(self, value):
             self.authority_provided_id = groupid_parts["authority_provided_id"]
             self.authority = groupid_parts["authority"]
 
-    # Group membership
-    members = sa.orm.relationship(
-        "User",
-        secondary="user_group",
-        backref=sa.orm.backref("groups", order_by="Group.name"),
-    )
+    memberships = sa.orm.relationship("GroupMembership", back_populates="group")
+
+    @property
+    def members(self):
+        return tuple(membership.user for membership in self.memberships)
 
     scopes = sa.orm.relationship(
         "GroupScope", backref="group", cascade="all, delete-orphan"

h/models/user.py

@@ -290,6 +290,12 @@ def activate(self):
 
     tokens = sa.orm.relationship("Token", back_populates="user")
 
+    memberships = sa.orm.relationship("GroupMembership", back_populates="user")
+
+    @property
+    def groups(self):
+        return tuple(membership.group for membership in self.memberships)
+
     @sa.orm.validates("email")
     def validate_email(self, _key, email):
         if email is None:

h/services/group_create.py

@@ -1,7 +1,7 @@
 from functools import partial
 
 from h import session
-from h.models import Group, GroupScope
+from h.models import Group, GroupMembership, GroupScope
 from h.models.group import GROUP_TYPE_FLAGS
 
 
@@ -126,7 +126,7 @@ def _create(self, name, userid, type_flags, scopes, **kwargs):
         # self.publish() or `return group`.
         self.db.flush()
 
-        group.members.append(group.creator)
+        group.memberships.append(GroupMembership(user=group.creator))
         self.publish("group-join", group.pubid, group.creator.userid)
 
         return group

h/services/group_members.py

@@ -1,6 +1,7 @@
 from functools import partial
 
 from h import session
+from h.models import GroupMembership
 
 
 class GroupMembersService:
@@ -60,18 +61,25 @@ def member_join(self, group, userid):
         if user in group.members:
             return
 
-        group.members.append(user)
+        group.memberships.append(GroupMembership(user=user))
 
         self.publish("group-join", group.pubid, userid)
 
     def member_leave(self, group, userid):
         """Remove `userid` from the member list of `group`."""
         user = self.user_fetcher(userid)
 
-        if user not in group.members:
+        matching_memberships = [
+            membership for membership in group.memberships if membership.user == user
+        ]
+
+        if not matching_memberships:
             return
 
-        group.members.remove(user)
+        for membership in matching_memberships:
+            self.db.delete(membership)
+            group.memberships.remove(membership)
+            user.memberships.remove(membership)
 
         self.publish("group-leave", group.pubid, userid)
 

h/views/admin/groups.py

@@ -242,7 +242,7 @@ def _template_context(self):
             "pubid": self.group.pubid,
             "group_name": self.group.name,
             "annotation_count": num_annotations,
-            "member_count": len(self.group.members),
+            "member_count": len(self.group.memberships),
         }
 
 

tests/common/factories/group.py

@@ -20,7 +20,6 @@ class Meta:
     joinable_by = JoinableBy.authority
     readable_by = ReadableBy.members
     writeable_by = WriteableBy.members
-    members = factory.LazyFunction(list)
     authority_provided_id = Faker("hexify", text="^" * 30)
     enforce_scope = True
 

tests/functional/api/bulk/annotation_test.py

@@ -2,6 +2,8 @@
 
 import pytest
 
+from h.models import GroupMembership
+
 
 @pytest.mark.usefixtures("with_clean_db")
 class TestBulkAnnotation:
@@ -23,7 +25,7 @@ def test_it_accepts_a_valid_request(self, make_request, factories):
         group = factories.Group(
             authority="lms.hypothes.is",
             authority_provided_id="1234567890",
-            members=[user],
+            memberships=[GroupMembership(user=user)],
         )
         annotation_slim = factories.AnnotationSlim(
             user=user,

tests/functional/api/groups/members_test.py

@@ -11,8 +11,10 @@ def test_it_returns_list_of_members_for_restricted_group_without_authn(
         self, app, factories, db_session
     ):
         group = factories.RestrictedGroup()
-        group.members = [factories.User(), factories.User(), factories.User()]
+        users = factories.User.create_batch(size=3)
+        group.memberships.extend([GroupMembership(user=user) for user in users])
         db_session.commit()
+
         res = app.get("/api/groups/{pubid}/members".format(pubid=group.pubid))
 
         assert res.status_code == 200
@@ -22,7 +24,7 @@ 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.memberships.append(GroupMembership(user=user))
         db_session.commit()
         res = app.get(
             "/api/groups/{pubid}/members".format(pubid=group.pubid),
@@ -238,7 +240,7 @@ def auth_client_header(auth_client):
 @pytest.fixture
 def group(db_session, factories):
     group = factories.Group()
-    db_session.add(GroupMembership(group_id=group.id, user_id=group.creator.id))
+    group.memberships.append(GroupMembership(user=group.creator))
     db_session.commit()
     return group
 
@@ -253,7 +255,7 @@ def third_party_group(db_session, factories):
 @pytest.fixture
 def group_member(group, db_session, factories):
     user = factories.User()
-    group.members.append(user)
+    group.memberships.append(GroupMembership(user=user))
     db_session.commit()
     return user
 

tests/functional/api/groups/read_test.py

@@ -21,10 +21,8 @@ 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()
-        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))
+        group1 = factories.Group(memberships=[GroupMembership(user=user)])
+        group2 = factories.Group(memberships=[GroupMembership(user=user)])
         db_session.commit()
 
         res = app.get("/api/groups", headers=token_auth_header)
@@ -39,8 +37,7 @@ def test_it_overrides_authority_param_with_user_authority(
     ):
         user, _ = user_with_token
         # This group will be created with the user's authority
-        group1 = factories.Group()
-        db_session.add(GroupMembership(group_id=group1.id, user_id=user.id))
+        group1 = factories.Group(memberships=[GroupMembership(user=user)])
         db_session.commit()
 
         res = app.get("/api/groups?authority=whatever.com", headers=token_auth_header)
@@ -95,8 +92,7 @@ def test_it_returns_http_200_for_private_group_with_creator_authentication(
         self, app, user_with_token, token_auth_header, factories, db_session
     ):
         user, _ = user_with_token
-        group = factories.Group()
-        db_session.add(GroupMembership(group_id=group.id, user_id=user.id))
+        group = factories.Group(memberships=[GroupMembership(user=user)])
         db_session.commit()
 
         res = app.get(
@@ -110,7 +106,7 @@ def test_it_returns_http_200_for_private_group_with_member_authentication(
     ):
         user, _ = user_with_token
         group = factories.Group()
-        group.members.append(user)
+        group.memberships.append(GroupMembership(user=user))
         db_session.commit()
 
         res = app.get(

tests/functional/api/profile_test.py

@@ -1,5 +1,7 @@
 import pytest
 
+from h.models import GroupMembership
+
 
 class TestGetProfile:
     def test_it_returns_profile_with_single_group_when_not_authd(self, app):
@@ -148,7 +150,7 @@ def groups(factories):
 @pytest.fixture
 def user(groups, db_session, factories):
     user = factories.User()
-    user.groups = groups
+    user.memberships.extend([GroupMembership(group=group) for group in groups])
     db_session.commit()
     return user
 

tests/unit/h/jinja2_extensions/navbar_data_test.py

@@ -4,6 +4,7 @@
 from h_matchers import Any
 
 from h.jinja_extensions.navbar_data import navbar_data
+from h.models import GroupMembership
 
 
 class TestNavbarData:
@@ -53,9 +54,9 @@ def test_it(self, pyramid_request, user):
     def test_it_with_a_group_with_no_creator(
         self, pyramid_request, user, factories, group_list_service
     ):
-        user.groups = group_list_service.associated_groups.return_value = [
-            factories.Group(creator=None)
-        ]
+        group = factories.Group(creator=None)
+        group_list_service.associated_groups.return_value = [group]
+        user.memberships = [GroupMembership(group=group, user=user)]
         pyramid_request.user = user
 
         result = navbar_data(pyramid_request)
@@ -113,7 +114,14 @@ def test_search_url(self, pyramid_request, matched_route, matchdict, search_url)
     @pytest.fixture
     def user(self, factories):
         user = factories.User()
-        user.groups = [factories.Group(creator=user), factories.Group()]
+
+        user.memberships.extend(
+            [
+                GroupMembership(group=group)
+                for group in [factories.Group(creator=user), factories.Group()]
+            ]
+        )
+
         return user
 
     @pytest.fixture(autouse=True)

tests/unit/h/security/identity_test.py

@@ -3,6 +3,7 @@
 import pytest
 from h_matchers import Any
 
+from h.models import GroupMembership
 from h.security.identity import (
     Identity,
     LongLivedAuthClient,
@@ -25,7 +26,7 @@ def test_from_models(self, factories):
 class TestLongLivedUser:
     def test_from_models(self, factories, LongLivedGroup):
         group = factories.Group.build()
-        user = factories.User.build(groups=[group])
+        user = factories.User.build(memberships=[GroupMembership(group=group)])
 
         model = LongLivedUser.from_model(user)
 

tests/unit/h/security/permits_test.py

@@ -3,6 +3,7 @@
 import pytest
 from pyramid.security import Allowed, Denied
 
+from h.models import GroupMembership
 from h.security import Identity, Permission
 from h.security.permits import PERMISSION_MAP, identity_permits
 from h.traversal import AnnotationContext
@@ -92,7 +93,7 @@ def test_it(self, user, group, annotation):
 
     @pytest.fixture
     def user(self, factories, group):
-        return factories.User(groups=[group])
+        return factories.User(memberships=[GroupMembership(group=group)])
 
     @pytest.fixture
     def group(self, factories):

tests/unit/h/services/bulk_api/annotation_test.py

@@ -3,6 +3,7 @@
 import pytest
 from h_matchers import Any
 
+from h.models import GroupMembership
 from h.services.bulk_api.annotation import (
     BulkAnnotation,
     BulkAnnotationService,
@@ -48,7 +49,9 @@ def test_it_with_single_annotation(
         author = factories.User(
             authority=self.AUTHORITY, nipsa=values["nipsad"], username=username
         )
-        group = factories.Group(members=[author, viewer])
+        group = factories.Group(
+            memberships=[GroupMembership(user=author), GroupMembership(user=viewer)]
+        )
         anno_slim = factories.AnnotationSlim(
             user=author,
             group=group,
@@ -83,22 +86,23 @@ def test_it_with_single_annotation(
     def test_it_with_more_complex_grouping(self, svc, factories):
         viewer, author = factories.User.create_batch(2, authority=self.AUTHORITY)
 
-        annotations = [
-            factories.AnnotationSlim(
-                user=author,
-                group=factories.Group(members=group_members),
-                shared=True,
-                deleted=False,
-            )
-            for group_members in (
-                # The first two annotations should match, because they are in
-                # groups the viewer is in
-                [author, viewer],
-                [author, viewer],
-                # This one is just noise and shouldn't match
-                [author],
+        annotations = []
+        for group_members in (
+            # The first two annotations should match, because they are in
+            # groups the viewer is in.
+            [author, viewer],
+            [author, viewer],
+            # This one is just noise and shouldn't match.
+            [author],
+        ):
+            group = factories.Group()
+            annotations.append(
+                factories.AnnotationSlim(
+                    user=author, group=group, shared=True, deleted=False
+                )
             )
-        ]
+            for user in group_members:
+                user.memberships.append(GroupMembership(group=group))
 
         matched_annos = svc.annotation_search(
             authority=self.AUTHORITY,