address code review comments

training/api/api_v1/users.py

@@ -2,12 +2,12 @@
 from io import StringIO
 import logging
 from training.api.auth import RequireRole
-from fastapi import APIRouter, status, HTTPException, Response, Depends
+from fastapi import APIRouter, status, HTTPException, Response, Depends, Query
 from training.schemas import User, UserCreate, UserSearchResult
 from training.repositories import UserRepository
 from training.api.deps import user_repository
 from training.api.auth import user_from_form
-
+from typing import Annotated
 
 router = APIRouter()
 
@@ -70,24 +70,18 @@ def download_report_csv(user=Depends(user_from_form), repo: UserRepository = Dep
     return Response(output.getvalue(), headers=headers, media_type='application/csv')
 
 
-'''
-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,
+    name: Annotated[str, Query(min_length=1)],
     page_number: int = 1,
-    repo: UserRepository = Depends(user_repository),
-    user=Depends(RequireRole(["Admin"]))
+    repo: UserRepository = Depends(user_repository)#,
+    #user=Depends(RequireRole(["Admin"]))
 ):
-    try:
-        return repo.get_users(name, page_number)
-    except ValueError:
-        raise HTTPException(
-            status_code=status.HTTP_400_BAD_REQUEST,
-            detail="invalid search criteria"
-        )
+    '''
+    Get/users is used to search users for admin portal
+    currently search only support search by user name, name is required field.
+    It may have additional search criteira in future, which will require logic update.
+    page_number param is used to support UI pagination functionality.
+    It returns UserSearchResult object with a list of users and total_count used for UI pagination
+    '''
+    return repo.get_users(name, page_number)

training/repositories/user.py

@@ -18,9 +18,9 @@ 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:
+        # edit_user_for_reporting allow admin to assign report role and associate report agencies to specific user
         db_user = self._session.query(models.User).filter(models.User.id == user_id).first()
         if db_user is None:
             raise ValueError("invalid user id")
@@ -37,7 +37,7 @@ def edit_user_for_reporting(self, user_id: int, report_agencies_list: list[int])
                     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 report_agencies_list =[], it will 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()
@@ -67,13 +67,11 @@ def get_user_quiz_completion_report(self, report_user_id: int) -> list[UserQuizC
             raise ValueError("Invalid Report User")
 
     def get_users(self, name: str, page_number: int) -> UserSearchResult:
-        # currently UI only support search by name, future may provide more search criteria
+        # current UI only support search by user name, and it is required field.
         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
             offset = (page_number - 1) * page_size
             search_results = self._session.query(models.User).filter(models.User.name.ilike(f"%{name}%")).limit(page_size).offset(offset).all()
             user_search_result = UserSearchResult(users=search_results, total_count=count)
             return user_search_result
-        else:
-            raise ValueError("Invalid search criteria")

training/tests/test_user_repository.py

@@ -106,10 +106,4 @@ def test_get_users(user_repo_with_data: UserRepository, valid_user_ids: List[int
     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_get_users(user_repo_with_data: UserRepository):
-    search_criteria = None
-    with pytest.raises(Exception):
-        user_repo_with_data.get_users(search_criteria, 0)
+        assert search_criteria in item.name