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.

Sign up for GitHub

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

[refactor] Use FilterByParent mixin in BaseEmailView #354 #426

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[refactor] Use FilterByParent mixin in BaseEmailView #354 #426

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d961dc3
[refactor] Use FilterByParent mixin in BaseEmailView #354
dee077 Jan 22, 2025
2499b65
Merge branch 'master' into issues/354-refactor-baseemailview
nemesifier Jan 27, 2025
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
2 changes: 0 additions & 2 deletions openwisp_users/api/mixins.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,6 @@ def get_queryset(self):

def assert_parent_exists(self):
parent_queryset = self.get_parent_queryset()
if not self.request.user.is_superuser:
parent_queryset = self.get_organization_queryset(parent_queryset)
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look like something that was requested, why are you changing this?

Copy link
Author

Choose a reason for hiding this comment

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

In the get_organization_queryset method,

def get_organization_queryset(self, qs):
        lookup = {self.organization_lookup: getattr(self.request.user, self._user_attr)}
        return qs.filter(**lookup)

self.organization_lookup has the value organization__in, but in the User model, there is no direct link to Organization. It is managed by OrganizationUser to obtain many-to-many relationships between the Organization and User models. So, it adds openwisp_user as the prefix of the organization field and makes the field name openwisp_user_organization. However, we are searching for the field name organization, which cannot be found and causes a field error.

What I have gone through:

  1. Remove this if condition if not self.request.user.is_superuser as it was not used in the previous implementation of BaseEmailView assert_parent_exists method. All test case passes so I have gone for this.
  2. I have tried changing the lookup to check for the openwisp_user_organization field instead, but it causes other test failures. The models defined in /test folder like Config, Book, Shelf etc. have ForeignKey to the organization so test on these expecting field names as organization.

Please provide your thoughts on this

try:
assert parent_queryset.exists()
except (AssertionError, ValidationError):
Expand Down
12 changes: 2 additions & 10 deletions openwisp_users/api/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
from allauth.account.models import EmailAddress
from django.contrib.auth import get_user_model
from django.core.exceptions import ValidationError
from django.utils.translation import gettext_lazy as _
from drf_yasg.utils import swagger_auto_schema
from rest_framework import pagination
from rest_framework.authtoken.views import ObtainAuthToken
from rest_framework.exceptions import NotFound
from rest_framework.generics import (
GenericAPIView,
ListCreateAPIView,
Expand All @@ -20,6 +18,7 @@

from openwisp_users.api.permissions import DjangoModelPermissions

from .mixins import FilterByParentOwned
from .mixins import ProtectedAPIMixin as BaseProtectedAPIMixin
from .serializers import (
ChangePasswordSerializer,
Expand Down Expand Up @@ -198,7 +197,7 @@ def update(self, request, *args, **kwargs):
)


class BaseEmailView(ProtectedAPIMixin, GenericAPIView):
class BaseEmailView(ProtectedAPIMixin, FilterByParentOwned, GenericAPIView):
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be:

Suggested change
class BaseEmailView(ProtectedAPIMixin, FilterByParentOwned, GenericAPIView):
class BaseEmailView(ProtectedAPIMixin, FilterByParentManaged, GenericAPIView):

That allows organization managers to use the endpoint.

model = EmailAddress
serializer_class = EmailAddressSerializer

Expand All @@ -209,13 +208,6 @@ def initial(self, *args, **kwargs):
super().initial(*args, **kwargs)
self.assert_parent_exists()

def assert_parent_exists(self):
try:
assert self.get_parent_queryset().exists()
except (AssertionError, ValidationError):
user_id = self.kwargs['pk']
raise NotFound(detail=_("User with ID '{}' not found.".format(user_id)))

def get_parent_queryset(self):
user = self.request.user

Expand Down
Loading