Moodle
  1. Moodle
  2. MDL-37558

Display the course short name for more pages "Display extended course names" setting is on.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.4, 2.4.1
    • Fix Version/s: 2.6
    • Component/s: Administration, Blocks
    • Labels:
    • Testing Instructions:
      Hide

      Test 1

      1. Log in as admin
      2. Enable courselistshortnames under course settings (Home ► Site administration ► Appearance ► Courses)
      3. Go to My home page
      4. Add Block "Course Overview" and Block "Courses"
      5. Make sure name of courses are like "COURSE_Short_NAME COURSE_FULL_NAME"
      6. Login as student
      7. Repeat Steps 3-5

      Test 2:

      1. Deselect courselistshortnames (See Test 1 steps 1-2)
      2. Repeat steps 3-7 from Test 1
      3. make sure you see course Fullname only.
      Show
      Test 1 Log in as admin Enable courselistshortnames under course settings (Home ► Site administration ► Appearance ► Courses) Go to My home page Add Block "Course Overview" and Block "Courses" Make sure name of courses are like "COURSE_Short_NAME COURSE_FULL_NAME" Login as student Repeat Steps 3-5 Test 2: Deselect courselistshortnames (See Test 1 steps 1-2) Repeat steps 3-7 from Test 1 make sure you see course Fullname only.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Rank:
      47210

      Description

      The tracker MDL-36259 added the ability to show the shortname on some of the admin pages. There are some others that I feel should also show the shortname. I am currently working on the patches for this for the below sections.

      • In regards to Meta courses displaying
        • /enrol/instances.php?id=##
        • /enrol/users.php?id=##
      • Blocks
        • Course Overview - giving it it's own option to show shortname

        Issue Links

          Activity

          Hide
          Rajesh Taneja added a comment -

          Thanks for reporting this and offer to provide a patch.

          I've put that on the backlog.

          If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.

          Show
          Rajesh Taneja added a comment - Thanks for reporting this and offer to provide a patch. I've put that on the backlog. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.
          Hide
          James Henestofel added a comment -

          I've finished my patch. Course Overview block from 2.3 to 2.4 had a major overhaul so I had to do it a little different for 2.3 but it works just the same.

          Show
          James Henestofel added a comment - I've finished my patch. Course Overview block from 2.3 to 2.4 had a major overhaul so I had to do it a little different for 2.3 but it works just the same.
          Hide
          Tim Hunt added a comment -

          I just had a quick look at the patch, and it looks OK to me, but could probably do with a more thorough peer review.

          Show
          Tim Hunt added a comment - I just had a quick look at the patch, and it looks OK to me, but could probably do with a more thorough peer review.
          Hide
          Rajesh Taneja added a comment -

          Thanks for the patch James,

          Few minor things you might want to consider:

          1. You might want to bump version for course_overview block (blocks/course_overview/version.php), as it's improvement.
          2. $course->fullname should use format_string, as before. (https://github.com/henesnarfel/moodle/compare/master...wip-MDL-37558-m25-showshortname#L1R110)

          FYI: Changed lines are within 180 chars, so they are fine (http://docs.moodle.org/dev/Coding_style#Maximum_Line_Length)

          Being improvement, not sure if this can go back to 23 and 24.

          Show
          Rajesh Taneja added a comment - Thanks for the patch James, Few minor things you might want to consider: You might want to bump version for course_overview block (blocks/course_overview/version.php), as it's improvement. $course->fullname should use format_string, as before. ( https://github.com/henesnarfel/moodle/compare/master...wip-MDL-37558-m25-showshortname#L1R110 ) FYI: Changed lines are within 180 chars, so they are fine ( http://docs.moodle.org/dev/Coding_style#Maximum_Line_Length ) Being improvement, not sure if this can go back to 23 and 24.
          Hide
          James Henestofel added a comment -

          1. increased the version date for today.
          2. added format string. Had to change it a little because it made it well beyond the 180 chars.

          As far as 23 and 24. I would like for at least the enrol shortname fix to go there. Not that worried about the course overview block fix being in 23 and 24.

          Show
          James Henestofel added a comment - 1. increased the version date for today. 2. added format string. Had to change it a little because it made it well beyond the 180 chars. As far as 23 and 24. I would like for at least the enrol shortname fix to go there. Not that worried about the course overview block fix being in 23 and 24.
          Hide
          Rajesh Taneja added a comment -

          Thanks for the quick fix James.
          Sorry you missed format_string at (blocks/course_overview/renderer.php line 110) https://github.com/henesnarfel/moodle/compare/master...wip-MDL-37558-m25-showshortname#L1R110

          I am adding integrators here to check if this can go back in 2.3
          AFAIK this issue could be split in two bugs

          1. Fixing enrol shortname (regression from MDL-36259)
          2. Improvement in course_overview block.

          Please wait for integrators to respond, before you do any changes.

          Cheers.

          Show
          Rajesh Taneja added a comment - Thanks for the quick fix James. Sorry you missed format_string at (blocks/course_overview/renderer.php line 110) https://github.com/henesnarfel/moodle/compare/master...wip-MDL-37558-m25-showshortname#L1R110 I am adding integrators here to check if this can go back in 2.3 AFAIK this issue could be split in two bugs Fixing enrol shortname (regression from MDL-36259 ) Improvement in course_overview block. Please wait for integrators to respond, before you do any changes. Cheers.
          Hide
          Dan Poltawski added a comment -

          The description makes this sound like a bug, not an improvement. i.e. the "Display extended course names" is not working eveywhere. Therefore, it makes sense to be back-ported.

          Looking (very quickly) at the code it looks like we are adding another setting for the course overview block. Is that necessary? Can it not respect the same setting?

          If we are introducing a new setting I do not think it can be backported.

          Show
          Dan Poltawski added a comment - The description makes this sound like a bug, not an improvement. i.e. the "Display extended course names" is not working eveywhere. Therefore, it makes sense to be back-ported. Looking (very quickly) at the code it looks like we are adding another setting for the course overview block. Is that necessary? Can it not respect the same setting? If we are introducing a new setting I do not think it can be backported.
          Hide
          Rajesh Taneja added a comment - - edited

          Thanks for quick response Dan,

          At first I thought the same, but unfortunately courselistshortnames can't be used for deciding course names on course_overview block as this is only meant for admin pages. So for sure this is an enhancement. But question remains, should this be mixed with course/category view? IMHO no.

          FYI:
          courselistshortnames - When showing lists of courses, or when referring to courses on administration screens, show the course short name as well as the full name. In fact, when you turn this setting on, the display uses the 'courseextendednamedisplay' language string, so you can changewhat is displayed using Language customisation.

          Show
          Rajesh Taneja added a comment - - edited Thanks for quick response Dan, At first I thought the same, but unfortunately courselistshortnames can't be used for deciding course names on course_overview block as this is only meant for admin pages. So for sure this is an enhancement. But question remains, should this be mixed with course/category view? IMHO no. FYI: courselistshortnames - When showing lists of courses, or when referring to courses on administration screens , show the course short name as well as the full name. In fact, when you turn this setting on, the display uses the 'courseextendednamedisplay' language string, so you can changewhat is displayed using Language customisation.
          Hide
          Rajesh Taneja added a comment -

          Hello. I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue.

          For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

          If you have any information about this issue or a possible fix please post it here

          Show
          Rajesh Taneja added a comment - Hello. I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment If you have any information about this issue or a possible fix please post it here
          Hide
          James Henestofel added a comment -

          Any more movement on this issue? If need be I can split out the course list block fix to another issue since it doesn't depend on the "Display extended course names" setting and has its own.

          Show
          James Henestofel added a comment - Any more movement on this issue? If need be I can split out the course list block fix to another issue since it doesn't depend on the "Display extended course names" setting and has its own.
          Hide
          Rajesh Taneja added a comment -

          Sorry for the delay, James.

          I am pushing it forward for peer-review.

          Show
          Rajesh Taneja added a comment - Sorry for the delay, James. I am pushing it forward for peer-review.
          Hide
          Frédéric Massart added a comment - - edited

          Hi everyone,

          here are some comments:

          • As mentioned before, I also think that the block course_overview should NOT define its own setting to control whether or not shortnames are displayed. It also seems that the setting definition has been changed as now it states: "If enabled, course short names will be displayed in addition to full names in course lists.". This setting is actually used to display the different course listings on the course page.
          • If ever course_overview needs its own setting, I don't really like it repeating the behaviour of get_course_display_name_for_list(), could we update this function to force the use of shortnames?
          • Enrolment seems OK to get this.
          • I don't think backport is an option:
            • If new setting is introduced
            • As the setting definition has changed between 2.5 and 2.6
          • The component could be specified in the commit message.
          • Also this conflicts as the master patch has been done for 2.5, rebase would be good .

          Cheers!
          Fred

          Show
          Frédéric Massart added a comment - - edited Hi everyone, here are some comments: As mentioned before, I also think that the block course_overview should NOT define its own setting to control whether or not shortnames are displayed. It also seems that the setting definition has been changed as now it states: "If enabled, course short names will be displayed in addition to full names in course lists." . This setting is actually used to display the different course listings on the course page. If ever course_overview needs its own setting, I don't really like it repeating the behaviour of get_course_display_name_for_list(), could we update this function to force the use of shortnames? Enrolment seems OK to get this. I don't think backport is an option: If new setting is introduced As the setting definition has changed between 2.5 and 2.6 The component could be specified in the commit message. Also this conflicts as the master patch has been done for 2.5, rebase would be good . Cheers! Fred
          Hide
          James Henestofel added a comment -

          Fred,

          • The reason the block has its own setting is because, as Rajesh mentioned, the courselistshortnames setting is only really meant for admin pages and since the course-list block isn't just for admins that's the reasoning for the custom setting. Either way is fine. I just need to see shortnames more often.
          • As for updating the function I'm not sure where that is and I think would need more input from others.
          • OK
          • that's fine to not backport. I was just learning how to use GIT and decided to make changes on all current versions
          • not sure exactly what you mean
          • didn't want to rebase until someone had a chance to look at it and let me know if anything needed done.

          Let me know. Thanks

          Show
          James Henestofel added a comment - Fred, The reason the block has its own setting is because, as Rajesh mentioned, the courselistshortnames setting is only really meant for admin pages and since the course-list block isn't just for admins that's the reasoning for the custom setting. Either way is fine. I just need to see shortnames more often. As for updating the function I'm not sure where that is and I think would need more input from others. OK that's fine to not backport. I was just learning how to use GIT and decided to make changes on all current versions not sure exactly what you mean didn't want to rebase until someone had a chance to look at it and let me know if anything needed done. Let me know. Thanks
          Hide
          Frédéric Massart added a comment -

          Hi James,

          indeed, Raj mentioned that and he was right, but it seems that we changed the setting definition which is now not restricted to admin pages any more (it is used on the front page), so it seems OK not to have a specific setting for the course overview. About the commit message, it's part of our guide lines to provide a descriptive commit message, it's not blocking your issue, but as I noticed it I thought I'd mention it (http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages).

          About the function to be modified if the setting remain, and the backport, those are just my thoughts and I'm just raising the points during the peer review. This is not blocking your issue, but you're welcome to consider them before you decide to push for integration .

          Cheers!
          Fred

          Show
          Frédéric Massart added a comment - Hi James, indeed, Raj mentioned that and he was right, but it seems that we changed the setting definition which is now not restricted to admin pages any more (it is used on the front page), so it seems OK not to have a specific setting for the course overview. About the commit message, it's part of our guide lines to provide a descriptive commit message, it's not blocking your issue, but as I noticed it I thought I'd mention it ( http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages ). About the function to be modified if the setting remain, and the backport, those are just my thoughts and I'm just raising the points during the peer review. This is not blocking your issue, but you're welcome to consider them before you decide to push for integration . Cheers! Fred
          Hide
          Rajesh Taneja added a comment -

          Thanks for spotting that Fred,

          It seems the change has come while merging English strings with en_fix (MDL-39642), not sure if we have a changed our stand on this configuration setting.
          But yes, as the definition is not restricted to admin pages, my +1 for not adding any more configurations.

          James, let me know if you want me to do anything on this.

          Show
          Rajesh Taneja added a comment - Thanks for spotting that Fred, It seems the change has come while merging English strings with en_fix ( MDL-39642 ), not sure if we have a changed our stand on this configuration setting. But yes, as the definition is not restricted to admin pages, my +1 for not adding any more configurations. James, let me know if you want me to do anything on this.
          Hide
          James Henestofel added a comment -

          Raj,
          If we are going to negate the new block setting and use the current courselistshortnames setting it may be best to scrap what I've done and start over. I'm assuming since 2.5 is already out this would be a new feature for 2.6.

          So after the holiday weekend I can work on this again and try and find any areas I feel that need the shortname listed. I'll propose the places I would like to see the shortname added to see if it would be alright before I work on it.

          Show
          James Henestofel added a comment - Raj, If we are going to negate the new block setting and use the current courselistshortnames setting it may be best to scrap what I've done and start over. I'm assuming since 2.5 is already out this would be a new feature for 2.6. So after the holiday weekend I can work on this again and try and find any areas I feel that need the shortname listed. I'll propose the places I would like to see the shortname added to see if it would be alright before I work on it.
          Hide
          Rajesh Taneja added a comment -

          Thanks for working on this James,

          Yes, it make sense to start over again and yes it will be enhancement which will be master only.

          I am going to remove myself as assignee, but will keep watching it, to give a true sense of ownership.

          Show
          Rajesh Taneja added a comment - Thanks for working on this James, Yes, it make sense to start over again and yes it will be enhancement which will be master only. I am going to remove myself as assignee, but will keep watching it, to give a true sense of ownership.
          Hide
          Tim Hunt added a comment -

          Hand on a second. The fact that the course short name is not shown in the manage metacourse enrolment UI, even when the admin settings is turned on, is a bug. We are waiting for the fix at the OU. (In 2.4)

          The enrol/meta/lib.php change needs to go into all stable branches. Will you guys take care of that, or do I need to create a separate issue and submit that myself?

          Show
          Tim Hunt added a comment - Hand on a second. The fact that the course short name is not shown in the manage metacourse enrolment UI, even when the admin settings is turned on, is a bug. We are waiting for the fix at the OU. (In 2.4) The enrol/meta/lib.php change needs to go into all stable branches. Will you guys take care of that, or do I need to create a separate issue and submit that myself?
          Hide
          James Henestofel added a comment -

          Tim,
          I think you are right about the meta lib change being a bug. There actually may be more admin places that I think the shortname needs to show but I would have to look back through 2.2 to find any other admin places it should have been showing.

          As for the block change I guess that would be a new feature since they are now allowing that setting to affect non-admin pages. I'm going to look through any non-admin pages/blocks that I think should have the shortname shown and propose them here.

          IN SHORT...it would probably be good to have a separate issue with this being the new feature fix and the other(being handled by you if you would like) being the bug fixes back to 2.2 for the courselistshortnames

          Show
          James Henestofel added a comment - Tim, I think you are right about the meta lib change being a bug. There actually may be more admin places that I think the shortname needs to show but I would have to look back through 2.2 to find any other admin places it should have been showing. As for the block change I guess that would be a new feature since they are now allowing that setting to affect non-admin pages. I'm going to look through any non-admin pages/blocks that I think should have the shortname shown and propose them here. IN SHORT...it would probably be good to have a separate issue with this being the new feature fix and the other(being handled by you if you would like) being the bug fixes back to 2.2 for the courselistshortnames
          Hide
          Tim Hunt added a comment -

          OK, I will make a separate issue for the bug fix, and submit it.

          Show
          Tim Hunt added a comment - OK, I will make a separate issue for the bug fix, and submit it.
          Hide
          Tim Hunt added a comment -

          MDL-40527 is the new issue.

          Show
          Tim Hunt added a comment - MDL-40527 is the new issue.
          Hide
          James Henestofel added a comment - - edited

          As of right now. The only places that I feel that could use the courselistshortnames setting.

          • Course List Block
          • Course Overview Block

          If its alright with everyone I will go ahead and change these blocks to use the courselistshortnames setting. Also any other pages that anyone else feels should use it.

          Also I was wondering about thoughts on being able to change the order that the course names are listed. So we could change from (SHORTNAME FULLNAME) to (FULLNAME SHORTNAME).

          Show
          James Henestofel added a comment - - edited As of right now. The only places that I feel that could use the courselistshortnames setting. Course List Block Course Overview Block If its alright with everyone I will go ahead and change these blocks to use the courselistshortnames setting. Also any other pages that anyone else feels should use it. Also I was wondering about thoughts on being able to change the order that the course names are listed. So we could change from (SHORTNAME FULLNAME) to (FULLNAME SHORTNAME).
          Hide
          James Henestofel added a comment -

          I've went ahead and implemented the fix for the Blocks.

          Also I think I've answered my question about the order of the Shortname and Fullname. I can just use the language customization tool to display it how I would like.

          Show
          James Henestofel added a comment - I've went ahead and implemented the fix for the Blocks. Also I think I've answered my question about the order of the Shortname and Fullname. I can just use the language customization tool to display it how I would like.
          Hide
          James Henestofel added a comment -

          Any movement or thoughts on this??

          Show
          James Henestofel added a comment - Any movement or thoughts on this??
          Hide
          Frédéric Massart added a comment -

          Hi James,

          sorry I missed your comments, is this ready for a second peer review?

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Hi James, sorry I missed your comments, is this ready for a second peer review? Cheers, Fred
          Hide
          James Henestofel added a comment -

          Fred,
          Sure, I think its ready for the review. Would you like this to be rebased first or after the review if this gets integrated?

          Show
          James Henestofel added a comment - Fred, Sure, I think its ready for the review. Would you like this to be rebased first or after the review if this gets integrated?
          Hide
          Frédéric Massart added a comment -

          I will review it again without the rebase. Thanks.

          Show
          Frédéric Massart added a comment - I will review it again without the rebase. Thanks.
          Hide
          Frédéric Massart added a comment -

          Hi James,

          thanks for the patch. You just have a tiny indentation issue in block/course_overview. Also you have removed the 2nd and 3rd parameter sent to format_string(), I don't think you should.

          Please fix those, rebase, and I will then push your patch for integration.

          Many thanks!

          Fred

          Show
          Frédéric Massart added a comment - Hi James, thanks for the patch. You just have a tiny indentation issue in block/course_overview. Also you have removed the 2nd and 3rd parameter sent to format_string(), I don't think you should. Please fix those, rebase, and I will then push your patch for integration. Many thanks! Fred
          Hide
          James Henestofel added a comment -

          Hi Fred,
          I fixed those issues and rebased. Thank you very much for your work.

          Show
          James Henestofel added a comment - Hi Fred, I fixed those issues and rebased. Thank you very much for your work.
          Hide
          Frédéric Massart added a comment -

          Thank you!

          Show
          Frédéric Massart added a comment - Thank you!
          Hide
          Sam Hemelryk added a comment -

          Thanks James this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks James this has been integrated now
          Hide
          Andrew Nicols added a comment -

          Passing - works as described.

          Show
          Andrew Nicols added a comment - Passing - works as described.
          Hide
          Dan Poltawski added a comment -

          You did it!

          Thanks for your contribution, this change has been merged upstream and is now available on your local git mirror and on download sites shortly.

          Show
          Dan Poltawski added a comment - You did it! Thanks for your contribution, this change has been merged upstream and is now available on your local git mirror and on download sites shortly.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: