Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Adjust server-side timers when resuming an execution #2438

Merged

Conversation

hectoras
Copy link
Contributor

@hectoras hectoras commented Jan 15, 2024

Associated Jira issue: TR-5911

Changelog:

  • Makes the ConcurringSessionService to store the timestamp when a test is suspended because another one has been launched.
  • Introduces a method to adjust the timers for a test based on the current timestamp and the timestamp when the test was paused
    • This method is called by the LTI Delivery Provider when a paused test is resumed (here)
    • It tried to use the suspension timestamp if available; otherwise, it uses the highest timestamp associated with an item
    • The suspension timestamp is stored as part of the test session in the same way as the suspension reason was stored
  • Fixes a bug when handling the item duration cache, as the old code assumed item durations would only be updated for the currently-running test.
    • If the test is not running, getCurrentRouteItem() may be null

@hectoras hectoras changed the title feat: Adjust server timers when resuming an execution feat: Adjust server-side timers when resuming an execution Jan 15, 2024
@hectoras hectoras force-pushed the feat/TR-5911-adjust-server-timers-when-resuming-an-execution branch from e225c2e to 8da35ae Compare January 16, 2024 14:13
Copy link

github-actions bot commented Jan 16, 2024

Front-end summary Node 18

💯 Total ✅ Passed ⏭️ Skipped ❌ Failed
708 708 0 0

model/Service/ConcurringSessionService.php Outdated Show resolved Hide resolved
model/Service/ConcurringSessionService.php Outdated Show resolved Hide resolved
model/Service/ConcurringSessionService.php Outdated Show resolved Hide resolved
model/Service/ConcurringSessionService.php Outdated Show resolved Hide resolved
Comment on lines 200 to 202
$timestamp = $timer->getLastTimestamp(
$testSession->getItemTags($item)
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Optional]

Suggested change
$timestamp = $timer->getLastTimestamp(
$testSession->getItemTags($item)
);
$timestamp = $timer->getLastTimestamp($testSession->getItemTags($item));

model/Service/ConcurringSessionService.php Outdated Show resolved Hide resolved
$sources = [
$routeItem->getAssessmentTest(),
$this->getAssessmentTest(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure, that we can use $this (TestSession) instead of RoutItem ?

Copy link
Contributor Author

@hectoras hectoras Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solves a bug: If this ends up being called on a test (lets call it test B) other than the one we are current running (lets call it test A), $routeItem is null. That is, getCurrentRouteItem() returns null when called from a request associated with the test runner session for test A to get the current item for test B.

    protected function getCurrentRouteItem()
    {
        if ($this->isRunning() === true) {
            return $this->getRoute()->current();
        }

        return false;
    }

The test we are pausing (B) is / was running, but not in the current request (which is "bound" to A), and we need this to work also in that case (so the elapsed time for the item of test B is stored when suspending the test).

Both $routeItem->getAssessmentItemRef() and $this->getAssessmentTest() return an instance of AssessmentTest that refers to the same test.

Copy link
Contributor

@jsconan jsconan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested the feature and it looks to work well. Additional checks would be needed at QA side to validate that all is working seamlessly without impacting the other places.

I only have one concern regarding the added logs, which are great but too verbose IMO. I'd rather advise reducing this by moving them to a lower level (trace or debug).

Review Checklist

  • New code is covered by tests (if applicable)
  • Tests are running successfully (old and new ones) on my local machine (if applicable)
  • New code respects code style rules
  • New code respects best practices
  • New code is not subject to concurrency issues (if applicable)
  • Feature is working correctly on my local machine (if applicable)
  • Acceptance criteria are respected
  • Pull request title and description are meaningful
  • Pull request's target is not master
  • Commits are following conventional commits
  • Commits messages are meaningful
  • Commits are atomic

@hectoras hectoras requested a review from shpran January 17, 2024 16:17
Copy link

Version

Target Version 48.3.0
Last version 48.2.1

There are 0 BREAKING CHANGE, 1 feature, 5 fixes

@hectoras
Copy link
Contributor Author

These changes passed QA, therefore I'm merging this PR.

@hectoras hectoras merged commit d66b9e5 into develop Jan 25, 2024
5 of 6 checks passed
@hectoras hectoras deleted the feat/TR-5911-adjust-server-timers-when-resuming-an-execution branch January 25, 2024 14:55
@hectoras
Copy link
Contributor Author

Released as v48.3.0.

hectoras added a commit that referenced this pull request Jan 29, 2024
* feat: Adjust server timers when resuming an execution

* chore: Get the former order for duration cache updates

* fix: Explicitly provide the component to apply the adjustment in

* chore: Revert to the simpler approach applying the adjustment tot he test session as a whole

* fix: Store the pause timestamp associated to all execution IDs

* chore: Coding style improvements

* chore: Use a lower log level for debug messages in ConcurringSessionService

* fix: Don't try to update the duration cache for non-available sources

* fix: Pass the execution user ID when fetching the service context

* fix: Store the item duration instead of the current timestamp on test suspension

* test: Update unit tests

---------

Signed-off-by: Héctor Arroyo <[email protected]>
Co-authored-by: Andrei Shapiro <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants