Moodle
  1. Moodle
  2. MDL-32975

Option to sort My Courses list alphabetically

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.3, 2.4.1, 2.5
    • Fix Version/s: 2.4.2
    • Component/s: Navigation
    • Labels:
    • Testing Instructions:
      Hide
      1. Log in as a student with enrolments
      2. Expand the my courses branch and check things are sorted by sortorder.
      3. Change $CFG->navsortmycoursessort to 'fullname'
      4. Expand the my courses branch and check things are sorted by fullname.
      5. Change $CFG->navsortmycoursessort to 'shortname'
      6. Expand the my courses branch and check things are sorted by shortname.
      7. Change $CFG->navsortmycoursessort to 'idnumber'
      8. Expand the my courses branch and check things are sorted by idnumber.
      9. Disable JS
      10. Click "My courses"
      11. Check when you are on the my courses page that the my course branch is expanded and contains your courses.
      Show
      Log in as a student with enrolments Expand the my courses branch and check things are sorted by sortorder. Change $CFG->navsortmycoursessort to 'fullname' Expand the my courses branch and check things are sorted by fullname. Change $CFG->navsortmycoursessort to 'shortname' Expand the my courses branch and check things are sorted by shortname. Change $CFG->navsortmycoursessort to 'idnumber' Expand the my courses branch and check things are sorted by idnumber. Disable JS Click "My courses" Check when you are on the my courses page that the my course branch is expanded and contains your courses.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull 2.4 Branch:
      wip-MDL-32975-m24
    • Pull Master Branch:
      wip-MDL-32975-m25
    • Rank:
      40146

      Description

      In the My Courses block in 1.9.x, courses were sorted alphabetically without the ability to sort them any other way. (Line 41 of blocks/course_list/block_course_list.php)

      In the new My Courses listing in 2.2.x navigation, courses are sorted by 'sortorder' - without the ability to sort them any other way. (Line 1062 of lib/navigationlib.php).

      Why was there a change?

      Our staff are enrolled in many courses (up to 20 or 30 in some cases), spread across many categories, and 'sortorder' makes no sense to them visually. The listing actually appears quite random now.

      Would it be possible to add a flag in the navigation administration settings to 'Sort My Courses list alphabetically'?

      On our site, I will need to hack the sort fields sent to function enrol_get_my_courses from 'visible DESC, sortorder ASC' to 'visible DESC, fullname ASC', which maintains the sort that our staff have been used to.

        Issue Links

          Activity

          Hide
          Richard van Iwaarden added a comment -

          Same problem here, we have the courses numbered. In Moodle 1.9 we had a very nice list of approxx. 30 courses listed the way we wanted it.

          Now, the courselist is a mess. Students are searching a lot!

          Show
          Richard van Iwaarden added a comment - Same problem here, we have the courses numbered. In Moodle 1.9 we had a very nice list of approxx. 30 courses listed the way we wanted it. Now, the courselist is a mess. Students are searching a lot!
          Hide
          Andrew Nightingale added a comment -

          To keep this thread alive we are having similar issues here. Teaching staff with large course loads are finding it very inconvenient to find courses in the 'My Courses' list and we've had significant negative feedback from learners.

          Show
          Andrew Nightingale added a comment - To keep this thread alive we are having similar issues here. Teaching staff with large course loads are finding it very inconvenient to find courses in the 'My Courses' list and we've had significant negative feedback from learners.
          Hide
          Normand Charette added a comment -

          We have the same problem here. Students and Instructors who have been here a long time have to weed through old courses. They too have been searching courses by using Ctrl+F or https://..../course/search.php. We would like an option to sort My Courses (within the Navigation Block) list alphabetically. Our learners are frustrated with this setup. Help please and thank you.

          Show
          Normand Charette added a comment - We have the same problem here. Students and Instructors who have been here a long time have to weed through old courses. They too have been searching courses by using Ctrl+F or https://..../course/search.php . We would like an option to sort My Courses (within the Navigation Block) list alphabetically. Our learners are frustrated with this setup. Help please and thank you.
          Hide
          Matt Fedorko added a comment -

          The Moodle roadmap (http://docs.moodle.org/dev/Roadmap) includes a fix for Usability of the front page in MDL-31830

          Show
          Matt Fedorko added a comment - The Moodle roadmap ( http://docs.moodle.org/dev/Roadmap ) includes a fix for Usability of the front page in MDL-31830
          Hide
          Sam Hemelryk added a comment -

          Thanks for creating this issue Michael, and for replying + voting.

          I'm putting forward a patch that adds a setting to control how the my courses branch is sorted.
          I've also fixed a bug wherby with JS disabled you were not able to expand the my courses branch without visiting a course you are enrolled in.

          Show
          Sam Hemelryk added a comment - Thanks for creating this issue Michael, and for replying + voting. I'm putting forward a patch that adds a setting to control how the my courses branch is sorted. I've also fixed a bug wherby with JS disabled you were not able to expand the my courses branch without visiting a course you are enrolled in.
          Hide
          Sam Hemelryk added a comment -

          Up for peer-review now

          Show
          Sam Hemelryk added a comment - Up for peer-review now
          Hide
          Sam Hemelryk added a comment -

          Submitting this for integration review in order to attempt to catch the releases.

          Show
          Sam Hemelryk added a comment - Submitting this for integration review in order to attempt to catch the releases.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Sam... just I find these lines (2570-2575) a bit "strange":

          https://github.com/samhemelryk/moodle/compare/master...wip-MDL-32975-m25#L2R2570

          why is "$CFG->navsortmycoursessort !== 'sortorder'" an exception and the ordering happens before/after the "visible DESC" based on that? Is there any logic / reason?

          I'd go to something simpler like:

          $sortorder = 'visible DESC, sortorder ASC';
          if (!empty($CFG->navsortmycoursessort)) {
              $sortorder = 'visible DESC, ' . $CFG->navsortmycoursessort . ' ASC,';
          }
          $courses = enrol_get_my_courses(null, $sortorder);
          

          More yet, I don't know if the 1st line is needed or no. Everybody will have one value in $CFG->navsortmycoursessort after upgrading, isn't it?

          FYC, I keep this open...

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Sam... just I find these lines (2570-2575) a bit "strange": https://github.com/samhemelryk/moodle/compare/master...wip-MDL-32975-m25#L2R2570 why is "$CFG->navsortmycoursessort !== 'sortorder'" an exception and the ordering happens before/after the "visible DESC" based on that? Is there any logic / reason? I'd go to something simpler like: $sortorder = 'visible DESC, sortorder ASC'; if (!empty($CFG->navsortmycoursessort)) { $sortorder = 'visible DESC, ' . $CFG->navsortmycoursessort . ' ASC,'; } $courses = enrol_get_my_courses( null , $sortorder); More yet, I don't know if the 1st line is needed or no. Everybody will have one value in $CFG->navsortmycoursessort after upgrading, isn't it? FYC, I keep this open...
          Hide
          Sam Hemelryk added a comment -

          Aha quite right Eloy, that bit of code could be greatly improved.

          I've pushed up two updated branches now to simplify that code.
          I've also squashed the branches to a single commit each.

          Many thanks
          sam

          Show
          Sam Hemelryk added a comment - Aha quite right Eloy, that bit of code could be greatly improved. I've pushed up two updated branches now to simplify that code. I've also squashed the branches to a single commit each. Many thanks sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Aha new one looks nicer. I'm just adding one version minibump on top to avoid anybody getting undefined notices before rolling weeklies.

          Show
          Eloy Lafuente (stronk7) added a comment - Aha new one looks nicer. I'm just adding one version minibump on top to avoid anybody getting undefined notices before rolling weeklies.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (24 & master), thanks!

          PS: Not really sure why I integrated this to 24 (ssshhh!!).

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (24 & master), thanks! PS: Not really sure why I integrated this to 24 (ssshhh!!).
          Hide
          Damyon Wiese added a comment -

          I'm getting a broken MyHome with this change:

          Coding error detected, it must be fixed by a programmer: moodle_database::get_in_or_equal() does not accept empty arrays
          Debug info:
          Error code: codingerror
          Stack trace:
          line 665 of /lib/dml/moodle_database.php: coding_exception thrown
          line 2599 of /lib/navigationlib.php: call to moodle_database->get_in_or_equal()
          line 1107 of /lib/navigationlib.php: call to global_navigation->load_courses_enrolled()
          line 2894 of /lib/navigationlib.php: call to global_navigation->initialise()
          line 766 of /lib/pagelib.php: call to navbar->has_items()
          line 4 of /theme/base/layout/general.php: call to moodle_page->has_navbar()
          line 835 of /lib/outputrenderers.php: call to include()
          line 765 of /lib/outputrenderers.php: call to core_renderer->render_page_layout()
          line 153 of /my/index.php: call to core_renderer->header()

          Show
          Damyon Wiese added a comment - I'm getting a broken MyHome with this change: Coding error detected, it must be fixed by a programmer: moodle_database::get_in_or_equal() does not accept empty arrays Debug info: Error code: codingerror Stack trace: line 665 of /lib/dml/moodle_database.php: coding_exception thrown line 2599 of /lib/navigationlib.php: call to moodle_database->get_in_or_equal() line 1107 of /lib/navigationlib.php: call to global_navigation->load_courses_enrolled() line 2894 of /lib/navigationlib.php: call to global_navigation->initialise() line 766 of /lib/pagelib.php: call to navbar->has_items() line 4 of /theme/base/layout/general.php: call to moodle_page->has_navbar() line 835 of /lib/outputrenderers.php: call to include() line 765 of /lib/outputrenderers.php: call to core_renderer->render_page_layout() line 153 of /my/index.php: call to core_renderer->header()
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Aha, confirmed, can reproduce it with:

          $CFG->navshowmycoursecategories = true;

          if I access the my page having no enrolled courses.

          Looking... also seems to be a discrepancy in the use of the show_my_categories() method (passing bool true for... ???).

          Show
          Eloy Lafuente (stronk7) added a comment - Aha, confirmed, can reproduce it with: $CFG->navshowmycoursecategories = true; if I access the my page having no enrolled courses. Looking... also seems to be a discrepancy in the use of the show_my_categories() method (passing bool true for... ???).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I think this should be enough:

          -        if ($this->show_my_categories(true)) {
          +        if (!empty($courses) and $this->show_my_categories()) {
          

          for your consideration... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I think this should be enough: - if ($ this ->show_my_categories( true )) { + if (!empty($courses) and $ this ->show_my_categories()) { for your consideration... ciao
          Hide
          Sam Hemelryk added a comment -

          Thanks guys, I've pushed a fix up for that now.

          Show
          Sam Hemelryk added a comment - Thanks guys, I've pushed a fix up for that now.
          Hide
          Ankit Agarwal added a comment -

          Hi Sam,
          Is this is supposed to be case-sensitive search?
          am getting following result:-
          Asd > Test > asd

          Will pass/fail based on your response.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Sam, Is this is supposed to be case-sensitive search? am getting following result:- Asd > Test > asd Will pass/fail based on your response. Thanks
          Hide
          Sam Hemelryk added a comment -

          Hi Raj,

          At the moment it is using database sort to order the mycourses.
          Really its probably not a great method (just convenient) and the system should probably be using textlib methods for localised sorting.
          The question I suppose is do we leave it in or don't we?

          My vote is to leave it in, as much of the code would stay the same, and open a new bug so that I can inject some locallised sorting where required.

          However I think getting another integrators opinion is the best approach here.

          Show
          Sam Hemelryk added a comment - Hi Raj, At the moment it is using database sort to order the mycourses. Really its probably not a great method (just convenient) and the system should probably be using textlib methods for localised sorting. The question I suppose is do we leave it in or don't we? My vote is to leave it in, as much of the code would stay the same, and open a new bug so that I can inject some locallised sorting where required. However I think getting another integrators opinion is the best approach here.
          Hide
          Ankit Agarwal added a comment -

          Raj

          Anyways as suggested, I will wait for Eloy or another integrator to comment here. Thanks

          Show
          Ankit Agarwal added a comment - Raj Anyways as suggested, I will wait for Eloy or another integrator to comment here. Thanks
          Hide
          Dan Poltawski added a comment -

          Hi,

          Someone has reported a problem with this on the QA site: MDL-38345

          I am not sure if a fix has come for the problem of navsortmycoursessort being unset? If not I think that we should test for that variable being unset because lots of people seem to be running into it before the settings are saved.

          Show
          Dan Poltawski added a comment - Hi, Someone has reported a problem with this on the QA site: MDL-38345 I am not sure if a fix has come for the problem of navsortmycoursessort being unset? If not I think that we should test for that variable being unset because lots of people seem to be running into it before the settings are saved.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Well I remember myself adding version bump commit for exactly that case, and it worked here... how is it possible to end the upgrade process without getting navsortmycoursessort set?

          In any case, I agree we can easily isset/isempty it to be 100% in the safe side.

          Looking for a solution @ MDL-38345, so I think you can pass this, Ankit (as we have commented in jabber).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Well I remember myself adding version bump commit for exactly that case, and it worked here... how is it possible to end the upgrade process without getting navsortmycoursessort set? In any case, I agree we can easily isset/isempty it to be 100% in the safe side. Looking for a solution @ MDL-38345 , so I think you can pass this, Ankit (as we have commented in jabber). Ciao
          Hide
          Ankit Agarwal added a comment -

          Passing as suggested!
          Thanks

          Show
          Ankit Agarwal added a comment - Passing as suggested! Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities.

          Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied).

          Thanks, closing as fixed!

          Show
          Eloy Lafuente (stronk7) added a comment - This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities. Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied). Thanks, closing as fixed!
          Hide
          Helen Foster added a comment -

          I'm sorry that the new language strings for this new setting were not reviewed before being committed and that the testing instructions never mentioned the new UI setting.

          The new language strings contained several typos which I have just fixed in the en_fix lang pack.

          Maybe I'm having a slow day today, because I found the new language string navsortmycoursessort_help really unclear and it took me half an hour to figure out how things worked. I have reworded the string in en_fix to hopefully make it more easily understandable.

          Unfortunately it will be two months before the en_fix language pack is once again merged with the en language pack. This means that everyone who looks at http://docs.moodle.org/dev/Moodle_2.4.2_release_notes and checks the highlight 'There is an option to sort My Courses list alphabetically' will find a setting with typos and unclear description. In addition, language pack maintainers will translate the unclear description and will then have to re-translate it in two months time. ;-(

          Sam and other developers, please note for future reference: I'm always happy to review new language strings. Please just add me as a watcher and the comment something like 'Adding Helen as a watcher to review new lang strings'.

          Also, for a new UI setting, please let's mention it in the testing instructions and hopefully avoid this kind of thing happening again.

          Show
          Helen Foster added a comment - I'm sorry that the new language strings for this new setting were not reviewed before being committed and that the testing instructions never mentioned the new UI setting. The new language strings contained several typos which I have just fixed in the en_fix lang pack. Maybe I'm having a slow day today, because I found the new language string navsortmycoursessort_help really unclear and it took me half an hour to figure out how things worked. I have reworded the string in en_fix to hopefully make it more easily understandable. Unfortunately it will be two months before the en_fix language pack is once again merged with the en language pack. This means that everyone who looks at http://docs.moodle.org/dev/Moodle_2.4.2_release_notes and checks the highlight 'There is an option to sort My Courses list alphabetically' will find a setting with typos and unclear description. In addition, language pack maintainers will translate the unclear description and will then have to re-translate it in two months time. ;-( Sam and other developers, please note for future reference: I'm always happy to review new language strings. Please just add me as a watcher and the comment something like 'Adding Helen as a watcher to review new lang strings'. Also, for a new UI setting, please let's mention it in the testing instructions and hopefully avoid this kind of thing happening again.
          Hide
          Helen Foster added a comment -

          Removing the docs_required label as this new setting is now mentioned in http://docs.moodle.org/en/Navigation

          Great work Sam and everyone who collaborated in this improvement. Thanks also to all watchers and voters for your interest.

          Show
          Helen Foster added a comment - Removing the docs_required label as this new setting is now mentioned in http://docs.moodle.org/en/Navigation Great work Sam and everyone who collaborated in this improvement. Thanks also to all watchers and voters for your interest.
          Hide
          Helen Foster added a comment -

          Following a chat with Eloy and en_fix merger David, I have created a new issue MDL-38457 for fixing the lang strings.

          Show
          Helen Foster added a comment - Following a chat with Eloy and en_fix merger David, I have created a new issue MDL-38457 for fixing the lang strings.

            People

            • Votes:
              10 Vote for this issue
              Watchers:
              14 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: