From 49002d50a0d12cd4ba0ebc59bce83afdf33afa03 Mon Sep 17 00:00:00 2001 From: lizerk Date: Wed, 22 Apr 2026 14:10:55 +0300 Subject: [PATCH] Prevent duplicate request employee actions --- .../0049_employeeaction_source_request.py | 106 ++++++++ backend/employees/models.py | 15 ++ backend/employees/services/request_actions.py | 113 +++++++++ backend/employees/signals/common.py | 43 +--- .../commands/migrate_requests_to_actions.py | 128 ++++------ backend/requests_app/tasks.py | 82 +++--- .../requests_app/test_request_actions.py | 235 ++++++++++++++++++ 7 files changed, 570 insertions(+), 152 deletions(-) create mode 100644 backend/employees/migrations/0049_employeeaction_source_request.py create mode 100644 backend/employees/services/request_actions.py create mode 100644 backend/tests/requests_app/test_request_actions.py diff --git a/backend/employees/migrations/0049_employeeaction_source_request.py b/backend/employees/migrations/0049_employeeaction_source_request.py new file mode 100644 index 00000000..e448434e --- /dev/null +++ b/backend/employees/migrations/0049_employeeaction_source_request.py @@ -0,0 +1,106 @@ +# Generated by Django 5.2.4 on 2026-04-22 10:52 + +import django.db.models.deletion +from django.db import migrations, models + + +def backfill_source_request_and_remove_duplicates(apps, schema_editor): + EmployeeAction = apps.get_model("employees", "EmployeeAction") + Request = apps.get_model("requests_app", "Request") + + request_ids = set() + for action in EmployeeAction.objects.exclude(extra__isnull=True).only( + "id", + "extra", + ): + if isinstance(action.extra, dict) and action.extra.get("request_id"): + request_ids.add(action.extra["request_id"]) + + existing_request_ids = set( + Request.objects.filter(id__in=request_ids).values_list("id", flat=True) + ) + + for action in EmployeeAction.objects.exclude(extra__isnull=True).only( + "id", + "extra", + "source_request_id", + ): + if action.source_request_id: + continue + if not isinstance(action.extra, dict): + continue + request_id = action.extra.get("request_id") + if request_id in existing_request_ids: + action.source_request_id = request_id + action.save(update_fields=["source_request"]) + + from django.db.models import Count, Min + + duplicate_groups = ( + EmployeeAction.objects.filter(source_request__isnull=False) + .values("source_request_id", "action") + .annotate(first_id=Min("id"), action_count=Count("id")) + .filter(action_count__gt=1) + ) + + for group in duplicate_groups: + ( + EmployeeAction.objects.filter( + source_request_id=group["source_request_id"], + action=group["action"], + ) + .exclude(id=group["first_id"]) + .delete() + ) + + +def noop_reverse(apps, schema_editor): + pass + + +class Migration(migrations.Migration): + + dependencies = [ + ("employees", "0048_alter_employeeaction_action_and_more"), + ("requests_app", "0011_remove_requestcomment_model"), + ] + + operations = [ + migrations.AddField( + model_name="employeeaction", + name="source_request", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name="employee_actions", + to="requests_app.request", + verbose_name="Источник-заявление", + ), + ), + migrations.AddField( + model_name="historicalemployeeaction", + name="source_request", + field=models.ForeignKey( + blank=True, + db_constraint=False, + null=True, + on_delete=django.db.models.deletion.DO_NOTHING, + related_name="+", + to="requests_app.request", + verbose_name="Источник-заявление", + ), + ), + migrations.RunPython( + backfill_source_request_and_remove_duplicates, + reverse_code=noop_reverse, + ), + migrations.AddConstraint( + model_name="employeeaction", + constraint=models.UniqueConstraint( + condition=models.Q(("source_request__isnull", False)), + fields=("source_request", "action"), + name="uniq_employee_action_source_request_action", + ), + ), + ] diff --git a/backend/employees/models.py b/backend/employees/models.py index fb50a0d4..c09d1864 100644 --- a/backend/employees/models.py +++ b/backend/employees/models.py @@ -337,6 +337,14 @@ class EmployeeAction(models.Model): action = models.CharField( "Кадровое событие", max_length=50, choices=ACTION_CHOICES ) + source_request = models.ForeignKey( + "requests_app.Request", + related_name="employee_actions", + on_delete=models.SET_NULL, + null=True, + blank=True, + verbose_name="Источник-заявление", + ) date = models.DateTimeField("Дата действия") comment = models.TextField("Комментарий/причина", blank=True) extra = models.JSONField("Дополнительно", blank=True, null=True) @@ -346,6 +354,13 @@ class EmployeeAction(models.Model): verbose_name = "Кадровое событие" verbose_name_plural = "Кадровые события" ordering = ["-date"] + constraints = [ + models.UniqueConstraint( + fields=["source_request", "action"], + condition=models.Q(source_request__isnull=False), + name="uniq_employee_action_source_request_action", + ), + ] def __str__(self): return ( diff --git a/backend/employees/services/request_actions.py b/backend/employees/services/request_actions.py new file mode 100644 index 00000000..702c7108 --- /dev/null +++ b/backend/employees/services/request_actions.py @@ -0,0 +1,113 @@ +"""Idempotent EmployeeAction creation for approved requests.""" + +from __future__ import annotations + +from datetime import date, datetime, time +from typing import Any + +from django.db import IntegrityError, transaction +from django.utils import timezone + +from employees.models import EmployeeAction + + +def as_action_datetime(value: date | datetime | None, *, hour: int = 12) -> datetime: + """Normalize request dates to aware datetimes for EmployeeAction.date.""" + if value is None: + return timezone.now() + if isinstance(value, datetime): + return value if timezone.is_aware(value) else timezone.make_aware(value) + return timezone.make_aware(datetime.combine(value, time(hour=hour))) + + +def build_request_action_comment(request, prefix: str = "Заявление") -> str: + comment = f"{prefix} #{request.id}" + if getattr(request, "comment", ""): + comment += f": {request.comment[:200]}" + return comment + + +def get_existing_request_action(request, action_type: str) -> EmployeeAction | None: + """Find an action by the new FK, falling back to legacy extra.request_id.""" + existing = ( + EmployeeAction.objects.filter(source_request=request, action=action_type) + .order_by("id") + .first() + ) + if existing: + return existing + + return ( + EmployeeAction.objects.filter( + source_request__isnull=True, + extra__request_id=request.id, + action=action_type, + ) + .order_by("id") + .first() + ) + + +def create_request_action( + *, + request, + action_type: str, + action_date: date | datetime | None, + comment: str, + extra: dict[str, Any] | None = None, +) -> tuple[EmployeeAction, bool]: + """ + Create one EmployeeAction per request/action pair. + + Returns: + tuple: (action, created) + """ + extra_data = dict(extra or {}) + extra_data["request_id"] = request.id + + legacy = get_existing_request_action(request, action_type) + if legacy: + updates: list[str] = [] + if legacy.source_request_id is None: + legacy.source_request = request + updates.append("source_request") + if ( + not isinstance(legacy.extra, dict) + or legacy.extra.get("request_id") is None + ): + current_extra = legacy.extra if isinstance(legacy.extra, dict) else {} + legacy.extra = {**current_extra, "request_id": request.id} + updates.append("extra") + if updates: + try: + legacy.save(update_fields=updates) + except IntegrityError: + existing = EmployeeAction.objects.get( + source_request=request, + action=action_type, + ) + return existing, False + return legacy, False + + defaults = { + "employee": request.employee, + "date": as_action_datetime(action_date), + "comment": comment, + "extra": extra_data, + } + + try: + with transaction.atomic(): + action, created = EmployeeAction.objects.get_or_create( + source_request=request, + action=action_type, + defaults=defaults, + ) + except IntegrityError: + action = EmployeeAction.objects.get( + source_request=request, + action=action_type, + ) + created = False + + return action, created diff --git a/backend/employees/signals/common.py b/backend/employees/signals/common.py index b01d6ccd..132a779c 100644 --- a/backend/employees/signals/common.py +++ b/backend/employees/signals/common.py @@ -7,7 +7,7 @@ """ import logging -from datetime import date, datetime, time +from datetime import datetime from django.db.models.signals import post_save, pre_save from django.dispatch import receiver @@ -20,6 +20,10 @@ from employees.constants import ( ACTION_ON_SICK_LEAVE, ) from employees.models import Employee, EmployeeAction +from employees.services.request_actions import ( + build_request_action_comment, + create_request_action, +) logger = logging.getLogger(__name__) @@ -122,33 +126,20 @@ def _create_immediate_action(request): if not action_type: return - # Проверяем дубли - if EmployeeAction.objects.filter( - extra__request_id=request.id, action=action_type - ).exists(): - logger.warning(f"EmployeeAction already exists for Request #{request.id}") - return - - # Создаем событие - action_date = _as_action_datetime( - request.date_from or request.decided_at or timezone.now() - ) - action_comment = f"Заявление #{request.id}" - if request.comment: - action_comment += f": {request.comment[:200]}" - try: - action = EmployeeAction.objects.create( - employee=request.employee, - action=action_type, - date=action_date, - comment=action_comment, + action, created = create_request_action( + request=request, + action_type=action_type, + action_date=request.date_from or request.decided_at or timezone.now(), + comment=build_request_action_comment(request), extra={ - "request_id": request.id, "approved_by": (request.approver.id if request.approver else None), "immediate": True, }, ) + if not created: + logger.warning(f"EmployeeAction already exists for Request #{request.id}") + return # Применяем эффекты (деактивация, LDAP sync) _apply_action_effects(action) @@ -206,14 +197,6 @@ def _schedule_delayed_action(request): ) -def _as_action_datetime(value): - if isinstance(value, datetime): - return value if timezone.is_aware(value) else timezone.make_aware(value) - if isinstance(value, date): - return timezone.make_aware(datetime.combine(value, time(hour=12))) - return value - - def _apply_action_effects(action): """Применяет эффекты кадрового события (как в EmployeeActionViewSet).""" try: diff --git a/backend/requests_app/management/commands/migrate_requests_to_actions.py b/backend/requests_app/management/commands/migrate_requests_to_actions.py index 6092888a..78065dd6 100644 --- a/backend/requests_app/management/commands/migrate_requests_to_actions.py +++ b/backend/requests_app/management/commands/migrate_requests_to_actions.py @@ -7,19 +7,23 @@ Management команда для миграции существующих за from datetime import datetime, timedelta from django.core.management.base import BaseCommand -from django.db import transaction from django.utils import timezone from requests_app.models import Request from requests_app.enums import RequestStatus -from employees.models import EmployeeAction from employees.signals import IMMEDIATE_ACTION_MAPPING, SCHEDULED_ACTION_MAPPING +from employees.services.request_actions import ( + build_request_action_comment, + create_request_action, + get_existing_request_action, +) # Маппинг для событий возврата из отпуска/больничного RETURN_ACTION_MAPPING = { "vacation": "returned_from_leave", - "sick_leave": "returned_from_leave", + "sick_leave": "returned_from_sick_leave", + "day_off": "returned_from_day_off", } @@ -71,13 +75,8 @@ class Command(BaseCommand): if not action_type: continue - # Проверяем, существует ли уже событие для этой заявки - existing = EmployeeAction.objects.filter( - extra__request_id=request.id, action=action_type - ).exists() - - # Флаг - нужно ли создавать основное событие - should_create_main = not existing + existing = get_existing_request_action(request, action_type) + should_create_main = existing is None if existing: skipped_count += 1 @@ -94,15 +93,14 @@ class Command(BaseCommand): return_date = None if request.type in RETURN_ACTION_MAPPING and request.date_to: - return_action = RETURN_ACTION_MAPPING[request.type] + return_action = RETURN_ACTION_MAPPING.get(request.type) return_date = request.date_to + timedelta(days=1) - # Проверяем что возврат ещё не создан - existing_return = EmployeeAction.objects.filter( - extra__request_id=request.id, action=return_action - ).exists() - - should_create_return = not existing_return + if return_action: + existing_return = get_existing_request_action( + request, return_action + ) + should_create_return = existing_return is None # Если нечего создавать - пропускаем if not should_create_main and not should_create_return: @@ -127,14 +125,8 @@ class Command(BaseCommand): datetime.combine(raw_date, datetime.min.time()) ) - # Формируем комментарий - action_comment = f"Заявление #{request.id}" - if request.comment: - action_comment += f": {request.comment[:200]}" - # Готовим extra данные extra_data = { - "request_id": request.id, "approved_by": request.approver.id if request.approver else None, @@ -147,12 +139,10 @@ class Command(BaseCommand): if should_create_main: self.stdout.write( self.style.SUCCESS( - f" WOULD CREATE: Request #{request.id} ({ - request.type - }) → " - f"EmployeeAction ({action_type}) for { - request.employee - } " + f" WOULD CREATE: Request #{request.id} " + f"({request.type}) → " + f"EmployeeAction ({action_type}) " + f"for {request.employee} " f"on {action_date.date()}" ) ) @@ -169,20 +159,19 @@ class Command(BaseCommand): return_count += 1 else: try: - with transaction.atomic(): - # Создаём основное событие - if should_create_main: - action = EmployeeAction.objects.create( - employee=request.employee, - action=action_type, - date=action_date, - comment=action_comment, - extra=extra_data, - ) - - # Применяем эффекты (деактивация, LDAP sync) + # Создаём основное событие + if should_create_main: + action, created = create_request_action( + request=request, + action_type=action_type, + action_date=request.date_from + or request.decided_at + or request.created_at, + comment=build_request_action_comment(request), + extra=extra_data, + ) + if created: self._apply_effects(action) - created_count += 1 self.stdout.write( self.style.SUCCESS( @@ -192,40 +181,29 @@ class Command(BaseCommand): f"for {request.employee}" ) ) + else: + skipped_count += 1 - # Создаём событие возврата - if should_create_return: - return_comment = ( - f"Автоматически: окончание " - f"{request.get_type_display().lower()} " - f"(заявка #{request.id})" - ) - - # Конвертируем date в datetime с timezone - return_datetime = timezone.make_aware( - datetime.combine( - return_date, datetime.min.time() - ) - ) - - return_action_obj = EmployeeAction.objects.create( - employee=request.employee, - action=return_action, - date=return_datetime, - comment=return_comment, - extra={ - "request_id": request.id, - "auto_return": True, - "migrated": True, - "migration_date": ( - timezone.now().isoformat() - ), - }, - ) - - # Применяем эффекты для возврата + # Создаём событие возврата + if should_create_return and return_action and return_date: + return_comment = ( + f"Автоматически: окончание " + f"{request.get_type_display().lower()} " + f"(заявка #{request.id})" + ) + return_action_obj, created = create_request_action( + request=request, + action_type=return_action, + action_date=return_date, + comment=return_comment, + extra={ + "auto_return": True, + "migrated": True, + "migration_date": timezone.now().isoformat(), + }, + ) + if created: self._apply_effects(return_action_obj) - return_count += 1 self.stdout.write( self.style.SUCCESS( @@ -235,6 +213,8 @@ class Command(BaseCommand): f"({return_action}) on {return_date}" ) ) + else: + skipped_count += 1 except Exception as e: errors.append((request.id, str(e))) diff --git a/backend/requests_app/tasks.py b/backend/requests_app/tasks.py index de7d8c34..efbdabd8 100644 --- a/backend/requests_app/tasks.py +++ b/backend/requests_app/tasks.py @@ -28,7 +28,10 @@ def create_scheduled_action(self, request_id): ACTION_ON_LEAVE, ACTION_ON_SICK_LEAVE, ) - from employees.models import EmployeeAction + from employees.services.request_actions import ( + build_request_action_comment, + create_request_action, + ) try: request = Request.objects.get(id=request_id) @@ -56,29 +59,19 @@ def create_scheduled_action(self, request_id): ) return - # Проверяем, что событие еще не создано - if EmployeeAction.objects.filter( - extra__request_id=request_id, action=action_type - ).exists(): - logger.info(f"Action already exists for Request #{request_id}, skipping") - return - - # Создаем событие - action_comment = f"Заявление #{request.id}" - if request.comment: - action_comment += f": {request.comment[:200]}" - - action = EmployeeAction.objects.create( - employee=request.employee, - action=action_type, - date=timezone.now(), - comment=action_comment, + action, created = create_request_action( + request=request, + action_type=action_type, + action_date=request.date_from, + comment=build_request_action_comment(request), extra={ - "request_id": request.id, "approved_by": (request.approver.id if request.approver else None), "scheduled": True, }, ) + if not created: + logger.info(f"Action already exists for Request #{request_id}, skipping") + return logger.info( f"Created scheduled EmployeeAction #{action.id} ({action_type}) " @@ -128,7 +121,7 @@ def schedule_auto_return(self, request_id): ACTION_RETURNED_FROM_LEAVE, ACTION_RETURNED_FROM_SICK_LEAVE, ) - from employees.models import EmployeeAction + from employees.services.request_actions import create_request_action try: request = Request.objects.get(id=request_id) @@ -155,28 +148,26 @@ def schedule_auto_return(self, request_id): ) return - # Проверяем, что событие возврата еще не создано - if EmployeeAction.objects.filter( - extra__request_id=request_id, action=return_action - ).exists(): - logger.info(f"Return action already exists for Request #{request_id}") - return - - # Создаем событие возврата return_comment = ( f"Автоматически: окончание " f"{request.get_type_display().lower()} " f"(заявка #{request.id})" ) - - action = EmployeeAction.objects.create( - employee=request.employee, - action=return_action, - date=timezone.now(), - comment=return_comment, - extra={"request_id": request.id, "auto_return": True}, + return_date = ( + request.date_to + timedelta(days=1) if request.date_to else None ) + action, created = create_request_action( + request=request, + action_type=return_action, + action_date=return_date, + comment=return_comment, + extra={"auto_return": True}, + ) + if not created: + logger.info(f"Return action already exists for Request #{request_id}") + return + logger.info( f"Created auto-return EmployeeAction #{action.id} " f"({return_action}) for Request #{request_id}" @@ -205,7 +196,7 @@ def cleanup_missed_returns(): """ from .models import Request from .enums import RequestStatus - from employees.models import EmployeeAction + from employees.services.request_actions import create_request_action logger.info("Running cleanup_missed_returns task") @@ -233,24 +224,19 @@ def cleanup_missed_returns(): if not return_action: continue - # Проверяем, что события возврата нет - if EmployeeAction.objects.filter( - extra__request_id=req.id, action=return_action - ).exists(): - continue - - # Создаем пропущенное событие возврата try: - action = EmployeeAction.objects.create( - employee=req.employee, - action=return_action, - date=req.date_to + timedelta(days=1), + action, created = create_request_action( + request=req, + action_type=return_action, + action_date=req.date_to + timedelta(days=1), comment=( f"Автоматически восстановлено: окончание " f"{req.get_type_display().lower()} (заявка #{req.id})" ), - extra={"request_id": req.id, "auto_return": True, "cleanup": True}, + extra={"auto_return": True, "cleanup": True}, ) + if not created: + continue created_count += 1 logger.info( f"Cleanup: created return action #{action.id} for Request #{req.id}" diff --git a/backend/tests/requests_app/test_request_actions.py b/backend/tests/requests_app/test_request_actions.py new file mode 100644 index 00000000..a33799d1 --- /dev/null +++ b/backend/tests/requests_app/test_request_actions.py @@ -0,0 +1,235 @@ +from __future__ import annotations + +from datetime import date, timedelta + +import pytest +from django.db import connection +from django.db.migrations.executor import MigrationExecutor +from django.utils import timezone + +from employees.models import EmployeeAction +from employees.services.request_actions import create_request_action +from requests_app.enums import RequestStatus +from requests_app.models import Request +from requests_app.tasks import create_scheduled_action, schedule_auto_return + + +pytestmark = pytest.mark.django_db + + +def _approved_request( + *, + employee, + approver, + type_: str = "vacation", + date_from: date | None = None, + date_to: date | None = None, +) -> Request: + return Request.objects.create( + employee=employee, + approver=approver, + type=type_, + status=RequestStatus.APPROVED, + date_from=date_from, + date_to=date_to, + ) + + +def test_scheduled_task_is_idempotent(user_factory): + employee = user_factory() + approver = user_factory() + start_date = date(2026, 4, 20) + request = _approved_request( + employee=employee, + approver=approver, + date_from=start_date, + ) + + create_scheduled_action.apply(args=[request.id]) + create_scheduled_action.apply(args=[request.id]) + + actions = EmployeeAction.objects.filter( + source_request=request, + action="on_leave", + ) + assert actions.count() == 1 + + action = actions.get() + assert action.extra["request_id"] == request.id + assert action.extra["scheduled"] is True + assert action.date.date() == start_date + + +def test_auto_return_task_is_idempotent(user_factory): + employee = user_factory() + approver = user_factory() + end_date = date(2026, 4, 25) + request = _approved_request( + employee=employee, + approver=approver, + date_from=date(2026, 4, 20), + date_to=end_date, + ) + + schedule_auto_return.apply(args=[request.id]) + schedule_auto_return.apply(args=[request.id]) + + actions = EmployeeAction.objects.filter( + source_request=request, + action="returned_from_leave", + ) + assert actions.count() == 1 + + action = actions.get() + assert action.extra["request_id"] == request.id + assert action.extra["auto_return"] is True + assert action.date.date() == end_date + timedelta(days=1) + + +def test_immediate_approval_is_idempotent(user_factory): + employee = user_factory() + approver = user_factory() + request = Request.objects.create( + employee=employee, + type="transfer", + status=RequestStatus.PENDING, + date_from=date(2026, 5, 1), + ) + + request.status = RequestStatus.APPROVED + request.approver = approver + request.save() + request.comment = "Повторное сохранение" + request.save() + + actions = EmployeeAction.objects.filter( + source_request=request, + action="transferred", + ) + assert actions.count() == 1 + assert actions.get().extra["request_id"] == request.id + + +def test_helper_returns_existing_legacy_action(user_factory): + employee = user_factory() + approver = user_factory() + request = _approved_request( + employee=employee, + approver=approver, + type_="transfer", + date_from=date(2026, 5, 1), + ) + existing = EmployeeAction.objects.create( + employee=employee, + action="transferred", + date=timezone.datetime( + 2026, + 5, + 1, + 12, + tzinfo=timezone.get_current_timezone(), + ), + comment="legacy", + extra={"request_id": request.id, "legacy": True}, + ) + + action, created = create_request_action( + request=request, + action_type="transferred", + action_date=request.date_from, + comment="new", + extra={"immediate": True}, + ) + + assert created is False + assert action.id == existing.id + action.refresh_from_db() + assert action.source_request_id == request.id + assert action.extra["request_id"] == request.id + + +@pytest.mark.django_db(transaction=True) +def test_migration_0049_backfills_source_request_and_removes_duplicates(): + def migrate_to(target): + executor = MigrationExecutor(connection) + executor.loader.build_graph() + executor.migrate(target) + return executor.loader.project_state(target).apps + + def migrate_to_latest(): + executor = MigrationExecutor(connection) + executor.loader.build_graph() + executor.migrate(executor.loader.graph.leaf_nodes()) + + try: + old_apps = migrate_to( + [ + ("employees", "0048_alter_employeeaction_action_and_more"), + ("requests_app", "0011_remove_requestcomment_model"), + ] + ) + Employee = old_apps.get_model("employees", "Employee") + OldRequest = old_apps.get_model("requests_app", "Request") + OldEmployeeAction = old_apps.get_model("employees", "EmployeeAction") + + employee = Employee.objects.create( + email="migration-employee@example.com", + phone_number="+79990000001", + first_name="Migration", + last_name="Employee", + is_active=True, + email_verified=True, + ) + approver = Employee.objects.create( + email="migration-approver@example.com", + phone_number="+79990000002", + first_name="Migration", + last_name="Approver", + is_active=True, + email_verified=True, + ) + request = OldRequest.objects.create( + employee=employee, + approver=approver, + type="vacation", + status="approved", + date_from=date(2026, 4, 20), + date_to=date(2026, 4, 25), + ) + first = OldEmployeeAction.objects.create( + employee=employee, + action="on_leave", + date=timezone.datetime( + 2026, + 4, + 20, + 12, + tzinfo=timezone.get_current_timezone(), + ), + comment="first", + extra={"request_id": request.id}, + ) + OldEmployeeAction.objects.create( + employee=employee, + action="on_leave", + date=timezone.datetime( + 2026, + 4, + 20, + 12, + tzinfo=timezone.get_current_timezone(), + ), + comment="duplicate", + extra={"request_id": request.id}, + ) + + migrate_to([("employees", "0049_employeeaction_source_request")]) + + actions = EmployeeAction.objects.filter( + source_request_id=request.id, + action="on_leave", + ) + assert actions.count() == 1 + assert actions.get().id == first.id + finally: + migrate_to_latest()