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.

Sign up for GitHub

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

Jump to bottom

feat: make the first item consider testPart pre-conditions #367

Merged
merged 26 commits into from
Oct 13, 2023
Merged

feat: make the first item consider testPart pre-conditions #367

merged 26 commits into from
Oct 13, 2023

Conversation

gabrielfs7
Copy link
Contributor

@gabrielfs7 gabrielfs7 commented Sep 22, 2023 •
edited
Loading

https://oat-sa.atlassian.net/browse/TR-5381
https://oat-sa.atlassian.net/browse/TR-5759
https://oat-sa.atlassian.net/browse/TR-5760

Goal

  • Allow pre-conditions from test part to be evaluated.
  • Do not throw an exception if a branching rule of an item/section in one test part points to an item/section in another test part (or to the test part itself). Instead, we will move on to the next element as if there were no branching rules.

Related PRs

@shpran shpran mentioned this pull request Sep 25, 2023
Draft
bugalot
bugalot requested changes Sep 25, 2023
View reviewed changes
test/qtismtest/runtime/tests/RouteTest.php
@@ -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)

src/qtism/runtime/tests/Route.php
Comment on lines -1122 to -1123
$msg = 'Branchings to items outside of the current testPart is forbidden by the QTI 2.1 specification.';
throw new OutOfBoundsException($msg);
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 •
edited
Loading

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

src/qtism/runtime/tests/Route.php
@@ -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.

#367 (comment)

src/qtism/runtime/tests/AbstractSessionManager.php Outdated
@@ -200,6 +202,7 @@ protected function createRoute(AssessmentTest $test): Route
// 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());
$route->getFirstRouteItem()->addPreConditions($testPart->getPreConditions());
Copy link
Contributor

Choose a reason for hiding this comment

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

Preconditions treatment for testParts should be transferred to line 222-223. This will not work, as we are dealing with the assessmentSections of the tree.

testParts management is at line 222-223.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this one I used a bit different approach now, so the routeItem retains the knowledge of which pre-conditions to use as it needs to respect testPart/section/item pre-conditions.

I've added it here https://github.com/oat-sa/qti-sdk/pull/367/files#diff-f33df5e2727b0618d5b89c14e69336ff58245c1453d97a797446bc2336d38cceR276

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've also fixed an issue that allowed an item to be accessed in case 1 of the preconditions are met instead of all preconditions are met.

👉 85f9d10

cc: @julien-sebire

gabrielfs7 and others added 10 commits September 25, 2023 10:45
@gabrielfs7
chore: add test part preconditions to item just one time
a01223b
@shpran
chore: rely on original branch rules instead of setting them to the l…
e5c592e 
…ast item
@shpran
refactor: move logic to get branch rules to Route class
c623ab4
@gabrielfs7
feat: create method to return effective preconditions
613f985
@gabrielfs7
chore: make the test part and section pre-conditions to be applied in…
fd854bf 
… item level
@shpran
refactor: move branch selection to the RouteItem
831e5ca
@shpran
Merge branch 'feat/TR-5381/add-preconditions-from-test-parts' of http…
0c660a9 
…s://github.com/oat-sa/qti-sdk into feat/rely-on-original-branch-rules-or-preconditions
@shpran
chore: use last parent item section instead of currunt item subsection
d104236
@shpran
refactor: add methods to check if the item, subsection or section is …
c2738e6 
…the last in list
@shpran
Merge pull request #370 from oat-sa/feat/rely-on-original-branch-rule…
f5765cb 
…s-or-preconditions

chore: rely on original branch rules instead of setting them to the last item
shpran
shpran approved these changes Sep 28, 2023
View reviewed changes
Copy link
Contributor

@shpran shpran left a comment

Choose a reason for hiding this comment

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

  • New code is covered by tests (if applicable)
  • Tests are running successfully (old and new ones) on my local machine (if applicable)
  • New code is respecting code style rules
  • New code is respecting 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

hectoras
hectoras approved these changes Sep 29, 2023
View reviewed changes
Copy link

@hectoras hectoras left a comment

Choose a reason for hiding this comment

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

  • Pull request title and description are meaningful
  • Pull request's target is not master
  • Target branch is correct
  • Commits are following conventional commits
  • Change log provided
  • New code is covered by tests (if applicable)
  • New code is respecting code style rules
  • New code is respecting best practices
  • New code is not subject to concurrency issues (if applicable)
  • Feature is working correctly on my local machine (if applicable)
  • Tests are running successfully (old and new ones) on my local machine (if applicable)
  • Acceptance criteria are respected
  • Kitchen links are not exposed in the description

@shpran shpran force-pushed the feat/TR-5381/add-preconditions-from-test-parts branch from da61af4 to a5b719a Compare October 11, 2023 08:30
gabrielfs7
gabrielfs7 commented Oct 11, 2023
View reviewed changes
src/qtism/runtime/tests/AssessmentTestSession.php
Comment on lines +2521 to +2531
/** @var BranchRule $branchRule */
foreach ($branchRules as $branchRule) {
$engine = new ExpressionEngine($branchRule->getExpression(), $this);
$condition = $engine->process();

if ($condition !== null && $condition->getValue() === true) {
$route->branch($branchRule->getTarget());

break 2;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should encapsulate this in a method rather than repeat it here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

private function evaluateBranchRules(BranchRuleCollection $branchRules): ?BranchRule
{
    foreach ($branchRules as $branchRule) {
        $engine = new ExpressionEngine($branchRule->getExpression(), $this);
        $condition = $engine->process();

        if ($condition !== null && $condition->getValue() === true) {
            return $branchRule;
        }
    }
}

@gabrielfs7 gabrielfs7 merged commit 6e7e419 into develop Oct 13, 2023
3 checks passed
@gabrielfs7 gabrielfs7 deleted the feat/TR-5381/add-preconditions-from-test-parts branch October 13, 2023 11:26
@gabrielfs7 gabrielfs7 mentioned this pull request Oct 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hectoras hectoras hectoras approved these changes

@shpran shpran shpran approved these changes

@julien-sebire julien-sebire Awaiting requested review from julien-sebire

@bugalot bugalot Awaiting requested review from bugalot

@erkobridee erkobridee Awaiting requested review from erkobridee

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@gabrielfs7 @hectoras @bugalot @shpran