archive and unarchive and delete api integration by QeyCoder · Pull Request #39 · Labellerr/SDKPython

QeyCoder · 2025-12-15T16:50:06Z

No description provided.

github-actions · 2025-12-15T16:51:42Z

Code Review Summary

I've reviewed PR #39 for the archive, unarchive, and delete API integration. Overall, the implementation is functional but there are several issues that should be addressed:

✅ Positives

Good test coverage with both unit and integration tests
Methods follow existing patterns in the codebase
The StrEnum to Enum change is correct for Python 3.7+ compatibility

⚠️ Issues Found

1. Inconsistent Header Casing

The codebase uses lowercase content-type in most places (lines 42, 128, 211, 263, 285, 576, 613, 661), but the new methods use lowercase in one place and uppercase Content-Type exists in some legacy code. Should standardize to lowercase for consistency.

2. Missing request_id Parameter

The import_users method (line 642-643) doesn't pass request_id to make_request, but the new methods do. Should verify if this is intentional or if import_users needs updating.

3. Inconsistent Response Handling

import_users: returns response.get("response") (extracts nested data)
New methods: return full response object directly
Should clarify the expected return format and document it.

4. Weak Integration Test Assertions

The integration test wraps calls in try-except but only checks for exceptions. It should:

Verify response structure/status codes
Check actual state changes (e.g., project is actually archived)
Validate error responses

5. Unused Import in Unit Tests

LabellerrUsers is imported but never used (line 5 of test_projects.py)

6. Test Fixture Issue

Integration test fixture has parameter client but creates a new LabellerrClient inside (line 19-22), making the parameter pointless.

7. Code Style Issues

Extra blank lines after method definitions (lines 672, 684, 695)
Inconsistent with the rest of the file

8. Missing Return Type Hints

Other methods in the codebase lack type hints, but for new code, consider adding them for better IDE support.

9. No Validation of Project State

The delete method doesn't check if project exists or is in a valid state for deletion. Consider adding state validation.

See inline comments for specific locations and suggested fixes.

github-actions · 2025-12-15T16:52:12Z

+
+        payload = json.dumps({"project_id": self.project_id})
+
+        headers = {"content-type": "application/json"}


Inconsistent header casing: The codebase predominantly uses lowercase content-type (see lines 42, 128, 211, 263, 576, 613). Recommend changing to lowercase for consistency:

Suggested change

headers = {"content-type": "application/json"}

headers = {"content-type": "application/json"}

github-actions · 2025-12-15T16:52:13Z

+
+

Code style: Extra blank line here is inconsistent with other methods in this file. Should be removed for consistency.

github-actions · 2025-12-15T16:52:14Z

+
+    def delete(self):


Code style: Extra blank line should be removed for consistency with other methods.

github-actions · 2025-12-15T16:52:15Z

+        url = f"{constants.BASE_URL}/projects/project/{self.project_id}?client_id={self.client.client_id}&uuid={unique_id}"
+


Code style: Extra blank line should be removed for consistency.

github-actions · 2025-12-15T16:52:16Z

+        return self.client.make_request(
+            "POST",
+            url,
+            extra_headers=headers,
+            request_id=unique_id,
+            data=payload,
+        )


Inconsistent response handling: This method returns the raw response, but other similar methods like import_users (line 645) return response.get(\"response\"). Should clarify and document the expected return format. Consider either:

Extracting nested response for consistency, or

Documenting why this returns full response object

github-actions · 2025-12-15T16:52:17Z

+        return self.client.make_request(
+            "DELETE",
+            url,
+            extra_headers=headers,
+            request_id=unique_id,
+        )


Missing validation: Consider validating:

Project exists before attempting deletion

User has permission to delete

Project isn't already deleted

Also, response handling: Same issue as archive() - should document if full response or nested data should be returned.

github-actions · 2025-12-15T16:52:19Z

+from unittest.mock import patch, Mock
+import json
+from labellerr.core.projects.image_project import ImageProject
+from labellerr.core.users.base import LabellerrUsers


Unused import: LabellerrUsers is imported but never used in this file. Should be removed.

github-actions · 2025-12-15T16:52:20Z

+    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"]


Test coverage gap: The test only verifies the request was made, but should also verify:

The request includes the correct uuid in the URL

The request_id parameter matches the uuid

The payload is properly formatted JSON

Consider adding these assertions:

assert kwargs.get(\"request_id\") is not None assert f\"uuid={kwargs['request_id']}\" in args[1]

github-actions · 2025-12-15T16:52:21Z

+    # 1. Archive
+    print(f"Archiving project {project.project_id}...")
+    try:
+        project.archive()
+    except Exception as e:
+        pytest.fail(f"Failed to archive project: {e}")


Weak assertions: The test should verify:

Response status/success indicator

Response structure matches expected format

Consider checking project state via a GET request to confirm it's actually archived

Current implementation only checks that no exception was raised, which is insufficient for integration testing.

github-actions · 2025-12-15T16:52:22Z

+    print(f"Deleting project {project.project_id}...")
+    try:
+        project.delete()
+    except Exception as e:
+        pytest.fail(f"Failed to delete project: {e}")


Test completeness: After deleting, the test should:

Verify the delete response indicates success

Optionally attempt to fetch the project and verify it returns a 404 or "not found" error

Check that subsequent operations on the deleted project fail appropriately

This ensures the delete actually worked and wasn't just a no-op.

github-actions · 2025-12-15T16:52:24Z

+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):


Good fix: Changing from StrEnum (Python 3.11+) to str, Enum is correct for maintaining Python 3.7+ compatibility as specified in pyproject.toml. The behavior is equivalent and this ensures backward compatibility. ✅

ximihoque · 2025-12-30T06:06:00Z

+        payload = json.dumps({"project_id": self.project_id})
+
+        headers = {"content-type": "application/json"}
+        if self.client.api_key:


no need for this check, it will always be there

ximihoque · 2025-12-30T06:06:28Z

+        """
+        unique_id = str(uuid.uuid4())
+
+        url = f"{constants.BASE_URL}/projects/project/{self.project_id}?client_id={self.client.client_id}&uuid={unique_id}"


i think the url is incorrect

ximihoque · 2025-12-30T06:14:10Z

+
+        return self.client.make_request(
+            "DELETE",
+            url,
+            extra_headers=headers,
+            request_id=unique_id,
+        )


since we don't allow delete http method, lets use POST

archive and unarchive and delete api integration

4467433

github-actions Bot reviewed Dec 15, 2025

View reviewed changes

QeyCoder marked this pull request as ready for review December 22, 2025 17:06

ximihoque suggested changes Dec 30, 2025

View reviewed changes


		payload = json.dumps({"project_id": self.project_id})

		headers = {"content-type": "application/json"}

		url = f"{constants.BASE_URL}/projects/project/{self.project_id}?client_id={self.client.client_id}&uuid={unique_id}"

Uh oh!

Conversation

QeyCoder commented Dec 15, 2025

Uh oh!

github-actions Bot commented Dec 15, 2025

Code Review Summary

✅ Positives

⚠️ Issues Found

1. Inconsistent Header Casing

2. Missing request_id Parameter

3. Inconsistent Response Handling

4. Weak Integration Test Assertions

5. Unused Import in Unit Tests

6. Test Fixture Issue

7. Code Style Issues

8. Missing Return Type Hints

9. No Validation of Project State

Uh oh!

github-actions Bot Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

ximihoque Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

ximihoque Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

ximihoque Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants