From 61c0cbc887cfa4e494d00a490a8e2aad5f056b9b Mon Sep 17 00:00:00 2001
From: Kevin Papst <kevinpapst@users.noreply.github.com>
Date: Tue, 27 Oct 2020 18:35:22 +0100
Subject: [PATCH] fix auto-stop when starting timesheet with tags (#2067)

---
 src/Repository/TimesheetRepository.php   | 16 ++++++
 src/Timesheet/TimesheetService.php       | 66 +++++++++++++-----------
 tests/Timesheet/TimesheetServiceTest.php |  6 ++-
 3 files changed, 55 insertions(+), 33 deletions(-)

diff --git a/src/Repository/TimesheetRepository.php b/src/Repository/TimesheetRepository.php
index d73716c7..74f64ac3 100644
--- a/src/Repository/TimesheetRepository.php
+++ b/src/Repository/TimesheetRepository.php
@@ -120,6 +120,22 @@ class TimesheetRepository extends EntityRepository
         }
     }
 
+    public function begin()
+    {
+        $this->getEntityManager()->beginTransaction();
+    }
+
+    public function commit()
+    {
+        $this->getEntityManager()->flush();
+        $this->getEntityManager()->commit();
+    }
+
+    public function rollback()
+    {
+        $this->getEntityManager()->rollback();
+    }
+
     /**
      * @param Timesheet $timesheet
      * @throws \Doctrine\ORM\ORMException
diff --git a/src/Timesheet/TimesheetService.php b/src/Timesheet/TimesheetService.php
index 38a988d5..801d98e9 100644
--- a/src/Timesheet/TimesheetService.php
+++ b/src/Timesheet/TimesheetService.php
@@ -145,18 +145,25 @@ final class TimesheetService
             throw new AccessDeniedException('You are not allowed to start this timesheet record');
         }
 
-        $this->validateTimesheet($timesheet);
-
+        $this->repository->begin();
         try {
-            $this->stopActiveEntries($timesheet);
-        } catch (ValidationFailedException $vex) {
-            // could happen for timesheets that were started in the future (end before begin)
-            throw new ValidationFailedException($vex->getViolations(), 'Cannot stop running timesheet');
-        }
+            $this->validateTimesheet($timesheet);
 
-        $this->dispatcher->dispatch(new TimesheetCreatePreEvent($timesheet));
-        $this->repository->save($timesheet);
-        $this->dispatcher->dispatch(new TimesheetCreatePostEvent($timesheet));
+            $this->dispatcher->dispatch(new TimesheetCreatePreEvent($timesheet));
+            $this->repository->save($timesheet);
+            $this->dispatcher->dispatch(new TimesheetCreatePostEvent($timesheet));
+
+            try {
+                $this->stopActiveEntries($timesheet);
+            } catch (ValidationFailedException $vex) {
+                // could happen for timesheets that were started in the future (end before begin)
+                throw new ValidationFailedException($vex->getViolations(), 'Cannot stop running timesheet');
+            }
+            $this->repository->commit();
+        } catch (\Exception $ex) {
+            $this->repository->rollback();
+            throw $ex;
+        }
 
         return $timesheet;
     }
@@ -170,6 +177,7 @@ final class TimesheetService
      */
     public function updateTimesheet(Timesheet $timesheet): Timesheet
     {
+        // FIXME stop active entries upon update
         // there is at least one edge case which leads to a problem:
         // if you do not allow overlapping entries, you cannot restart a timesheet by removing the
         // end date if another timesheet is running, because the check for existing timesheets will always trigger
@@ -187,7 +195,7 @@ final class TimesheetService
     }
 
     /**
-     * Does NOT validate the given timesheet!
+     * Does NOT validate the given timesheets!
      *
      * @param array $timesheets
      * @return array
@@ -256,7 +264,9 @@ final class TimesheetService
     }
 
     /**
-     * Stops all active records for the current user, besides the given $timesheet.
+     * Stops active records if more than allowed are running for the timesheet user.
+     *
+     * The given $timesheet will be ignored and not stopped (assuming it is the latest one that was re-started).
      *
      * @param Timesheet $timesheet
      * @return int
@@ -265,28 +275,22 @@ final class TimesheetService
      */
     private function stopActiveEntries(Timesheet $timesheet): int
     {
-        $user = $timesheet->getUser();
         $hardLimit = $this->configuration->getActiveEntriesHardLimit();
-        $activeEntries = $this->repository->getActiveEntries($user);
+        $activeEntries = $this->repository->getActiveEntries($timesheet->getUser());
+
+        if (empty($activeEntries)) {
+            return 0;
+        }
+
+        $activeEntries = array_reverse($activeEntries);
+        $needsStop = \count($activeEntries) - $hardLimit;
         $counter = 0;
 
-        // reduce limit by one:
-        // this method is only called when a new entry is started
-        // -> all entries, including the new one must not exceed the $limit
-        $limit = $hardLimit - 1;
-
-        if (\count($activeEntries) > $limit) {
-            $i = 1;
-            foreach ($activeEntries as $activeEntry) {
-                if ($i > $limit && $timesheet->getId() !== $activeEntry->getId()) {
-                    if ($hardLimit > 1) {
-                        throw new ValidationException('timesheet.start.exceeded_limit');
-                    }
-
-                    $this->stopTimesheet($activeEntry);
-                    $counter++;
-                }
-                $i++;
+        foreach ($activeEntries as $activeEntry) {
+            if ($timesheet->getId() !== $activeEntry->getId() && $needsStop > 0) {
+                $this->stopTimesheet($activeEntry);
+                $needsStop--;
+                $counter++;
             }
         }
 
diff --git a/tests/Timesheet/TimesheetServiceTest.php b/tests/Timesheet/TimesheetServiceTest.php
index ab64da47..47cc2dda 100644
--- a/tests/Timesheet/TimesheetServiceTest.php
+++ b/tests/Timesheet/TimesheetServiceTest.php
@@ -128,12 +128,14 @@ class TimesheetServiceTest extends TestCase
         $timesheet2->expects($this->once())->method('setBegin');
         $timesheet2->expects($this->once())->method('setEnd');
 
+        $newTimesheet = new Timesheet();
+
         $repository = $this->createMock(TimesheetRepository::class);
-        $repository->method('getActiveEntries')->willReturn([$timesheet1, $timesheet2]);
+        $repository->method('getActiveEntries')->willReturn([$newTimesheet, $timesheet1, $timesheet2]);
 
         $sut = $this->getSut($authorizationChecker, null, null, $repository);
 
-        $sut->saveNewTimesheet(new Timesheet());
+        $sut->saveNewTimesheet($newTimesheet);
     }
 
     public function testCannotRestartedPersistedTimesheet()