From 5308b420ba13be738f10763712e726b9c7107bbd Mon Sep 17 00:00:00 2001 From: Nathan Franklin Date: Tue, 7 Feb 2023 14:37:56 -0600 Subject: [PATCH] task/DES-2381: fix created project's response to include deletable (#111) * Fix created project's response to include deletable Fixes DES-2381 * Clean some code * Fix queries * Fix tests * Fix pep8 errors * Fix model's relationships * Fix getting custom system usr retrieval * Remove unused logging --- geoapi/custom/designsafe/project_users.py | 3 --- geoapi/models/project.py | 5 +++-- geoapi/models/users.py | 3 ++- geoapi/services/projects.py | 17 +++++++++-------- geoapi/services/users.py | 4 ++-- geoapi/tasks/external_data.py | 10 ++++++---- geoapi/tests/api_tests/test_projects_routes.py | 11 +++++++++++ geoapi/tests/api_tests/test_projects_service.py | 1 + geoapi/tests/api_tests/test_users_service.py | 11 +++++------ geoapi/tests/custom/test_designsafe.py | 4 ++++ geoapi/utils/agave.py | 5 ++--- 11 files changed, 45 insertions(+), 29 deletions(-) diff --git a/geoapi/custom/designsafe/project_users.py b/geoapi/custom/designsafe/project_users.py index 982770f7..504fe7e9 100644 --- a/geoapi/custom/designsafe/project_users.py +++ b/geoapi/custom/designsafe/project_users.py @@ -1,7 +1,4 @@ from urllib.parse import quote -from geoapi.log import logging - -logger = logging.getLogger(__name__) def get_system_users(tenant_id, jwt, system_id: str): diff --git a/geoapi/models/project.py b/geoapi/models/project.py index 93dd73c5..7795f75d 100644 --- a/geoapi/models/project.py +++ b/geoapi/models/project.py @@ -17,7 +17,7 @@ class ProjectUser(Base): creator = Column(Boolean, nullable=False, default=False) admin = Column(Boolean, nullable=False, default=False) project = relationship('Project', backref=backref('project_users', cascade="all, delete-orphan")) - user = relationship('User') + user = relationship('User', viewonly=True) def __repr__(self): return f'' @@ -41,7 +41,8 @@ class Project(Base): users = relationship('User', secondary='projects_users', - back_populates='projects') + back_populates='projects', + overlaps="project,project_users") point_clouds = relationship('PointCloud', cascade="all, delete-orphan") def __repr__(self): diff --git a/geoapi/models/users.py b/geoapi/models/users.py index a8271570..a95d1050 100644 --- a/geoapi/models/users.py +++ b/geoapi/models/users.py @@ -15,7 +15,8 @@ class User(Base): streetviews = relationship('Streetview', cascade="all, delete-orphan") projects = relationship('Project', secondary='projects_users', - back_populates='users', lazy="joined") + back_populates='users', lazy="joined", + overlaps="project,project_users") def __repr__(self): return ''.format(self.username, self.id) diff --git a/geoapi/services/projects.py b/geoapi/services/projects.py index e68491a2..9fb212d6 100644 --- a/geoapi/services/projects.py +++ b/geoapi/services/projects.py @@ -48,7 +48,7 @@ def create(data: dict, user: User) -> Project: project.project_users[0].creator = True db_session.add(project) db_session.commit() - + setattr(project, 'deletable', True) return project @staticmethod @@ -100,11 +100,8 @@ def list(user: User) -> List[Project]: :param user: User :return: List[Project] """ - projects_and_project_user = db_session.query(Project, ProjectUser) \ .join(ProjectUser) \ - .filter(User.username == user.username) \ - .filter(User.tenant_id == user.tenant_id) \ .filter(ProjectUser.user_id == user.id) \ .order_by(desc(Project.created)) \ .all() @@ -130,8 +127,11 @@ def get(project_id: Optional[int] = None, uuid: Optional[str] = None, user: User raise ValueError("project_id or uid is required") if project and user and not is_anonymous(user): - project_user = db_session.query(ProjectUser).filter(Project.id == project.id).filter(User.id == user.id).first() - setattr(project, 'deletable', project_user.admin or project_user.creator) + project_user = db_session.query(ProjectUser)\ + .filter(ProjectUser.project_id == project.id)\ + .filter(ProjectUser.user_id == user.id).one_or_none() + if project_user: + setattr(project, 'deletable', project_user.admin or project_user.creator) return project @staticmethod @@ -292,8 +292,9 @@ def addUserToProject(projectId: int, username: str, admin: bool) -> None: proj.users.append(user) db_session.commit() - project_user = db_session.query(ProjectUser).filter(Project.id == projectId)\ - .filter(User.id == user.id).first() + project_user = db_session.query(ProjectUser)\ + .filter(ProjectUser.project_id == projectId)\ + .filter(ProjectUser.user_id == user.id).one() project_user.admin = admin db_session.commit() diff --git a/geoapi/services/users.py b/geoapi/services/users.py index 9c846442..ec3051a8 100644 --- a/geoapi/services/users.py +++ b/geoapi/services/users.py @@ -41,7 +41,7 @@ def canAccess(user: User, projectId: int) -> bool: .join(Project)\ .filter(ProjectUser.user_id == user.id)\ .filter(Project.tenant_id == user.tenant_id)\ - .filter(ProjectUser.project_id == projectId).first() + .filter(ProjectUser.project_id == projectId).one_or_none() if up: return True return False @@ -52,7 +52,7 @@ def is_admin_or_creator(user: User, projectId: int) -> bool: .join(Project) \ .filter(ProjectUser.user_id == user.id) \ .filter(Project.tenant_id == user.tenant_id) \ - .filter(ProjectUser.project_id == projectId).first() + .filter(ProjectUser.project_id == projectId).one_or_none() if up: return up.admin or up.creator return False diff --git a/geoapi/tasks/external_data.py b/geoapi/tasks/external_data.py index 6285e47f..2848ffb9 100644 --- a/geoapi/tasks/external_data.py +++ b/geoapi/tasks/external_data.py @@ -10,7 +10,7 @@ from geoapi.celery_app import app from geoapi.exceptions import InvalidCoordinateReferenceSystem, MissingServiceAccount -from geoapi.models import User, Project, ProjectUser, ObservableDataProject, Task +from geoapi.models import User, ProjectUser, ObservableDataProject, Task from geoapi.utils.agave import (AgaveUtils, SystemUser, get_system_users, get_metadata_using_service_account, AgaveFileGetError, AgaveListingError) from geoapi.log import logger @@ -344,7 +344,9 @@ def refresh_observable_projects(): current_users = set([SystemUser(username=u.user.username, admin=u.admin) for u in o.project.project_users]) updated_users = set(get_system_users(o.project.tenant_id, importing_user.jwt, o.system_id)) - current_creator = db_session.query(ProjectUser).filter(Project.id == o.id).filter(ProjectUser.creator is True).one_or_none() + current_creator = db_session.query(ProjectUser)\ + .filter(ProjectUser.project_id == o.id)\ + .filter(ProjectUser.creator is True).one_or_none() if current_users != updated_users: logger.info("Updating users from:{} to:{}".format(current_users, updated_users)) @@ -362,9 +364,9 @@ def refresh_observable_projects(): db_session.commit() if current_creator: - # reset the creator + # reset the creator by finding that updated user again and updating it. current_creator = db_session.query(ProjectUser)\ - .filter(Project.id == o.id)\ + .filter(ProjectUser.project_id == o.id)\ .filter(ProjectUser.user_id == current_creator.user_id)\ .one_or_none() if current_creator: diff --git a/geoapi/tests/api_tests/test_projects_routes.py b/geoapi/tests/api_tests/test_projects_routes.py index 549616d7..deb7b5f8 100644 --- a/geoapi/tests/api_tests/test_projects_routes.py +++ b/geoapi/tests/api_tests/test_projects_routes.py @@ -24,6 +24,17 @@ def test_get_projects_but_not_admin_or_creator(test_client, user2, projects_fixt assert data[0]["deletable"] is False +def test_get_projects_with_multiple(test_client, user1, projects_fixture2, projects_fixture): + resp = test_client.get('/projects/', headers={'x-jwt-assertion-test': user1.jwt}) + data = resp.get_json() + assert resp.status_code == 200 + assert len(data) == 2 + assert data[0]["deletable"] is True + assert data[0]["id"] == projects_fixture.id + assert data[1]["id"] == projects_fixture2.id + assert data[1]["deletable"] is True + + def test_get_projects_not_allowed(test_client): resp = test_client.get('/projects/') assert resp.status_code == 403 diff --git a/geoapi/tests/api_tests/test_projects_service.py b/geoapi/tests/api_tests/test_projects_service.py index b01586c5..672659a6 100644 --- a/geoapi/tests/api_tests/test_projects_service.py +++ b/geoapi/tests/api_tests/test_projects_service.py @@ -21,6 +21,7 @@ def test_create_project(): assert len(proj.users) == 1 assert proj.name == "test name" assert proj.description == "test description" + assert proj.deletable def test_delete_project(projects_fixture, remove_project_assets_mock, user1): diff --git a/geoapi/tests/api_tests/test_users_service.py b/geoapi/tests/api_tests/test_users_service.py index 8266bb6a..8f7c06cc 100644 --- a/geoapi/tests/api_tests/test_users_service.py +++ b/geoapi/tests/api_tests/test_users_service.py @@ -1,8 +1,7 @@ from geoapi.services.users import UserService from geoapi.services.projects import ProjectsService from geoapi.exceptions import ApiException -from geoapi.models.project import ProjectUser, Project -from geoapi.models.users import User +from geoapi.models.project import ProjectUser from geoapi.db import db_session import pytest @@ -54,8 +53,8 @@ def test_add_existing_user_to_project(user1, user2, projects_fixture): ProjectsService.addUserToProject(projectId=projects_fixture.id, username=user2.username, admin=False) assert UserService.canAccess(user2, projects_fixture.id) - project_user = db_session.query(ProjectUser).filter(Project.id == projects_fixture.id) \ - .filter(User.id == user2.id).first() + project_user = db_session.query(ProjectUser).filter(ProjectUser.project_id == projects_fixture.id) \ + .filter(ProjectUser.user_id == user2.id).one_or_none() assert project_user.admin is False assert UserService.canAccess(user1, projects_fixture.id) @@ -65,8 +64,8 @@ def test_add_existing_user_to_project_as_admin(user1, user2, projects_fixture): ProjectsService.addUserToProject(projectId=projects_fixture.id, username=user2.username, admin=True) assert UserService.canAccess(user2, projects_fixture.id) - project_user = db_session.query(ProjectUser).filter(Project.id == projects_fixture.id) \ - .filter(User.id == user2.id).first() + project_user = db_session.query(ProjectUser).filter(ProjectUser.project_id == projects_fixture.id) \ + .filter(ProjectUser.user_id == user2.id).one_or_none() assert project_user.admin is True assert UserService.canAccess(user1, projects_fixture.id) diff --git a/geoapi/tests/custom/test_designsafe.py b/geoapi/tests/custom/test_designsafe.py index 3bbcc92a..923567b9 100644 --- a/geoapi/tests/custom/test_designsafe.py +++ b/geoapi/tests/custom/test_designsafe.py @@ -18,3 +18,7 @@ def test_get_system_users(requests_mock, project_response): users = get_system_users(tenant_id="DESIGNSAFE", jwt="dummy", system_id=f"project-{uuid}") users_as_list_of_dict = [{u.username: u.admin} for u in users] assert users_as_list_of_dict == [{'user_pi': True}, {'user_copi': True}, {'user3': False}, {'user4': False}] + + users = get_system_users(tenant_id="designsafe", jwt="dummy", system_id=f"project-{uuid}") + users_as_list_of_dict = [{u.username: u.admin} for u in users] + assert users_as_list_of_dict == [{'user_pi': True}, {'user_copi': True}, {'user3': False}, {'user4': False}] diff --git a/geoapi/utils/agave.py b/geoapi/utils/agave.py index 2bafd12d..d45435a3 100644 --- a/geoapi/utils/agave.py +++ b/geoapi/utils/agave.py @@ -308,9 +308,8 @@ def get_system_users(tenant_id, jwt, system_id: str) -> List[SystemUser]: :return: list of users with admin status """ - if tenant_id in custom_system_user_retrieval: - return custom_system_user_retrieval[tenant_id](tenant_id, jwt, system_id) - + if tenant_id.upper() in custom_system_user_retrieval: + return custom_system_user_retrieval[tenant_id.upper()](tenant_id, jwt, system_id) return get_default_system_users(tenant_id, jwt, system_id)