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: make the first item consider testPart pre-conditions #367

Merged
merged 26 commits into from
Oct 13, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
fe05368
feat: make the first item consider testPart pre-conditions
gabrielfs7 Sep 22, 2023
9437903
chore: ignore branch rule if item/section from one test part pointing…
shpran Sep 22, 2023
b472bb0
test: update unit tests
shpran Sep 22, 2023
7430ae7
feat: implement branchRules at testPart level.
bugalot Sep 22, 2023
a01223b
chore: add test part preconditions to item just one time
gabrielfs7 Sep 25, 2023
e5c592e
chore: rely on original branch rules instead of setting them to the l…
shpran Sep 25, 2023
c623ab4
refactor: move logic to get branch rules to Route class
shpran Sep 26, 2023
613f985
feat: create method to return effective preconditions
gabrielfs7 Sep 26, 2023
fd854bf
chore: make the test part and section pre-conditions to be applied in…
gabrielfs7 Sep 26, 2023
831e5ca
refactor: move branch selection to the RouteItem
shpran Sep 26, 2023
0c660a9
Merge branch 'feat/TR-5381/add-preconditions-from-test-parts' of http…
shpran Sep 26, 2023
d104236
chore: use last parent item section instead of currunt item subsection
shpran Sep 26, 2023
c2738e6
refactor: add methods to check if the item, subsection or section is …
shpran Sep 26, 2023
f5765cb
Merge pull request #370 from oat-sa/feat/rely-on-original-branch-rule…
shpran Sep 26, 2023
1bd8c24
chore: remove necessity to add pre-conditions while factoring route
gabrielfs7 Sep 26, 2023
24c9b35
chore: add unit test to cover pre-condition on test part and section
gabrielfs7 Sep 26, 2023
85f9d10
chore: simplify test and cover all cases on a single xml
gabrielfs7 Sep 27, 2023
0657936
test: add test to check branching rules on each test element: test pa…
shpran Sep 27, 2023
12bc4ed
Merge branch 'feat/TR-5381/add-preconditions-from-test-parts' of http…
shpran Sep 27, 2023
c4b6a1b
chore: add and class props to SectionPart, rely on them
shpran Sep 28, 2023
045e56c
chore: use while instead of foreach, rely on getParent method
shpran Sep 28, 2023
68bd42b
chore: remove comment
shpran Sep 28, 2023
41360e6
chore: use do-while
shpran Sep 28, 2023
5f1a8b2
feat: add specific method to test preconditions on session that can b…
gabrielfs7 Oct 5, 2023
97cfaf2
feat: handle test part preconditions when non-linear
gabrielfs7 Oct 9, 2023
a5b719a
feat: allow to branch to the same test part, do not ignore test part …
shpran Oct 11, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/qtism/data/AssessmentSection.php
Original file line number Diff line number Diff line change
Expand Up @@ -320,4 +320,11 @@ public function getComponents(): QtiComponentCollection

return new QtiComponentCollection($comp);
}

public function isLastSectionPart(SectionPart $sectionPart): bool
{
$sectionParts = $this->getSectionParts()->getArrayCopy();

return end($sectionParts) === $sectionPart;
}
}
7 changes: 7 additions & 0 deletions src/qtism/data/TestPart.php
Original file line number Diff line number Diff line change
Expand Up @@ -419,4 +419,11 @@ public function __clone()
{
$this->setObservers(new SplObjectStorage());
}

public function isLastSection(AssessmentSection $assessmentSection): bool
{
$sections = $this->getAssessmentSections()->getArrayCopy();

return end($sections) === $assessmentSection;
}
}
14 changes: 4 additions & 10 deletions src/qtism/runtime/tests/AbstractSessionManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
use qtism\data\IAssessmentItem;
use qtism\data\NavigationMode;
use qtism\data\SubmissionMode;
use qtism\data\TestPart;

/**
* The AbstractSessionManager class is a bed for instantiating
Expand Down Expand Up @@ -148,6 +149,7 @@ protected function createRoute(AssessmentTest $test): Route
{
$routeStack = [];

/** @var TestPart $testPart */
foreach ($test->getTestParts() as $testPart) {
$assessmentSectionStack = [];

Expand Down Expand Up @@ -192,16 +194,6 @@ protected function createRoute(AssessmentTest $test): Route
$route->appendRoute($r);
}

// Add to the last item of the selection the branch rules of the AssessmentSection/testPart
// on which the selection is applied... Only if the route contains something (empty assessmentSection edge case).
if ($route->count() > 0) {
$route->getLastRouteItem()->addBranchRules($current->getBranchRules());

// Do the same as for branch rules for pre conditions, except that they must be
// attached on the first item of the route.
$route->getFirstRouteItem()->addPreConditions($current->getPreConditions());
}

array_push($routeStack, $route);
array_pop($assessmentSectionStack);
} elseif ($current instanceof AssessmentItemRef) {
Expand All @@ -216,6 +208,8 @@ protected function createRoute(AssessmentTest $test): Route

$finalRoutes = $routeStack;
$route = new SelectableRoute();

/** @var SelectableRoute $finalRoute */
foreach ($finalRoutes as $finalRoute) {
$route->appendRoute($finalRoute);
}
Expand Down
22 changes: 14 additions & 8 deletions src/qtism/runtime/tests/AssessmentTestSession.php
Original file line number Diff line number Diff line change
Expand Up @@ -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) 2013-2020 (original work) Open Assessment Technologies SA (under the project TAO-PRODUCT);
* Copyright (c) 2013-2023 (original work) Open Assessment Technologies SA (under the project TAO-PRODUCT);
*
* @author Jérôme Bogaerts <[email protected]>
* @license GPLv2
Expand Down Expand Up @@ -2406,10 +2406,12 @@ protected function nextRouteItem($ignoreBranchings = false, $ignorePreConditions
$stop = false;

while ($route->valid() === true && $stop === false) {
hectoras marked this conversation as resolved.
Show resolved Hide resolved
$branchRules = $route->current()->getEffectiveBranchRules();
$numberOfBranchRules = $branchRules->count();

// Branchings?
if ($ignoreBranchings === false && count($route->current()->getBranchRules()) > 0 && $this->mustApplyBranchRules() === true) {
$branchRules = $route->current()->getBranchRules();
for ($i = 0; $i < count($branchRules); $i++) {
if ($ignoreBranchings === false && $numberOfBranchRules > 0 && $this->mustApplyBranchRules() === true) {
hectoras marked this conversation as resolved.
Show resolved Hide resolved
for ($i = 0; $i < $numberOfBranchRules; $i++) {
$engine = new ExpressionEngine($branchRules[$i]->getExpression(), $this);
$condition = $engine->process();
if ($condition !== null && $condition->getValue() === true) {
Expand Down Expand Up @@ -2438,17 +2440,21 @@ protected function nextRouteItem($ignoreBranchings = false, $ignorePreConditions
}

// Preconditions on target?
if ($ignorePreConditions === false && $route->valid() === true && ($preConditions = $route->current()->getPreConditions()) && count($preConditions) > 0 && $this->mustApplyPreConditions() === true) {
if ($ignorePreConditions === false && $route->valid() === true && ($preConditions = $route->current()->getEffectivePreConditions()) && count($preConditions) > 0 && $this->mustApplyPreConditions() === true) {
$preConditionFailed = false;

hectoras marked this conversation as resolved.
Show resolved Hide resolved
for ($i = 0; $i < count($preConditions); $i++) {
$engine = new ExpressionEngine($preConditions[$i]->getExpression(), $this);
$condition = $engine->process();

if ($condition !== null && $condition->getValue() === true) {
// The item must be presented.
$stop = true;
if ($condition === null || $condition->getValue() === false) {
// The item must NOT be presented.
$preConditionFailed = true;
break;
}
}

$stop = !$preConditionFailed;
} else {
$stop = true;
}
Expand Down
18 changes: 10 additions & 8 deletions src/qtism/runtime/tests/Route.php
Original file line number Diff line number Diff line change
Expand Up @@ -1117,10 +1117,11 @@ public function branch($identifier): void

if ($targetRouteItems[$occurence]->getTestPart() !== $this->current()->getTestPart()) {
// From IMS QTI:
// In case of an item or section, the target must refer to an item or section
// in the same testPart [...]
$msg = 'Branchings to items outside of the current testPart is forbidden by the QTI 2.1 specification.';
throw new OutOfBoundsException($msg);
Comment on lines -1122 to -1123
Copy link
Contributor

Choose a reason for hiding this comment

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

As per $msg, branching to items outside of the current testPart (in case of source is an item or a section) is forbidden. So we should not allow it.

Copy link
Contributor

@shpran shpran Sep 25, 2023

Choose a reason for hiding this comment

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

Yes. But instead of throwing an exception, we just ignore such branch rules and going to the next item (like there are no invalid branch rules).

https://oat-sa.atlassian.net/browse/TR-5760 - ACs 9-14

the Branch Rule is ignored

// In the case of an item or section, the target must refer to an item or section in the same test-part
// that has not yet been presented.
$this->next();

return;
}

$this->setPosition($this->getRouteItemPosition($targetRouteItems[$occurence]));
Expand All @@ -1133,10 +1134,11 @@ public function branch($identifier): void
if (isset($assessmentSectionIdentifierMap[$id])) {
if ($assessmentSectionIdentifierMap[$id][0]->getTestPart() !== $this->current()->getTestPart()) {
// From IMS QTI:
// In case of an item or section, the target must refer to an item or section
// in the same testPart [...]
$msg = 'Branchings to assessmentSections outside of the current testPart is forbidden by the QTI 2.1 specification.';
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor

Choose a reason for hiding this comment

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

throw new OutOfBoundsException($msg);
// In the case of an item or section, the target must refer to an item or section in the same test-part
// that has not yet been presented.
$this->next();

return;
}

// We branch to the first RouteItem belonging to the section.
Expand Down
89 changes: 89 additions & 0 deletions src/qtism/runtime/tests/RouteItem.php
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,30 @@ public function getPreConditions(): PreConditionCollection
return $this->preConditions;
}

/**
* Get the PreConditions that actually need to be applied considering all the parent elements: TestPart and Section
*
* @return PreConditionCollection A collection of PreCondition objects.
*/
public function getEffectivePreConditions(): PreConditionCollection
{
$routeItemPreConditions = new PreConditionCollection([]);

foreach ($this->getTestPart()->getPreConditions() as $preCondition) {
$routeItemPreConditions->attach($preCondition);
}

foreach ($this->getAssessmentSection()->getPreConditions() as $preCondition) {
$routeItemPreConditions->attach($preCondition);
}

foreach ($this->getPreConditions() as $preCondition) {
$routeItemPreConditions->attach($preCondition);
}

return $routeItemPreConditions;
}

/**
* Set the PreCondition objects to be applied prior to the RouteItem.
*
Expand Down Expand Up @@ -426,4 +450,69 @@ public function getTimeLimits($excludeItem = false): RouteTimeLimitsCollection

return $timeLimits;
}

public function getEffectiveBranchRules(): BranchRuleCollection
{
if ($this->getBranchRules()->count() > 0) {
return $this->getBranchRules();
}

$sectionBranchRules = $this->getEffectiveSectionBranchRules();

if ($sectionBranchRules === null || $sectionBranchRules->count() > 0) {
hectoras marked this conversation as resolved.
Show resolved Hide resolved
return $sectionBranchRules ?? new BranchRuleCollection();
}
shpran marked this conversation as resolved.
Show resolved Hide resolved

$testPart = $this->getTestPart();
$currentItemSections = $this->getAssessmentSections()->getArrayCopy();

if (!$testPart->isLastSection($currentItemSections[0])) {
return new BranchRuleCollection();
}

return $testPart->getBranchRules();
}

/**
* Selects branching rules from the section/subsection.
* Branching rules will be selected only if the item or subsection is the last one in the parent section.
*
* @return BranchRuleCollection|null Returns the branching rules for the last section or null if the
* element/subsection is not the last.
*/
private function getEffectiveSectionBranchRules(): ?BranchRuleCollection
{
/** @var AssessmentSection[] $sections */
$sections = $this->getAssessmentSections()->getArrayCopy();

// Remove the current section from the section list, as this section contains a list of items, not sections.
$currentSection = array_pop($sections);

if ($currentSection === null || !$currentSection->isLastSectionPart($this->getAssessmentItemRef())) {
return null;
shpran marked this conversation as resolved.
Show resolved Hide resolved
}

if ($currentSection->getBranchRules()->count() > 0) {
hectoras marked this conversation as resolved.
Show resolved Hide resolved
return $currentSection->getBranchRules();
}

$lastSection = $currentSection;

// Iterate through parent sections.
// Note: $sections should not contain the current section, as `$section->getSectionParts()` would then return a
// list of items instead of sections.
foreach (array_reverse($sections) as $section) {
if (!$section->isLastSectionPart($lastSection)) {
return null;
}

if ($section->getBranchRules()->count() > 0) {
return $section->getBranchRules();
}

$lastSection = $section;
}

return new BranchRuleCollection();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -248,4 +248,62 @@ public function testBranchingOnPreconditon(): void
$this::assertNull($session['Q03.SCORE']);
$this::assertSame(1.0, $session['Q04.SCORE']->getValue());
}

public function testBranchingOnTestPartsSimple1(): void
{
$session = self::instantiate(self::samplesDir() . 'custom/runtime/branchings/branchings_testparts.xml');
$session->beginTestSession();

// We are starting at item Q01, in testPart P01.
$this->assertEquals('Q01', $session->getCurrentAssessmentItemRef()->getIdentifier());

// Let's jump to testPart P03.
$session->beginAttempt();
$session->endAttempt(new State([new ResponseVariable('RESPONSE', Cardinality::SINGLE, BaseType::IDENTIFIER, new QtiIdentifier('GotoP03'))]));
$session->moveNext();

// We expect to land in testPart P03, item Q03.
$this->assertEquals('Q03', $session->getCurrentAssessmentItemRef()->getIdentifier());
$this->assertEquals('P03', $session->getCurrentTestPart()->getIdentifier());
}

public function testBranchingRules(): void
{
$session = self::instantiate(self::samplesDir() . 'custom/runtime/branchings/branching_rules.xml');
$session->beginTestSession();

$this->assertEquals('testPart-1', $session->getCurrentTestPart()->getIdentifier());
$this->assertEquals('assessmentSection-1', $session->getCurrentAssessmentSection()->getIdentifier());
$this->assertEquals('item-1', $session->getCurrentAssessmentItemRef()->getIdentifier());

$session->moveNext();

$this->assertEquals('testPart-1', $session->getCurrentTestPart()->getIdentifier());
$this->assertEquals('assessmentSection-1', $session->getCurrentAssessmentSection()->getIdentifier());
$this->assertEquals('item-3', $session->getCurrentAssessmentItemRef()->getIdentifier());

$session->moveNext();

$this->assertEquals('testPart-1', $session->getCurrentTestPart()->getIdentifier());
$this->assertEquals('assessmentSection-3', $session->getCurrentAssessmentSection()->getIdentifier());
$this->assertEquals('item-5', $session->getCurrentAssessmentItemRef()->getIdentifier());

$session->moveNext();

$this->assertEquals('testPart-3', $session->getCurrentTestPart()->getIdentifier());
$this->assertEquals('assessmentSection-5', $session->getCurrentAssessmentSection()->getIdentifier());
$this->assertEquals('item-7', $session->getCurrentAssessmentItemRef()->getIdentifier());

$session->moveNext();

$this->assertEquals('testPart-4', $session->getCurrentTestPart()->getIdentifier());
$this->assertEquals('subsection-1', $session->getCurrentAssessmentSection()->getIdentifier());
$this->assertEquals('item-9', $session->getCurrentAssessmentItemRef()->getIdentifier());

$session->moveNext();

$this->assertEquals('testPart-5', $session->getCurrentTestPart()->getIdentifier());
$this->assertEquals('assessmentSection-8', $session->getCurrentAssessmentSection()->getIdentifier());
$this->assertEquals('item-11', $session->getCurrentAssessmentItemRef()->getIdentifier());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -163,4 +163,30 @@ public function testKillerTestEpicWin(): void

$this::assertFalse($testSession->isRunning());
}

public function testTestParAndSectionAndItemPreConditionOnLinearTestWorks(): void
{
$testSession = self::instantiate(self::samplesDir() . 'custom/runtime/preconditions/preconditions_on_test_part_section_item_combined_linear.xml');
$testSession->beginTestSession();
$testSession->beginAttempt();
$testSession->moveNext();

// P02, S03, Q04 are skipped due precondition, but Q04.1 is passed
$this::assertSame($testSession->getRoute()->current()->getAssessmentItemRef()->getIdentifier(), 'Q04.1');

$testSession->moveNext();

// P05 precondition passed
$this::assertSame($testSession->getRoute()->current()->getAssessmentItemRef()->getIdentifier(), 'Q05');

$testSession->moveNext();

// S06 precondition passed
$this::assertSame($testSession->getRoute()->current()->getAssessmentItemRef()->getIdentifier(), 'Q06');

$testSession->moveNext();

// S07 precondition passed
$this::assertSame($testSession->getRoute()->current()->getAssessmentItemRef()->getIdentifier(), 'Q07');
}
}
23 changes: 13 additions & 10 deletions test/qtismtest/runtime/tests/RouteTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -601,10 +601,10 @@ public function testBranchUnknownTarget(): void

public function testBranchToAssessmentItemRefOutsideOfTestPart(): void
{
$route = self::buildSimpleRoute(Route::class, 2, 1);
$this->expectException(OutOfBoundsException::class);
Copy link
Contributor

Choose a reason for hiding this comment

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

These test case should not be changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now, instead of throwing an exception, we are ignoring branching rules and moving to the next item (like there are no branching rules)

$this->expectExceptionMessage('Branchings to items outside of the current testPart is forbidden by the QTI 2.1 specification.');
$route->branch('Q2');
$route = self::buildSimpleRoute(Route::class, 2, 2);
$route->branch('Q3');

$this->assertEquals('Q2', $route->current()->getAssessmentItemRef()->getIdentifier());
}

public function testBranchToSameTestPart(): void
Expand Down Expand Up @@ -655,24 +655,27 @@ public function testBranchToSectionOutsideOfTestPart(): void

$assessmentItemRef1 = new AssessmentItemRef('Q1', 'Q1.xml');
$assessmentItemRef2 = new AssessmentItemRef('Q2', 'Q2.xml');
$assessmentItemRef3 = new AssessmentItemRef('Q3', 'Q3.xml');

$assessmentSection1 = new AssessmentSection('S1', 'Section 1', true);
$assessmentSection1->setSectionParts(new SectionPartCollection([$assessmentItemRef1]));
$assessmentSection1->setSectionParts(new SectionPartCollection([$assessmentItemRef1, $assessmentItemRef2]));

$assessmentSection2 = new AssessmentSection('S2', 'Section 2', true);
$assessmentSection2->setSectionParts(new SectionPartCollection([$assessmentItemRef2]));
$assessmentSection2->setSectionParts(new SectionPartCollection([$assessmentItemRef3]));

$testPart1 = new TestPart('T1', new AssessmentSectionCollection([$assessmentSection1]));
$testPart2 = new TestPart('T2', new AssessmentSectionCollection([$assessmentSection2]));
$assessmentTest = new AssessmentTest('Test1', 'Test 1', new TestPartCollection([$testPart1, $testPart2]));

$route->addRouteItem($assessmentItemRef1, $assessmentSection1, $testPart1, $assessmentTest);
$route->addRouteItem($assessmentItemRef2, $assessmentSection2, $testPart2, $assessmentTest);

$this->expectException(OutOfBoundsException::class);
$this->expectExceptionMessage('Branchings to assessmentSections outside of the current testPart is forbidden by the QTI 2.1 specification.');
$route->addRouteItem($assessmentItemRef2, $assessmentSection1, $testPart1, $assessmentTest);
$route->addRouteItem($assessmentItemRef3, $assessmentSection2, $testPart2, $assessmentTest);

$route->branch('S2');
$currentRoute = $route->current();

$this->assertEquals('Q2', $currentRoute->getAssessmentItemRef()->getIdentifier());
$this->assertEquals('S1', $currentRoute->getAssessmentSection()->getIdentifier());
}

public function testGetRouteItemPositionUnknownRouteItem(): void
Expand Down
Loading
Loading