Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨(teams) add Team dependencies #560

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ and this project adheres to

### Added

- ✨(teams) add Team dependencies #560
- ✨(anct) fetch and display organization names of communes #583
- ✨(frontend) display email if no username #562
- 🧑‍💻(oidc) add ability to pull registration ID (e.g. SIRET) from OIDC #577
Expand Down
7 changes: 6 additions & 1 deletion src/backend/core/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
from django.contrib.auth import admin as auth_admin
from django.utils.translation import gettext_lazy as _

from treebeard.admin import TreeAdmin
from treebeard.forms import movenodeform_factory

from mailbox_manager.admin import MailDomainAccessInline

from . import models
Expand Down Expand Up @@ -118,9 +121,10 @@ class TeamServiceProviderInline(admin.TabularInline):


@admin.register(models.Team)
class TeamAdmin(admin.ModelAdmin):
class TeamAdmin(TreeAdmin):
"""Team admin interface declaration."""

form = movenodeform_factory(models.Team)
inlines = (TeamAccessInline, TeamWebhookInline, TeamServiceProviderInline)
exclude = ("service_providers",) # Handled by the inline
list_display = (
Expand All @@ -129,6 +133,7 @@ class TeamAdmin(admin.ModelAdmin):
"updated_at",
)
search_fields = ("name",)
readonly_fields = ("path", "depth", "numchild")


@admin.register(models.TeamAccess)
Expand Down
14 changes: 10 additions & 4 deletions src/backend/core/api/client/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -259,18 +259,24 @@ class Meta:
model = models.Team
fields = [
"id",
"name",
"accesses",
"abilities",
"accesses",
"created_at",
"updated_at",
"depth",
"name",
"numchild",
"path",
"service_providers",
"updated_at",
]
read_only_fields = [
"id",
"accesses",
"abilities",
"accesses",
"created_at",
"depth",
"numchild",
"path",
"updated_at",
]

Expand Down
43 changes: 39 additions & 4 deletions src/backend/core/api/client/viewsets.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
"""API endpoints"""

import operator
from functools import reduce

from django.conf import settings
from django.db.models import OuterRef, Q, Subquery
from django.db.models import OuterRef, Q, Subquery, Value
from django.db.models.functions import Coalesce

from rest_framework import (
decorators,
Expand Down Expand Up @@ -299,23 +303,54 @@ class TeamViewSet(
permission_classes = [permissions.AccessPermission]
serializer_class = serializers.TeamSerializer
filter_backends = [filters.OrderingFilter]
ordering_fields = ["created_at"]
ordering_fields = ["created_at", "name", "path"]
ordering = ["-created_at"]
queryset = models.Team.objects.all()
pagination_class = None

def get_queryset(self):
"""Custom queryset to get user related teams."""
teams_queryset = models.Team.objects.filter(
accesses__user=self.request.user,
)
depth_path = teams_queryset.values("depth", "path")

if not depth_path:
return models.Team.objects.none()

user_role_query = models.TeamAccess.objects.filter(
user=self.request.user, team=OuterRef("pk")
).values("role")[:1]

return (
models.Team.objects.prefetch_related("accesses", "service_providers")
.filter(
accesses__user=self.request.user,
reduce(
operator.or_,
(
Q(
# The team the user has access to
depth=d["depth"],
path=d["path"],
)
| Q(
# The parent team the user has access to
depth__lt=d["depth"],
path__startswith=d["path"][: models.Team.steplen],
organization_id=self.request.user.organization_id,
)
for d in depth_path
),
),
)
# Abilities are computed based on logged-in user's role for the team
# and if the user does not have access, it's ok to consider them as a member
# because it's a parent team.
.annotate(
user_role=Coalesce(
Subquery(user_role_query), Value(models.RoleChoices.MEMBER.value)
)
)
.annotate(user_role=Subquery(user_role_query))
)

def perform_create(self, serializer):
Expand Down
8 changes: 7 additions & 1 deletion src/backend/core/api/resource_server/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,19 @@ class Meta:
model = models.Team
fields = [
"id",
"name",
"created_at",
"depth",
"name",
"numchild",
"path",
"updated_at",
]
read_only_fields = [
"id",
"created_at",
"depth",
"numchild",
"path",
"updated_at",
]

Expand Down
41 changes: 38 additions & 3 deletions src/backend/core/api/resource_server/viewsets.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
"""Resource server API endpoints"""

from django.db.models import OuterRef, Prefetch, Subquery
import operator
from functools import reduce

from django.db.models import OuterRef, Prefetch, Q, Subquery, Value
from django.db.models.functions import Coalesce

from rest_framework import (
filters,
Expand Down Expand Up @@ -56,6 +60,14 @@ class TeamViewSet( # pylint: disable=too-many-ancestors

def get_queryset(self):
"""Custom queryset to get user related teams."""
teams_queryset = models.Team.objects.filter(
accesses__user=self.request.user,
)
depth_path = teams_queryset.values("depth", "path")

if not depth_path:
return models.Team.objects.none()

user_role_query = models.TeamAccess.objects.filter(
user=self.request.user, team=OuterRef("pk")
).values("role")[:1]
Expand All @@ -74,10 +86,33 @@ def get_queryset(self):
service_provider_prefetch,
)
.filter(
accesses__user=self.request.user,
reduce(
operator.or_,
(
Q(
# The team the user has access to
depth=d["depth"],
path=d["path"],
)
| Q(
# The parent team the user has access to
depth__lt=d["depth"],
path__startswith=d["path"][: models.Team.steplen],
organization_id=self.request.user.organization_id,
)
for d in depth_path
),
),
service_providers__audience_id=service_provider_audience,
)
.annotate(user_role=Subquery(user_role_query))
# Abilities are computed based on logged-in user's role for the team
# and if the user does not have access, it's ok to consider them as a member
# because it's a parent team.
.annotate(
user_role=Coalesce(
Subquery(user_role_query), Value(models.RoleChoices.MEMBER.value)
)
)
)

def perform_create(self, serializer):
Expand Down
1 change: 0 additions & 1 deletion src/backend/core/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,6 @@ class TeamFactory(factory.django.DjangoModelFactory):

class Meta:
model = models.Team
django_get_or_create = ("name",)
skip_postgeneration_save = True

name = factory.Sequence(lambda n: f"team{n}")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
# Generated by Django 5.1.3 on 2024-11-26 13:55

from django.db import migrations, models

from treebeard.numconv import NumConv


def update_team_paths(apps, schema_editor):
Team = apps.get_model("core", "Team")
alphabet = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
steplen = 5

# Initialize NumConv with the specified custom alphabet
converter = NumConv(len(alphabet), alphabet)

nodes = Team.objects.all().order_by("created_at")
for i, node in enumerate(nodes, 1):
node.depth = 1 # root nodes are at depth 1

# Use NumConv to encode the index `i` to a base representation and
# pad it to the specified step length using the custom alphabet
node.path = converter.int2str(i).rjust(steplen, alphabet[0])

if nodes:
Team.objects.bulk_update(nodes, ["depth", "path"])


class Migration(migrations.Migration):

dependencies = [
('core', '0009_contacts_add_index_on_data_emails'),
]

operations = [
migrations.AddField(
model_name='team',
name='depth',
field=models.PositiveIntegerField(default=0),
preserve_default=False,
),
migrations.AddField(
model_name='team',
name='numchild',
field=models.PositiveIntegerField(default=0),
),
migrations.AddField(
model_name='team',
name='path',
field=models.CharField(default='', max_length=400, db_collation="C"),
preserve_default=False,
),
migrations.RunPython(update_team_paths, migrations.RunPython.noop),
migrations.AlterField(
model_name="team",
name="path",
field=models.CharField(unique=True, max_length=400, db_collation="C"),
),
]
38 changes: 37 additions & 1 deletion src/backend/core/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

import jsonschema
from timezone_field import TimeZoneField
from treebeard.mp_tree import MP_Node, MP_NodeManager

from core.enums import WebhookStatusChoices
from core.plugins.loader import organization_plugins_run_after_create
Expand Down Expand Up @@ -633,7 +634,34 @@ def __str__(self):
return f"{self.user!s} is {self.role:s} in organization {self.organization!s}"


class Team(BaseModel):
class TeamManager(MP_NodeManager):
"""
Custom manager for the Team model, to manage complexity/automation.
"""

def create(self, parent_id=None, **kwargs):
"""
Replace the default create method to ease the Team creation process.

Notes:
- the `add_*` methods from django-treebeard does not support the "using db".
Which means it will always use the default db.
- the `add_*` methods from django-treebeard does not support the "force_insert".

"""
if parent_id is None:
return self.model.add_root(**kwargs)

# Retrieve parent object, because django-treebeard uses raw queries for most
# write operations, and raw queries don’t update the django objects of the db
# entries they modify. See caveats in the django-treebeard documentation.
# This might be changed later if we never do any operation on the parent object
# before creating the child.
# Beware the N+1 here.
return self.get(pk=parent_id).add_child(**kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not expect the parent object directly? It seems to me what we expect when playing with objects.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't use the object directly to be safer (even if sub-optimized) because as stated in django-treebeard documentation, it's important to have nodes up to date when doing such operations.

Do you think I should update the comment?



class Team(MP_Node, BaseModel):
"""
Represents the link between teams and users, specifying the role a user has in a team.

Expand All @@ -643,6 +671,12 @@ class Team(BaseModel):
Team `service_providers`.
"""

# Allow up to 80 nested teams with 62^5 (916_132_832) root nodes
alphabet = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
# Django treebeard does not allow max_length = None...
steplen = 5
path = models.CharField(max_length=5 * 80, unique=True, db_collation="C")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if this collation impacts the use of "startswith" 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the C collation can improve database performance for certain operations, like sorting and comparison, because it uses simple byte-wise comparison rather than more complex locale-aware rules. However, this comes at the cost of losing locale-specific behaviors, such as case insensitivity and proper sorting for different languages.

So I guess it's only better :) Actually it still works properly in the specific case of "startswith" but I can't tell for the performance improvement or not.


name = models.CharField(max_length=100)

users = models.ManyToManyField(
Expand All @@ -664,6 +698,8 @@ class Team(BaseModel):
blank=True,
)

objects = TeamManager()

class Meta:
db_table = "people_team"
ordering = ("name",)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,10 @@ def test_api_teams_create_authenticated_new_service_provider(
assert response.json() == {
"created_at": team.created_at.strftime("%Y-%m-%dT%H:%M:%S.%fZ"),
"id": str(team.pk),
"depth": team.depth,
"name": "my team",
"numchild": team.numchild,
"path": team.path,
"updated_at": team.updated_at.strftime("%Y-%m-%dT%H:%M:%S.%fZ"),
}

Expand Down
Loading
Loading