♻️(backend) refactor get_select_options to take definitions dict

This will allow us to simplify the get_abilities method. It is also
more efficient because we have computed this definitions dict and
the the get_select_options method was doing the conversion again.
This commit is contained in:
Samuel Paccoud - DINUM
2025-04-24 08:59:30 +02:00
committed by Anthony LC
parent 18d46acd75
commit 8f67e382ba
4 changed files with 78 additions and 131 deletions

View File

@@ -61,63 +61,52 @@ class LinkReachChoices(PriorityTextChoices):
) # Any authenticated user can access the document ) # Any authenticated user can access the document
PUBLIC = "public", _("Public") # Even anonymous users can access the document PUBLIC = "public", _("Public") # Even anonymous users can access the document
@classmethod @classmethod
def get_select_options(cls, ancestors_links): def get_select_options(cls, link_reach, link_role):
""" """
Determines the valid select options for link reach and link role depending on the Determines the valid select options for link reach and link role depending on the
list of ancestors' link reach/role. list of ancestors' link reach/role definitions.
Args:
ancestors_links: List of dictionaries, each with 'link_reach' and 'link_role' keys
representing the reach and role of ancestors links.
Returns: Returns:
Dictionary mapping possible reach levels to their corresponding possible roles. Dictionary mapping possible reach levels to their corresponding possible roles.
""" """
# If no ancestors, return all options # If no ancestors, return all options
if not ancestors_links: if not link_reach:
return { return {
reach: LinkRoleChoices.values if reach != cls.RESTRICTED else None reach: LinkRoleChoices.values if reach != cls.RESTRICTED else None
for reach in cls.values for reach in cls.values
} }
# Initialize result with all possible reaches and role options as sets # Initialize the result for all reaches with possible roles
result = { result = {
reach: set(LinkRoleChoices.values) if reach != cls.RESTRICTED else None reach: set(LinkRoleChoices.values) if reach != cls.RESTRICTED else None
for reach in cls.values for reach in cls.values
} }
# Group roles by reach level # Handle special rules directly with early returns for efficiency
reach_roles = defaultdict(set)
for link in ancestors_links:
reach_roles[link["link_reach"]].add(link["link_role"])
# Rule 1: public/editor → override everything if link_role == LinkRoleChoices.EDITOR:
if LinkRoleChoices.EDITOR in reach_roles.get(cls.PUBLIC, set()): # Rule 1: public/editor → override everything
return {cls.PUBLIC: [LinkRoleChoices.EDITOR]} if link_reach == cls.PUBLIC:
return {cls.PUBLIC: [LinkRoleChoices.EDITOR]}
# Rule 2: authenticated/editor # Rule 2: authenticated/editor
if LinkRoleChoices.EDITOR in reach_roles.get(cls.AUTHENTICATED, set()): if link_reach == cls.AUTHENTICATED:
result[cls.AUTHENTICATED].discard(LinkRoleChoices.READER) result[cls.AUTHENTICATED].discard(LinkRoleChoices.READER)
result.pop(cls.RESTRICTED, None) result.pop(cls.RESTRICTED, None)
# Rule 3: public/reader if link_role == LinkRoleChoices.READER:
if LinkRoleChoices.READER in reach_roles.get(cls.PUBLIC, set()): # Rule 3: public/reader
result.pop(cls.AUTHENTICATED, None) if link_reach == cls.PUBLIC:
result.pop(cls.RESTRICTED, None) result.pop(cls.AUTHENTICATED, None)
result.pop(cls.RESTRICTED, None)
# Rule 4: authenticated/reader # Rule 4: authenticated/reader
if LinkRoleChoices.READER in reach_roles.get(cls.AUTHENTICATED, set()): if link_reach == cls.AUTHENTICATED:
result.pop(cls.RESTRICTED, None) result.pop(cls.RESTRICTED, None)
# Clean up: remove empty entries and convert sets to ordered lists # Convert sets to ordered lists where applicable
cleaned = {} return {
for reach in cls.values: reach: sorted(roles, key=LinkRoleChoices.get_priority) if roles else roles
if reach in result: for reach, roles in result.items()
if result[reach]: }
cleaned[reach] = [
r for r in LinkRoleChoices.values if r in result[reach]
]
else:
# Could be [] or None (for RESTRICTED reach)
cleaned[reach] = result[reach]
return cleaned

View File

@@ -819,7 +819,9 @@ class Document(MP_Node, BaseModel):
"ancestors_links_definitions": { "ancestors_links_definitions": {
k: list(v) for k, v in ancestors_links_definitions.items() k: list(v) for k, v in ancestors_links_definitions.items()
}, },
"link_select_options": LinkReachChoices.get_select_options(ancestors_links), "link_select_options": LinkReachChoices.get_select_options(
ancestors_links_definitions
),
"tree": can_get, "tree": can_get,
"update": can_update, "update": can_update,
"versions_destroy": is_owner_or_admin, "versions_destroy": is_owner_or_admin,

View File

@@ -1,6 +1,7 @@
""" """
Tests for Documents API endpoint in impress's core app: retrieve Tests for Documents API endpoint in impress's core app: retrieve
""" """
# pylint: disable=too-many-lines
import random import random
from datetime import timedelta from datetime import timedelta
@@ -93,6 +94,7 @@ def test_api_documents_retrieve_anonymous_public_parent():
assert response.status_code == 200 assert response.status_code == 200
links = document.get_ancestors().values("link_reach", "link_role") links = document.get_ancestors().values("link_reach", "link_role")
links_definitions = document.get_ancestors_links_definitions(links)
assert response.json() == { assert response.json() == {
"id": str(document.id), "id": str(document.id),
"abilities": { "abilities": {
@@ -117,7 +119,9 @@ def test_api_documents_retrieve_anonymous_public_parent():
"favorite": False, "favorite": False,
"invite_owner": False, "invite_owner": False,
"link_configuration": False, "link_configuration": False,
"link_select_options": models.LinkReachChoices.get_select_options(links), "link_select_options": models.LinkReachChoices.get_select_options(
links_definitions
),
"media_auth": True, "media_auth": True,
"media_check": True, "media_check": True,
"move": False, "move": False,
@@ -272,6 +276,7 @@ def test_api_documents_retrieve_authenticated_public_or_authenticated_parent(rea
assert response.status_code == 200 assert response.status_code == 200
links = document.get_ancestors().values("link_reach", "link_role") links = document.get_ancestors().values("link_reach", "link_role")
links_definitions = document.get_ancestors_links_definitions(links)
assert response.json() == { assert response.json() == {
"id": str(document.id), "id": str(document.id),
"abilities": { "abilities": {
@@ -295,10 +300,12 @@ def test_api_documents_retrieve_authenticated_public_or_authenticated_parent(rea
"favorite": True, "favorite": True,
"invite_owner": False, "invite_owner": False,
"link_configuration": False, "link_configuration": False,
"link_select_options": models.LinkReachChoices.get_select_options(links), "link_select_options": models.LinkReachChoices.get_select_options(
links_definitions
),
"move": False,
"media_auth": True, "media_auth": True,
"media_check": True, "media_check": True,
"move": False,
"partial_update": grand_parent.link_role == "editor", "partial_update": grand_parent.link_role == "editor",
"restore": False, "restore": False,
"retrieve": True, "retrieve": True,
@@ -458,6 +465,7 @@ def test_api_documents_retrieve_authenticated_related_parent():
) )
assert response.status_code == 200 assert response.status_code == 200
links = document.get_ancestors().values("link_reach", "link_role") links = document.get_ancestors().values("link_reach", "link_role")
links_definitions = document.get_ancestors_links_definitions(links)
ancestors_roles = list({grand_parent.link_role, parent.link_role}) ancestors_roles = list({grand_parent.link_role, parent.link_role})
assert response.json() == { assert response.json() == {
"id": str(document.id), "id": str(document.id),
@@ -479,7 +487,9 @@ def test_api_documents_retrieve_authenticated_related_parent():
"favorite": True, "favorite": True,
"invite_owner": access.role == "owner", "invite_owner": access.role == "owner",
"link_configuration": access.role in ["administrator", "owner"], "link_configuration": access.role in ["administrator", "owner"],
"link_select_options": models.LinkReachChoices.get_select_options(links), "link_select_options": models.LinkReachChoices.get_select_options(
links_definitions
),
"media_auth": True, "media_auth": True,
"media_check": True, "media_check": True,
"move": access.role in ["administrator", "owner"], "move": access.role in ["administrator", "owner"],

View File

@@ -1192,29 +1192,33 @@ def test_models_documents_restore_complex_bis(django_assert_num_queries):
@pytest.mark.parametrize( @pytest.mark.parametrize(
"ancestors_links, select_options", "reach, role, select_options",
[ [
# One ancestor # One ancestor
( (
[{"link_reach": "public", "link_role": "reader"}], "public",
"reader",
{ {
"public": ["reader", "editor"], "public": ["reader", "editor"],
}, },
), ),
([{"link_reach": "public", "link_role": "editor"}], {"public": ["editor"]}), ("public", "editor", {"public": ["editor"]}),
( (
[{"link_reach": "authenticated", "link_role": "reader"}], "authenticated",
"reader",
{ {
"authenticated": ["reader", "editor"], "authenticated": ["reader", "editor"],
"public": ["reader", "editor"], "public": ["reader", "editor"],
}, },
), ),
( (
[{"link_reach": "authenticated", "link_role": "editor"}], "authenticated",
"editor",
{"authenticated": ["editor"], "public": ["reader", "editor"]}, {"authenticated": ["editor"], "public": ["reader", "editor"]},
), ),
( (
[{"link_reach": "restricted", "link_role": "reader"}], "restricted",
"reader",
{ {
"restricted": None, "restricted": None,
"authenticated": ["reader", "editor"], "authenticated": ["reader", "editor"],
@@ -1222,94 +1226,36 @@ def test_models_documents_restore_complex_bis(django_assert_num_queries):
}, },
), ),
( (
[{"link_reach": "restricted", "link_role": "editor"}], "restricted",
"editor",
{ {
"restricted": None, "restricted": None,
"authenticated": ["reader", "editor"], "authenticated": ["reader", "editor"],
"public": ["reader", "editor"], "public": ["reader", "editor"],
}, },
), ),
# Multiple ancestors with different roles
(
[
{"link_reach": "public", "link_role": "reader"},
{"link_reach": "public", "link_role": "editor"},
],
{"public": ["editor"]},
),
(
[
{"link_reach": "authenticated", "link_role": "reader"},
{"link_reach": "authenticated", "link_role": "editor"},
],
{"authenticated": ["editor"], "public": ["reader", "editor"]},
),
(
[
{"link_reach": "restricted", "link_role": "reader"},
{"link_reach": "restricted", "link_role": "editor"},
],
{
"restricted": None,
"authenticated": ["reader", "editor"],
"public": ["reader", "editor"],
},
),
# Multiple ancestors with different reaches
(
[
{"link_reach": "authenticated", "link_role": "reader"},
{"link_reach": "public", "link_role": "reader"},
],
{
"public": ["reader", "editor"],
},
),
(
[
{"link_reach": "restricted", "link_role": "reader"},
{"link_reach": "authenticated", "link_role": "reader"},
{"link_reach": "public", "link_role": "reader"},
],
{
"public": ["reader", "editor"],
},
),
# Multiple ancestors with mixed reaches and roles
(
[
{"link_reach": "authenticated", "link_role": "editor"},
{"link_reach": "public", "link_role": "reader"},
],
{"public": ["reader", "editor"]},
),
(
[
{"link_reach": "authenticated", "link_role": "reader"},
{"link_reach": "public", "link_role": "editor"},
],
{"public": ["editor"]},
),
(
[
{"link_reach": "restricted", "link_role": "editor"},
{"link_reach": "authenticated", "link_role": "reader"},
],
{
"authenticated": ["reader", "editor"],
"public": ["reader", "editor"],
},
),
(
[
{"link_reach": "restricted", "link_role": "reader"},
{"link_reach": "authenticated", "link_role": "editor"},
],
{"authenticated": ["editor"], "public": ["reader", "editor"]},
),
# No ancestors (edge case) # No ancestors (edge case)
( (
[], "public",
None,
{
"public": ["reader", "editor"],
"authenticated": ["reader", "editor"],
"restricted": None,
},
),
(
None,
"reader",
{
"public": ["reader", "editor"],
"authenticated": ["reader", "editor"],
"restricted": None,
},
),
(
None,
None,
{ {
"public": ["reader", "editor"], "public": ["reader", "editor"],
"authenticated": ["reader", "editor"], "authenticated": ["reader", "editor"],
@@ -1318,9 +1264,9 @@ def test_models_documents_restore_complex_bis(django_assert_num_queries):
), ),
], ],
) )
def test_models_documents_get_select_options(ancestors_links, select_options): def test_models_documents_get_select_options(reach, role, select_options):
"""Validate that the "get_select_options" method operates as expected.""" """Validate that the "get_select_options" method operates as expected."""
assert models.LinkReachChoices.get_select_options(ancestors_links) == select_options assert models.LinkReachChoices.get_select_options(reach, role) == select_options
def test_models_documents_compute_ancestors_links_no_highest_readable(): def test_models_documents_compute_ancestors_links_no_highest_readable():