Page MenuHome
Differential D9035 Diff 29621 profiles/queries.py

Changeset View

Standalone View

View Options

profiles/queries.py

  • This file was added.
from typing import List
import logging
from django.contrib.auth.models import User, Group
sybrenUnsubmitted
Done
Inline Actions

This module should have a docstring that explains what it's for. What kind of code is expected to call these functions?

sybren: This module should have a docstring that explains what it's for. What kind of code is expected…
logger = logging.getLogger(__name__)
sybrenUnsubmitted
Not Done
Inline Actions

This is an indication that this functionality should actually be in a custom model manager of OAuthUserInfo.

sybren: This is an indication that this functionality should actually be in a custom model manager of…
raillaAuthorUnsubmitted
Done
Inline Actions

I think a lot of what's implemented here should be part of blender_id_oauth_client (@Francesco Siddi (fsiddi) disagrees afaik), but I'd like to avoid changing it now, and instead implement profiles in a way that this functionality could be "hotswapped" later if it gets implemented in blender_id_oauth_client.

railla: I think a lot of what's implemented here should be part of `blender_id_oauth_client` (@fsiddi…
sybrenUnsubmitted
Not Done
Inline Actions

👍

sybren: :+1:
def set_groups(user: User, group_names: List[str]) -> None:
"""
Set user groups to match the given list of `group_names`.
If a group with a particular name doesn't exist, create one.
"""
group_names = set(group_names)
current_groups = user.groups.all()
current_group_names = {group.name for group in current_groups}
names_to_add_to = group_names - current_group_names
# Look up all groups this user is now being added to and make sure they actually exist
groups_to_add_to = []
for group_name in names_to_add_to:
group, _ = Group.objects.get_or_create(name=group_name)
sybrenUnsubmitted
Not Done
Inline Actions

This is of quadratic complexity (len(group_names) × len(current_groups)). How about something like this?

def set_groups(user: User, group_names: Set[str]) -> None:
    current_group_names = {group.name for group in current_groups}

    names_to_add_to = group_names - current_group_names
    groups_to_add_to = [Group.objects.get_or_create(name=group_name) for group_name in names_to_add_to]
    user.groups.add(*groups_to_add_to)

    names_to_remove_from = current_group_names - group_names
    groups_to_remove_from = [
        group := Group.objects.filter(name=group_name).first
        for group_name in names_to_remove_from
        if group is not None]
    user.groups.remove(*groups_to_remove_from)

Disclaimer: I haven't tested this code ;-)
I think that for computing which groups to add/remove, the set semantics makes things more readable. What do you think about such an approach?

sybren: This is of quadratic complexity (`len(group_names)` × `len(current_groups)`). How about…
raillaAuthorUnsubmitted
Done
Inline Actions

Yes, it does indeed look more readable with sets.

Updated the code, with few adjustments: get_or_create doesn't "fit" into list comprehension and an extra Group.object.filter query can be avoided.

railla: Yes, it does indeed look more readable with `set`s. Updated the code, with few adjustments…
groups_to_add_to.append(group)
if groups_to_add_to:
logger.warning(f'Adding user #{user.pk} to the following groups: {groups_to_add_to}')
user.groups.add(*groups_to_add_to)
names_to_remove_from = current_group_names - group_names
# Remove user from the groups that are no longer in the user info payload
groups_to_remove_from = [
group for group in current_groups if group.name in names_to_remove_from
]
if groups_to_remove_from:
logger.warning(f'Removing user #{user.pk} from the groups: {groups_to_remove_from}')
user.groups.remove(*groups_to_remove_from)