fix: Don't fetch the current item for a null test session (#2469)

* fix: Don't fetch the current item for a null test session * chore: Use DI to fetch services from ConcurringSessionService * chore: Fix tests (cherry picked from commit c22ae07)
oat-sa · Jun 10, 2024 · e579937 · e579937
1 parent 1508e93
commit e579937
Show file tree

Hide file tree

Showing 3 changed files with 146 additions and 31 deletions.
diff --git a/model/Container/TestQtiServiceProvider.php b/model/Container/TestQtiServiceProvider.php
@@ -15,9 +15,7 @@
  * along with this program; if not, write to the Free Software
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  *
- * Copyright (c) 2021-2023 (original work) Open Assessment Technologies SA;
- *
- * @author Ricardo Quintanilha <[email protected]>
+ * Copyright (c) 2021-2024 (original work) Open Assessment Technologies SA;
  */
 
 declare(strict_types=1);
@@ -47,7 +45,9 @@
 use oat\taoQtiTest\model\Service\StoreTraceVariablesService;
 use oat\taoQtiTest\model\Service\TimeoutService;
 use oat\taoQtiTest\models\runner\QtiRunnerService;
+use oat\taoQtiTest\models\runner\time\TimerAdjustmentServiceInterface;
 use oat\taoQtiTest\models\TestModelService;
+use oat\taoQtiTest\models\TestSessionService;
 use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;
 
 use function Symfony\Component\DependencyInjection\Loader\Configurator\service;
@@ -172,6 +172,8 @@ public function __invoke(ContainerConfigurator $configurator): void
                     service(DeliveryExecutionService::SERVICE_ID),
                     service(FeatureFlagChecker::class),
                     service(StateServiceInterface::SERVICE_ID),
+                    service(TestSessionService::SERVICE_ID),
+                    service(TimerAdjustmentServiceInterface::SERVICE_ID),
                 ]
             );
     }

diff --git a/model/Service/ConcurringSessionService.php b/model/Service/ConcurringSessionService.php
@@ -23,7 +23,6 @@
 namespace oat\taoQtiTest\model\Service;
 
 use common_Exception;
-use oat\oatbox\service\ServiceManager;
 use oat\tao\model\featureFlag\FeatureFlagCheckerInterface;
 use oat\taoDelivery\model\execution\DeliveryExecution;
 use oat\taoDelivery\model\execution\DeliveryExecutionInterface;
@@ -33,7 +32,7 @@
 use oat\taoQtiTest\models\container\QtiTestDeliveryContainer;
 use oat\taoQtiTest\models\runner\QtiRunnerService;
 use oat\taoQtiTest\models\runner\QtiRunnerServiceContext;
-use oat\taoQtiTest\models\runner\time\TimerAdjustmentService;
+use oat\taoQtiTest\models\runner\time\TimerAdjustmentServiceInterface;
 use oat\taoQtiTest\models\TestSessionService;
 use PHPSession;
 use Psr\Log\LoggerInterface;
@@ -44,6 +43,7 @@
 class ConcurringSessionService
 {
     private const PAUSE_REASON_CONCURRENT_TEST = 'PAUSE_REASON_CONCURRENT_TEST';
+
     /**
      * @var string Controls whether a delivery execution state should be kept as is or reset each time it starts.
      *             `false` – the state will be reset on each restart.
@@ -58,6 +58,8 @@ class ConcurringSessionService
     private DeliveryExecutionService $deliveryExecutionService;
     private FeatureFlagCheckerInterface $featureFlagChecker;
     private StateServiceInterface $stateService;
+    private TestSessionService $testSessionService;
+    private TimerAdjustmentServiceInterface $timerAdjustmentService;
     private ?PHPSession $currentSession;
 
     public function __construct(
@@ -67,15 +69,19 @@ public function __construct(
         DeliveryExecutionService $deliveryExecutionService,
         FeatureFlagCheckerInterface $featureFlagChecker,
         StateServiceInterface $stateService,
+        TestSessionService $testSessionService,
+        TimerAdjustmentServiceInterface $timerAdjustmentService,
         PHPSession $currentSession = null
     ) {
         $this->logger = $logger;
         $this->qtiRunnerService = $qtiRunnerService;
         $this->runtimeService = $runtimeService;
         $this->deliveryExecutionService = $deliveryExecutionService;
         $this->featureFlagChecker = $featureFlagChecker;
-        $this->currentSession = $currentSession ?? PHPSession::singleton();
         $this->stateService = $stateService;
+        $this->testSessionService = $testSessionService;
+        $this->timerAdjustmentService = $timerAdjustmentService;
+        $this->currentSession = $currentSession ?? PHPSession::singleton();
     }
 
     public function pauseActiveDeliveryExecutionsForUser($activeExecution): void
@@ -166,9 +172,9 @@ public function adjustTimers(DeliveryExecution $execution): void
             )
         );
 
-        $testSession = $this->getTestSessionService()->getTestSession($execution);
+        $testSession = $this->testSessionService->getTestSession($execution);
 
-        if ($testSession->getCurrentAssessmentItemRef()) {
+        if ($testSession && $testSession->getCurrentAssessmentItemRef()) {
             $duration = $testSession->getTimerDuration(
                 $testSession->getCurrentAssessmentItemRef()->getIdentifier(),
                 $testSession->getTimerTarget()
@@ -182,10 +188,10 @@ public function adjustTimers(DeliveryExecution $execution): void
                 )
             );
 
-            $ids = [
+            $ids = array_unique([
                 $execution->getIdentifier(),
                 $execution->getOriginalIdentifier()
-            ];
+            ]);
 
             foreach ($ids as $executionId) {
                 $key = "itemDuration-{$executionId}";
@@ -210,10 +216,14 @@ public function adjustTimers(DeliveryExecution $execution): void
                 if ($delta > 0) {
                     $this->logger->debug(sprintf('Adjusting timers by %d s', $delta));
 
-                    $this->getTimerAdjustmentService()->increase($testSession, $delta);
+                    $this->timerAdjustmentService->increase(
+                        $testSession,
+                        $delta,
+                        TimerAdjustmentServiceInterface::TYPE_TIME_ADJUSTMENT
+                    );
 
                     $testSession->suspend();
-                    $this->getTestSessionService()->persist($testSession);
+                    $this->testSessionService->persist($testSession);
                 }
             }
         }
@@ -297,21 +307,4 @@ private function getContextByDeliveryExecution(DeliveryExecutionInterface $execu
             $execution->getUserIdentifier()
         );
     }
-
-    private function getTimerAdjustmentService(): TimerAdjustmentService
-    {
-        /** @noinspection PhpIncompatibleReturnTypeInspection */
-        return $this->getServiceManager()->get(TimerAdjustmentService::SERVICE_ID);
-    }
-
-    private function getTestSessionService(): TestSessionService
-    {
-        /** @noinspection PhpIncompatibleReturnTypeInspection */
-        return $this->getServiceManager()->get(TestSessionService::SERVICE_ID);
-    }
-
-    private function getServiceManager()
-    {
-        return ServiceManager::getServiceManager();
-    }
 }
diff --git a/test/unit/model/Service/ConcurringSessionServiceTest.php b/test/unit/model/Service/ConcurringSessionServiceTest.php
@@ -15,7 +15,7 @@
  * along with this program; if not, write to the Free Software
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  *
- * Copyright (c) 2023 (original work) Open Assessment Technologies SA;
+ * Copyright (c) 2023-2024 (original work) Open Assessment Technologies SA.
  */
 
 declare(strict_types=1);
@@ -33,6 +33,8 @@
 use oat\taoQtiTest\models\runner\QtiRunnerService;
 use oat\taoQtiTest\models\runner\QtiRunnerServiceContext;
 use oat\taoQtiTest\models\runner\session\TestSession;
+use oat\taoQtiTest\models\runner\time\TimerAdjustmentServiceInterface;
+use oat\taoQtiTest\models\TestSessionService;
 use oat\taoTests\models\runner\time\TimePoint;
 use PHPSession;
 use PHPUnit\Framework\TestCase;
@@ -49,6 +51,8 @@ class ConcurringSessionServiceTest extends TestCase
     private PHPSession $currentSession;
     private ConcurringSessionService $subject;
     private StateServiceInterface $stateService;
+    private TestSessionService $testSessionService;
+    private TimerAdjustmentServiceInterface $timerAdjustmentService;
 
     protected function setUp(): void
     {
@@ -58,6 +62,8 @@ protected function setUp(): void
         $this->featureFlagChecker = $this->createMock(FeatureFlagCheckerInterface::class);
         $this->currentSession = $this->createMock(PHPSession::class);
         $this->stateService = $this->createMock(StateServiceInterface::class);
+        $this->testSessionService = $this->createMock(TestSessionService::class);
+        $this->timerAdjustmentService = $this->createMock(TimerAdjustmentServiceInterface::class);
 
         $this->subject = new ConcurringSessionService(
             $this->createMock(LoggerInterface::class),
@@ -66,6 +72,8 @@ protected function setUp(): void
             $this->deliveryExecutionService,
             $this->featureFlagChecker,
             $this->stateService,
+            $this->testSessionService,
+            $this->timerAdjustmentService,
             $this->currentSession
         );
     }
@@ -271,7 +279,7 @@ public function testPausesExecutionsForOtherDeliveries(): void
         $this->subject->pauseConcurrentSessions($execution);
     }
 
-    public function testPauseActiveDeliveryExecutionsForUser()
+    public function testPauseActiveDeliveryExecutionsForUser(): void
     {
         $this->featureFlagChecker
             ->method('isEnabled')
@@ -422,4 +430,116 @@ public function testPauseActiveDeliveryExecutionsForUser()
 
         $this->subject->pauseActiveDeliveryExecutionsForUser($execution);
     }
+
+    public function testAdjustTimersAddsAPositiveAdjustment(): void
+    {
+        $execution = $this->createMock(DeliveryExecution::class);
+        $execution
+            ->expects($this->atLeastOnce())
+            ->method('getIdentifier')
+            ->willReturn('https://example.com/execution/1');
+        $execution
+            ->expects($this->atLeastOnce())
+            ->method('getOriginalIdentifier')
+            ->willReturn('https://example.com/execution/1');
+
+        $itemRef = $this->createMock(AssessmentItemRef::class);
+        $itemRef
+            ->expects($this->once())
+            ->method('getIdentifier')
+            ->willReturn('itemRef');
+
+        $duration = $this->createMock(QtiDuration::class);
+        $duration
+            ->expects($this->atLeastOnce())
+            ->method('getSeconds')
+            ->with(true)
+            ->willReturn(4);
+
+        $testSession = $this->createMock(TestSession::class);
+        $testSession
+            ->expects($this->exactly(2))
+            ->method('getCurrentAssessmentItemRef')
+            ->willReturn($itemRef);
+        $testSession
+            ->expects($this->once())
+            ->method('getTimerTarget')
+            ->willReturn(TimePoint::TARGET_SERVER);
+        $testSession
+            ->expects($this->once())
+            ->method('getTimerDuration')
+            ->with('itemRef', TimePoint::TARGET_SERVER)
+            ->willReturn($duration);
+
+        $this->testSessionService
+            ->expects($this->once())
+            ->method('getTestSession')
+            ->with($execution)
+            ->willReturn($testSession);
+
+        $this->currentSession
+            ->expects($this->once())
+            ->method('hasAttribute')
+            ->with('itemDuration-https://example.com/execution/1')
+            ->willReturn(true);
+
+        $this->currentSession
+            ->expects($this->once())
+            ->method('getAttribute')
+            ->with('itemDuration-https://example.com/execution/1')
+            ->willReturn(1.5);
+
+        $this->currentSession
+            ->expects($this->once())
+            ->method('removeAttribute')
+            ->with('itemDuration-https://example.com/execution/1');
+
+        $this->timerAdjustmentService
+            ->expects($this->once())
+            ->method('increase')
+            ->with(
+                $testSession,
+                ceil(2.5),
+                TimerAdjustmentServiceInterface::TYPE_TIME_ADJUSTMENT
+            );
+
+        $testSession
+            ->expects($this->once())
+            ->method('suspend');
+
+        $this->testSessionService
+            ->expects($this->once())
+            ->method('persist')
+            ->with($testSession);
+
+        $this->subject->adjustTimers($execution);
+    }
+
+    public function testAdjustTimersSkipsAdjustmentIfNoTestSessionExists(): void
+    {
+        $execution = $this->createMock(DeliveryExecution::class);
+        $execution
+            ->method('getIdentifier')
+            ->willReturn('https://example.com/execution/1');
+
+        $this->testSessionService
+            ->expects($this->once())
+            ->method('getTestSession')
+            ->with($execution)
+            ->willReturn(null);
+
+        $this->currentSession
+            ->expects($this->never())
+            ->method('removeAttribute');
+
+        $this->timerAdjustmentService
+            ->expects($this->never())
+            ->method('increase');
+
+        $this->testSessionService
+            ->expects($this->never())
+            ->method('persist');
+
+        $this->subject->adjustTimers($execution);
+    }
 }