From 5a2a43bf51bc3c735a972c1e186183c701d0b897 Mon Sep 17 00:00:00 2001 From: tristan Date: Tue, 19 May 2026 11:07:38 +0200 Subject: [PATCH] refactor(leave) : address Task 3 review (helper, dead param, phase nature, regression test) - Extract private helper `exerciseYearForDate(date, isForfait)` to dedupe the date->leave-exercise-year expression duplicated across `clampYearToPhase` and `resolveFirstComputationYear` (4 copies collapsed into 1 helper + 4 call sites). - Remove the unused `ContractPhase $phase` parameter from `resolveLeavePeriodBounds`: the body never reads it (the phase cap is applied later by `resolvePeriodBounds`). - Add `ContractNature $contractNature` to `ContractPhase` DTO, populated from the first period of the group by `EmployeeContractPhaseResolver`. Drop the `resolveNatureForPhase` lookup in `EmployeeLeaveSummaryProvider` in favor of `$phase->contractNature`. Expose `contractNature` in `Employee::getContractPhases()` array shape for frontend use. - Fix regression for terminated employees calling `computeYearSummary` without an explicit phase (LeaveRecapRowBuilder, DumpVerificationSnapshotCommand). Before the refactor the period bounds, accrual end and taken end were NOT capped at the contract end for terminated employees, because `Employee::getCurrentContractEndDate()` returns null when no period covers "today". The new fallback phase (`isCurrent=false`, real `endDate`) was silently capping `to`. Add an internal `applyPhaseEndCap` flag, true when phase is explicit, false for legacy callers, threaded through `resolvePeriodBounds`, `resolveAccrualCalculationEndDate` and `resolveTakenCalculationEndDate`. - Add regression test `testTerminatedEmployeeWithoutExplicitPhaseSkipsPhaseEndCap` proving that legacy callers keep the natural exercise upper bound while explicit phase callers get the cap. - Add `contractNature` assertion in `EmployeeContractPhaseResolverTest`. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Dto/Contracts/ContractPhase.php | 2 + src/Entity/Employee.php | 20 ++-- .../EmployeeContractPhaseResolver.php | 1 + src/State/EmployeeLeaveSummaryProvider.php | 108 +++++++++--------- .../EmployeeContractPhaseResolverTest.php | 1 + .../EmployeeLeaveSummaryProviderTest.php | 62 ++++++++++ 6 files changed, 131 insertions(+), 63 deletions(-) diff --git a/src/Dto/Contracts/ContractPhase.php b/src/Dto/Contracts/ContractPhase.php index 2161449..0758642 100644 --- a/src/Dto/Contracts/ContractPhase.php +++ b/src/Dto/Contracts/ContractPhase.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace App\Dto\Contracts; +use App\Enum\ContractNature; use App\Enum\ContractType; use DateTimeImmutable; @@ -21,5 +22,6 @@ final readonly class ContractPhase public ?DateTimeImmutable $endDate, public array $periodIds, public bool $isCurrent, + public ContractNature $contractNature, ) {} } diff --git a/src/Entity/Employee.php b/src/Entity/Employee.php index 5461f65..e254f64 100644 --- a/src/Entity/Employee.php +++ b/src/Entity/Employee.php @@ -439,7 +439,8 @@ class Employee * startDate: string, * endDate: ?string, * periodIds: list, - * isCurrent: bool + * isCurrent: bool, + * contractNature: string * }> */ #[Groups(['employee:read'])] @@ -449,14 +450,15 @@ class Employee return array_map( static fn (ContractPhase $phase): array => [ - 'id' => $phase->id, - 'contractType' => $phase->contractType->value, - 'weeklyHours' => $phase->weeklyHours, - 'isDriver' => $phase->isDriver, - 'startDate' => $phase->startDate->format('Y-m-d'), - 'endDate' => $phase->endDate?->format('Y-m-d'), - 'periodIds' => $phase->periodIds, - 'isCurrent' => $phase->isCurrent, + 'id' => $phase->id, + 'contractType' => $phase->contractType->value, + 'weeklyHours' => $phase->weeklyHours, + 'isDriver' => $phase->isDriver, + 'startDate' => $phase->startDate->format('Y-m-d'), + 'endDate' => $phase->endDate?->format('Y-m-d'), + 'periodIds' => $phase->periodIds, + 'isCurrent' => $phase->isCurrent, + 'contractNature' => $phase->contractNature->value, ], $resolver->resolvePhases($this), ); diff --git a/src/Service/Contracts/EmployeeContractPhaseResolver.php b/src/Service/Contracts/EmployeeContractPhaseResolver.php index 5109905..b3001c5 100644 --- a/src/Service/Contracts/EmployeeContractPhaseResolver.php +++ b/src/Service/Contracts/EmployeeContractPhaseResolver.php @@ -78,6 +78,7 @@ final readonly class EmployeeContractPhaseResolver endDate: $endDate, periodIds: array_map(static fn (EmployeeContractPeriod $p): int => (int) $p->getId(), $group), isCurrent: $isCurrent, + contractNature: $first->getContractNatureEnum(), ); } } diff --git a/src/State/EmployeeLeaveSummaryProvider.php b/src/State/EmployeeLeaveSummaryProvider.php index dc0a53d..85749c1 100644 --- a/src/State/EmployeeLeaveSummaryProvider.php +++ b/src/State/EmployeeLeaveSummaryProvider.php @@ -173,6 +173,12 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface */ public function computeYearSummary(Employee $employee, int $targetYear, float $paidLeaveDays = 0.0, ?DateTimeImmutable $asOfDate = null, ?ContractPhase $phase = null): ?array { + // Track whether a phase was provided explicitly. When the caller supplies $phase, + // we apply the phase-end cap on period bounds. When we fall back to resolveCurrentPhase + // (legacy callers without phase awareness, e.g. LeaveRecapRowBuilder), we preserve + // the pre-phase-cap behavior to avoid changing observable results for terminated + // employees (the resolved fallback phase would otherwise unduly cap `to`). + $applyPhaseEndCap = null !== $phase; $phase ??= $this->resolveCurrentPhase($employee); if (null === $phase) { return null; @@ -188,7 +194,7 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface $targetSummary = null; for ($year = $firstYear; $year <= $targetYear; ++$year) { - [$from, $to] = $this->resolvePeriodBounds($employee, $year, $phase); + [$from, $to] = $this->resolvePeriodBounds($employee, $year, $phase, $applyPhaseEndCap); $leavePolicy = $this->resolveLeavePolicy($employee, $phase, $from, $to); if (null === $leavePolicy) { if ($year === $targetYear) { @@ -233,8 +239,8 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface } $effectiveAsOfDate = ($year === $targetYear) ? $asOfDate : null; - $accrualCalculationEnd = $this->resolveAccrualCalculationEndDate($leavePolicy['ruleCode'], $year, $to, $employee, $phase, $effectiveAsOfDate); - $takenCalculationEnd = $this->resolveTakenCalculationEndDate($to, $employee, $phase, $effectiveAsOfDate); + $accrualCalculationEnd = $this->resolveAccrualCalculationEndDate($leavePolicy['ruleCode'], $year, $to, $employee, $phase, $effectiveAsOfDate, $applyPhaseEndCap); + $takenCalculationEnd = $this->resolveTakenCalculationEndDate($to, $employee, $phase, $effectiveAsOfDate, $applyPhaseEndCap); $suspensions = $this->suspensionDaysCalculator->applyFirstMonthGrace( $this->resolveSuspensionsForPeriod($employee, $effectiveFrom, $to) ); @@ -474,21 +480,10 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface private function clampYearToPhase(int $year, ContractPhase $phase, bool $isForfait): int { - $firstYear = $isForfait - ? (int) $phase->startDate->format('Y') - : ((int) $phase->startDate->format('n') >= 6 - ? (int) $phase->startDate->format('Y') + 1 - : (int) $phase->startDate->format('Y')); - - $endDate = $phase->endDate; - $lastYear = null; - if ($endDate instanceof DateTimeImmutable) { - $lastYear = $isForfait - ? (int) $endDate->format('Y') - : ((int) $endDate->format('n') >= 6 - ? (int) $endDate->format('Y') + 1 - : (int) $endDate->format('Y')); - } + $firstYear = $this->exerciseYearForDate($phase->startDate, $isForfait); + $lastYear = $phase->endDate instanceof DateTimeImmutable + ? $this->exerciseYearForDate($phase->endDate, $isForfait) + : null; if ($year < $firstYear) { return $firstYear; @@ -500,6 +495,22 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface return $year; } + /** + * Map a date to the leave exercise year it belongs to. + * - Forfait: exercise = calendar year. + * - Non-forfait: exercise N runs from June (N-1) to May (N); dates in June-December + * map to N+1, January-May map to N. + */ + private function exerciseYearForDate(DateTimeImmutable $date, bool $isForfait): int + { + $year = (int) $date->format('Y'); + if ($isForfait) { + return $year; + } + + return (int) $date->format('n') >= 6 ? $year + 1 : $year; + } + private function resolveTargetPhase(Employee $employee): ContractPhase { $raw = $this->requestStack->getCurrentRequest()?->query->get('phaseId'); @@ -622,16 +633,18 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface DateTimeImmutable $periodEnd, Employee $employee, ContractPhase $phase, - ?DateTimeImmutable $asOfDate = null + ?DateTimeImmutable $asOfDate = null, + bool $applyPhaseEndCap = true ): ?DateTimeImmutable { $reference = $asOfDate ?? new DateTimeImmutable('today'); $currentYear = LeaveRuleCode::FORFAIT_218->value === $ruleCode ? (int) $reference->format('Y') : $this->resolveCurrentLeaveYear($reference); - // When viewing a closed phase, treat its end date as the reference cutoff: + // When viewing a closed phase explicitly, treat its end date as the reference cutoff: // accrual is bounded to the phase end, never running to "today". - if (!$phase->isCurrent && null !== $phase->endDate) { + // Legacy callers (no explicit phase) skip this cap to preserve pre-phase behavior. + if ($applyPhaseEndCap && !$phase->isCurrent && null !== $phase->endDate) { $end = $phase->endDate < $periodEnd ? $phase->endDate : $periodEnd; } elseif ($year < $currentYear) { $end = $periodEnd; @@ -648,7 +661,9 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface // Cap at contract end date if the employee has left (only meaningful when // viewing the current phase; closed phases are already capped above). - if ($phase->isCurrent) { + // Legacy callers (no explicit phase) always evaluate this branch to mimic + // the pre-phase behavior, which relied on getCurrentContractEndDate(). + if (!$applyPhaseEndCap || $phase->isCurrent) { $contractEndRaw = $employee->getCurrentContractEndDate(); if (null !== $end && null !== $contractEndRaw && '' !== trim($contractEndRaw)) { $contractEnd = $this->parseYmdDate($contractEndRaw); @@ -665,7 +680,8 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface DateTimeImmutable $periodEnd, Employee $employee, ContractPhase $phase, - ?DateTimeImmutable $asOfDate = null + ?DateTimeImmutable $asOfDate = null, + bool $applyPhaseEndCap = true ): ?DateTimeImmutable { $end = $periodEnd; @@ -674,11 +690,14 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface } // Closed phase: cap taken-absence accounting at the phase end. - if (!$phase->isCurrent && null !== $phase->endDate && $phase->endDate < $end) { + // Skip for legacy callers (no explicit phase) to preserve pre-phase behavior. + if ($applyPhaseEndCap && !$phase->isCurrent && null !== $phase->endDate && $phase->endDate < $end) { $end = $phase->endDate; } - if ($phase->isCurrent) { + // Legacy callers (no explicit phase) always use the live contract end date, + // mirroring the pre-phase implementation. + if (!$applyPhaseEndCap || $phase->isCurrent) { $contractEndRaw = $employee->getCurrentContractEndDate(); if (null !== $end && null !== $contractEndRaw && '' !== trim($contractEndRaw)) { $contractEnd = $this->parseYmdDate($contractEndRaw); @@ -733,8 +752,8 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface ]; } - // Resolve nature from the period defining the phase (use the phase's first period). - $nature = $this->resolveNatureForPhase($employee, $phase); + // Resolve nature directly from the phase DTO (populated by EmployeeContractPhaseResolver). + $nature = $phase->contractNature; if (ContractNature::CDI !== $nature && ContractNature::CDD !== $nature) { return null; } @@ -934,19 +953,21 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface /** * @return array{DateTimeImmutable, DateTimeImmutable} */ - private function resolvePeriodBounds(Employee $employee, int $year, ContractPhase $phase): array + private function resolvePeriodBounds(Employee $employee, int $year, ContractPhase $phase, bool $applyPhaseEndCap = true): array { if (ContractType::FORFAIT === $phase->contractType) { [$from, $to] = $this->resolveForfaitYearBounds($employee, $year, $phase); } else { - [$from, $to] = $this->resolveLeavePeriodBounds($year, $phase); + [$from, $to] = $this->resolveLeavePeriodBounds($year); } // Cap to the phase boundaries (applies to both modes). + // The end cap is skipped when the phase was not explicitly provided (legacy callers), + // to preserve pre-phase-cap behavior for terminated employees. if ($phase->startDate > $from) { $from = $phase->startDate; } - if (null !== $phase->endDate && $phase->endDate < $to) { + if ($applyPhaseEndCap && null !== $phase->endDate && $phase->endDate < $to) { $to = $phase->endDate; } @@ -956,7 +977,7 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface /** * @return array{DateTimeImmutable, DateTimeImmutable} */ - private function resolveLeavePeriodBounds(int $leaveYear, ContractPhase $phase): array + private function resolveLeavePeriodBounds(int $leaveYear): array { // Exercice CP "2026" = du 1er juin 2025 au 31 mai 2026. $from = new DateTimeImmutable(sprintf('%d-06-01 00:00:00', $leaveYear - 1)); @@ -1020,11 +1041,7 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface : $this->resolveCurrentLeaveYear(new DateTimeImmutable('today')); // Do not go before the exercice containing $phase->startDate. - $phaseFirstYear = $isForfait - ? (int) $phase->startDate->format('Y') - : ((int) $phase->startDate->format('n') >= 6 - ? (int) $phase->startDate->format('Y') + 1 - : (int) $phase->startDate->format('Y')); + $phaseFirstYear = $this->exerciseYearForDate($phase->startDate, $isForfait); $history = $employee->getContractHistory(); if ([] === $history) { @@ -1049,11 +1066,7 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface return max($phaseFirstYear, $candidate); } - $firstYear = $isForfait - ? (int) $oldestStartDate->format('Y') - : ((int) $oldestStartDate->format('n') >= 6 - ? (int) $oldestStartDate->format('Y') + 1 - : (int) $oldestStartDate->format('Y')); + $firstYear = $this->exerciseYearForDate($oldestStartDate, $isForfait); $oldestBalanceYear = $this->leaveBalanceRepository->findEarliestYearForEmployee($employee); if (null !== $oldestBalanceYear && $oldestBalanceYear < $firstYear) { @@ -1063,19 +1076,6 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface return max($phaseFirstYear, $firstYear); } - private function resolveNatureForPhase(Employee $employee, ContractPhase $phase): ?ContractNature - { - // Find the period at the start of the phase to determine its nature. - foreach ($employee->getContractPeriods() as $period) { - if ((int) $period->getId() === $phase->id) { - return $period->getContractNatureEnum(); - } - } - - // Fallback: nature of the current period (legacy behavior). - return ContractNature::tryFrom($employee->getCurrentContractNature()); - } - private function parseYmdDate(string $value): ?DateTimeImmutable { $date = DateTimeImmutable::createFromFormat('!Y-m-d', trim($value)); diff --git a/tests/Service/Contracts/EmployeeContractPhaseResolverTest.php b/tests/Service/Contracts/EmployeeContractPhaseResolverTest.php index 746fde3..eed47d4 100644 --- a/tests/Service/Contracts/EmployeeContractPhaseResolverTest.php +++ b/tests/Service/Contracts/EmployeeContractPhaseResolverTest.php @@ -32,6 +32,7 @@ final class EmployeeContractPhaseResolverTest extends TestCase self::assertSame(ContractType::H39, $phases[0]->contractType); self::assertTrue($phases[0]->isCurrent); self::assertNull($phases[0]->endDate); + self::assertSame(ContractNature::CDI, $phases[0]->contractNature); } public function testThreeConsecutivePeriodsSameSignatureCollapseIntoSinglePhase(): void diff --git a/tests/State/EmployeeLeaveSummaryProviderTest.php b/tests/State/EmployeeLeaveSummaryProviderTest.php index f26583b..44a6e7f 100644 --- a/tests/State/EmployeeLeaveSummaryProviderTest.php +++ b/tests/State/EmployeeLeaveSummaryProviderTest.php @@ -241,10 +241,72 @@ final class EmployeeLeaveSummaryProviderTest extends TestCase self::assertSame(2026, $year); } + // ----------------------------------------------------------------------- + // Regression: terminated-employee path through `computeYearSummary` without + // an explicit phase (legacy callers: LeaveRecapRowBuilder, + // DumpVerificationSnapshotCommand). Before the phase-aware refactor, the + // period bounds were NOT capped at the contract end for terminated + // employees (because Employee::getCurrentContractEndDate() returns null + // when no period covers "today"). The new code resolves a fallback phase + // whose `isCurrent` is false, which would otherwise cap `to` at the phase + // end — a behavior change for legacy callers. The flag `applyPhaseEndCap` + // toggles this cap so legacy callers get the pre-refactor behavior. + // ----------------------------------------------------------------------- + + public function testTerminatedEmployeeWithoutExplicitPhaseSkipsPhaseEndCap(): void + { + // Terminated employee: H39 phase ending 2024-12-31 (well in the past). + $employee = $this->buildTerminatedEmployee('2020-06-01', '2024-12-31'); + $phases = new EmployeeContractPhaseResolver()->resolvePhases($employee); + self::assertCount(1, $phases); + $phase = $phases[0]; + self::assertFalse($phase->isCurrent, 'Sanity: terminated phase must not be flagged as current.'); + + $provider = $this->buildProvider([]); + + // applyPhaseEndCap=false → mimics legacy callers (no explicit phase): + // the upper bound MUST stay at the natural leave-year end (May 31). + [$fromLegacy, $toLegacy] = $this->invokePrivate($provider, 'resolvePeriodBounds', $employee, 2025, $phase, false); + self::assertSame('2024-06-01', $fromLegacy->format('Y-m-d')); + self::assertSame('2025-05-31', $toLegacy->format('Y-m-d')); + + // applyPhaseEndCap=true → explicit-phase callers get the cap at phase end. + [$fromCap, $toCap] = $this->invokePrivate($provider, 'resolvePeriodBounds', $employee, 2025, $phase, true); + self::assertSame('2024-06-01', $fromCap->format('Y-m-d')); + self::assertSame('2024-12-31', $toCap->format('Y-m-d')); + } + // ----------------------------------------------------------------------- // Test harness helpers. // ----------------------------------------------------------------------- + /** + * Build a terminated-employee fixture: a single H39 period ending before today. + */ + private function buildTerminatedEmployee(string $start, string $end): Employee + { + $employee = new Employee(); + $this->setEntityId($employee, 2); + + $contract = new Contract(); + $contract->setName('39H'); + $contract->setTrackingMode(TrackingMode::TIME->value); + $contract->setWeeklyHours(39); + + $period = new EmployeeContractPeriod(); + $this->setEntityId($period, 10); + $period->setEmployee($employee); + $period->setContract($contract); + $period->setStartDate(new DateTimeImmutable($start)); + $period->setEndDate(new DateTimeImmutable($end)); + $period->setContractNature(ContractNature::CDI); + $period->setIsDriver(false); + + $employee->getContractPeriods()->add($period); + + return $employee; + } + /** * Build a two-period employee transitioning from H39 to FORFAIT. */