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

Feature/#554 filter destruction lists #572

Open
wants to merge 3 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
32 changes: 32 additions & 0 deletions backend/src/openarchiefbeheer/accounts/api/views.py
Copy link
Collaborator

Choose a reason for hiding this comment

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

These endpoints are getting a bit out of hand, maybe we should refactor to use filters?
So then we would have GET /api/v1/users?role=reviewer ?

Copy link
Contributor Author

@svenvandescheur svenvandescheur Dec 20, 2024

Choose a reason for hiding this comment

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

Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,38 @@
from .serializers import UserSerializer


@extend_schema(
tags=["Users"],
summary=_("Users list"),
description=_("List all the users."),
responses={
200: UserSerializer(many=True),
},
)
class UsersView(ListAPIView):
serializer_class = UserSerializer

def get_queryset(self) -> QuerySet[User]:
return User.objects.all()
Comment on lines +20 to +24
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason for not doing:

Suggested change
class UsersView(ListAPIView):
serializer_class = UserSerializer
def get_queryset(self) -> QuerySet[User]:
return User.objects.all()
class UsersView(ListAPIView):
serializer_class = UserSerializer
queryset = User.objects.all()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • For endpoints using the manager method that filter on permissions, this may cause issues when initializing as the queryset is initialized straight away.
  • For the this particular case its uses the get_queryset method for consistency.



@extend_schema(
tags=["Users"],
summary=_("Record managers list"),
description=_(
"List all the users that have the permission to create destruction lists."
),
responses={
200: UserSerializer(many=True),
},
)
class RecordManagersView(ListAPIView):
serializer_class = UserSerializer

def get_queryset(self) -> QuerySet[User]:
return User.objects.record_managers()


@extend_schema(
tags=["Users"],
summary=_("Main reviewers list"),
Expand Down
61 changes: 44 additions & 17 deletions backend/src/openarchiefbeheer/accounts/managers.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,50 @@


class UserQuerySet(QuerySet):
def record_managers(self) -> "UserQuerySet":
"""
Returns a QuerySet of users that have the permission to start a destruction list.
"""
permission = Permission.objects.get(codename="can_start_destruction")
return self._users_with_permission(permission)

def reviewers(self) -> "UserQuerySet":
"""
Returns a QuerySet of users that have the permission to review a destruction list.
"""
return self.filter( # noqa
Q(groups__permissions__codename="can_review_destruction")
| Q(user_permissions__codename="can_review_destruction")
| Q(groups__permissions__codename="can_review_final_list")
| Q(user_permissions__codename="can_review_final_list")
).distinct()

def main_reviewers(self) -> "UserQuerySet":
"""
Returns a QuerySet of users that have the permission to review a destruction list (main reviewers ony).
"""
permission = Permission.objects.get(codename="can_review_destruction")
return self._users_with_permission(permission)

def co_reviewers(self) -> "UserQuerySet":
"""
Returns a QuerySet of users that have the permission to review a destruction list (co reviewers ony).
"""
permission = Permission.objects.get(codename="can_co_review_destruction")
return self._users_with_permission(permission)

def archivists(self) -> "UserQuerySet":
"""
Returns a QuerySet of users that have the permission to perform a final review on a destruction list.
"""
permission = Permission.objects.get(codename="can_review_final_list")
return self._users_with_permission(permission)

def _users_with_permission(self, permission: Permission) -> "UserQuerySet":
return self.filter( # noqa
Copy link
Collaborator

Choose a reason for hiding this comment

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

why the added noqa?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My IDE resolves the type to be QuerySet but AFAIK if executed in this context it will return a UserQuerySet.

Q(groups__permissions=permission) | Q(user_permissions=permission)
).distinct()

def annotate_permissions(self) -> "UserQuerySet":
"""
Adds `user_permission_codenames` and `group_permission_codenames` as `ArrayField` to the current QuerySet
Expand Down Expand Up @@ -71,20 +115,3 @@ def create_superuser(self, username, email, password, **extra_fields):
raise ValueError("Superuser must have is_superuser=True.")

return self._create_user(username, email, password, **extra_fields)

def _users_with_permission(self, permission: Permission) -> UserQuerySet:
return self.filter(
Q(groups__permissions=permission) | Q(user_permissions=permission)
).distinct()

def main_reviewers(self) -> UserQuerySet:
permission = Permission.objects.get(codename="can_review_destruction")
return self._users_with_permission(permission)

def archivists(self) -> UserQuerySet:
permission = Permission.objects.get(codename="can_review_final_list")
return self._users_with_permission(permission)

def co_reviewers(self) -> UserQuerySet:
permission = Permission.objects.get(codename="can_co_review_destruction")
return self._users_with_permission(permission)
61 changes: 61 additions & 0 deletions backend/src/openarchiefbeheer/accounts/tests/test_managers.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,67 @@


class UserQuerySetTests(TestCase):
def test_can_start_destruction_user_permission(self):
user = UserFactory.create()
record_manager = UserFactory.create(post__can_start_destruction=True)
qs = User.objects.record_managers()
self.assertEqual(len(qs), 1)
self.assertIn(record_manager, qs)
self.assertNotIn(user, qs)

def test_can_start_destruction_group_permission(self):
record_managers = Group.objects.create()
record_managers.permissions.add(
Permission.objects.get(
codename="can_start_destruction",
)
)

user = UserFactory.create()
record_manager = UserFactory.create()
record_manager.groups.add(record_managers)

qs = User.objects.record_managers()
self.assertEqual(len(qs), 1)
self.assertIn(record_manager, qs)
self.assertNotIn(user, qs)

def test_can_review_destruction_user_permission(self):
user = UserFactory.create()
reviewer = UserFactory.create(post__can_review_destruction=True)
archivist = UserFactory.create(post__can_review_final_list=True)
qs = User.objects.reviewers()
self.assertEqual(len(qs), 2)
self.assertIn(reviewer, qs)
self.assertIn(archivist, qs)
self.assertNotIn(user, qs)

def test_can_review_destruction_group_permission(self):
reviewers = Group.objects.create(name="reviewers")
reviewers.permissions.add(
Permission.objects.get(
codename="can_review_destruction",
)
)
archivists = Group.objects.create(name="archivists")
archivists.permissions.add(
Permission.objects.get(
codename="can_review_final_list",
)
)

user = UserFactory.create()
reviewer = UserFactory.create()
reviewer.groups.add(reviewers)
archivist = UserFactory.create()
archivist.groups.add(archivists)

qs = User.objects.reviewers()
self.assertEqual(len(qs), 2)
self.assertIn(reviewer, qs)
self.assertIn(archivist, qs)
self.assertNotIn(user, qs)

def test_annotate_permissions(self):
content_type = ContentType.objects.get_for_model(DestructionList)

Expand Down
29 changes: 29 additions & 0 deletions backend/src/openarchiefbeheer/api/tests/test_role_endpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,35 @@ def test_user_not_logged_in(self):

self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)

def test_retrieve_users(self):
admin = UserFactory.create(is_superuser=True)
UserFactory.create_batch(2)

self.client.force_authenticate(user=admin)
response = self.client.get(reverse("api:users"))

self.assertEqual(response.status_code, status.HTTP_200_OK)

data = response.json()

self.assertEqual(len(data), 3)

def test_retrieve_record_managers(self):
admin = UserFactory.create(is_superuser=True)
UserFactory.create_batch(2, post__can_start_destruction=True)
UserFactory.create_batch(2, post__can_start_destruction=False)

self.client.force_authenticate(user=admin)
response = self.client.get(reverse("api:record-managers"))

self.assertEqual(response.status_code, status.HTTP_200_OK)

data = response.json()

self.assertEqual(len(data), 2)
self.assertTrue(data[0]["role"]["canStartDestruction"])
self.assertTrue(data[1]["role"]["canStartDestruction"])

def test_retrieve_reviewers(self):
admin = UserFactory.create(is_superuser=True)
UserFactory.create_batch(2, post__can_review_destruction=True)
Expand Down
12 changes: 12 additions & 0 deletions backend/src/openarchiefbeheer/api/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
ArchivistsView,
CoReviewersView,
MainReviewersView,
RecordManagersView,
UsersView,
WhoAmIView,
)
from openarchiefbeheer.config.api.views import ArchiveConfigView, OIDCInfoView
Expand Down Expand Up @@ -114,6 +116,16 @@
"v1/",
include(
[
path(
"users/",
UsersView.as_view(),
name="users",
),
path(
"record-managers/",
RecordManagersView.as_view(),
name="record-managers",
),
path("reviewers/", MainReviewersView.as_view(), name="reviewers"),
path("archivists/", ArchivistsView.as_view(), name="archivists"),
path("co-reviewers/", CoReviewersView.as_view(), name="co-reviewers"),
Expand Down
23 changes: 20 additions & 3 deletions backend/src/openarchiefbeheer/destruction/api/filtersets.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,15 @@

from django_filters import (
BooleanFilter,
CharFilter,
ChoiceFilter,
FilterSet,
NumberFilter,
OrderingFilter,
UUIDFilter,
)

from ..constants import InternalStatus
from ..constants import InternalStatus, ListRole, ListStatus
from ..models import (
DestructionList,
DestructionListCoReview,
Expand Down Expand Up @@ -93,15 +95,30 @@ def filter_order_review_ignored(


class DestructionListFilterset(FilterSet):
name = CharFilter(lookup_expr="icontains")
status = ChoiceFilter(choices=ListStatus.choices)
author = NumberFilter()
reviewer = NumberFilter(
field_name="assignees__user",
method="filter_reviewer",
)
assignee = NumberFilter(
field_name="assignee",
help_text="The pk of the user currently assigned to the list.",
)
ordering = OrderingFilter(fields=("created", "created"))

def filter_reviewer(self, queryset, name, value):
return queryset.filter(
Q(assignees__role=ListRole.main_reviewer)
| Q(assignees__role=ListRole.co_reviewer),
assignees__user=value,
)

ordering = OrderingFilter(fields=(("created", "created"), ("name", "name")))

class Meta:
model = DestructionList
fields = ("assignee", "ordering")
fields = ("name", "status", "author", "reviewer", "assignee", "ordering")


class DestructionListReviewFilterset(FilterSet):
Expand Down
Loading
Loading