From b6400f5d5d23c799c671287bf312bde822396847 Mon Sep 17 00:00:00 2001 From: Amanda Zhu Date: Tue, 17 Jun 2025 14:50:55 +1000 Subject: [PATCH 1/3] add inital_request_time to service and resource --- routers/bpa_register.py | 2 ++ schemas/service.py | 2 ++ tests/test_bpa_register.py | 57 +++----------------------------------- tests/test_models.py | 55 +++++++++++++++++++++++------------- 4 files changed, 43 insertions(+), 73 deletions(-) diff --git a/routers/bpa_register.py b/routers/bpa_register.py index fc3b20b0..34c049bd 100644 --- a/routers/bpa_register.py +++ b/routers/bpa_register.py @@ -46,6 +46,7 @@ async def register_bpa_user( name=settings.organizations[org_id], status="pending", last_updated=now, + initial_request_time=now, updated_by="system", ).model_dump(mode="json") bpa_resources.append(resource) @@ -56,6 +57,7 @@ async def register_bpa_user( id="bpa", status="pending", last_updated=now, + initial_request_time=now, updated_by="system", resources=bpa_resources, ) diff --git a/schemas/service.py b/schemas/service.py index 2e286398..7ff7b9df 100644 --- a/schemas/service.py +++ b/schemas/service.py @@ -9,6 +9,7 @@ class Resource(BaseModel): status: Literal["approved", "revoked", "pending"] id: str last_updated: Optional[datetime] = None + initial_request_time: Optional[datetime] = None updated_by: Optional[str] = None def approve(self): @@ -23,6 +24,7 @@ class Service(BaseModel): id: str status: Literal["approved", "revoked", "pending"] last_updated: datetime + initial_request_time: Optional[datetime] = None updated_by: str resources: List[Resource] = Field(default_factory=list) diff --git a/tests/test_bpa_register.py b/tests/test_bpa_register.py index a53c47e0..5445f3a3 100644 --- a/tests/test_bpa_register.py +++ b/tests/test_bpa_register.py @@ -47,7 +47,6 @@ def test_to_biocommons_register_data(valid_registration_data): ) assert register_data.username == bpa_data.username assert register_data.name == bpa_data.fullname - # Test we fill the registration_from field in app_metadata assert register_data.app_metadata.registration_from == "bpa" @@ -84,6 +83,7 @@ def test_successful_registration( for resource in bpa_service["resources"]: assert "last_updated" in resource assert "updated_by" in resource + assert "initial_request_time" in resource assert resource["updated_by"] == "system" assert ( @@ -106,17 +106,18 @@ def test_service_and_resources_have_updated_by_system(): "status": "pending", "last_updated": datetime.now(UTC), "updated_by": "system", + "initial_request_time": datetime.now(UTC), } ], ) assert service.updated_by == "system" assert service.resources[0].updated_by == "system" - + assert hasattr(service.resources[0], "initial_request_time") + assert isinstance(service.resources[0].initial_request_time, datetime) def test_registration_duplicate_user( test_client, mock_auth_token, mocker, valid_registration_data ): - """Test registration with duplicate user""" mock_response = MagicMock() mock_response.status_code = 409 mock_response.json.return_value = {"message": "User already exists"} @@ -132,7 +133,6 @@ def test_registration_duplicate_user( def test_registration_auth0_error( test_client, mock_auth_token, mocker, valid_registration_data ): - """Test registration with Auth0 API error""" mock_response = MagicMock() mock_response.status_code = 400 mock_response.json.return_value = {"message": "Invalid request"} @@ -148,7 +148,6 @@ def test_registration_auth0_error( def test_registration_with_invalid_organization( test_client, mock_auth_token, mocker, valid_registration_data ): - """Test registration with invalid organization ID""" data = valid_registration_data.copy() data["organizations"] = {"invalid-org-id": True} @@ -159,7 +158,6 @@ def test_registration_with_invalid_organization( def test_registration_request_validation(test_client): - """Test request validation""" invalid_data = { "username": "testuser", "email": "invalid-email", @@ -174,7 +172,6 @@ def test_registration_request_validation(test_client): def test_no_selected_organizations( test_client, mock_auth_token, mocker, valid_registration_data ): - """Test registration with no organizations selected""" data = valid_registration_data.copy() data["organizations"] = { "bpa-bioinformatics-workshop": False, @@ -199,55 +196,9 @@ def test_no_selected_organizations( def test_empty_organizations_dict( test_client, mock_auth_token, mocker, valid_registration_data ): - """Test registration with empty organizations dictionary""" data = valid_registration_data.copy() data["organizations"] = {} mock_response = MagicMock() mock_response.status_code = 201 mock_response.json.return_value = {"user_id": "auth0|123"} - - mock_post = mocker.patch("httpx.AsyncClient.post", return_value=mock_response) - - response = test_client.post("/bpa/register", json=data) - - assert response.status_code == 200 - called_data = mock_post.call_args[1]["json"] - bpa_service = called_data["app_metadata"]["services"][0] - assert len(bpa_service["resources"]) == 0 - - -def test_registration_email_format(test_client, valid_registration_data): - """Test email format validation""" - data = valid_registration_data.copy() - data["email"] = "invalid-email" - - response = test_client.post("/bpa/register", json=data) - - assert response.status_code == 422 - assert "email" in response.json()["detail"][0]["loc"] - - -def test_all_organizations_selected( - test_client, - mock_auth_token, - mock_settings, - mocker, - valid_registration_data, -): - """Test registration with all organizations selected""" - data = valid_registration_data.copy() - data["organizations"] = {k: True for k in mock_settings.organizations.keys()} - - mock_response = MagicMock() - mock_response.status_code = 201 - mock_response.json.return_value = {"user_id": "auth0|123"} - - mock_post = mocker.patch("httpx.AsyncClient.post", return_value=mock_response) - - response = test_client.post("/bpa/register", json=data) - - assert response.status_code == 200 - called_data = mock_post.call_args[1]["json"] - bpa_service = called_data["app_metadata"]["services"][0] - assert len(bpa_service["resources"]) == len(mock_settings.organizations) diff --git a/tests/test_models.py b/tests/test_models.py index 491aa33d..c716f465 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -19,9 +19,6 @@ def frozen_time(): def test_approve_service(frozen_time): - """ - Test we can approve a service and set metadata correctly. - """ service = Service(name="Test Service", id="service1", status="pending", last_updated=FROZEN_TIME - timedelta(hours=1), updated_by="") service.approve(updated_by="admin@example.com") @@ -31,9 +28,6 @@ def test_approve_service(frozen_time): def test_approve_service_from_app_metadata(frozen_time): - """ - Test we can approve a service by ID from BiocommonsAppMetadata. - """ service = Service(name="Test Service", id="service1", status="pending", last_updated=FROZEN_TIME - timedelta(hours=1), updated_by="") other = Service(name="Other Service", id="service2", status="pending", @@ -47,9 +41,6 @@ def test_approve_service_from_app_metadata(frozen_time): def test_revoke_service(frozen_time): - """ - Test we can revoke a service and set metadata correctly. - """ service = Service(name="Test Service", id="service1", status="approved", last_updated=FROZEN_TIME - timedelta(hours=1), updated_by="") service.revoke(updated_by="admin@example.com") @@ -59,9 +50,6 @@ def test_revoke_service(frozen_time): def test_revoke_service_from_app_metadata(frozen_time): - """ - Test we can revoke a service by ID from BiocommonsAppMetadata. - """ service = Service(name="Test Service", id="service1", status="approved", last_updated=FROZEN_TIME - timedelta(hours=1), updated_by="") other = Service(name="Other Service", id="service2", status="approved", @@ -73,39 +61,66 @@ def test_revoke_service_from_app_metadata(frozen_time): assert service.last_updated == FROZEN_TIME assert other.status == "approved" -def test_approve_resource(): - resource = Resource(name="Test Resource", id="resource1", status="pending") + +def test_approve_resource(frozen_time): + resource = Resource( + name="Test Resource", + id="resource1", + status="pending", + initial_request_time=FROZEN_TIME + ) resource.approve() assert resource.status == "approved" + assert resource.initial_request_time == FROZEN_TIME -def test_approve_resource_from_service(): - resource = Resource(name="Test Resource", id="resource1", status="pending") +def test_approve_resource_from_service(frozen_time): + resource = Resource( + name="Test Resource", + id="resource1", + status="pending", + initial_request_time=FROZEN_TIME + ) service = Service(name="Test Service", id="service1", status="approved", last_updated=FROZEN_TIME - timedelta(hours=1), updated_by="", resources=[resource]) service.approve_resource(resource_id="resource1") assert resource.status == "approved" + assert resource.initial_request_time == FROZEN_TIME -def test_approve_resource_from_pending_service(): +def test_approve_resource_from_pending_service(frozen_time): """ Test that trying to approve a resource from a pending service raises an error. """ - resource = Resource(name="Test Resource", id="resource1", status="pending") + resource = Resource( + name="Test Resource", + id="resource1", + status="pending", + initial_request_time=FROZEN_TIME + ) + service = Service(name="Test Service", id="service1", status="pending", last_updated=FROZEN_TIME - timedelta(hours=1), updated_by="", resources=[resource]) with pytest.raises(PermissionError, match="Service must be approved before approving a resource."): service.approve_resource(resource_id="resource1") assert resource.status == "pending" + assert resource.initial_request_time == FROZEN_TIME -def test_approve_resource_from_app_metadata(): - resource = Resource(name="Test Resource", id="resource1", status="pending") +def test_approve_resource_from_app_metadata(frozen_time): + resource = Resource( + name="Test Resource", + id="resource1", + status="pending", + initial_request_time=FROZEN_TIME + ) service = Service(name="Test Service", id="service1", status="approved", last_updated=FROZEN_TIME - timedelta(hours=1), updated_by="", resources=[resource]) app_metadata = AppMetadataFactory.build(services=[service]) app_metadata.approve_resource(service_id="service1", resource_id="resource1", updated_by="admin@example.com") + assert resource.status == "approved" + assert resource.initial_request_time == FROZEN_TIME From e8e387a88ca405c393e7d848f7c962313b0da904 Mon Sep 17 00:00:00 2001 From: Amanda Zhu Date: Tue, 17 Jun 2025 15:03:21 +1000 Subject: [PATCH 2/3] restore original unit tests --- tests/test_bpa_register.py | 51 ++++++++++++++++++++++++++++++++++++++ tests/test_models.py | 9 ++++--- 2 files changed, 57 insertions(+), 3 deletions(-) diff --git a/tests/test_bpa_register.py b/tests/test_bpa_register.py index 5445f3a3..be2d9535 100644 --- a/tests/test_bpa_register.py +++ b/tests/test_bpa_register.py @@ -118,6 +118,7 @@ def test_service_and_resources_have_updated_by_system(): def test_registration_duplicate_user( test_client, mock_auth_token, mocker, valid_registration_data ): + """Test registration with duplicate user""" mock_response = MagicMock() mock_response.status_code = 409 mock_response.json.return_value = {"message": "User already exists"} @@ -133,6 +134,7 @@ def test_registration_duplicate_user( def test_registration_auth0_error( test_client, mock_auth_token, mocker, valid_registration_data ): + """Test registration with Auth0 API error""" mock_response = MagicMock() mock_response.status_code = 400 mock_response.json.return_value = {"message": "Invalid request"} @@ -148,6 +150,7 @@ def test_registration_auth0_error( def test_registration_with_invalid_organization( test_client, mock_auth_token, mocker, valid_registration_data ): + """Test registration with invalid organization ID""" data = valid_registration_data.copy() data["organizations"] = {"invalid-org-id": True} @@ -158,6 +161,7 @@ def test_registration_with_invalid_organization( def test_registration_request_validation(test_client): + """Test request validation""" invalid_data = { "username": "testuser", "email": "invalid-email", @@ -172,6 +176,7 @@ def test_registration_request_validation(test_client): def test_no_selected_organizations( test_client, mock_auth_token, mocker, valid_registration_data ): + """Test registration with no organizations selected""" data = valid_registration_data.copy() data["organizations"] = { "bpa-bioinformatics-workshop": False, @@ -196,9 +201,55 @@ def test_no_selected_organizations( def test_empty_organizations_dict( test_client, mock_auth_token, mocker, valid_registration_data ): + """Test registration with empty organizations dictionary""" data = valid_registration_data.copy() data["organizations"] = {} mock_response = MagicMock() mock_response.status_code = 201 mock_response.json.return_value = {"user_id": "auth0|123"} + + mock_post = mocker.patch("httpx.AsyncClient.post", return_value=mock_response) + + response = test_client.post("/bpa/register", json=data) + + assert response.status_code == 200 + called_data = mock_post.call_args[1]["json"] + bpa_service = called_data["app_metadata"]["services"][0] + assert len(bpa_service["resources"]) == 0 + + +def test_registration_email_format(test_client, valid_registration_data): + """Test email format validation""" + data = valid_registration_data.copy() + data["email"] = "invalid-email" + + response = test_client.post("/bpa/register", json=data) + + assert response.status_code == 422 + assert "email" in response.json()["detail"][0]["loc"] + + +def test_all_organizations_selected( + test_client, + mock_auth_token, + mock_settings, + mocker, + valid_registration_data, +): + """Test registration with all organizations selected""" + data = valid_registration_data.copy() + data["organizations"] = {k: True for k in mock_settings.organizations.keys()} + + mock_response = MagicMock() + mock_response.status_code = 201 + mock_response.json.return_value = {"user_id": "auth0|123"} + + mock_post = mocker.patch("httpx.AsyncClient.post", return_value=mock_response) + + response = test_client.post("/bpa/register", json=data) + + assert response.status_code == 200 + called_data = mock_post.call_args[1]["json"] + bpa_service = called_data["app_metadata"]["services"][0] + assert len(bpa_service["resources"]) == len(mock_settings.organizations) diff --git a/tests/test_models.py b/tests/test_models.py index c716f465..ae290273 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -19,6 +19,7 @@ def frozen_time(): def test_approve_service(frozen_time): + """Test we can approve a service and set metadata correctly.""" service = Service(name="Test Service", id="service1", status="pending", last_updated=FROZEN_TIME - timedelta(hours=1), updated_by="") service.approve(updated_by="admin@example.com") @@ -28,6 +29,7 @@ def test_approve_service(frozen_time): def test_approve_service_from_app_metadata(frozen_time): + """Test we can approve a service by ID from BiocommonsAppMetadata.""" service = Service(name="Test Service", id="service1", status="pending", last_updated=FROZEN_TIME - timedelta(hours=1), updated_by="") other = Service(name="Other Service", id="service2", status="pending", @@ -41,6 +43,7 @@ def test_approve_service_from_app_metadata(frozen_time): def test_revoke_service(frozen_time): + """Test we can revoke a service and set metadata correctly.""" service = Service(name="Test Service", id="service1", status="approved", last_updated=FROZEN_TIME - timedelta(hours=1), updated_by="") service.revoke(updated_by="admin@example.com") @@ -50,6 +53,7 @@ def test_revoke_service(frozen_time): def test_revoke_service_from_app_metadata(frozen_time): + """Test we can revoke a service by ID from BiocommonsAppMetadata.""" service = Service(name="Test Service", id="service1", status="approved", last_updated=FROZEN_TIME - timedelta(hours=1), updated_by="") other = Service(name="Other Service", id="service2", status="approved", @@ -75,6 +79,7 @@ def test_approve_resource(frozen_time): def test_approve_resource_from_service(frozen_time): + """Test that trying to approve a resource from a pending service raises an error.""" resource = Resource( name="Test Resource", id="resource1", @@ -90,9 +95,7 @@ def test_approve_resource_from_service(frozen_time): def test_approve_resource_from_pending_service(frozen_time): - """ - Test that trying to approve a resource from a pending service raises an error. - """ + """Test that trying to approve a resource from a pending service raises an error.""" resource = Resource( name="Test Resource", id="resource1", From 01175fd8adc6339adbc52d4d6ed526a4fc6a01e0 Mon Sep 17 00:00:00 2001 From: Amanda Zhu Date: Tue, 17 Jun 2025 15:39:05 +1000 Subject: [PATCH 3/3] add initial_request_time to galaxy registration, add missing places for initial request time --- routers/bpa_register.py | 2 +- routers/user.py | 1 + schemas/biocommons.py | 1 + schemas/service.py | 2 +- 4 files changed, 4 insertions(+), 2 deletions(-) diff --git a/routers/bpa_register.py b/routers/bpa_register.py index 34c049bd..b62d081a 100644 --- a/routers/bpa_register.py +++ b/routers/bpa_register.py @@ -55,9 +55,9 @@ async def register_bpa_user( bpa_service = Service( name="Bioplatforms Australia Data Portal", id="bpa", + initial_request_time=now, status="pending", last_updated=now, - initial_request_time=now, updated_by="system", resources=bpa_resources, ) diff --git a/routers/user.py b/routers/user.py index 15c7f03a..c21281fc 100644 --- a/routers/user.py +++ b/routers/user.py @@ -176,6 +176,7 @@ async def request_service( new_service = Service( name=service_request.name, id=service_request.id, + initial_request_time=datetime.now(timezone.utc), status="pending", last_updated=datetime.now(timezone.utc), updated_by=user.access_token.sub, diff --git a/schemas/biocommons.py b/schemas/biocommons.py index 70c430b7..e384f03d 100644 --- a/schemas/biocommons.py +++ b/schemas/biocommons.py @@ -139,6 +139,7 @@ def from_galaxy_registration( galaxy_service = Service( name="Galaxy Australia", id="galaxy", + initial_request_time=datetime.now(), status="approved", last_updated=datetime.now(), updated_by="" diff --git a/schemas/service.py b/schemas/service.py index 7ff7b9df..4d24c97e 100644 --- a/schemas/service.py +++ b/schemas/service.py @@ -22,9 +22,9 @@ def revoke(self): class Service(BaseModel): name: str id: str + initial_request_time: Optional[datetime] = None status: Literal["approved", "revoked", "pending"] last_updated: datetime - initial_request_time: Optional[datetime] = None updated_by: str resources: List[Resource] = Field(default_factory=list)