Moodle
  1. Moodle
  2. MDL-34704

context_course::get_context_name should respect $CFG->courselistshortnames

    Details

    • Testing Instructions:
      Hide

      This is about what happens when the setting "Display extended course names" setting on Site administration > Appearance > Courses is turned on. Really you should test both with that on, to make sure it works, and with it off, to make sure there are no regressions.

      1. Go to a user's profile.

      2. Click the Roles > This user's role assignments link in the setting block.

      3. Verify that any course names are shown with short-name first (if the admin setting is on).

      4. Check index.php, course/index.php and course/category.php, to make sure that course names there are also shown with the short-name first (if the admin setting is on).

      Show
      This is about what happens when the setting "Display extended course names" setting on Site administration > Appearance > Courses is turned on. Really you should test both with that on, to make sure it works, and with it off, to make sure there are no regressions. 1. Go to a user's profile. 2. Click the Roles > This user's role assignments link in the setting block. 3. Verify that any course names are shown with short-name first (if the admin setting is on). 4. Check index.php, course/index.php and course/category.php, to make sure that course names there are also shown with the short-name first (if the admin setting is on).
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      43166

      Description

      This got reported because admin/roles/usersroles.php does not display the course short name even if this config options is turned on.

      I think it makes sense to respect that option whenever we print the non-short version of the course context name.

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          Petr, does this make sense to you?

          I expect OU people would like this to go into all branches 2.2+. What do you think?

          Show
          Tim Hunt added a comment - Petr, does this make sense to you? I expect OU people would like this to go into all branches 2.2+. What do you think?
          Hide
          Petr Škoda added a comment -

          I am not sure the get_course_display_name_for_list() is appropriate here, I would not personally expect it to be used there because context names do not know where they are used - in list or not. +0

          Show
          Petr Škoda added a comment - I am not sure the get_course_display_name_for_list() is appropriate here, I would not personally expect it to be used there because context names do not know where they are used - in list or not. +0
          Hide
          Tim Hunt added a comment -

          I just did a search, the places where get_context_name is called are in admin-y parts of the interface. (There are only 15 instances if you want to check yourself.) They are all places that could probably count as lists of courses.

          I think that any Moodle site that turns on $CFG->courselistshortnames would like to have the course shortname in all those places.

          Show
          Tim Hunt added a comment - I just did a search, the places where get_context_name is called are in admin-y parts of the interface. (There are only 15 instances if you want to check yourself.) They are all places that could probably count as lists of courses. I think that any Moodle site that turns on $CFG->courselistshortnames would like to have the course shortname in all those places.
          Hide
          Petr Škoda added a comment -

          Hmm, why is get_course_display_name_for_list() not using internally a language string instead? The hardcoded concat seems a bit weird to me there. What if somebody wants idnumber there? Or the shortname at the end in square brackets?

          Show
          Petr Škoda added a comment - Hmm, why is get_course_display_name_for_list() not using internally a language string instead? The hardcoded concat seems a bit weird to me there. What if somebody wants idnumber there? Or the shortname at the end in square brackets?
          Hide
          Tim Hunt added a comment -

          I don't know. I just moved the definition of that function to a different file, so I did not have to require_once course/lib.php. If you want to create a separate issue about fixing that function, please do, but please can we not let that confuse the issue here.

          Show
          Tim Hunt added a comment - I don't know. I just moved the definition of that function to a different file, so I did not have to require_once course/lib.php. If you want to create a separate issue about fixing that function, please do, but please can we not let that confuse the issue here.
          Hide
          Petr Škoda added a comment -

          sorry, but I think that get_course_display_name_for_list() is an ugly hack, I personally do not mind if this gets integrated or not, but I am not going to encourage you.

          Show
          Petr Škoda added a comment - sorry, but I think that get_course_display_name_for_list() is an ugly hack, I personally do not mind if this gets integrated or not, but I am not going to encourage you.
          Hide
          Tim Hunt added a comment -

          Revised version done using a lang string when the configuration option is on.

          Show
          Tim Hunt added a comment - Revised version done using a lang string when the configuration option is on.
          Hide
          Helen Foster added a comment -

          Hi Tim,

          I have reviewed the new language strings as requested and find that having a language string '{$a->shortname} {$a->fullname}' to be very odd. Surely site config stuff should be controlled by admin settings and not via language customisation, and the use of place holders in language strings should be restricted to language-specific issues?

          Adding David as a watcher in case he wishes to comment.

          Show
          Helen Foster added a comment - Hi Tim, I have reviewed the new language strings as requested and find that having a language string '{$a->shortname} {$a->fullname}' to be very odd. Surely site config stuff should be controlled by admin settings and not via language customisation, and the use of place holders in language strings should be restricted to language-specific issues? Adding David as a watcher in case he wishes to comment.
          Hide
          Tim Hunt added a comment -

          Submitting for integration.

          Show
          Tim Hunt added a comment - Submitting for integration.
          Hide
          Tim Hunt added a comment -

          I actually submitted this for integration before seeing Helen's comment. We had a discussion about it: http://moodle.org/local/chatlogs/index.php?conversationid=10742

          I still think my patch is the way to go.

          (If we were starting from scratch now, I would be inclined to not use an admin setting at all, just use a lang string, but it is too late for that now.)

          Show
          Tim Hunt added a comment - I actually submitted this for integration before seeing Helen's comment. We had a discussion about it: http://moodle.org/local/chatlogs/index.php?conversationid=10742 I still think my patch is the way to go. (If we were starting from scratch now, I would be inclined to not use an admin setting at all, just use a lang string, but it is too late for that now.)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Switching issue type to bug. Observing the setting everywhere is correct, not doing it, incorrect.

          Integrated (22, 23 and master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Switching issue type to bug. Observing the setting everywhere is correct, not doing it, incorrect. Integrated (22, 23 and master), thanks!
          Hide
          Rossiani Wijaya added a comment -

          This looks good.

          Test passed.

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

          Fixed STOP Closed STOP Thanks STOP

          Yay, imagination! Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Fixed STOP Closed STOP Thanks STOP Yay, imagination! Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: