-
Notifications
You must be signed in to change notification settings - Fork 4
archive and unarchive and delete api integration #39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -643,3 +643,64 @@ def import_users(self, from_project: "LabellerrProject"): | |
| "POST", url, extra_headers={"Content-Type": "application/json"} | ||
| ) | ||
| return response.get("response") | ||
|
|
||
| def archive(self, unarchive=False): | ||
| """ | ||
| Archives or unarchives the project. | ||
|
|
||
| :param unarchive: If True, unarchives the project. Default is False (archive). | ||
| :return: Response from the server. | ||
| :raises LabellerrError: If the operation fails. | ||
| """ | ||
| unique_id = str(uuid.uuid4()) | ||
| action = "unarchive" if unarchive else "archive" | ||
| url = f"{constants.BASE_URL}/projects/{action}?client_id={self.client.client_id}&uuid={unique_id}" | ||
|
|
||
| payload = json.dumps({"project_id": self.project_id}) | ||
|
|
||
| headers = {"content-type": "application/json"} | ||
| if self.client.api_key: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need for this check, it will always be there |
||
| headers["email_id"] = self.client.api_key | ||
|
|
||
| return self.client.make_request( | ||
| "POST", | ||
| url, | ||
| extra_headers=headers, | ||
| request_id=unique_id, | ||
| data=payload, | ||
| ) | ||
|
Comment on lines
+665
to
+671
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent response handling: This method returns the raw response, but other similar methods like
|
||
|
|
||
|
|
||
|
Comment on lines
+672
to
+673
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Code style: Extra blank line here is inconsistent with other methods in this file. Should be removed for consistency. |
||
| def unarchive(self): | ||
| """ | ||
| Unarchives the project. | ||
| Alias for archive(unarchive=True). | ||
|
|
||
| :return: Response from the server. | ||
| :raises LabellerrError: If the operation fails. | ||
| """ | ||
| return self.archive(unarchive=True) | ||
|
|
||
|
|
||
| def delete(self): | ||
|
Comment on lines
+684
to
+685
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Code style: Extra blank line should be removed for consistency with other methods. |
||
| """ | ||
| Deletes the project. | ||
|
|
||
| :return: Response from the server. | ||
| :raises LabellerrError: If the operation fails. | ||
| """ | ||
| unique_id = str(uuid.uuid4()) | ||
|
|
||
| url = f"{constants.BASE_URL}/projects/project/{self.project_id}?client_id={self.client.client_id}&uuid={unique_id}" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think the url is incorrect |
||
|
|
||
|
Comment on lines
+694
to
+695
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Code style: Extra blank line should be removed for consistency. |
||
|
|
||
| headers = {} | ||
| if self.client.api_key: | ||
| headers["email_id"] = self.client.api_key | ||
|
|
||
| return self.client.make_request( | ||
| "DELETE", | ||
| url, | ||
| extra_headers=headers, | ||
| request_id=unique_id, | ||
| ) | ||
|
Comment on lines
+701
to
+706
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing validation: Consider validating:
Also, response handling: Same issue as
Comment on lines
+700
to
+706
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since we don't allow delete http method, lets use POST |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,15 +3,15 @@ | |
| """ | ||
|
|
||
| import os | ||
| from enum import StrEnum | ||
| from enum import Enum | ||
| from typing import List, Literal | ||
| from uuid import UUID | ||
|
|
||
| from .base import DatasetDataType | ||
| from pydantic import BaseModel, Field, field_validator | ||
|
|
||
|
|
||
| class DataSetScope(StrEnum): | ||
| class DataSetScope(str, Enum): | ||
|
Comment on lines
+6
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good fix: Changing from |
||
| project = "project" | ||
| client = "client" | ||
| public = "public" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,3 +48,31 @@ def test_create_project(create_project_fixture): | |
|
|
||
| assert project.project_id is not None | ||
| assert isinstance(project.project_id, str) | ||
|
|
||
|
|
||
| def test_archive_and_delete_project(create_project_fixture): | ||
|
|
||
| project = create_project_fixture | ||
| assert project.project_id is not None | ||
|
|
||
| # 1. Archive | ||
| print(f"Archiving project {project.project_id}...") | ||
| try: | ||
| project.archive() | ||
| except Exception as e: | ||
| pytest.fail(f"Failed to archive project: {e}") | ||
|
Comment on lines
+58
to
+63
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Weak assertions: The test should verify:
Current implementation only checks that no exception was raised, which is insufficient for integration testing. |
||
|
|
||
| # 2. Unarchive (test the alias) | ||
| print(f"Unarchiving project {project.project_id}...") | ||
| try: | ||
| project.unarchive() | ||
| except Exception as e: | ||
| pytest.fail(f"Failed to unarchive project: {e}") | ||
|
|
||
| # 3. Archive again (to test delete on archived project if valid, or just normal delete) | ||
| # Let's delete it while active | ||
| print(f"Deleting project {project.project_id}...") | ||
| try: | ||
| project.delete() | ||
| except Exception as e: | ||
| pytest.fail(f"Failed to delete project: {e}") | ||
|
Comment on lines
+74
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test completeness: After deleting, the test should:
This ensures the delete actually worked and wasn't just a no-op. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| import pytest | ||
| from unittest.mock import patch, Mock | ||
| import json | ||
| from labellerr.core.projects.image_project import ImageProject | ||
| from labellerr.core.users.base import LabellerrUsers | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unused import: |
||
|
|
||
| @pytest.fixture | ||
| def client(): | ||
| """Create a mock client""" | ||
| from labellerr.client import LabellerrClient | ||
| client = Mock(spec=LabellerrClient) | ||
| client.client_id = "test-client-id" | ||
| client.api_key = "test-api-key" | ||
| client.api_secret = "test-api-secret" | ||
| return client | ||
|
|
||
| @pytest.fixture | ||
| def project(client): | ||
| """Create a test project instance""" | ||
| project_data = { | ||
| "project_id": "test_project_id", | ||
| "data_type": "image", | ||
| "attached_datasets": [], | ||
| } | ||
| # Use __new__ to avoid initialization logic if needed, or just mock it | ||
| proj = ImageProject.__new__(ImageProject) | ||
| proj.client = client | ||
| proj._LabellerrProject__project_id_input = "test_project_id" | ||
| proj._LabellerrProject__project_data = project_data | ||
| return proj | ||
|
|
||
| @pytest.mark.unit | ||
| class TestProjectLifecycle: | ||
| """Tests for project lifecycle methods: archive, unarchive, delete""" | ||
|
|
||
| def test_archive_project(self, project, client): | ||
| """Test archiving a project""" | ||
| mock_response = {"status": "success", "msg": "Project archived"} | ||
|
|
||
| with patch.object(client, "make_request", return_value=mock_response) as mock_req: | ||
| response = project.archive() | ||
|
|
||
| assert response == mock_response | ||
|
|
||
| # Verify the request | ||
| mock_req.assert_called_once() | ||
| args, kwargs = mock_req.call_args | ||
| assert args[0] == "POST" | ||
| assert "/projects/archive" in args[1] | ||
| assert kwargs["data"] is not None | ||
| assert '"project_id": "test_project_id"' in kwargs["data"] | ||
|
Comment on lines
+36
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test coverage gap: The test only verifies the request was made, but should also verify:
Consider adding these assertions: assert kwargs.get(\"request_id\") is not None
assert f\"uuid={kwargs['request_id']}\" in args[1] |
||
|
|
||
| def test_unarchive_project(self, project, client): | ||
| """Test unarchiving a project""" | ||
| mock_response = {"status": "success", "msg": "Project unarchived"} | ||
|
|
||
| with patch.object(client, "make_request", return_value=mock_response) as mock_req: | ||
| # Test direct call to archive(unarchive=True) | ||
| response = project.archive(unarchive=True) | ||
| assert response == mock_response | ||
|
|
||
| args, kwargs = mock_req.call_args | ||
| assert "/projects/unarchive" in args[1] | ||
|
|
||
| # Test alias unarchive() | ||
| mock_req.reset_mock() | ||
| response = project.unarchive() | ||
| assert response == mock_response | ||
|
|
||
| args, kwargs = mock_req.call_args | ||
| assert "/projects/unarchive" in args[1] | ||
|
|
||
| def test_delete_project(self, project, client): | ||
| """Test deleting a project""" | ||
| mock_response = {"status": "success", "msg": "Project deleted"} | ||
|
|
||
| with patch.object(client, "make_request", return_value=mock_response) as mock_req: | ||
| response = project.delete() | ||
|
|
||
| assert response == mock_response | ||
|
|
||
| # Verify the request | ||
| mock_req.assert_called_once() | ||
| args, kwargs = mock_req.call_args | ||
| assert args[0] == "DELETE" | ||
| assert "/projects/project/test_project_id" in args[1] | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent header casing: The codebase predominantly uses lowercase
content-type(see lines 42, 128, 211, 263, 576, 613). Recommend changing to lowercase for consistency: