Modified User Route for User Management Redesign (#2595)

* Added get user route - Added get user route - For fetching details of other users - Added additional permissions check for change password - Allowing district admins and above to change password - Enable username search for FacilityUsers * added tests * Added additional tests - Added last_login as a field in UserSerializer - To access login time for user detail queries - Tweaked error messages to follow similar format * added last_login to read_only as well * switched to using UserListView's get route * changed password perms * filter facility users by search fields * add test for coverage * simplified perms for get * reverting unneeded changes * fix * Remove redundant check --------- Co-authored-by: Vignesh Hari <[email protected]>
ohcnetwork · Dec 10, 2024 · 2b4f1dd · 2b4f1dd
1 parent 9a20757
commit 2b4f1dd
Show file tree

Hide file tree

Showing 7 changed files with 97 additions and 12 deletions.
diff --git a/care/facility/api/viewsets/facility_users.py b/care/facility/api/viewsets/facility_users.py
@@ -16,6 +16,7 @@ class UserFilter(filters.FilterSet):
         choices=[(key, key) for key in User.TYPE_VALUE_MAP],
         coerce=lambda role: User.TYPE_VALUE_MAP[role],
     )
+    username = filters.CharFilter(field_name="username", lookup_expr="icontains")
 
     class Meta:
         model = User

diff --git a/care/facility/tests/test_facilityuser_api.py b/care/facility/tests/test_facilityuser_api.py
@@ -20,6 +20,9 @@ def setUpTestData(cls) -> None:
         cls.facility2 = cls.create_facility(
             cls.super_user, cls.district, cls.local_body
         )
+        cls.user2 = cls.create_user(
+            "dummystaff", cls.district, home_facility=cls.facility2
+        )
 
     def setUp(self) -> None:
         self.client.force_authenticate(self.super_user)
@@ -32,6 +35,15 @@ def test_get_queryset_with_prefetching(self):
         self.assertEqual(response.status_code, status.HTTP_200_OK)
         self.assertNumQueries(2)
 
+    def test_get_queryset_with_search(self):
+        response = self.client.get(
+            f"/api/v1/facility/{self.facility2.external_id}/get_users/?username={self.user2.username}"
+        )
+
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+        self.assertEqual(len(response.data["results"]), 1)
+        self.assertEqual(response.data["results"][0]["username"], self.user2.username)
+
     def test_link_new_facility(self):
         response = self.client.get("/api/v1/facility/")
 

diff --git a/care/users/api/serializers/user.py b/care/users/api/serializers/user.py
@@ -335,6 +335,7 @@ class Meta:
             "pf_auth",
             "read_profile_picture_url",
             "user_flags",
+            "last_login",
         )
         read_only_fields = (
             "is_superuser",
@@ -347,6 +348,7 @@ class Meta:
             "pf_endpoint",
             "pf_p256dh",
             "pf_auth",
+            "last_login",
         )
 
     extra_kwargs = {"url": {"lookup_field": "username"}}

diff --git a/care/users/api/viewsets/users.py b/care/users/api/viewsets/users.py
@@ -155,6 +155,9 @@ def get_queryset(self):
 
     def get_object(self) -> User:
         try:
+            if self.action == "retrieve":
+                username = self.kwargs.get("username")
+                return get_object_or_404(User, username=username)
             return super().get_object()
         except Http404 as e:
             error = "User not found"

diff --git a/care/users/models.py b/care/users/models.py
@@ -364,7 +364,7 @@ def has_read_permission(request):
         return True
 
     def has_object_read_permission(self, request):
-        return request.user.is_superuser or self == request.user
+        return True
 
     @staticmethod
     def has_write_permission(request):

diff --git a/care/users/tests/test_api.py b/care/users/tests/test_api.py
@@ -22,6 +22,10 @@ def setUpTestData(cls) -> None:
         cls.user = cls.create_user("staff1", cls.district)
         cls.user_data = cls.get_user_data(cls.district, 40)
 
+        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)
+
     def setUp(self):
         self.client.force_authenticate(self.super_user)
 
@@ -51,6 +55,7 @@ def get_detail_representation(self, obj=None) -> dict:
             "video_connect_link": obj.video_connect_link,
             "read_profile_picture_url": obj.profile_picture_url,
             "user_flags": [],
+            "last_login": obj.last_login,
             **self.get_local_body_district_state_representation(obj),
         }
 
@@ -67,9 +72,11 @@ def test_superuser_can_view(self):
         data = self.user_data.copy()
         data["date_of_birth"] = str(data["date_of_birth"])
         data.pop("password")
+        user_data = self.get_detail_representation(self.user)
+        user_data.pop("created_by")
         self.assertDictEqual(
             res_data_json,
-            self.get_detail_representation(self.user),
+            user_data,
         )
 
     def test_superuser_can_modify(self):
@@ -137,10 +144,38 @@ def setUpTestData(cls) -> None:
         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.data_3.update(
+            {
+                "username": "user_3",
+                "password": "password",
+                "user_type": User.TYPE_VALUE_MAP["Doctor"],
+            }
+        )
         cls.user_3 = cls.create_user(**cls.data_3)
         cls.link_user_with_facility(cls.user_3, cls.facility, cls.super_user)
 
+        cls.data_4 = cls.get_user_data(cls.district)
+        cls.data_4.update(
+            {
+                "username": "user_4",
+                "password": "password",
+                "user_type": User.TYPE_VALUE_MAP["DistrictAdmin"],
+            }
+        )
+        cls.user_4 = cls.create_user(**cls.data_4)
+        cls.link_user_with_facility(cls.user_4, cls.facility, cls.super_user)
+
+        cls.data_5 = cls.get_user_data(cls.district)
+        cls.data_5.update(
+            {
+                "username": "user_5",
+                "password": "password",
+                "user_type": User.TYPE_VALUE_MAP["WardAdmin"],
+            }
+        )
+        cls.user_5 = cls.create_user(**cls.data_5)
+        cls.link_user_with_facility(cls.user_5, cls.facility, cls.super_user)
+
     def test_user_can_access_url(self):
         """Test user can access the url by location"""
         username = self.user.username
@@ -152,7 +187,7 @@ def test_user_can_read_all_users_within_accessible_facility(self):
         response = self.client.get("/api/v1/users/")
         self.assertEqual(response.status_code, status.HTTP_200_OK)
         res_data_json = response.json()
-        self.assertEqual(res_data_json["count"], 2)
+        self.assertEqual(res_data_json["count"], 3)
         results = res_data_json["results"]
         self.assertIn(self.user.id, {r["id"] for r in results})
         self.assertIn(self.user_3.id, {r["id"] for r in results})
@@ -176,12 +211,12 @@ def test_user_can_modify_themselves(self):
             User.objects.get(username=username).date_of_birth, date(2005, 4, 1)
         )
 
-    def test_user_cannot_read_others(self):
-        """Test 1 user can read the attributes of the other user"""
-        username = self.data_2["username"]
+    def test_user_can_read_others(self):
+        """Test 1 user can read the attributes of any other user"""
+        username = self.user_2.username
         response = self.client.get(f"/api/v1/users/{username}/")
-        self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)
-        self.assertEqual(response.json()["detail"], "User not found")
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+        self.assertEqual(response.json()["first_name"], self.user_2.first_name)
 
     def test_user_cannot_modify_others(self):
         """Test a user can't modify others"""
@@ -207,6 +242,38 @@ def test_user_cannot_delete_others(self):
             User.objects.get(username=self.data_2[field]).username,
         )
 
+    def test_user_cannot_change_password_of_others(self):
+        """Test a user cannot change password of others"""
+        username = self.data_2["username"]
+        password = self.data_2["password"]
+        response = self.client.put(
+            "/api/v1/password_change/",
+            {
+                "username": username,
+                "old_password": password,
+                "new_password": "password2",
+            },
+        )
+        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+
+    def test_user_with_districtadmin_access_can_modify_others(self):
+        """Test a user with district admin access can modify others underneath the hierarchy"""
+        self.client.force_authenticate(self.user_4)
+        username = self.data_2["username"]
+        response = self.client.patch(
+            f"/api/v1/users/{username}/",
+            {
+                "date_of_birth": date(2005, 4, 1),
+            },
+        )
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+        self.assertEqual(response.json()["date_of_birth"], "2005-04-01")
+
+    def test_user_gets_error_when_accessing_user_details_with_invalid_username(self):
+        """Test a user gets error when accessing user details with invalid username"""
+        response = self.client.get("/api/v1/users/foobar/")
+        self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)
+
 
 class TestUserFilter(TestUtils, APITestCase):
     @classmethod

diff --git a/care/utils/tests/test_utils.py b/care/utils/tests/test_utils.py
@@ -764,7 +764,7 @@ def get_local_body_district_state_representation(self, obj):
         response.update(self.get_state_representation(getattr(obj, "state", None)))
         return response
 
-    def get_local_body_representation(self, local_body: LocalBody):
+    def get_local_body_representation(self, local_body: LocalBody | None):
         if local_body is None:
             return {"local_body": None, "local_body_object": None}
         return {
@@ -778,7 +778,7 @@ def get_local_body_representation(self, local_body: LocalBody):
             },
         }
 
-    def get_district_representation(self, district: District):
+    def get_district_representation(self, district: District | None):
         if district is None:
             return {"district": None, "district_object": None}
         return {
@@ -790,7 +790,7 @@ def get_district_representation(self, district: District):
             },
         }
 
-    def get_state_representation(self, state: State):
+    def get_state_representation(self, state: State | None):
         if state is None:
             return {"state": None, "state_object": None}
         return {"state": state.id, "state_object": {"id": state.id, "name": state.name}}