Moodle
  1. Moodle
  2. MDL-36259

Some pages does not display the course short name even if "Display extended course names" setting is on.

    Details

    • Testing Instructions:
      Hide

      Please test on all DBs. Particularly oracle.

      Turn on the setting "Display extended course names" setting on Site administration > Appearance > Courses.

      Go these page URLs:
      1) /user/profile.php - View a user's course-specific profile page.
      2) /user/view.php - View a user's full profile page.

      For (1) and (2) the user profile will be displayed and the "Course profiles" field should list courses with the shortname as the prefix.

      3) /enrol/meta/addinstance.php (Enable the course meta enrolment method then add it to a course)

      The dropdown list of courses should list courses with the shortname as the prefix.

      4) /course/search.php?modulelist... - Go to Admin -> Plugins -> Manage activities, and click on the link that tells you there are n forums, to see which courses those forums are in.
      5) /course/search.php?blocklist... - Similar, but for blocks.

      For (4) and (5) the list of courses should display with the shortname as the prefix when in editing is turned on.

      6) /report/log/index.php - the logs report - site-wide, course or activity.

      The dropdown course select lists display courses with the shortname as the prefix.

      Show
      Please test on all DBs. Particularly oracle. Turn on the setting "Display extended course names" setting on Site administration > Appearance > Courses. Go these page URLs: 1) /user/profile.php - View a user's course-specific profile page. 2) /user/view.php - View a user's full profile page. For (1) and (2) the user profile will be displayed and the "Course profiles" field should list courses with the shortname as the prefix. 3) /enrol/meta/addinstance.php (Enable the course meta enrolment method then add it to a course) The dropdown list of courses should list courses with the shortname as the prefix. 4) /course/search.php?modulelist... - Go to Admin -> Plugins -> Manage activities, and click on the link that tells you there are n forums, to see which courses those forums are in. 5) /course/search.php?blocklist... - Similar, but for blocks. For (4) and (5) the list of courses should display with the shortname as the prefix when in editing is turned on. 6) /report/log/index.php - the logs report - site-wide, course or activity. The dropdown course select lists display courses with the shortname as the prefix.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      This issue reports and provides the fix to improve on the work done in MDL-34704.

      A few admin screens display course fullname that does not respect the setting "Display extended course names" setting on Site administration > Appearance > Courses.

      I went through the Moodle site/code and found 4 areas of the site to modify and improve:
      1) When viewing user profile, the course profiles field does not respect the setting (/user/profile.php and /user/view.php)
      2) Course metadata link page's dropdown (/enrol/instances.php)
      3) The pages that list the courses a module or block is found in (/moodle/course/search.php?modulelist... and /course/search.php?blocklist....)
      4) The course dropdown filter when viewing logs (/report/log/index.php)

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Michael de Raadt added a comment -

            Thanks for reporting that and providing a patch.

            Tim: I've assigned this to you as you were involved in the linked issue. Feel free to assign to Petr if you don't wish to take it.

            Show
            Michael de Raadt added a comment - Thanks for reporting that and providing a patch. Tim: I've assigned this to you as you were involved in the linked issue. Feel free to assign to Petr if you don't wish to take it.
            Hide
            Tim Hunt added a comment -

            This is almost spot on. Just one minor performance issue in user/profile.php. Moving the code to get context instance out of the loop means that we will be requesting more contexts, which may mean more DB queries.

            Whether or not it does mean more DB queries is actually a complex question, because of the caching.

            In this case it is a problem, but is fixable. If you look at some other places that use enrol_get_all_users_courses, you will see that they call context_helper::preload_from_record($rec); which avoids the extra DB query. You should add that to profile.php.

            Sorry, I have not explained that very well, but if you look at the code, you should be able to work out what is going on. If not, I can explain on Monday.

            Show
            Tim Hunt added a comment - This is almost spot on. Just one minor performance issue in user/profile.php. Moving the code to get context instance out of the loop means that we will be requesting more contexts, which may mean more DB queries. Whether or not it does mean more DB queries is actually a complex question, because of the caching. In this case it is a problem, but is fixable. If you look at some other places that use enrol_get_all_users_courses, you will see that they call context_helper::preload_from_record($rec); which avoids the extra DB query. You should add that to profile.php. Sorry, I have not explained that very well, but if you look at the code, you should be able to work out what is going on. If not, I can explain on Monday.
            Hide
            Thanh Le added a comment -

            Tim: I have prepared a single commit for fixing the remaining areas on Moodle where shortname is not displayed correctly when requested in the admin settings. The code is based on the latest Moodle core code.

            The commit can be found at:
            https://github.com/dtle/moodle/tree/MDL-36259_final

            Diff:
            https://github.com/dtle/moodle/commit/1fefec8140a5d9f4718c4bea5ea0fc904ea992a6

            Should be ready for integration into Moodle.

            Show
            Thanh Le added a comment - Tim: I have prepared a single commit for fixing the remaining areas on Moodle where shortname is not displayed correctly when requested in the admin settings. The code is based on the latest Moodle core code. The commit can be found at: https://github.com/dtle/moodle/tree/MDL-36259_final Diff: https://github.com/dtle/moodle/commit/1fefec8140a5d9f4718c4bea5ea0fc904ea992a6 Should be ready for integration into Moodle.
            Hide
            Tim Hunt added a comment -

            https://github.com/dtle/moodle/compare/master...MDL-36259 still contains other commits.

            Now, either the separate commits make the change easier to understand, in which case you need to fix the commit comments (http://docs.moodle.org/dev/Commit_cheat_sheet); or you need to squash it down to one commit, because that is more appropriate.

            Show
            Tim Hunt added a comment - https://github.com/dtle/moodle/compare/master...MDL-36259 still contains other commits. Now, either the separate commits make the change easier to understand, in which case you need to fix the commit comments ( http://docs.moodle.org/dev/Commit_cheat_sheet ); or you need to squash it down to one commit, because that is more appropriate.
            Hide
            Tim Hunt added a comment -

            1. context_helper::preload_from_record returns the new context, so

            context_helper::preload_from_record($mycourse);
            $ccontext = context_course::instance($mycourse->id);
            

            would be clearer as

            $ccontext = context_helper::preload_from_record($mycourse);
            

            Do you think it is worth making the change?

            2. Also, the Pull Master Diff URL: currently goes to a 404 page (presumably because amending the commit changed the hash). Changing this to the https://github.com/dtle/moodle/compare/master...MDL-36259 form is more robust.

            3. Do we want to try to get this into the MOODLE_23_STABLE branch too? This is a bug fix. If so, you need to cherry-pick the fix (once we are happy it is finished.) (See http://tjhunt.blogspot.co.uk/2012/03/fixing-bug-in-moodle-core-mechanics.html if you want a reminder, or look at some of the other bugs on http://tracker.moodle.org/secure/Dashboard.jspa?selectPageId=11350 to see what state you are trying to get the bug into.)

            Show
            Tim Hunt added a comment - 1. context_helper::preload_from_record returns the new context, so context_helper::preload_from_record($mycourse); $ccontext = context_course::instance($mycourse->id); would be clearer as $ccontext = context_helper::preload_from_record($mycourse); Do you think it is worth making the change? 2. Also, the Pull Master Diff URL: currently goes to a 404 page (presumably because amending the commit changed the hash). Changing this to the https://github.com/dtle/moodle/compare/master...MDL-36259 form is more robust. 3. Do we want to try to get this into the MOODLE_23_STABLE branch too? This is a bug fix. If so, you need to cherry-pick the fix (once we are happy it is finished.) (See http://tjhunt.blogspot.co.uk/2012/03/fixing-bug-in-moodle-core-mechanics.html if you want a reminder, or look at some of the other bugs on http://tracker.moodle.org/secure/Dashboard.jspa?selectPageId=11350 to see what state you are trying to get the bug into.)
            Hide
            Tim Hunt added a comment -

            Thanh pointed out that context_helper::preload_from_record does not actually return the record, but it looks to me like that might be a bug: MDL-37115.

            Show
            Tim Hunt added a comment - Thanh pointed out that context_helper::preload_from_record does not actually return the record, but it looks to me like that might be a bug: MDL-37115 .
            Hide
            Tim Hunt added a comment -

            However, all the other places in the code that call context_helper::preload_from_record assume it does not return anything, so perhaps I am just confused.

            Apart from that, the patch looks good, apart from one thing. get_context_name already calls format_string on the result, so we should not call it again.

            (get_course_display_name_for_list does not call format_string, and so we must call it ourselves, and it seems we do.)

            So, I have one small fixup commit: https://github.com/timhunt/moodle/commit/8049f45210e3b19820d014e4144dc52836aa5754, and I think that the change that should be submitted to integration is https://github.com/timhunt/moodle/compare/MDL-36259

            Thanh, could you take a quick look at that, and confirm you are happy, then I will squash it back down to one commit, and submit for integration. Thanks.

            Show
            Tim Hunt added a comment - However, all the other places in the code that call context_helper::preload_from_record assume it does not return anything, so perhaps I am just confused. Apart from that, the patch looks good, apart from one thing. get_context_name already calls format_string on the result, so we should not call it again. (get_course_display_name_for_list does not call format_string, and so we must call it ourselves, and it seems we do.) So, I have one small fixup commit: https://github.com/timhunt/moodle/commit/8049f45210e3b19820d014e4144dc52836aa5754 , and I think that the change that should be submitted to integration is https://github.com/timhunt/moodle/compare/MDL-36259 Thanh, could you take a quick look at that, and confirm you are happy, then I will squash it back down to one commit, and submit for integration. Thanks.
            Hide
            Thanh Le added a comment -

            Tim, I'm OK with your revised changes. Feel free to integrate it.

            Show
            Thanh Le added a comment - Tim, I'm OK with your revised changes. Feel free to integrate it.
            Hide
            Tim Hunt added a comment -

            I think this should go onto stable branches (probably only 2.3+) because the currently display of course names with shortnames is inconsistent, and that is a bug. However, I will leave the final decision with the integrators.

            Please cherry-pick master -> 2.4.

            Show
            Tim Hunt added a comment - I think this should go onto stable branches (probably only 2.3+) because the currently display of course names with shortnames is inconsistent, and that is a bug. However, I will leave the final decision with the integrators. Please cherry-pick master -> 2.4.
            Hide
            Dan Poltawski added a comment -

            I've integrated this now (master, 24 and 23).

            Please could you expand the testing instructions though as I do not know what is meant by steps 4 and 5. More detail on this would help the tester significantly.

            Show
            Dan Poltawski added a comment - I've integrated this now (master, 24 and 23). Please could you expand the testing instructions though as I do not know what is meant by steps 4 and 5. More detail on this would help the tester significantly.
            Hide
            Dan Poltawski added a comment -

            ps. I added a commit to correct the SQL alias case to match its definition. Not sure if that would actually cause problems - I think it might on sql server.

            Show
            Dan Poltawski added a comment - ps. I added a commit to correct the SQL alias case to match its definition. Not sure if that would actually cause problems - I think it might on sql server.
            Hide
            Tim Hunt added a comment -

            Thanks Dan. I am impressed at how sharp your eyes are, we completely missed that case mismatch. I have also tried to clarify the testing instructions.

            Show
            Tim Hunt added a comment - Thanks Dan. I am impressed at how sharp your eyes are, we completely missed that case mismatch. I have also tried to clarify the testing instructions.
            Hide
            Damyon Wiese added a comment -

            Amended the test instructions for step 3 (Looked at the patch to see what was meant).

            Show
            Damyon Wiese added a comment - Amended the test instructions for step 3 (Looked at the patch to see what was meant).
            Hide
            Damyon Wiese added a comment -

            Passed on all databases on 23 and master.

            Yay!

            Show
            Damyon Wiese added a comment - Passed on all databases on 23 and master. Yay!
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Changes are now upstream, thanks for your collaboration!

            If you are going to have any celebration next days, enjoy with your gang, if not, too!

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Changes are now upstream, thanks for your collaboration! If you are going to have any celebration next days, enjoy with your gang, if not, too! Ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: