From 630153b5c69cef6356b195538cb5dcdbbf8dda13 Mon Sep 17 00:00:00 2001 From: weiwang-gsa Date: Thu, 17 Aug 2023 10:28:44 -0400 Subject: [PATCH] code refactoring for users --- .../src/components/AdminIndex.vue | 69 ------------------- .../src/components/AdminSearchUser.vue | 9 +-- .../__tests__/AdminSearchUser.spec.js | 4 +- training/api/api_v1/users.py | 35 ++++------ training/repositories/user.py | 11 +-- training/tests/test_api_users.py | 32 ++------- training/tests/test_user_repository.py | 9 ++- 7 files changed, 35 insertions(+), 134 deletions(-) delete mode 100644 training-front-end/src/components/AdminIndex.vue diff --git a/training-front-end/src/components/AdminIndex.vue b/training-front-end/src/components/AdminIndex.vue deleted file mode 100644 index 68b7cd9b..00000000 --- a/training-front-end/src/components/AdminIndex.vue +++ /dev/null @@ -1,69 +0,0 @@ - - - diff --git a/training-front-end/src/components/AdminSearchUser.vue b/training-front-end/src/components/AdminSearchUser.vue index 450a3b96..afbbe065 100644 --- a/training-front-end/src/components/AdminSearchUser.vue +++ b/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}` diff --git a/training-front-end/src/components/__tests__/AdminSearchUser.spec.js b/training-front-end/src/components/__tests__/AdminSearchUser.spec.js index f09ee473..13a76abf 100644 --- a/training-front-end/src/components/__tests__/AdminSearchUser.spec.js +++ b/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 () => { diff --git a/training/api/api_v1/users.py b/training/api/api_v1/users.py index 25e50eab..db55b167 100644 --- a/training/api/api_v1/users.py +++ b/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, diff --git a/training/repositories/user.py b/training/repositories/user.py index b9f1d895..d8ce69c1 100644 --- a/training/repositories/user.py +++ b/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") \ No newline at end of file diff --git a/training/tests/test_api_users.py b/training/tests/test_api_users.py index 0132b099..0aa1d92b 100644 --- a/training/tests/test_api_users.py +++ b/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}"} diff --git a/training/tests/test_user_repository.py b/training/tests/test_user_repository.py index 9a31ce1a..881b5283 100644 --- a/training/tests/test_user_repository.py +++ b/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) \ No newline at end of file