From 1853d008e9a4921d4122da5a8c8c5019d3fb42f5 Mon Sep 17 00:00:00 2001 From: busya Date: Thu, 14 May 2026 17:38:52 +0300 Subject: [PATCH] fix(translate): CR1-CR2 migration + H1-H4 code review bugs CR1: Add needs_review, language_overridden to translation_languages migration CR2: Add per_language_metrics to translation_metric_snapshots migration H1: BulkFindReplaceService.preview() now uses replacement_text (not find pattern) H2: get_run_records now includes per-language data via selectinload H3: Preview per-language approve/reject uses correct (jobId, rowId, lang) args H4: _compute_config_hash now includes target_languages --- ...74bfe_add_missing_columns_needs_review_.py | 37 +++++++++++++++++++ .../src/api/routes/translate/_run_routes.py | 1 + .../translate/__tests__/test_orchestrator.py | 5 ++- backend/src/plugins/translate/orchestrator.py | 20 ++++++++-- backend/src/plugins/translate/service.py | 8 ++-- .../translate/TranslationPreview.svelte | 4 +- 6 files changed, 64 insertions(+), 11 deletions(-) create mode 100644 backend/alembic/versions/c4a3a2f74bfe_add_missing_columns_needs_review_.py diff --git a/backend/alembic/versions/c4a3a2f74bfe_add_missing_columns_needs_review_.py b/backend/alembic/versions/c4a3a2f74bfe_add_missing_columns_needs_review_.py new file mode 100644 index 00000000..975fed0e --- /dev/null +++ b/backend/alembic/versions/c4a3a2f74bfe_add_missing_columns_needs_review_.py @@ -0,0 +1,37 @@ +"""add missing columns needs_review language_overridden per_language_metrics + +Revision ID: c4a3a2f74bfe +Revises: ed310b33f02c +Create Date: 2026-05-14 17:30:36.865269 +""" +from typing import Sequence, Union +from alembic import op +import sqlalchemy as sa + +revision: str = 'c4a3a2f74bfe' +down_revision: Union[str, Sequence[str], None] = 'ed310b33f02c' +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + # CR1: needs_review + language_overridden on translation_languages + op.add_column('translation_languages', sa.Column('needs_review', sa.Boolean(), nullable=True, + comment='Flagged because source language could not be determined')) + op.add_column('translation_languages', sa.Column('language_overridden', sa.Boolean(), nullable=True, + comment='User manually overrode the auto-detected source language')) + + # CR1b: needs_review on translation_preview_languages + op.add_column('translation_preview_languages', sa.Column('needs_review', sa.Boolean(), nullable=True, + comment='Flagged because source language could not be determined')) + + # CR2: per_language_metrics on translation_metric_snapshots + op.add_column('translation_metric_snapshots', sa.Column('per_language_metrics', sa.JSON(), nullable=True, + comment='Per-language cumulative metrics: {lang: {cumulative_tokens, cumulative_cost, runs}}')) + + +def downgrade() -> None: + op.drop_column('translation_metric_snapshots', 'per_language_metrics') + op.drop_column('translation_preview_languages', 'needs_review') + op.drop_column('translation_languages', 'language_overridden') + op.drop_column('translation_languages', 'needs_review') diff --git a/backend/src/api/routes/translate/_run_routes.py b/backend/src/api/routes/translate/_run_routes.py index 67d9a5f8..e1288b1f 100644 --- a/backend/src/api/routes/translate/_run_routes.py +++ b/backend/src/api/routes/translate/_run_routes.py @@ -507,6 +507,7 @@ async def bulk_find_replace( run_id=run_id, pattern=payload.find_pattern, is_regex=payload.is_regex, + replacement_text=payload.replacement_text, target_language=payload.target_language, ) return { diff --git a/backend/src/plugins/translate/__tests__/test_orchestrator.py b/backend/src/plugins/translate/__tests__/test_orchestrator.py index 892a85de..3b00b6aa 100644 --- a/backend/src/plugins/translate/__tests__/test_orchestrator.py +++ b/backend/src/plugins/translate/__tests__/test_orchestrator.py @@ -548,9 +548,10 @@ class TestTranslationOrchestrator: record.status = "SUCCESS" record.error_message = None record.created_at = datetime.now(UTC) + record.languages = [] - db.query.return_value.filter.return_value.count.return_value = 1 - db.query.return_value.filter.return_value.order_by.return_value.offset.return_value.limit.return_value.all.return_value = [record] + db.query.return_value.options.return_value.filter.return_value.count.return_value = 1 + db.query.return_value.options.return_value.filter.return_value.order_by.return_value.offset.return_value.limit.return_value.all.return_value = [record] orch = TranslationOrchestrator(db, config_manager, "test-user") result = orch.get_run_records("run-1") diff --git a/backend/src/plugins/translate/orchestrator.py b/backend/src/plugins/translate/orchestrator.py index 919f7403..ab7afef9 100644 --- a/backend/src/plugins/translate/orchestrator.py +++ b/backend/src/plugins/translate/orchestrator.py @@ -23,7 +23,7 @@ from collections.abc import Callable from datetime import UTC, datetime from typing import Any -from sqlalchemy.orm import Session +from sqlalchemy.orm import Session, selectinload from ...core.config_manager import ConfigManager from ...core.logger import belief_scope, logger @@ -827,8 +827,10 @@ class TranslationOrchestrator: status_filter: str | None = None, ) -> dict[str, Any]: with belief_scope("TranslationOrchestrator.get_run_records"): - query = self.db.query(TranslationRecord).filter( - TranslationRecord.run_id == run_id + query = ( + self.db.query(TranslationRecord) + .options(selectinload(TranslationRecord.languages)) + .filter(TranslationRecord.run_id == run_id) ) if status_filter: @@ -856,6 +858,17 @@ class TranslationOrchestrator: "status": r.status, "error_message": r.error_message, "created_at": r.created_at.isoformat() if r.created_at else None, + "languages": [ + { + "language_code": tl.language_code, + "translated_value": tl.translated_value, + "final_value": tl.final_value or tl.translated_value, + "source_language_detected": tl.source_language_detected, + "status": tl.status, + "needs_review": tl.needs_review or False, + } + for tl in (r.languages or []) + ], } for r in records ], @@ -995,6 +1008,7 @@ class TranslationOrchestrator: "translation_column": job.translation_column, "context_columns": job.context_columns, "target_language": job.target_language, + "target_languages": sorted(job.target_languages) if job.target_languages else [], "provider_id": job.provider_id, "batch_size": job.batch_size, "upsert_strategy": job.upsert_strategy, diff --git a/backend/src/plugins/translate/service.py b/backend/src/plugins/translate/service.py index 5225d673..ea05b00d 100644 --- a/backend/src/plugins/translate/service.py +++ b/backend/src/plugins/translate/service.py @@ -865,6 +865,7 @@ class BulkFindReplaceService: run_id: str, pattern: str, is_regex: bool, + replacement_text: str, target_language: str, ) -> list[dict]: """Scan TranslationLanguage entries and return matching items without applying changes.""" @@ -888,7 +889,7 @@ class BulkFindReplaceService: source_term = record.source_sql if record else "" new_value = compiled.sub( - BulkFindReplaceService._get_replacement(pattern, is_regex), + replacement_text, current_value, ) if new_value != current_value: @@ -904,10 +905,9 @@ class BulkFindReplaceService: return preview_items @staticmethod - def _get_replacement(pattern: str, is_regex: bool) -> str: + def _get_replacement(replacement_text: str, is_regex: bool) -> str: """Return replacement pattern (for regex, use backrefs; for plain, use literal).""" - # The replacement_text is passed through at apply time - return pattern # Placeholder — actual replacement is done in apply() + return replacement_text @staticmethod def apply( diff --git a/frontend/src/lib/components/translate/TranslationPreview.svelte b/frontend/src/lib/components/translate/TranslationPreview.svelte index a89c7f01..71a251e2 100644 --- a/frontend/src/lib/components/translate/TranslationPreview.svelte +++ b/frontend/src/lib/components/translate/TranslationPreview.svelte @@ -420,7 +420,7 @@
{#if langStatus !== 'approved' && langStatus !== 'edited'} {#if langStatus !== 'rejected'}