From 68a1ce23b25b852ea57cf3630e92a07f3d7130fa Mon Sep 17 00:00:00 2001 From: Gokulram A Date: Mon, 4 Dec 2023 16:32:03 +0530 Subject: [PATCH 1/4] Refactor get_queryset method in UserViewSet --- care/users/api/viewsets/users.py | 35 +++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/care/users/api/viewsets/users.py b/care/users/api/viewsets/users.py index 9d590b546d..de1b1c79a9 100644 --- a/care/users/api/viewsets/users.py +++ b/care/users/api/viewsets/users.py @@ -1,6 +1,6 @@ from django.contrib.auth import get_user_model from django.core.cache import cache -from django.db.models import F +from django.db.models import F, Subquery from django_filters import rest_framework as filters from drf_spectacular.utils import extend_schema from dry_rest_permissions.generics import DRYPermissions @@ -122,6 +122,39 @@ class UserViewSet( # DRYPermissions(), # ] + def get_queryset(self): + if self.request.user.is_superuser: + return self.queryset + if ( + self.request.user.user_type == User.TYPE_VALUE_MAP["StateAdmin"] + or self.request.user.user_type == User.TYPE_VALUE_MAP["StateReadOnlyAdmin"] + ): + return self.queryset.filter( + state=self.request.user.state, + user_type__lt=User.TYPE_VALUE_MAP["StateAdmin"], + is_superuser=False, + ) + if ( + self.request.user.user_type == User.TYPE_VALUE_MAP["DistrictAdmin"] + or self.request.user.user_type + == User.TYPE_VALUE_MAP["DistrictReadOnlyAdmin"] + ): + return self.queryset.filter( + district=self.request.user.district, + user_type__lt=User.TYPE_VALUE_MAP["DistrictAdmin"], + is_superuser=False, + ) + else: + return self.queryset.filter( + facilityuser__facility_id__in=Subquery( + FacilityUser.objects.filter(user=self.request.user.id).values( + "facility_id" + ) + ), + user_type__lt=User.TYPE_VALUE_MAP["DistrictAdmin"], + is_superuser=False, + ).distinct() + def get_serializer_class(self): if self.action == "list": return UserListSerializer From 5d3421965c15e279991bb69930f4b725e8db34a3 Mon Sep 17 00:00:00 2001 From: Gokulram A Date: Thu, 7 Dec 2023 11:10:07 +0530 Subject: [PATCH 2/4] Used get_accessible_facilities instead of subquery to fetch the facilities linked to the user --- care/users/api/viewsets/users.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/care/users/api/viewsets/users.py b/care/users/api/viewsets/users.py index de1b1c79a9..20fde01064 100644 --- a/care/users/api/viewsets/users.py +++ b/care/users/api/viewsets/users.py @@ -1,6 +1,6 @@ from django.contrib.auth import get_user_model from django.core.cache import cache -from django.db.models import F, Subquery +from django.db.models import F from django_filters import rest_framework as filters from drf_spectacular.utils import extend_schema from dry_rest_permissions.generics import DRYPermissions @@ -22,6 +22,7 @@ UserListSerializer, UserSerializer, ) +from care.utils.cache.cache_allowed_facilities import get_accessible_facilities User = get_user_model() @@ -146,10 +147,8 @@ def get_queryset(self): ) else: return self.queryset.filter( - facilityuser__facility_id__in=Subquery( - FacilityUser.objects.filter(user=self.request.user.id).values( - "facility_id" - ) + facilityuser__facility_id__in=get_accessible_facilities( + self.request.user ), user_type__lt=User.TYPE_VALUE_MAP["DistrictAdmin"], is_superuser=False, From 2b402f8958126926193d545eeb79ec11c197e2a7 Mon Sep 17 00:00:00 2001 From: khavinshankar Date: Tue, 30 Apr 2024 15:43:12 +0530 Subject: [PATCH 3/4] fixed failing tests --- .../tests/test_unlink_district_admins.py | 11 ++++----- care/users/tests/test_api.py | 24 +++++++++++-------- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/care/facility/tests/test_unlink_district_admins.py b/care/facility/tests/test_unlink_district_admins.py index 419e5f83a7..673ae4e95e 100644 --- a/care/facility/tests/test_unlink_district_admins.py +++ b/care/facility/tests/test_unlink_district_admins.py @@ -49,11 +49,8 @@ def test_unlink_home_facility_admin_different_district(self): response = self.client.delete( "/api/v1/users/" + username + "/clear_home_facility/" ) - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.assertEqual( - response.json()["facility"], - "Cannot unlink User's Home Facility from other district", - ) + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + self.assertEqual(response.json()["detail"], "Not found.") def test_unlink_faciltity_admin_same_district(self): self.client.force_login(self.admin1) @@ -80,5 +77,5 @@ def test_unlink_faciltity_admin_different_district(self): "/api/v1/users/" + username + "/delete_facility/", {"facility": self.facility2.external_id}, ) - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.assertEqual(response.json()["facility"], "Facility Access not Present") + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + self.assertEqual(response.json()["detail"], "Not found.") diff --git a/care/users/tests/test_api.py b/care/users/tests/test_api.py index 18512436db..90ca8b45e4 100644 --- a/care/users/tests/test_api.py +++ b/care/users/tests/test_api.py @@ -123,29 +123,33 @@ def setUpTestData(cls) -> None: cls.local_body = cls.create_local_body(cls.district) cls.super_user = cls.create_super_user("su", cls.district) cls.facility = cls.create_facility(cls.super_user, cls.district, cls.local_body) + cls.user = cls.create_user("staff1", cls.district, home_facility=cls.facility) + cls.data_2 = cls.get_user_data(cls.district) cls.data_2.update({"username": "user_2", "password": "password"}) cls.user_2 = cls.create_user(**cls.data_2) + cls.data_3 = cls.get_user_data(cls.district) + cls.data_3.update({"username": "user_3", "password": "password"}) + cls.user_3 = cls.create_user(**cls.data_3) + cls.link_user_with_facility(cls.user_3, cls.facility, cls.super_user) + def test_user_can_access_url(self): """Test user can access the url by location""" username = self.user.username response = self.client.get(f"/api/v1/users/{username}/") self.assertEqual(response.status_code, status.HTTP_200_OK) - def test_user_can_read_all(self): - """Test user can read all""" + def test_user_can_read_all_users_within_accessible_facility(self): + """Test user can read all users within the accessible facility""" response = self.client.get("/api/v1/users/") - # test response code self.assertEqual(response.status_code, status.HTTP_200_OK) res_data_json = response.json() - # test total user count self.assertEqual(res_data_json["count"], 2) results = res_data_json["results"] - # test presence of usernames self.assertIn(self.user.id, {r["id"] for r in results}) - self.assertIn(self.user_2.id, {r["id"] for r in results}) + self.assertIn(self.user_3.id, {r["id"] for r in results}) def test_user_can_modify_themselves(self): """Test user can modify the attributes for themselves""" @@ -170,7 +174,8 @@ def test_user_cannot_read_others(self): """Test 1 user can read the attributes of the other user""" username = self.data_2["username"] response = self.client.get(f"/api/v1/users/{username}/") - self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + self.assertEqual(response.json()["detail"], "Not found.") def test_user_cannot_modify_others(self): """Test a user can't modify others""" @@ -183,15 +188,14 @@ def test_user_cannot_modify_others(self): "password": password, }, ) - self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + self.assertEqual(response.json()["detail"], "Not found.") def test_user_cannot_delete_others(self): """Test a user can't delete others""" field = "username" response = self.client.delete(f"/api/v1/users/{self.data_2[field]}/") - # test response code self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) - # test backend response(user_2 still exists) self.assertEqual( self.data_2[field], User.objects.get(username=self.data_2[field]).username, From 1030089b896f6bd09276700717807fce1b1af447 Mon Sep 17 00:00:00 2001 From: Aakash Singh Date: Tue, 30 Apr 2024 16:03:26 +0530 Subject: [PATCH 4/4] fix queryset --- care/users/api/viewsets/users.py | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/care/users/api/viewsets/users.py b/care/users/api/viewsets/users.py index 059d941b5d..7916d08418 100644 --- a/care/users/api/viewsets/users.py +++ b/care/users/api/viewsets/users.py @@ -1,5 +1,5 @@ from django.core.cache import cache -from django.db.models import F +from django.db.models import F, Q, Subquery from django_filters import rest_framework as filters from drf_spectacular.utils import extend_schema from dry_rest_permissions.generics import DRYPermissions @@ -123,33 +123,32 @@ class UserViewSet( def get_queryset(self): if self.request.user.is_superuser: return self.queryset - if ( - self.request.user.user_type == User.TYPE_VALUE_MAP["StateAdmin"] - or self.request.user.user_type == User.TYPE_VALUE_MAP["StateReadOnlyAdmin"] - ): - return self.queryset.filter( + query = Q(id=self.request.user.id) + if self.request.user.user_type >= User.TYPE_VALUE_MAP["StateReadOnlyAdmin"]: + query |= Q( state=self.request.user.state, user_type__lt=User.TYPE_VALUE_MAP["StateAdmin"], is_superuser=False, ) - if ( - self.request.user.user_type == User.TYPE_VALUE_MAP["DistrictAdmin"] - or self.request.user.user_type - == User.TYPE_VALUE_MAP["DistrictReadOnlyAdmin"] + elif ( + self.request.user.user_type >= User.TYPE_VALUE_MAP["DistrictReadOnlyAdmin"] ): - return self.queryset.filter( + query |= Q( district=self.request.user.district, user_type__lt=User.TYPE_VALUE_MAP["DistrictAdmin"], is_superuser=False, ) else: - return self.queryset.filter( - facilityuser__facility_id__in=get_accessible_facilities( - self.request.user + query |= Q( + id__in=Subquery( + FacilityUser.objects.filter( + facility_id__in=get_accessible_facilities(self.request.user) + ).values("user_id") ), user_type__lt=User.TYPE_VALUE_MAP["DistrictAdmin"], is_superuser=False, - ).distinct() + ) + return self.queryset.filter(query) def get_serializer_class(self): if self.action == "list":