mirror of
https://github.com/paperless-ngx/paperless-ngx.git
synced 2026-01-08 22:46:08 +01:00
refactor: Apply code review improvements to AI suggestions API
- Remove unused imports (Document from serializers, override_settings from tests) - Add explanatory comments to all empty except clauses - Create SUGGESTION_TYPE_CHOICES constant and SuggestionSerializerMixin for DRY - Implement type-specific validation (value_id for ID types, value_text for text types) - Remove redundant Document.DoesNotExist handler (already handled by DRF) - Optimize ai_suggestion_stats query (21 queries → 3 queries using aggregation) - Update documentation to clarify unsupported custom_field/workflow in apply endpoint Addresses all 14 code review comments from Copilot PR reviewer. Co-authored-by: dawnsystem <42047891+dawnsystem@users.noreply.github.com>
This commit is contained in:
copilot-swe-agent[bot]
parent
075f58734a
commit
e9d64e6aac
4 changed files with 77 additions and 57 deletions
|
|
@ -108,16 +108,16 @@ Apply an AI suggestion to a document and record user feedback.
|
|||
}
|
||||
```
|
||||
|
||||
**Suggestion Types:**
|
||||
**Supported Suggestion Types:**
|
||||
- `tag` - Tag assignment
|
||||
- `correspondent` - Correspondent assignment
|
||||
- `document_type` - Document type classification
|
||||
- `storage_path` - Storage path assignment
|
||||
- `custom_field` - Custom field value
|
||||
- `workflow` - Workflow assignment
|
||||
- `title` - Document title
|
||||
|
||||
**For ID-based suggestions (tag, correspondent, document_type, storage_path, workflow):**
|
||||
**Note:** Custom field and workflow suggestions are supported in the API response but not yet implemented in the apply endpoint.
|
||||
|
||||
**For ID-based suggestions (tag, correspondent, document_type, storage_path):**
|
||||
```json
|
||||
{
|
||||
"suggestion_type": "correspondent",
|
||||
|
|
@ -126,7 +126,7 @@ Apply an AI suggestion to a document and record user feedback.
|
|||
}
|
||||
```
|
||||
|
||||
**For text-based suggestions (title, custom_field):**
|
||||
**For text-based suggestions (title):**
|
||||
```json
|
||||
{
|
||||
"suggestion_type": "title",
|
||||
|
|
|
|||
|
|
@ -15,7 +15,6 @@ from documents.models import (
|
|||
AISuggestionFeedback,
|
||||
Correspondent,
|
||||
CustomField,
|
||||
Document,
|
||||
DocumentType,
|
||||
StoragePath,
|
||||
Tag,
|
||||
|
|
@ -23,6 +22,24 @@ from documents.models import (
|
|||
)
|
||||
|
||||
|
||||
# Suggestion type choices - used across multiple serializers
|
||||
SUGGESTION_TYPE_CHOICES = [
|
||||
'tag',
|
||||
'correspondent',
|
||||
'document_type',
|
||||
'storage_path',
|
||||
'custom_field',
|
||||
'workflow',
|
||||
'title',
|
||||
]
|
||||
|
||||
# Types that require value_id
|
||||
ID_REQUIRED_TYPES = ['tag', 'correspondent', 'document_type', 'storage_path', 'workflow']
|
||||
# Types that require value_text
|
||||
TEXT_REQUIRED_TYPES = ['title']
|
||||
# Types that can use either (custom_field can be ID or text)
|
||||
|
||||
|
||||
class TagSuggestionSerializer(serializers.Serializer):
|
||||
"""Serializer for tag suggestions."""
|
||||
|
||||
|
|
@ -122,6 +139,7 @@ class AISuggestionsSerializer(serializers.Serializer):
|
|||
'confidence': confidence,
|
||||
})
|
||||
except Tag.DoesNotExist:
|
||||
# Tag no longer exists in database; skip this suggestion
|
||||
pass
|
||||
data['tags'] = tag_suggestions
|
||||
|
||||
|
|
@ -136,6 +154,7 @@ class AISuggestionsSerializer(serializers.Serializer):
|
|||
'confidence': confidence,
|
||||
}
|
||||
except Correspondent.DoesNotExist:
|
||||
# Correspondent no longer exists in database; omit from suggestions
|
||||
pass
|
||||
|
||||
# Document Type
|
||||
|
|
@ -149,6 +168,7 @@ class AISuggestionsSerializer(serializers.Serializer):
|
|||
'confidence': confidence,
|
||||
}
|
||||
except DocumentType.DoesNotExist:
|
||||
# Document type no longer exists in database; omit from suggestions
|
||||
pass
|
||||
|
||||
# Storage Path
|
||||
|
|
@ -163,6 +183,7 @@ class AISuggestionsSerializer(serializers.Serializer):
|
|||
'confidence': confidence,
|
||||
}
|
||||
except StoragePath.DoesNotExist:
|
||||
# Storage path no longer exists in database; omit from suggestions
|
||||
pass
|
||||
|
||||
# Custom Fields
|
||||
|
|
@ -178,6 +199,7 @@ class AISuggestionsSerializer(serializers.Serializer):
|
|||
'confidence': confidence,
|
||||
})
|
||||
except CustomField.DoesNotExist:
|
||||
# Custom field no longer exists in database; skip this suggestion
|
||||
pass
|
||||
data['custom_fields'] = field_suggestions
|
||||
|
||||
|
|
@ -193,6 +215,7 @@ class AISuggestionsSerializer(serializers.Serializer):
|
|||
'confidence': confidence,
|
||||
})
|
||||
except Workflow.DoesNotExist:
|
||||
# Workflow no longer exists in database; skip this suggestion
|
||||
pass
|
||||
data['workflows'] = workflow_suggestions
|
||||
|
||||
|
|
@ -205,66 +228,65 @@ class AISuggestionsSerializer(serializers.Serializer):
|
|||
return data
|
||||
|
||||
|
||||
class ApplySuggestionSerializer(serializers.Serializer):
|
||||
class SuggestionSerializerMixin:
|
||||
"""
|
||||
Mixin to provide validation logic for suggestion serializers.
|
||||
"""
|
||||
def validate(self, attrs):
|
||||
"""Validate that the correct value field is provided for the suggestion type."""
|
||||
suggestion_type = attrs.get('suggestion_type')
|
||||
value_id = attrs.get('value_id')
|
||||
value_text = attrs.get('value_text')
|
||||
|
||||
# Types that require value_id
|
||||
if suggestion_type in ID_REQUIRED_TYPES and not value_id:
|
||||
raise serializers.ValidationError(
|
||||
f"value_id is required for suggestion_type '{suggestion_type}'"
|
||||
)
|
||||
|
||||
# Types that require value_text
|
||||
if suggestion_type in TEXT_REQUIRED_TYPES and not value_text:
|
||||
raise serializers.ValidationError(
|
||||
f"value_text is required for suggestion_type '{suggestion_type}'"
|
||||
)
|
||||
|
||||
# For custom_field, either is acceptable
|
||||
if suggestion_type == 'custom_field' and not value_id and not value_text:
|
||||
raise serializers.ValidationError(
|
||||
"Either value_id or value_text must be provided for custom_field"
|
||||
)
|
||||
|
||||
return attrs
|
||||
|
||||
|
||||
class ApplySuggestionSerializer(SuggestionSerializerMixin, serializers.Serializer):
|
||||
"""
|
||||
Serializer for applying AI suggestions.
|
||||
"""
|
||||
|
||||
suggestion_type = serializers.ChoiceField(
|
||||
choices=[
|
||||
'tag',
|
||||
'correspondent',
|
||||
'document_type',
|
||||
'storage_path',
|
||||
'custom_field',
|
||||
'workflow',
|
||||
'title',
|
||||
],
|
||||
choices=SUGGESTION_TYPE_CHOICES,
|
||||
required=True,
|
||||
)
|
||||
|
||||
value_id = serializers.IntegerField(required=False, allow_null=True)
|
||||
value_text = serializers.CharField(required=False, allow_blank=True)
|
||||
confidence = serializers.FloatField(required=True)
|
||||
|
||||
def validate(self, attrs):
|
||||
"""Validate that at least one value field is provided."""
|
||||
if not attrs.get('value_id') and not attrs.get('value_text'):
|
||||
raise serializers.ValidationError(
|
||||
"Either value_id or value_text must be provided"
|
||||
)
|
||||
return attrs
|
||||
|
||||
|
||||
class RejectSuggestionSerializer(serializers.Serializer):
|
||||
class RejectSuggestionSerializer(SuggestionSerializerMixin, serializers.Serializer):
|
||||
"""
|
||||
Serializer for rejecting AI suggestions.
|
||||
"""
|
||||
|
||||
suggestion_type = serializers.ChoiceField(
|
||||
choices=[
|
||||
'tag',
|
||||
'correspondent',
|
||||
'document_type',
|
||||
'storage_path',
|
||||
'custom_field',
|
||||
'workflow',
|
||||
'title',
|
||||
],
|
||||
choices=SUGGESTION_TYPE_CHOICES,
|
||||
required=True,
|
||||
)
|
||||
|
||||
value_id = serializers.IntegerField(required=False, allow_null=True)
|
||||
value_text = serializers.CharField(required=False, allow_blank=True)
|
||||
confidence = serializers.FloatField(required=True)
|
||||
|
||||
def validate(self, attrs):
|
||||
"""Validate that at least one value field is provided."""
|
||||
if not attrs.get('value_id') and not attrs.get('value_text'):
|
||||
raise serializers.ValidationError(
|
||||
"Either value_id or value_text must be provided"
|
||||
)
|
||||
return attrs
|
||||
|
||||
|
||||
class AISuggestionFeedbackSerializer(serializers.ModelSerializer):
|
||||
|
|
|
|||
|
|
@ -5,7 +5,6 @@ Tests for AI Suggestions API endpoints.
|
|||
from unittest import mock
|
||||
|
||||
from django.contrib.auth.models import User
|
||||
from django.test import override_settings
|
||||
from rest_framework import status
|
||||
from rest_framework.test import APITestCase
|
||||
|
||||
|
|
|
|||
|
|
@ -1386,8 +1386,6 @@ class UnifiedSearchViewSet(DocumentViewSet):
|
|||
|
||||
return Response(serializer.validated_data)
|
||||
|
||||
except Document.DoesNotExist:
|
||||
return Response({"detail": "Document not found"}, status=404)
|
||||
except Exception as e:
|
||||
logger.error(f"Error getting AI suggestions for document {pk}: {e}", exc_info=True)
|
||||
return Response(
|
||||
|
|
@ -1559,19 +1557,20 @@ class UnifiedSearchViewSet(DocumentViewSet):
|
|||
# Calculate accuracy rate
|
||||
accuracy_rate = (total_applied / total_feedbacks * 100) if total_feedbacks > 0 else 0
|
||||
|
||||
# Get statistics by suggestion type
|
||||
# Get statistics by suggestion type using a single aggregated query
|
||||
stats_by_type = AISuggestionFeedback.objects.values('suggestion_type').annotate(
|
||||
total=Count('id'),
|
||||
applied=Count('id', filter=Q(status=AISuggestionFeedback.STATUS_APPLIED)),
|
||||
rejected=Count('id', filter=Q(status=AISuggestionFeedback.STATUS_REJECTED))
|
||||
)
|
||||
|
||||
# Build the by_type dictionary using the aggregated results
|
||||
by_type = {}
|
||||
for suggestion_type, _ in AISuggestionFeedback.SUGGESTION_TYPES:
|
||||
type_feedbacks = AISuggestionFeedback.objects.filter(
|
||||
suggestion_type=suggestion_type
|
||||
)
|
||||
type_applied = type_feedbacks.filter(
|
||||
status=AISuggestionFeedback.STATUS_APPLIED
|
||||
).count()
|
||||
type_rejected = type_feedbacks.filter(
|
||||
status=AISuggestionFeedback.STATUS_REJECTED
|
||||
).count()
|
||||
type_total = type_applied + type_rejected
|
||||
for stat in stats_by_type:
|
||||
suggestion_type = stat['suggestion_type']
|
||||
type_total = stat['total']
|
||||
type_applied = stat['applied']
|
||||
type_rejected = stat['rejected']
|
||||
|
||||
by_type[suggestion_type] = {
|
||||
'total': type_total,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue