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 Master Branch:
      wip-MDL-32975-m25

      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.

        Gliffy Diagrams

          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: