code refactoring for users

training-front-end/src/components/AdminSearchUser.vue

@@ -13,7 +13,7 @@
   const PAGE_SIZE = 25
 
   const base_url = import.meta.env.PUBLIC_API_BASE_URL
-  const report_url = `${base_url}/api/v1/users/search-users-by-name/`
+  const report_url = `${base_url}/api/v1/users/`
   const update_url = `${base_url}/api/v1/users/edit-user-for-reporting/`
 
   const currentPage = ref(0)
@@ -33,8 +33,9 @@
 
   async function search() {
     noResults.value = false
-    const url = new URL(`${report_url}${searchTerm.value}`)
-    url.search = new URLSearchParams({page_number: currentPage.value + 1})
+    const url = new URL(`${report_url}`)
+    url.search = new URLSearchParams({name: searchTerm.value, page_number: currentPage.value + 1})
+
     try {
       const response = await fetch(
         url, {
@@ -64,7 +65,7 @@
     try {
       const response = await fetch(
         url, { 
-          method: "PUT", 
+          method: "PATCH", 
           headers: { 
             'Content-Type': 'application/json',
             'Authorization': `Bearer ${user.value.jwt}` 

training-front-end/src/components/__tests__/AdminSearchUser.spec.js

@@ -69,7 +69,7 @@ describe('AdminAgencySelect', async () => {
 
     expect(updateFetchSpy).nthCalledWith(1, expect.any(URL), {
       body: '[10]',
-      method: 'PUT',
+      method: 'PATCH',
       headers: {
         'Authorization': 'Bearer some-token-value',
         'Content-Type': 'application/json'
@@ -118,7 +118,7 @@ describe('AdminAgencySelect', async () => {
     linkElements[1].trigger('click')
 
     expect(fetchSpy).toBeCalledTimes(2)
-    expect(fetchSpy.mock.lastCall[0].search).toBe('?page_number=2')
+    expect(fetchSpy.mock.lastCall[0].search).toBe('?name=Steeply&page_number=2')
   })
 
   it('displays no results message', async () => {

training/api/api_v1/users.py

@@ -1,7 +1,6 @@
 import csv
 from io import StringIO
 import logging
-from typing import List
 from training.api.auth import RequireRole
 from fastapi import APIRouter, status, HTTPException, Response, Depends
 from training.schemas import User, UserCreate, UserSearchResult
@@ -30,21 +29,8 @@ def create_user(
     return db_user
 
 
-@router.get("/users", response_model=List[User])
-def get_users(
-    agency_id: int | None = None,
-    repo: UserRepository = Depends(user_repository),
-    user=Depends(RequireRole(["Admin"]))
-):
-    if agency_id:
-        return repo.find_by_agency(agency_id)
-    else:
-        # If agency_id is <= 0 or None, return all users:
-        return repo.find_all()
-
-
-@router.put("/users/edit-user-for-reporting", response_model=User)
-def edit_user_by_id(
+@router.patch("/users/edit-user-for-reporting", response_model=User)
+def edit_user_for_reporting(
     user_id: int,
     agency_id_list: list[int],
     repo: UserRepository = Depends(user_repository),
@@ -84,15 +70,22 @@ def download_report_csv(user=Depends(user_from_form), repo: UserRepository = Dep
     return Response(output.getvalue(), headers=headers, media_type='application/csv')
 
 
-@router.get("/users/search-users-by-name/{name}", response_model=UserSearchResult)
-def search_users_by_name(
-    name: str,
-    page_number: int,
+'''
+Get/users is used to search users for admin portal, currently only support search by user name, it may have additional search criteira in future.
+page_number param is used to support UI pagination functionality.
+It returns UserSearchResult object with a list of users and total_count returned.
+'''
+
+
+@router.get("/users", response_model=UserSearchResult)
+def get_users(
+    name: str | None = None,
+    page_number: int = 1,
     repo: UserRepository = Depends(user_repository),
     user=Depends(RequireRole(["Admin"]))
 ):
     try:
-        return repo.search_users_by_name(name, page_number)
+        return repo.get_users(name, page_number)
     except ValueError:
         raise HTTPException(
             status_code=status.HTTP_400_BAD_REQUEST,

training/repositories/user.py

@@ -19,6 +19,7 @@ def find_by_email(self, email: str) -> models.User | None:
     def find_by_agency(self, agency_id: int) -> list[models.User]:
         return self._session.query(models.User).filter(models.User.agency_id == agency_id).all()
 
+    # edit_user_for_reporting is function allow admin to modify specific user and assign report role and associate report agencies to the user
     def edit_user_for_reporting(self, user_id: int, report_agencies_list: list[int]) -> models.User:
         db_user = self._session.query(models.User).filter(models.User.id == user_id).first()
         if db_user is None:
@@ -30,11 +31,13 @@ def edit_user_for_reporting(self, user_id: int, report_agencies_list: list[int])
                 if report_role:
                     db_user.roles.append(report_role)
                 else:
+                    # if Report role is not in DB, add it to DB (should not happen if data is prepopulated properly via seed.py and no direct DB removal)
                     role = models.Role(name="Report")
                     self._session.add(role)
                     self._session.commit()
                     db_user.roles.append(role)
         else:
+            # if report_agencies_list =[], it means remove all user associated agencies and thus remove user report role.
             if len(report_role_exist) > 0:
                 db_user.roles = [obj for obj in db_user.roles if obj.name != "Report"]
         db_user.report_agencies.clear()
@@ -47,9 +50,6 @@ def edit_user_for_reporting(self, user_id: int, report_agencies_list: list[int])
         self._session.commit()
         return db_user
 
-    def get_admins_users(self) -> list[models.User]:
-        return self._session.query(models.User).filter(models.User.roles.any(name='Admin')).all()
-
     def get_user_quiz_completion_report(self, report_user_id: int) -> list[UserQuizCompletionReportData]:
         report_user = self.find_by_id(report_user_id)
         if report_user and report_user.report_agencies:
@@ -66,7 +66,8 @@ def get_user_quiz_completion_report(self, report_user_id: int) -> list[UserQuizC
         else:
             raise ValueError("Invalid Report User")
 
-    def search_users_by_name(self, name: str, page_number: int) -> UserSearchResult:
+    def get_users(self, name: str, page_number: int) -> UserSearchResult:
+        # currently UI only support search by name, future may provide more search criteria
         if (name and name.strip() != '' and page_number > 0):
             count = self._session.query(models.User).filter(models.User.name.ilike(f"%{name}%")).count()
             page_size = 25
@@ -75,4 +76,4 @@ def search_users_by_name(self, name: str, page_number: int) -> UserSearchResult:
             user_search_result = UserSearchResult(users=search_results, total_count=count)
             return user_search_result
         else:
-            raise ValueError("Invalid search criteria")
+            raise ValueError("Invalid search criteria")

training/tests/test_api_users.py

@@ -53,36 +53,12 @@ def test_create_user_duplicate(goodJWT, mock_user_repo: UserRepository):
 
 
 @patch('training.config.settings', 'JWT_SECRET', 'super_secret')
-def test_get_users_all(goodJWT, mock_user_repo: UserRepository):
-    users = [UserSchemaFactory.build() for x in range(3)]
-    mock_user_repo.find_all.return_value = users
-    response = client.get(
-        "/api/v1/users",
-        headers={"Authorization": f"Bearer {goodJWT}"}
-    )
-    assert response.status_code == status.HTTP_200_OK
-    assert len(response.json()) == 3
-
-
-@patch('training.config.settings', 'JWT_SECRET', 'super_secret')
-def test_get_users_by_agency(goodJWT, mock_user_repo: UserRepository):
-    users = [UserSchemaFactory.build(agency_id=2) for x in range(5)]
-    mock_user_repo.find_by_agency.return_value = users
-    response = client.get(
-        "/api/v1/users?agency_id=2",
-        headers={"Authorization": f"Bearer {goodJWT}"}
-    )
-    assert response.status_code == status.HTTP_200_OK
-    assert len(response.json()) == 5
-
-
-@patch('training.config.settings', 'JWT_SECRET', 'super_secret')
-def test_search_users_by_name(goodJWT, mock_user_repo: UserRepository):
+def test_get_users(goodJWT, mock_user_repo: UserRepository):
     users = [UserSchemaFactory.build(name="test name") for x in range(2)]
     user_search_result = UserSearchResult(users=users, total_count=2)
-    mock_user_repo.search_users_by_name.return_value = user_search_result
+    mock_user_repo.get_users.return_value = user_search_result
     response = client.get(
-        "/api/v1/users/search-users-by-name/test?page_number=1",
+        "/api/v1/users?name=test&page_number=1",
         headers={"Authorization": f"Bearer {goodJWT}"}
     )
     assert response.status_code == status.HTTP_200_OK
@@ -102,7 +78,7 @@ def test_edit_user_for_reporting(mock_user_repo: UserRepository, goodJWT: str):
     mock_user_repo.edit_user_for_reporting.return_value = user
     user_id = user.id
     URL = f"/api/v1/users/edit-user-for-reporting?user_id={user_id}"
-    response = client.put(
+    response = client.patch(
         URL,
         json=[3],
         headers={"Authorization": f"Bearer {goodJWT}"}

training/tests/test_user_repository.py

@@ -99,18 +99,17 @@ def test_invalid_edit_user_for_reporting(user_repo_with_data: UserRepository):
         user_repo_with_data.create(invalid_user_id, invalid_agency_id_list)
 
 
-def test_search_users_by_name(user_repo_with_data: UserRepository, valid_user_ids: List[int]):
+def test_get_users(user_repo_with_data: UserRepository, valid_user_ids: List[int]):
     valid_user_id = valid_user_ids[0]
     db_user = user_repo_with_data.find_by_id(valid_user_id)
     search_criteria = db_user.name[:-1]
-    result = user_repo_with_data.search_users_by_name(search_criteria, 1)
+    result = user_repo_with_data.get_users(search_criteria, 1)
     assert result is not None
     for item in result.users:
         assert search_criteria in item.name
 
 
-def test_invalid_search_users_by_name(user_repo_with_data: UserRepository):
+def test_invalid_get_users(user_repo_with_data: UserRepository):
     search_criteria = None
-
     with pytest.raises(Exception):
-        user_repo_with_data.search_users_by_name(search_criteria, 0)
+        user_repo_with_data.get_users(search_criteria, 0)