fix(backend): fix pagination bug in review lookup and implement per-review auto-approval

- Add get_pending_review_by_node_exec_id() for direct review lookup
- Replace paginated search with direct lookup to avoid missing reviews beyond page 1
- Implement per-review auto_approve_future toggle for granular control
- Fix log deduplication for embedding generation warnings
- Remove unnecessary f-string prefixes per linter feedback
- Fix all test mocks to use correct functions (get_pending_reviews_for_user vs get_pending_review_by_node_exec_id)
- All 15 review route tests passing
This commit is contained in:
Zamil Majdy
2026-01-22 21:24:15 -05:00
parent e6ca904326
commit 9be3ec58ae
3 changed files with 75 additions and 40 deletions

View File

@@ -160,11 +160,11 @@ def test_process_review_action_approve_success(
"""Test successful review approval"""
# Mock the route functions
# Mock get_pending_reviews_for_user (called to find the graph_exec_id)
# Mock get_pending_review_by_node_exec_id (called to find the graph_exec_id)
mock_get_reviews_for_user = mocker.patch(
"backend.api.features.executions.review.routes.get_pending_reviews_for_user"
"backend.api.features.executions.review.routes.get_pending_review_by_node_exec_id"
)
mock_get_reviews_for_user.return_value = [sample_pending_review]
mock_get_reviews_for_user.return_value = sample_pending_review
mock_get_reviews_for_execution = mocker.patch(
"backend.api.features.executions.review.routes.get_pending_reviews_for_execution"
@@ -239,11 +239,11 @@ def test_process_review_action_reject_success(
"""Test successful review rejection"""
# Mock the route functions
# Mock get_pending_reviews_for_user (called to find the graph_exec_id)
# Mock get_pending_review_by_node_exec_id (called to find the graph_exec_id)
mock_get_reviews_for_user = mocker.patch(
"backend.api.features.executions.review.routes.get_pending_reviews_for_user"
"backend.api.features.executions.review.routes.get_pending_review_by_node_exec_id"
)
mock_get_reviews_for_user.return_value = [sample_pending_review]
mock_get_reviews_for_user.return_value = sample_pending_review
# Mock get_graph_execution_meta to return execution in REVIEW status
mock_get_graph_exec = mocker.patch(
@@ -333,11 +333,11 @@ def test_process_review_action_mixed_success(
# Mock the route functions
# Mock get_pending_reviews_for_user (called to find the graph_exec_id)
# Mock get_pending_review_by_node_exec_id (called to find the graph_exec_id)
mock_get_reviews_for_user = mocker.patch(
"backend.api.features.executions.review.routes.get_pending_reviews_for_user"
"backend.api.features.executions.review.routes.get_pending_review_by_node_exec_id"
)
mock_get_reviews_for_user.return_value = [sample_pending_review]
mock_get_reviews_for_user.return_value = sample_pending_review
mock_get_reviews_for_execution = mocker.patch(
"backend.api.features.executions.review.routes.get_pending_reviews_for_execution"
@@ -471,11 +471,11 @@ def test_process_review_action_review_not_found(
reviewed_at=None,
)
# Mock get_pending_reviews_for_user (called to find the graph_exec_id)
# Mock get_pending_review_by_node_exec_id (called to find the graph_exec_id)
mock_get_reviews_for_user = mocker.patch(
"backend.api.features.executions.review.routes.get_pending_reviews_for_user"
"backend.api.features.executions.review.routes.get_pending_review_by_node_exec_id"
)
mock_get_reviews_for_user.return_value = [nonexistent_review]
mock_get_reviews_for_user.return_value = nonexistent_review
# Mock get_graph_execution_meta to return execution in REVIEW status
mock_get_graph_exec = mocker.patch(
@@ -523,11 +523,11 @@ def test_process_review_action_partial_failure(
test_user_id: str,
) -> None:
"""Test handling of partial failures in review processing"""
# Mock get_pending_reviews_for_user (called to find the graph_exec_id)
# Mock get_pending_review_by_node_exec_id (called to find the graph_exec_id)
mock_get_reviews_for_user = mocker.patch(
"backend.api.features.executions.review.routes.get_pending_reviews_for_user"
"backend.api.features.executions.review.routes.get_pending_review_by_node_exec_id"
)
mock_get_reviews_for_user.return_value = [sample_pending_review]
mock_get_reviews_for_user.return_value = sample_pending_review
# Mock get_graph_execution_meta to return execution in REVIEW status
mock_get_graph_exec = mocker.patch(
@@ -591,11 +591,11 @@ def test_process_review_action_invalid_node_exec_id(
reviewed_at=None,
)
# Mock get_pending_reviews_for_user (called to find the graph_exec_id)
# Mock get_pending_review_by_node_exec_id (called to find the graph_exec_id)
mock_get_reviews_for_user = mocker.patch(
"backend.api.features.executions.review.routes.get_pending_reviews_for_user"
"backend.api.features.executions.review.routes.get_pending_review_by_node_exec_id"
)
mock_get_reviews_for_user.return_value = [invalid_review]
mock_get_reviews_for_user.return_value = invalid_review
# Mock get_graph_execution_meta to return execution in REVIEW status
mock_get_graph_exec = mocker.patch(
@@ -643,11 +643,11 @@ def test_process_review_action_auto_approve_creates_auto_approval_records(
test_user_id: str,
) -> None:
"""Test that auto_approve_future_actions flag creates auto-approval records"""
# Mock get_pending_reviews_for_user (called to find the graph_exec_id)
# Mock get_pending_review_by_node_exec_id (called to find the graph_exec_id)
mock_get_reviews_for_user = mocker.patch(
"backend.api.features.executions.review.routes.get_pending_reviews_for_user"
"backend.api.features.executions.review.routes.get_pending_review_by_node_exec_id"
)
mock_get_reviews_for_user.return_value = [sample_pending_review]
mock_get_reviews_for_user.return_value = sample_pending_review
# Mock process_all_reviews
mock_process_all_reviews = mocker.patch(
@@ -763,11 +763,11 @@ def test_process_review_action_without_auto_approve_still_loads_settings(
test_user_id: str,
) -> None:
"""Test that execution context is created with settings even without auto-approve"""
# Mock get_pending_reviews_for_user (called to find the graph_exec_id)
# Mock get_pending_review_by_node_exec_id (called to find the graph_exec_id)
mock_get_reviews_for_user = mocker.patch(
"backend.api.features.executions.review.routes.get_pending_reviews_for_user"
"backend.api.features.executions.review.routes.get_pending_review_by_node_exec_id"
)
mock_get_reviews_for_user.return_value = [sample_pending_review]
mock_get_reviews_for_user.return_value = sample_pending_review
# Mock process_all_reviews
mock_process_all_reviews = mocker.patch(
@@ -902,11 +902,11 @@ def test_process_review_action_auto_approve_only_applies_to_approved_reviews(
reviewed_at=FIXED_NOW,
)
# Mock get_pending_reviews_for_user (called to find the graph_exec_id)
# Mock get_pending_review_by_node_exec_id (called to find the graph_exec_id)
mock_get_reviews_for_user = mocker.patch(
"backend.api.features.executions.review.routes.get_pending_reviews_for_user"
"backend.api.features.executions.review.routes.get_pending_review_by_node_exec_id"
)
mock_get_reviews_for_user.return_value = [approved_review]
mock_get_reviews_for_user.return_value = approved_review
# Mock process_all_reviews
mock_process_all_reviews = mocker.patch(
@@ -1004,12 +1004,14 @@ def test_process_review_action_per_review_auto_approve_granularity(
test_user_id: str,
) -> None:
"""Test that auto-approval can be set per-review (granular control)"""
# Mock get_pending_reviews_for_user - return all 3 reviews
# Mock get_pending_review_by_node_exec_id - return different reviews based on node_exec_id
mock_get_reviews_for_user = mocker.patch(
"backend.api.features.executions.review.routes.get_pending_reviews_for_user"
"backend.api.features.executions.review.routes.get_pending_review_by_node_exec_id"
)
mock_get_reviews_for_user.return_value = [
PendingHumanReviewModel(
# Create a mapping of node_exec_id to review
review_map = {
"node_1_auto": PendingHumanReviewModel(
node_exec_id="node_1_auto",
user_id=test_user_id,
graph_exec_id="test_graph_exec",
@@ -1024,7 +1026,7 @@ def test_process_review_action_per_review_auto_approve_granularity(
processed=False,
created_at=FIXED_NOW,
),
PendingHumanReviewModel(
"node_2_manual": PendingHumanReviewModel(
node_exec_id="node_2_manual",
user_id=test_user_id,
graph_exec_id="test_graph_exec",
@@ -1039,7 +1041,7 @@ def test_process_review_action_per_review_auto_approve_granularity(
processed=False,
created_at=FIXED_NOW,
),
PendingHumanReviewModel(
"node_3_auto": PendingHumanReviewModel(
node_exec_id="node_3_auto",
user_id=test_user_id,
graph_exec_id="test_graph_exec",
@@ -1054,7 +1056,14 @@ def test_process_review_action_per_review_auto_approve_granularity(
processed=False,
created_at=FIXED_NOW,
),
]
}
# Use side_effect to return different reviews based on node_exec_id parameter
def mock_get_review_by_id(node_exec_id: str, _user_id: str):
return review_map.get(node_exec_id)
mock_get_reviews_for_user.side_effect = mock_get_review_by_id
# Mock process_all_reviews - return 3 approved reviews
mock_process_all_reviews = mocker.patch(

View File

@@ -14,6 +14,7 @@ from backend.data.execution import (
from backend.data.graph import get_graph_settings
from backend.data.human_review import (
create_auto_approval_record,
get_pending_review_by_node_exec_id,
get_pending_reviews_for_execution,
get_pending_reviews_for_user,
has_pending_reviews_for_graph_exec,
@@ -136,12 +137,14 @@ async def process_review_action(
detail="At least one review must be provided",
)
# Get graph execution ID from pending reviews to validate execution status
all_pending = await get_pending_reviews_for_user(user_id)
matching_review = next(
(r for r in all_pending if r.node_exec_id in all_request_node_ids),
None,
)
# Get graph execution ID by directly looking up one of the requested reviews
# Use direct lookup to avoid pagination issues (can't miss reviews beyond first page)
matching_review = None
for node_exec_id in all_request_node_ids:
review = await get_pending_review_by_node_exec_id(node_exec_id, user_id)
if review:
matching_review = review
break
if not matching_review:
raise HTTPException(

View File

@@ -220,6 +220,29 @@ async def get_or_create_human_review(
)
async def get_pending_review_by_node_exec_id(
node_exec_id: str, user_id: str
) -> Optional["PendingHumanReviewModel"]:
"""
Get a pending review by its node execution ID.
Args:
node_exec_id: The node execution ID to look up
user_id: User ID for authorization (only returns if review belongs to this user)
Returns:
The pending review if found and belongs to user, None otherwise
"""
review = await PendingHumanReview.prisma().find_unique(
where={"nodeExecId": node_exec_id}
)
if not review or review.userId != user_id or review.status != ReviewStatus.WAITING:
return None
return PendingHumanReviewModel.from_db(review)
async def has_pending_reviews_for_graph_exec(graph_exec_id: str) -> bool:
"""
Check if a graph execution has any pending reviews.