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:
    • Rank:
      45042

      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)

        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: