Merge groups_users table into GroupMemberships class

Acrop146 · Acrop146 · commit 485882b06293 · 2025-10-22T11:14:12.000+02:00
diff --git a/opengever/activity/sources.py b/opengever/activity/sources.py
@@ -3,7 +3,7 @@
 from opengever.ogds.base.sources import AllTeamsSource
 from opengever.ogds.base.sources import AssignedUsersSource
 from opengever.ogds.base.sources import BaseMultipleSourcesQuerySource
-from opengever.ogds.models.group import groups_users
+from opengever.ogds.models.group_membership import groups_users
 from opengever.ogds.models.service import ogds_service
 from opengever.ogds.models.user import User
 from opengever.workspace import is_workspace_feature_enabled
diff --git a/opengever/api/globalindex.py b/opengever/api/globalindex.py
@@ -5,7 +5,7 @@
 from opengever.globalindex.model.task import AVOID_DUPLICATES_STRATEGY_LOCAL
 from opengever.globalindex.model.task import Task
 from opengever.ogds.models.group import Group
-from opengever.ogds.models.group import groups_users
+from opengever.ogds.models.group_membership import groups_users
 from opengever.ogds.models.team import Team
 from opengever.task.helper import task_type_value_helper
 from plone.restapi.interfaces import ISerializeToJson
diff --git a/opengever/api/group.py b/opengever/api/group.py
@@ -3,7 +3,7 @@
 from opengever.base.security import elevated_privileges
 from opengever.base.utils import check_group_plugin_configuration
 from opengever.ogds.models.group import Group
-from opengever.ogds.models.group import groups_users
+from opengever.ogds.models.group_membership import groups_users
 from opengever.ogds.models.service import ogds_service
 from opengever.ogds.models.user import User
 from plone import api
diff --git a/opengever/api/ogdsmembership_note.py b/opengever/api/ogdsmembership_note.py
@@ -1,5 +1,5 @@
 from opengever.base.model import create_session
-from opengever.ogds.models.group import GroupMembership
+from opengever.ogds.models.group_membership import GroupMembership
 from plone.restapi.deserializer import json_body
 from plone.restapi.services import Service
 from sqlalchemy import and_
diff --git a/opengever/api/ogdsuserlisting.py b/opengever/api/ogdsuserlisting.py
@@ -1,8 +1,8 @@
 from datetime import datetime
 from opengever.api.ogdslistingbase import OGDSListingBaseService
 from opengever.base.visible_users_and_groups_filter import visible_users_and_groups_filter
-from opengever.ogds.models.group import GroupMembership
-from opengever.ogds.models.group import groups_users
+from opengever.ogds.models.group_membership import GroupMembership
+from opengever.ogds.models.group_membership import groups_users
 from opengever.ogds.models.user import User
 import re
 
diff --git a/opengever/api/serializer.py b/opengever/api/serializer.py
@@ -20,8 +20,8 @@
 from opengever.ogds.base.actor import Actor
 from opengever.ogds.models.admin_unit import AdminUnit
 from opengever.ogds.models.group import Group
-from opengever.ogds.models.group import GroupMembership
-from opengever.ogds.models.group import groups_users
+from opengever.ogds.models.group_membership import GroupMembership
+from opengever.ogds.models.group_membership import groups_users
 from opengever.ogds.models.org_unit import OrgUnit
 from opengever.ogds.models.team import Team
 from opengever.ogds.models.user import User
diff --git a/opengever/api/tests/test_group_memberships.py b/opengever/api/tests/test_group_memberships.py
@@ -1,6 +1,6 @@
 from ftw.testbrowser import browsing
 from opengever.base.model import create_session
-from opengever.ogds.models.group import GroupMembership
+from opengever.ogds.models.group_membership import GroupMembership
 from opengever.testing import IntegrationTestCase
 import json
 
diff --git a/opengever/api/tests/test_ogdsuserlisting.py b/opengever/api/tests/test_ogdsuserlisting.py
@@ -1,6 +1,6 @@
 from datetime import date
 from ftw.testbrowser import browsing
-from opengever.ogds.models.group import GroupMembership
+from opengever.ogds.models.group_membership import GroupMembership
 from opengever.ogds.models.service import ogds_service
 from opengever.ogds.models.user import User
 from opengever.testing import IntegrationTestCase
diff --git a/opengever/base/visible_users_and_groups_filter.py b/opengever/base/visible_users_and_groups_filter.py
@@ -1,4 +1,4 @@
-from opengever.ogds.models.group import groups_users
+from opengever.ogds.models.group_membership import groups_users
 from opengever.ogds.models.service import ogds_service
 from opengever.workspace import is_workspace_feature_enabled
 from plone import api
diff --git a/opengever/contentstats/active_user_stats.py b/opengever/contentstats/active_user_stats.py
@@ -3,7 +3,7 @@
 from opengever.meeting.model import Committee
 from opengever.ogds.base.utils import get_current_admin_unit
 from opengever.ogds.models.group import Group
-from opengever.ogds.models.group import groups_users
+from opengever.ogds.models.group_membership import groups_users
 from opengever.ogds.models.org_unit import OrgUnit
 from opengever.ogds.models.user import User
 
diff --git a/opengever/globalindex/browser/report.py b/opengever/globalindex/browser/report.py
@@ -10,7 +10,7 @@
 from opengever.globalindex.utils import get_selected_items
 from opengever.ogds.base.utils import get_current_admin_unit
 from opengever.ogds.models.group import Group
-from opengever.ogds.models.group import GroupMembership
+from opengever.ogds.models.group_membership import GroupMembership
 from opengever.ogds.models.service import ogds_service
 from opengever.task.helper import task_type_value_helper
 from Products.statusmessages.interfaces import IStatusMessage
diff --git a/opengever/globalindex/tests/test_reporter.py b/opengever/globalindex/tests/test_reporter.py
@@ -1,7 +1,7 @@
 from datetime import datetime
 from ftw.testbrowser import browsing
 from opengever.base.model import create_session
-from opengever.ogds.models.group import GroupMembership
+from opengever.ogds.models.group_membership import GroupMembership
 from opengever.testing import IntegrationTestCase
 from openpyxl import load_workbook
 from tempfile import NamedTemporaryFile
diff --git a/opengever/ogds/auth/plugin.py b/opengever/ogds/auth/plugin.py
@@ -3,7 +3,7 @@
 from logging import getLogger
 from OFS.Cache import Cacheable
 from opengever.ogds.models.group import Group
-from opengever.ogds.models.group import groups_users
+from opengever.ogds.models.group_membership import groups_users
 from opengever.ogds.models.service import ogds_service
 from opengever.ogds.models.user import User
 from plone import api
diff --git a/opengever/ogds/base/hooks.py b/opengever/ogds/base/hooks.py
@@ -4,7 +4,7 @@
 from opengever.ogds.base.setup import create_example_client
 from opengever.ogds.base.setup import create_sql_tables
 from opengever.ogds.models.group import Group
-from opengever.ogds.models.group import groups_users
+from opengever.ogds.models.group_membership import groups_users
 from opengever.ogds.models.org_unit import OrgUnit
 from opengever.ogds.models.user import User
 import transaction
diff --git a/opengever/ogds/base/sources.py b/opengever/ogds/base/sources.py
@@ -14,7 +14,7 @@
 from opengever.ogds.base.utils import get_current_admin_unit
 from opengever.ogds.base.utils import get_current_org_unit
 from opengever.ogds.models.group import Group
-from opengever.ogds.models.group import groups_users
+from opengever.ogds.models.group_membership import groups_users
 from opengever.ogds.models.org_unit import OrgUnit
 from opengever.ogds.models.team import Team
 from opengever.ogds.models.user import User
diff --git a/opengever/ogds/models/admin_unit.py b/opengever/ogds/models/admin_unit.py
@@ -1,7 +1,7 @@
 from opengever.base.model import Base
 from opengever.base.model import UNIT_ID_LENGTH
 from opengever.base.model import UNIT_TITLE_LENGTH
-from opengever.ogds.models.group import groups_users
+from opengever.ogds.models.group_membership import groups_users
 from opengever.ogds.models.org_unit import OrgUnit
 from opengever.ogds.models.user import User
 from sqlalchemy import Boolean
diff --git a/opengever/ogds/models/group.py b/opengever/ogds/models/group.py
@@ -2,68 +2,24 @@
 from opengever.base.model import create_session
 from opengever.base.model import GROUP_ID_LENGTH
 from opengever.base.model import GROUP_TITLE_LENGTH
-from opengever.base.model import USER_ID_LENGTH
 from opengever.base.query import BaseQuery
+from opengever.ogds.models.group_membership import GroupMembership
+from opengever.ogds.models.group_membership import groups_users
 from opengever.ogds.models.team import Team
 from opengever.ogds.models.user import User
 from sqlalchemy import Boolean
 from sqlalchemy import Column
 from sqlalchemy import event
-from sqlalchemy import ForeignKey
 from sqlalchemy import func
 from sqlalchemy import Index
 from sqlalchemy import String
-from sqlalchemy import Table
-from sqlalchemy import Text
 from sqlalchemy.orm import backref
 from sqlalchemy.orm import relation
 from sqlalchemy.orm import relationship
 from sqlalchemy.sql import select
 from sqlalchemy.sql.expression import true
 
 
-# association table
-groups_users = Table(
-    'groups_users', Base.metadata,
-    Column('groupid', String(GROUP_ID_LENGTH),
-           ForeignKey('groups.groupid'), primary_key=True),
-    Column('userid', String(USER_ID_LENGTH),
-           ForeignKey('users.userid'), primary_key=True),
-    Column("note", Text, nullable=True),
-)
-
-
-class GroupMembership(Base):
-
-    __table__ = groups_users
-
-    group = relationship(
-        "Group",
-        back_populates="memberships",
-        primaryjoin="foreign(GroupMembership.groupid)==Group.groupid",
-    )
-    user = relationship(
-        "User",
-        back_populates="memberships",
-        primaryjoin="foreign(GroupMembership.userid)==User.userid",
-    )
-
-
-def create_additional_groups_users_indexes(table, connection, *args, **kw):
-    engine = connection.engine
-    if engine.dialect.name != 'sqlite':
-        # SQLite 3.7 (as used on Jenkins) doesn't support the syntax yet
-        # that SQLAlchemy produces for this functional index
-        ix = Index('ix_groups_users_userid_lower',
-                   func.lower(groups_users.c.userid))
-        ix.create(engine)
-
-
-event.listen(
-    groups_users, 'after_create',
-    create_additional_groups_users_indexes)
-
-
 class GroupQuery(BaseQuery):
 
     searchable_fields = ['groupid', 'title']
@@ -89,15 +45,16 @@ class Group(Base):
     is_local = Column(Boolean, default=False, nullable=True)
 
     users = relation(User, secondary=groups_users, order_by='User.lastname',
+                     lazy="selectin",
                      backref=backref('groups', order_by='Group.groupid'))
 
     memberships = relationship(
         "GroupMembership",
         back_populates="group",
         cascade="all, delete-orphan",
-        lazy="selectin",
-        primaryjoin="Group.groupid == foreign(GroupMembership.groupid)",
-        order_by="GroupMembership.userid",
+        lazy="noload",
+        order_by=lambda: GroupMembership.userid,
+        passive_deletes=True
     )
     teams = relationship(Team, back_populates="group")
 
diff --git a/opengever/ogds/models/group_membership.py b/opengever/ogds/models/group_membership.py
@@ -0,0 +1,47 @@
+from opengever.base.model import Base
+from opengever.base.model import GROUP_ID_LENGTH
+from opengever.base.model import USER_ID_LENGTH
+from sqlalchemy import Column
+from sqlalchemy import event
+from sqlalchemy import ForeignKey
+from sqlalchemy import func
+from sqlalchemy import Index
+from sqlalchemy import String
+from sqlalchemy import Text
+from sqlalchemy.orm import relationship
+
+
+class GroupMembership(Base):
+
+    __tablename__ = "groups_users"
+
+    groupid = Column(String(GROUP_ID_LENGTH),
+                     ForeignKey("groups.groupid", ondelete="CASCADE"),
+                     primary_key=True)
+
+    userid = Column(String(USER_ID_LENGTH),
+                    ForeignKey("users.userid", ondelete="CASCADE"),
+                    primary_key=True)
+
+    note = Column(Text, nullable=True)
+
+    group = relationship("Group", back_populates="memberships")
+    user = relationship("User", back_populates="memberships")
+
+
+groups_users = GroupMembership.__table__
+
+
+def create_additional_groups_users_indexes(table, connection, *args, **kw):
+    engine = connection.engine
+    if engine.dialect.name != 'sqlite':
+        # SQLite 3.7 (as used on Jenkins) doesn't support the syntax yet
+        # that SQLAlchemy produces for this functional index
+        ix = Index('ix_groups_users_userid_lower',
+                   func.lower(groups_users.c.userid))
+        ix.create(engine)
+
+
+event.listen(
+    groups_users, 'after_create',
+    create_additional_groups_users_indexes)
diff --git a/opengever/ogds/models/tests/test_group_membership.py b/opengever/ogds/models/tests/test_group_membership.py
@@ -1,23 +1,50 @@
-from opengever.ogds.models.group import GroupMembership
+from opengever.ogds.models.group import Group
+from opengever.ogds.models.group_membership import GroupMembership
 from opengever.ogds.models.tests.base import OGDSTestCase
+from opengever.ogds.models.user import User
+from sqlalchemy.orm import selectinload
 
 
 class TestGroupMembershipModel(OGDSTestCase):
 
+    def _load_group_with_memberships(self, group):
+        return (
+            self.session.query(Group)
+            .options(
+                selectinload(Group.memberships)
+                .selectinload(GroupMembership.user)
+            )
+            .filter_by(groupid=group.groupid)
+            .one()
+        )
+
+    def _load_user_with_memberships(self, user):
+        return (
+            self.session.query(User)
+            .options(
+                selectinload(User.memberships)
+                .selectinload(GroupMembership.group)
+            )
+            .filter_by(userid=user.userid)
+            .one()
+        )
+
     def test_create_membership_with_optional_note(self):
-        self.assertNotIn(self.john, self.members_b.users)
-        self.assertEqual(2, len(self.members_b.memberships))
+        self.member_b = self._load_group_with_memberships(self.members_b)
+        self.assertNotIn(self.john, self.member_b.users)
+        self.assertEqual(2, len(self.member_b.memberships))
 
         member_john = GroupMembership(
             groupid=self.members_b.groupid, userid=self.john.userid, note="Test Note"
         )
 
         self.session.add(member_john)
         self.session.flush()
-        self.session.expire(self.members_b, ["memberships", "users"])
+        self.session.expire(self.members_b, ['memberships', 'users'])
+        self.member_b = self._load_group_with_memberships(self.members_b)
         self.session.expire(self.john, ["groups"])
 
-        self.assertEqual(3, len(self.members_b.memberships))
+        self.assertEqual(3, len(self.member_b.memberships))
         self.assertEqual("Test Note", member_john.note)
 
         self.assertIn(self.john, self.members_b.users)
@@ -29,6 +56,7 @@ def test_delete_orphan_cascades_when_removed_from_collection(self):
         self.session.add_all([m1, m2])
         self.session.flush()
 
+        self.members_a = self._load_group_with_memberships(self.members_a)
         self.members_a.memberships.remove(m1)
         self.session.flush()
 
@@ -48,6 +76,7 @@ def test_delete_orphan_on_user_side(self):
             .one()
         )
 
+        self.peter = self._load_user_with_memberships(self.peter)
         self.peter.memberships.remove(member_peter)
         self.session.flush()
         self.session.expire(self.members_b, ["memberships", "users"])
diff --git a/opengever/ogds/models/user.py b/opengever/ogds/models/user.py
@@ -6,6 +6,7 @@
 from opengever.base.model import USER_ID_LENGTH
 from opengever.base.query import BaseQuery
 from opengever.base.types import UnicodeCoercingText
+from opengever.ogds.models.group_membership import GroupMembership
 from sqlalchemy import Boolean
 from sqlalchemy import Column
 from sqlalchemy import Date
@@ -106,9 +107,9 @@ class User(Base):
         "GroupMembership",
         back_populates="user",
         cascade="all, delete-orphan",
-        lazy="selectin",
-        primaryjoin="User.userid == foreign(GroupMembership.userid)",
-        order_by="GroupMembership.groupid",
+        lazy="noload",
+        order_by=lambda: GroupMembership.groupid,
+        passive_deletes=True
     )
 
     # A classmethod property needs to be defined on the metaclass
diff --git a/opengever/tabbedview/browser/personal_overview.py b/opengever/tabbedview/browser/personal_overview.py
@@ -12,7 +12,7 @@
 from opengever.ogds.base.utils import get_current_admin_unit
 from opengever.ogds.base.utils import get_current_org_unit
 from opengever.ogds.models.group import Group
-from opengever.ogds.models.group import groups_users
+from opengever.ogds.models.group_membership import groups_users
 from opengever.ogds.models.service import ogds_service
 from opengever.ogds.models.team import Team
 from opengever.tabbedview import _

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-from opengever.ogds.models.group import groups_users`
	`1`	`+from opengever.ogds.models.group_membership import groups_users`
`2`	`2`	`from opengever.ogds.models.service import ogds_service`
`3`	`3`	`from opengever.workspace import is_workspace_feature_enabled`
`4`	`4`	`from plone import api`