Skip to content

Exercise: Sort the learner tests list by title across pages - #9278

Merged
ywarnier merged 2 commits into
chamilo:masterfrom
ybtbeez:fix/issue-1416-student-tests-order
Oct 20, 2026
Merged

ywarnier merged 2 commits into
chamilo:masterfrom
ybtbeez:fix/issue-1416-student-tests-order

Conversation

@ybtbeez

@ybtbeez ybtbeez commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

Exercise: Sort the learner tests list by title across pages

Summary

Since #7450 (75ca0a1), the teacher's tests list in Exercise::exerciseGridResource() loads all tests, sorts them by title with strnatcasecmp() (natural and case-insensitive, so "Quiz 2" comes before "Quiz 10"), and then paginates with array_slice().

The learner list was still paginated by the database (setFirstResult/setMaxResults) without an ORDER BY. Only the 20 rows of the current page were sorted. With more than 20 tests, which tests ended up on which page depended on the order the database returned them in.

This PR moves the existing teacher sort and pagination out of the teacher-only branch, so both branches use them unchanged:

  • The teacher branch still builds its merged published and draft list.
  • The learner branch now loads its whole published list.
  • Both lists then go through the same usort(... strnatcasecmp ...), array_slice($from, $limit) and count().

The learner list now has exactly the same order as the teacher list. A database ORDER BY title would not give that, because it sorts "Quiz 10" before "Quiz 2".

Side effects for the learner list:

  • getCount() is no longer used.
  • setMaxResults() is no longer applied to a query that fetch-joins collections (links, files). Before, a test with several resource links could take up more than one of the 20 SQL rows. The total also came from a non-DISTINCT COUNT over the joined rows.

The manual re-ordering of tests that #1416 also asks for is not part of this PR.

Tests

New tests/CoreBundle/Legacy/ExerciseGridResourceTest.php. It creates 25 tests titled Quiz 1 to Quiz 25, with no zero-padding, in reverse order. So neither the insertion order nor an alphabetical order ("Quiz 1", "Quiz 10", ...) is the natural one. Then:

  • As a platform admin, the teacher list across both pages must be exactly Quiz 1 to Quiz 25 in natural order.
  • As a learner, the list across both pages must be identical to the teacher's.
  • Each page must be full (20, then 5) and report a total of 25.
  • Each user starts with a fresh session, because the table stores its sort column there.

I ran the same flow against a MariaDB 11.8 dev database. Each run was inside a transaction that was rolled back afterwards:

Code Default sql_mode ONLY_FULL_GROUP_BY
This PR pass pass
Previous attempt (DB ORDER BY title) fail: Quiz 10–Quiz 25 land on page 1 ahead of Quiz 5 —
master fail: learner page 1 = Quiz 6–Quiz 25, page 2 = Quiz 1–Quiz 5 fail

Static checks:

  • PHPStan: no new errors. exercise.class.php has the same 191 errors as before; the new test has none.
  • ECS: the test file is clean. ECS crashes with an internal Slevomat error on exercise.class.php. It crashes the same way on the unmodified master file.

Security

  • No query input changed and no orderBy() was added, so there is no SQL injection risk.
  • The learner query keeps its published-only filter.
  • Every per-row check (Exercise::read, learning-path membership, isVisible() for the course and session) still runs, unchanged, on each displayed row.
  • No new output.
  • Load: a learner request now hydrates all published tests of the course category, where it used to hydrate 20. The teacher branch already does this, with two queries. The expensive per-row work still runs only for the 20 tests on the page, so the extra cost is one hydration per test.

Known limitations

These predate this PR. It neither fixes nor worsens them.

  • Tests hidden from learners after pagination (because they belong to a learning path, or are not visible in the session) still leave the page short, and they are still counted in the total. Filtering them before pagination would mean running Exercise::read() for every test on every page view, or duplicating the visibility checks ahead of the loop. That is left for a follow-up.
  • Each page is re-sorted by SortableTable on the rendered, tag-stripped title. That title is natural and lowercased, but long titles are cut. So two titles that differ only after the cut point can swap places within a page. This happens the same way for teachers and learners.

Refs #1416

…amilo#1416

The learner list was paginated by the database without an ORDER BY, so only the rows of the current page were sorted and the order across pages was whatever the database returned. Load the learner list whole and run it through the same natural, case-insensitive title sort and array_slice pagination as the teacher list, so both views show the tests in exactly the same order.
@ywarnier
ywarnier merged commit f84ab3f into chamilo:master Oct 20, 2026
4 of 7 checks passed
@codecov

codecov Bot commented Oct 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 17.20%. Comparing base (26b01e9) to head (3bd9fb3).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #9278      +/-   ##
============================================
+ Coverage     17.13%   17.20%   +0.06%     
- Complexity    62160    62176      +16     
============================================
  Files          2522     2522              
  Lines        233279   233319      +40     
============================================
+ Hits          39981    40150     +169     
+ Misses       193298   193169     -129     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

2 participants