Uploaded image for project: '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:

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            timhunt 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
            timhunt 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            timhunt 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
            timhunt 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            timhunt 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
            timhunt 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            timhunt Tim Hunt added a comment -

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

            Show
            timhunt Tim Hunt added a comment - Revised version done using a lang string when the configuration option is on.
            Hide
            tsala 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
            tsala 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
            timhunt Tim Hunt added a comment -

            Submitting for integration.

            Show
            timhunt Tim Hunt added a comment - Submitting for integration.
            Hide
            timhunt 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
            timhunt 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
            stronk7 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
            stronk7 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
            rwijaya Rossiani Wijaya added a comment -

            This looks good.

            Test passed.

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

            Fixed STOP Closed STOP Thanks STOP

            Yay, imagination! Ciao

            Show
            stronk7 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:
                  Fix Release Date:
                  10/Sep/12