MALIO-DEV/SIRH
4
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases 67 Wiki Activity

refactor(leave) : address Task 3 review (helper, dead param, phase nature, regression test)

Browse Source 
- 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) <noreply@anthropic.com>
This commit is contained in:
Autin
2026-05-19 11:07:38 +02:00
parent 9efe0e81a0
commit 5a2a43bf51
6 changed files with 131 additions and 63 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
src/Dto/Contracts/ContractPhase.php
View File

@@ -4,6 +4,7 @@ declare(strict_types=1);
namespace App\Dto\Contracts; namespace App\Dto\Contracts;
use App\Enum\ContractNature;
use App\Enum\ContractType; use App\Enum\ContractType;
use DateTimeImmutable; use DateTimeImmutable;
@@ -21,5 +22,6 @@ final readonly class ContractPhase
public ?DateTimeImmutable $endDate, public ?DateTimeImmutable $endDate,
public array $periodIds, public array $periodIds,
public bool $isCurrent, public bool $isCurrent,
public ContractNature $contractNature,
) {} ) {}
} }

20
src/Entity/Employee.php
View File

@@ -439,7 +439,8 @@ class Employee
* startDate: string, * startDate: string,
* endDate: ?string, * endDate: ?string,
* periodIds: list<int>, * periodIds: list<int>,
* isCurrent: bool * isCurrent: bool,
* contractNature: string
* }> * }>
*/ */
#[Groups(['employee:read'])] #[Groups(['employee:read'])]
@@ -449,14 +450,15 @@ class Employee
return array_map( return array_map(
static fn (ContractPhase $phase): array => [ static fn (ContractPhase $phase): array => [
'id' => $phase->id, 'id' => $phase->id,
'contractType' => $phase->contractType->value, 'contractType' => $phase->contractType->value,
'weeklyHours' => $phase->weeklyHours, 'weeklyHours' => $phase->weeklyHours,
'isDriver' => $phase->isDriver, 'isDriver' => $phase->isDriver,
'startDate' => $phase->startDate->format('Y-m-d'), 'startDate' => $phase->startDate->format('Y-m-d'),
'endDate' => $phase->endDate?->format('Y-m-d'), 'endDate' => $phase->endDate?->format('Y-m-d'),
'periodIds' => $phase->periodIds, 'periodIds' => $phase->periodIds,
'isCurrent' => $phase->isCurrent, 'isCurrent' => $phase->isCurrent,
'contractNature' => $phase->contractNature->value,
], ],
$resolver->resolvePhases($this), $resolver->resolvePhases($this),
); );

1
src/Service/Contracts/EmployeeContractPhaseResolver.php
View File

@@ -78,6 +78,7 @@ final readonly class EmployeeContractPhaseResolver
endDate: $endDate, endDate: $endDate,
periodIds: array_map(static fn (EmployeeContractPeriod $p): int => (int) $p->getId(), $group), periodIds: array_map(static fn (EmployeeContractPeriod $p): int => (int) $p->getId(), $group),
isCurrent: $isCurrent, isCurrent: $isCurrent,
contractNature: $first->getContractNatureEnum(),
); );
} }
} }

108
src/State/EmployeeLeaveSummaryProvider.php
View File

@@ -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 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); $phase ??= $this->resolveCurrentPhase($employee);
if (null === $phase) { if (null === $phase) {
return null; return null;
@@ -188,7 +194,7 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface
$targetSummary = null; $targetSummary = null;
for ($year = $firstYear; $year <= $targetYear; ++$year) { 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); $leavePolicy = $this->resolveLeavePolicy($employee, $phase, $from, $to);
if (null === $leavePolicy) { if (null === $leavePolicy) {
if ($year === $targetYear) { if ($year === $targetYear) {
@@ -233,8 +239,8 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface
} }
$effectiveAsOfDate = ($year === $targetYear) ? $asOfDate : null; $effectiveAsOfDate = ($year === $targetYear) ? $asOfDate : null;
$accrualCalculationEnd = $this->resolveAccrualCalculationEndDate($leavePolicy['ruleCode'], $year, $to, $employee, $phase, $effectiveAsOfDate); $accrualCalculationEnd = $this->resolveAccrualCalculationEndDate($leavePolicy['ruleCode'], $year, $to, $employee, $phase, $effectiveAsOfDate, $applyPhaseEndCap);
$takenCalculationEnd = $this->resolveTakenCalculationEndDate($to, $employee, $phase, $effectiveAsOfDate); $takenCalculationEnd = $this->resolveTakenCalculationEndDate($to, $employee, $phase, $effectiveAsOfDate, $applyPhaseEndCap);
$suspensions = $this->suspensionDaysCalculator->applyFirstMonthGrace( $suspensions = $this->suspensionDaysCalculator->applyFirstMonthGrace(
$this->resolveSuspensionsForPeriod($employee, $effectiveFrom, $to) $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 private function clampYearToPhase(int $year, ContractPhase $phase, bool $isForfait): int
{ {
$firstYear = $isForfait $firstYear = $this->exerciseYearForDate($phase->startDate, $isForfait);
? (int) $phase->startDate->format('Y') $lastYear = $phase->endDate instanceof DateTimeImmutable
: ((int) $phase->startDate->format('n') >= 6 ? $this->exerciseYearForDate($phase->endDate, $isForfait)
? (int) $phase->startDate->format('Y') + 1 : null;
: (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'));
}
if ($year < $firstYear) { if ($year < $firstYear) {
return $firstYear; return $firstYear;
@@ -500,6 +495,22 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface
return $year; 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 private function resolveTargetPhase(Employee $employee): ContractPhase
{ {
$raw = $this->requestStack->getCurrentRequest()?->query->get('phaseId'); $raw = $this->requestStack->getCurrentRequest()?->query->get('phaseId');
@@ -622,16 +633,18 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface
DateTimeImmutable $periodEnd, DateTimeImmutable $periodEnd,
Employee $employee, Employee $employee,
ContractPhase $phase, ContractPhase $phase,
?DateTimeImmutable $asOfDate = null ?DateTimeImmutable $asOfDate = null,
bool $applyPhaseEndCap = true
): ?DateTimeImmutable { ): ?DateTimeImmutable {
$reference = $asOfDate ?? new DateTimeImmutable('today'); $reference = $asOfDate ?? new DateTimeImmutable('today');
$currentYear = LeaveRuleCode::FORFAIT_218->value === $ruleCode $currentYear = LeaveRuleCode::FORFAIT_218->value === $ruleCode
? (int) $reference->format('Y') ? (int) $reference->format('Y')
: $this->resolveCurrentLeaveYear($reference); : $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". // 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; $end = $phase->endDate < $periodEnd ? $phase->endDate : $periodEnd;
} elseif ($year < $currentYear) { } elseif ($year < $currentYear) {
$end = $periodEnd; $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 // Cap at contract end date if the employee has left (only meaningful when
// viewing the current phase; closed phases are already capped above). // 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(); $contractEndRaw = $employee->getCurrentContractEndDate();
if (null !== $end && null !== $contractEndRaw && '' !== trim($contractEndRaw)) { if (null !== $end && null !== $contractEndRaw && '' !== trim($contractEndRaw)) {
$contractEnd = $this->parseYmdDate($contractEndRaw); $contractEnd = $this->parseYmdDate($contractEndRaw);
@@ -665,7 +680,8 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface
DateTimeImmutable $periodEnd, DateTimeImmutable $periodEnd,
Employee $employee, Employee $employee,
ContractPhase $phase, ContractPhase $phase,
?DateTimeImmutable $asOfDate = null ?DateTimeImmutable $asOfDate = null,
bool $applyPhaseEndCap = true
): ?DateTimeImmutable { ): ?DateTimeImmutable {
$end = $periodEnd; $end = $periodEnd;
@@ -674,11 +690,14 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface
} }
// Closed phase: cap taken-absence accounting at the phase end. // 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; $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(); $contractEndRaw = $employee->getCurrentContractEndDate();
if (null !== $end && null !== $contractEndRaw && '' !== trim($contractEndRaw)) { if (null !== $end && null !== $contractEndRaw && '' !== trim($contractEndRaw)) {
$contractEnd = $this->parseYmdDate($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). // Resolve nature directly from the phase DTO (populated by EmployeeContractPhaseResolver).
$nature = $this->resolveNatureForPhase($employee, $phase); $nature = $phase->contractNature;
if (ContractNature::CDI !== $nature && ContractNature::CDD !== $nature) { if (ContractNature::CDI !== $nature && ContractNature::CDD !== $nature) {
return null; return null;
} }
@@ -934,19 +953,21 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface
/** /**
* @return array{DateTimeImmutable, DateTimeImmutable} * @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) { if (ContractType::FORFAIT === $phase->contractType) {
[$from, $to] = $this->resolveForfaitYearBounds($employee, $year, $phase); [$from, $to] = $this->resolveForfaitYearBounds($employee, $year, $phase);
} else { } else {
[$from, $to] = $this->resolveLeavePeriodBounds($year, $phase); [$from, $to] = $this->resolveLeavePeriodBounds($year);
} }
// Cap to the phase boundaries (applies to both modes). // 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) { if ($phase->startDate > $from) {
$from = $phase->startDate; $from = $phase->startDate;
} }
if (null !== $phase->endDate && $phase->endDate < $to) { if ($applyPhaseEndCap && null !== $phase->endDate && $phase->endDate < $to) {
$to = $phase->endDate; $to = $phase->endDate;
} }
@@ -956,7 +977,7 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface
/** /**
* @return array{DateTimeImmutable, DateTimeImmutable} * @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. // Exercice CP "2026" = du 1er juin 2025 au 31 mai 2026.
$from = new DateTimeImmutable(sprintf('%d-06-01 00:00:00', $leaveYear - 1)); $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')); : $this->resolveCurrentLeaveYear(new DateTimeImmutable('today'));
// Do not go before the exercice containing $phase->startDate. // Do not go before the exercice containing $phase->startDate.
$phaseFirstYear = $isForfait $phaseFirstYear = $this->exerciseYearForDate($phase->startDate, $isForfait);
? (int) $phase->startDate->format('Y')
: ((int) $phase->startDate->format('n') >= 6
? (int) $phase->startDate->format('Y') + 1
: (int) $phase->startDate->format('Y'));
$history = $employee->getContractHistory(); $history = $employee->getContractHistory();
if ([] === $history) { if ([] === $history) {
@@ -1049,11 +1066,7 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface
return max($phaseFirstYear, $candidate); return max($phaseFirstYear, $candidate);
} }
$firstYear = $isForfait $firstYear = $this->exerciseYearForDate($oldestStartDate, $isForfait);
? (int) $oldestStartDate->format('Y')
: ((int) $oldestStartDate->format('n') >= 6
? (int) $oldestStartDate->format('Y') + 1
: (int) $oldestStartDate->format('Y'));
$oldestBalanceYear = $this->leaveBalanceRepository->findEarliestYearForEmployee($employee); $oldestBalanceYear = $this->leaveBalanceRepository->findEarliestYearForEmployee($employee);
if (null !== $oldestBalanceYear && $oldestBalanceYear < $firstYear) { if (null !== $oldestBalanceYear && $oldestBalanceYear < $firstYear) {
@@ -1063,19 +1076,6 @@ final readonly class EmployeeLeaveSummaryProvider implements ProviderInterface
return max($phaseFirstYear, $firstYear); 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 private function parseYmdDate(string $value): ?DateTimeImmutable
{ {
$date = DateTimeImmutable::createFromFormat('!Y-m-d', trim($value)); $date = DateTimeImmutable::createFromFormat('!Y-m-d', trim($value));

1
tests/Service/Contracts/EmployeeContractPhaseResolverTest.php
View File

@@ -32,6 +32,7 @@ final class EmployeeContractPhaseResolverTest extends TestCase
self::assertSame(ContractType::H39, $phases[0]->contractType); self::assertSame(ContractType::H39, $phases[0]->contractType);
self::assertTrue($phases[0]->isCurrent); self::assertTrue($phases[0]->isCurrent);
self::assertNull($phases[0]->endDate); self::assertNull($phases[0]->endDate);
self::assertSame(ContractNature::CDI, $phases[0]->contractNature);
} }
public function testThreeConsecutivePeriodsSameSignatureCollapseIntoSinglePhase(): void public function testThreeConsecutivePeriodsSameSignatureCollapseIntoSinglePhase(): void

62
tests/State/EmployeeLeaveSummaryProviderTest.php
View File

@@ -241,10 +241,72 @@ final class EmployeeLeaveSummaryProviderTest extends TestCase
self::assertSame(2026, $year); 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. // 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. * Build a two-period employee transitioning from H39 to FORFAIT.
*/ */