Introduce group isolation. #7189
Conversation
akatsoulas
commented
Jan 20, 2026
akatsoulas
commented
- Optmize manager and model methods
- Add tests
- Add group documentation
escattone
left a comment
escattone
left a comment
There was a problem hiding this comment.
I'm still confused about the following in terms of visibility:
- What should happen if isolation is disabled for a private hierarchy?
- What should happen if isolation is disabled for a moderated hierarchy?
- Do members/leaders of non-root groups that have child groups see all of those child groups?
kitsune/groups/models.py
Outdated
| def is_root_node(self): | ||
| """Check if this is a root node using path length.""" | ||
| return len(self.path) == self.steplen |
There was a problem hiding this comment.
The equivalent of this method is already provided by the MP_Node sub-class as the is_root method.
kitsune/groups/models.py
Outdated
| root = self.get_root() | ||
| if root and root.leaders.filter(pk=user.pk).exists(): |
There was a problem hiding this comment.
I think the get_root() method will either return the root node or raise an exception, so I don't think it makes sense to check if root is truthy.
kitsune/groups/models.py
Outdated
| if self.visibility == self.Visibility.MODERATED: | ||
| return self.visible_to_groups.filter(user=user).exists() | ||
|
|
||
| if self.visibility == self.Visibility.PRIVATE: | ||
| return (self.group.user_set.filter(pk=user.pk).exists() or | ||
| self.can_moderate_group(user)) |
There was a problem hiding this comment.
Shouldn't the isolation_enabled setting affect the return value for these two cases?
Also, it seems like this view isn't handling the cases where the user is a member/leader of an ancestor group of the current group.
It'd be nice to make this more DRY and not repeat the logic of the GroupProfileManager.visible method.
kitsune/groups/models.py
Outdated
| root = self.get_root() | ||
| return root.leaders.filter(pk=user.pk).exists() if root else False |
There was a problem hiding this comment.
Likewise here, I don't think it makes sense to check the truthiness of root.
There was a problem hiding this comment.
I think we could rewrite the filtering within the visible method without running extra queries, but in the meantime, I have some questions below.
* Optmize manager and model methods * Add tests * Add group documentation
akatsoulas
force-pushed
the
block-sibling-visibility
branch
from
01c5913 to
03f0f28
Compare
|
|
||
| ### PRIVATE | ||
| - **Who can see**: Depends on `isolation_enabled` setting (default: True) | ||
| - **With isolation enabled**: Members/moderators see only their hierarchy (sibling isolation) |
There was a problem hiding this comment.
For me, it would be more clear to say "Members/moderators see only their ancestors and their own subtree (sibling isolation)". What do you think of that?
There was a problem hiding this comment.
The see both their ancestors and their descendants. Basically their whole branch/hierarchy so I think this is more accurate
|
|
||
| root_paths = {p.path[:steplen] for p in all_user_profiles} | ||
|
|
||
| root_nodes = self.filter(path__in=root_paths).prefetch_related( |
There was a problem hiding this comment.
I don't think the prefetch_related here will help with the is_root_member and is_root_moderator expressions below, because I think the .filter() on each of those querysets will initiate a fresh query?
2 participants
