Moodle
  1. Moodle
  2. MDL-34765

"Re-sort courses by name" in natural order

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.5, 2.3.2, 2.4
    • Fix Version/s: 2.2.6, 2.3.3
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide

      Test pre-requisites

      1. Create a few courses named as followed (keep the order and case):
      • Course 10
      • course 2
      • course 11
      • Course 0
      • Course 1

      Test steps

      1. Navigate to Home ► Courses ► Courses ► Add/edit courses
      2. Browse the category you've created the course in
      3. Click on the button 'Re-sort courses by name'
      4. Make sure the order makes sense (from a human point of view)
      Show
      Test pre-requisites Create a few courses named as followed (keep the order and case): Course 10 course 2 course 11 Course 0 Course 1 Test steps Navigate to Home ► Courses ► Courses ► Add/edit courses Browse the category you've created the course in Click on the button 'Re-sort courses by name' Make sure the order makes sense (from a human point of view)
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-34765-master
    • Rank:
      43248

      Description

      Moodle uses a very basic sortorder, provided by the DBMS. If you have e.g. the following courses:
      "Course 1", "Course 2", "Course 10", "Course 3" and press "Re-sort courses by name" on /course/category.php, you will get the courses in the following ("unnatural") order:

      "Course 1"
      "Course 10"
      "Course 2"
      "Course 3"

      Sorting should be done in PHP to improve the user experience.

      Replication steps:

      1. Create courses with numeric numbering similar to the list above
      2. Ensure that your site has more than one catagory
      3. Navigate to one of the courses
      4. In the breadcrumbs, click on the category name

      Expected result: Course names should be sorted in natural order

      Actual result: Course names are sorted in Unicode order.

        Issue Links

          Activity

          Hide
          Tobias Marx added a comment - - edited

          Here's a patch to /course/category.php that sorts courses by their "fullname" in a natural way. E.g. in above example:

          "Course 1"
          "Course 2"
          "Course 3"
          "Course 10"

          This is NOT thorougly tested! Use at your own risk!

          Show
          Tobias Marx added a comment - - edited Here's a patch to /course/category.php that sorts courses by their "fullname" in a natural way. E.g. in above example: "Course 1" "Course 2" "Course 3" "Course 10" This is NOT thorougly tested! Use at your own risk!
          Hide
          Michael de Raadt added a comment -

          Thanks for sharing your idea and your patch.

          I think there are a few other areas where course names are sorted where natural sorting could also be applied, particularly the navigation block. Perhaps you could extend the patch to include those.

          Show
          Michael de Raadt added a comment - Thanks for sharing your idea and your patch. I think there are a few other areas where course names are sorted where natural sorting could also be applied, particularly the navigation block. Perhaps you could extend the patch to include those.
          Hide
          Michael de Raadt added a comment -

          Actually, no. The course order in Navigation is quite different.

          Show
          Michael de Raadt added a comment - Actually, no. The course order in Navigation is quite different.
          Hide
          Frédéric Massart added a comment -

          Thanks for the patch Tobias, I have modified it a bit and integrated it.

          Reviewers, integrators, please note that anonymous functions are available since PHP 5.3 which is the minimum requirement since Moodle 2.1.

          Show
          Frédéric Massart added a comment - Thanks for the patch Tobias, I have modified it a bit and integrated it. Reviewers, integrators, please note that anonymous functions are available since PHP 5.3 which is the minimum requirement since Moodle 2.1.
          Hide
          Rajesh Taneja added a comment -

          Patch looks good Fred, pushing it for integration review.

          Show
          Rajesh Taneja added a comment - Patch looks good Fred, pushing it for integration review.
          Hide
          Dan Poltawski added a comment -

          Hi Guys,

          I need to check, but i'm almost certain that this is a database collation issue, the sort will be 'natural' if the database environment is setup right.

          Show
          Dan Poltawski added a comment - Hi Guys, I need to check, but i'm almost certain that this is a database collation issue, the sort will be 'natural' if the database environment is setup right.
          Hide
          Dan Poltawski added a comment - - edited

          I asked the expert (Eloy) and indeed he pointed out three things:

          "1) collation-dependent ordering in PHP is provided by textlib/collatorlib.
          2) It's better not to introduce it that way, because it will lead to different orderings here and there, and some places won't be processable (multi-page...) at all
          3) the solution is something to implement in DBs (it seems that, since PG 9.0/1 we can use collations specifics in the queries, but we need to develop it proper support from moodle_database. Although I'm not sure that all DBs are supporting natural ordering.

          But no matter of 2) and 3), for sure that patch is not the way (does not use our PHP colllation facilities)."

          So reopening about that. 3) is really the the solution. I think in this particular case we could ignore 2) because this changes the sortorder and will then be sorted that way? Although wow will it perform with very big numbers of courses?

          Show
          Dan Poltawski added a comment - - edited I asked the expert (Eloy) and indeed he pointed out three things: "1) collation-dependent ordering in PHP is provided by textlib/collatorlib. 2) It's better not to introduce it that way, because it will lead to different orderings here and there, and some places won't be processable (multi-page...) at all 3) the solution is something to implement in DBs (it seems that, since PG 9.0/1 we can use collations specifics in the queries, but we need to develop it proper support from moodle_database. Although I'm not sure that all DBs are supporting natural ordering. But no matter of 2) and 3), for sure that patch is not the way (does not use our PHP colllation facilities)." So reopening about that. 3) is really the the solution. I think in this particular case we could ignore 2) because this changes the sortorder and will then be sorted that way? Although wow will it perform with very big numbers of courses?
          Hide
          Frédéric Massart added a comment -

          Hi Dan, thanks for your feedback. I have amended my commit to use our collatorlib. Cheers!

          Show
          Frédéric Massart added a comment - Hi Dan, thanks for your feedback. I have amended my commit to use our collatorlib. Cheers!
          Hide
          Rajesh Taneja added a comment -

          Patch looks good Fred,

          Pushing for integration.

          Show
          Rajesh Taneja added a comment - Patch looks good Fred, Pushing for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

          Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          Hide
          Sam Hemelryk added a comment -

          Thanks Fred, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Fred, this has been integrated now
          Hide
          Tim Barker added a comment -

          Unit tests failed in the nightly build because of this. I'm not sure why but Fred is looking into this.

          Show
          Tim Barker added a comment - Unit tests failed in the nightly build because of this. I'm not sure why but Fred is looking into this.
          Hide
          Tim Barker added a comment -

          Failed unit tests in the nightly build, Fred is investigating.

          Show
          Tim Barker added a comment - Failed unit tests in the nightly build, Fred is investigating.
          Hide
          Frédéric Massart added a comment -

          Actually, the Unit Test that fails is related to section reordering, not course reordering. This issue should be tested again.

          Show
          Frédéric Massart added a comment - Actually, the Unit Test that fails is related to section reordering, not course reordering. This issue should be tested again.
          Hide
          Rossiani Wijaya added a comment -

          Hi Fred,

          There's an error for 2.2:

          Fatal error: Undefined class constant 'SORT_NATURAL' in /22/course/category.php on line 80
          

          Failing test. sorry.

          Show
          Rossiani Wijaya added a comment - Hi Fred, There's an error for 2.2: Fatal error: Undefined class constant 'SORT_NATURAL' in /22/course/category.php on line 80 Failing test. sorry.
          Hide
          Frédéric Massart added a comment -

          I have added a new commit to my 2.2 branch to fix this issue. Please note while testing that the ordering might not be as accurate as it could be on 2.3 and master due to lack of functionality in collatorlib. Cheers!

          Show
          Frédéric Massart added a comment - I have added a new commit to my 2.2 branch to fix this issue. Please note while testing that the ordering might not be as accurate as it could be on 2.3 and master due to lack of functionality in collatorlib. Cheers!
          Hide
          Aparup Banerjee added a comment -

          Thanks Fred, that (723c439) is integrated into 22 now. up for testing again.

          Show
          Aparup Banerjee added a comment - Thanks Fred, that (723c439) is integrated into 22 now. up for testing again.
          Hide
          Rossiani Wijaya added a comment -

          This looks good now.

          Thanks for fixing.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This looks good now. Thanks for fixing. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Gutta cavat lapidem, non vi sed saepe cadendo - Ovidio

          This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

          Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Gutta cavat lapidem, non vi sed saepe cadendo - Ovidio This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks!

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: