Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.4, 2.1.1, 2.2
    • Fix Version/s: 2.1.2, 2.2
    • Component/s: Course, Filters
    • Labels:
    • Testing Instructions:
      Hide
      1. Log in as an admin
      2. Enable the multilang filter (Settings > Plugins > Filters > Manage filters) and set it to be `on`
      3. Set the multilang filter to apply to both content and headings
      4. Enter a course and turn on editing
      5. Edit or create a course and give it a multilang shortname like the example at the bottom of these instructions.
      6. Browse around the site and make sure the correct course shortname is always shown. If you used the example string you simply need to check the `NZ` is never printed in the shortname. If it is then let me know where and we can look at properly formatting it.

      Areas to check

      • Backup and restore
      • The courses block
      • Blog
      • Calendar
      • Creating, editing and deleting a course
      • Course reports
      • Enrolments flatfile, ldap, meta, and paypal
      • Grade export
      • Grade overview report
      • Groups
      • Navigation
      • Modules: Assignment, chat, data, feedback, forum, glossary, quiz, scorm
      • Notes
      • Course tags
      • User course and site profiles

      Example multilang = <span class="multilang" lang="en">EN</span><span class="multilang" lang="NZ">NZ</span>

      Show
      Log in as an admin Enable the multilang filter (Settings > Plugins > Filters > Manage filters) and set it to be `on` Set the multilang filter to apply to both content and headings Enter a course and turn on editing Edit or create a course and give it a multilang shortname like the example at the bottom of these instructions. Browse around the site and make sure the correct course shortname is always shown. If you used the example string you simply need to check the `NZ` is never printed in the shortname. If it is then let me know where and we can look at properly formatting it. Areas to check Backup and restore The courses block Blog Calendar Creating, editing and deleting a course Course reports Enrolments flatfile, ldap, meta, and paypal Grade export Grade overview report Groups Navigation Modules: Assignment, chat, data, feedback, forum, glossary, quiz, scorm Notes Course tags User course and site profiles Example multilang = <span class="multilang" lang="en">EN</span><span class="multilang" lang="NZ">NZ</span>
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      wip-MDL-29189-master
    • Rank:
      18526

      Description

      This task involves reviewing all of the uses of course shortname for display to make sure it is formatted consistently

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          This will help resolve MDL-28305, MDL-28636, MDL-28697, and MDL-28698

          Show
          Sam Hemelryk added a comment - This will help resolve MDL-28305 , MDL-28636 , MDL-28697 , and MDL-28698
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          Putting this up for peer-review now.
          All three branches ensure that course shortname is now consistently formatted throughout Moodle.
          As well as that the master branch also increases the size of course.shortname so that it is actually usable for multilang formatting.

          Please note: I haven't added or changed formatting of course shortname when it is being used by PAGE->set_heading, PAGE->set_title, and navigation_node->get_content as there 3 functions are being evil and formatting strings internally themselves.
          There is an issue to see this fixed later on.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Putting this up for peer-review now. All three branches ensure that course shortname is now consistently formatted throughout Moodle. As well as that the master branch also increases the size of course.shortname so that it is actually usable for multilang formatting. Please note: I haven't added or changed formatting of course shortname when it is being used by PAGE->set_heading, PAGE->set_title, and navigation_node->get_content as there 3 functions are being evil and formatting strings internally themselves. There is an issue to see this fixed later on. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Rebased to weekly

          Show
          Sam Hemelryk added a comment - Rebased to weekly
          Hide
          Andrew Davis added a comment -

          Replace "Browse around the site and make sure the correct course shortname is always shown." with instructions telling the tester to check their user's language in their profile then check the locations before switching languages and rechecking. And explaining that they probably need to install a language pack to give them a 2nd language to select and that the 2nd language in their multi-lang course name must use this other language.

          What is the reason for always providign format_string() with a course context instead of allowing it to use the page context?

          Spelling error (form/from) in this comment in the upgrade code
          // Define index shortname (not unique) to be dropped form course

          Actually would you mind changing that comment to something like this?
          // MDL-29189 Increase course short name length to accommodate multi-lang strings. Requires dropping and readding an index.

          That way the comment tells you why this is being done rather than just telling you what is being done. Including the bug number is handy for the future.

          Avoid comments like the below one. You can read the code to figure out the what is being done so comments like this are more or less worthless.

          // Launch change of precision for field shortname
          $dbman->change_field_precision($table, $field);
          
          Show
          Andrew Davis added a comment - Replace "Browse around the site and make sure the correct course shortname is always shown." with instructions telling the tester to check their user's language in their profile then check the locations before switching languages and rechecking. And explaining that they probably need to install a language pack to give them a 2nd language to select and that the 2nd language in their multi-lang course name must use this other language. What is the reason for always providign format_string() with a course context instead of allowing it to use the page context? Spelling error (form/from) in this comment in the upgrade code // Define index shortname (not unique) to be dropped form course Actually would you mind changing that comment to something like this? // MDL-29189 Increase course short name length to accommodate multi-lang strings. Requires dropping and readding an index. That way the comment tells you why this is being done rather than just telling you what is being done. Including the bug number is handy for the future. Avoid comments like the below one. You can read the code to figure out the what is being done so comments like this are more or less worthless. // Launch change of precision for field shortname $dbman->change_field_precision($table, $field);
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,

          Thanks for looking at that patch for me.

          In regards to the points that you picked up, I will take a look at the instructions and will see if I can improve them. The user installing a second language and switching languages to check the filtering isn't required, the multilang filter strips all translations except for the user's current language or if there is no translation for that english. The example string I've got there will work perfectly for testing without regardless of the languages the user has installed, or what language they are using. Perhaps if I clarify things to say that they should check that NZ is not displayed anywhere in the page.

          Providing a context for format_string and format_text calls is a requirement. Internally those function resort to the PAGE context which aids us in ensuring backwards compatibility of those functions (implemented when we added context sensitive filtering) however the PAGE context is only correct if it happens to be the same context that the string was created within.
          When calling format_string and format_text we should always be providing the context that the string was created within, the reason being that the string may be created to take advantage of the filters applicable to the context and without those filters will likely result in garbage as is the case with the multilang filter.
          As an example imagine you have the multilang filter turned on within a specific course but no where else and the course shortname contains a multilang string when we print the frontpage course list if we don't provide the courses context format_string will resort to the PAGE->context which is the frontpage context where the filter is not turned on a consequently the multilang string won't be formatted as the user would have expected.

          As for the comments I had a wee chuckle about those, the comments you have picked up there are all comments generated automatically by the XMLDB editor (including the spelling mistake).
          I'll improve the comments shortly and will create an issue to see the typo in the XMLDB editor fixed up.
          The only thing that I won't do is add the bug number, I am very much against adding the bug number to a comment unless it is REALLY needed such as a TODO, the reason being that the bug number is only accurate until the next time the code is edited at which point anything can be going on and that comment will quickly become misleading. Personally I'd suggest always using a history tool such as git gui blame to work out the issues that have affected the code.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, Thanks for looking at that patch for me. In regards to the points that you picked up, I will take a look at the instructions and will see if I can improve them. The user installing a second language and switching languages to check the filtering isn't required, the multilang filter strips all translations except for the user's current language or if there is no translation for that english. The example string I've got there will work perfectly for testing without regardless of the languages the user has installed, or what language they are using. Perhaps if I clarify things to say that they should check that NZ is not displayed anywhere in the page. Providing a context for format_string and format_text calls is a requirement. Internally those function resort to the PAGE context which aids us in ensuring backwards compatibility of those functions (implemented when we added context sensitive filtering) however the PAGE context is only correct if it happens to be the same context that the string was created within. When calling format_string and format_text we should always be providing the context that the string was created within, the reason being that the string may be created to take advantage of the filters applicable to the context and without those filters will likely result in garbage as is the case with the multilang filter. As an example imagine you have the multilang filter turned on within a specific course but no where else and the course shortname contains a multilang string when we print the frontpage course list if we don't provide the courses context format_string will resort to the PAGE->context which is the frontpage context where the filter is not turned on a consequently the multilang string won't be formatted as the user would have expected. As for the comments I had a wee chuckle about those, the comments you have picked up there are all comments generated automatically by the XMLDB editor (including the spelling mistake). I'll improve the comments shortly and will create an issue to see the typo in the XMLDB editor fixed up. The only thing that I won't do is add the bug number, I am very much against adding the bug number to a comment unless it is REALLY needed such as a TODO, the reason being that the bug number is only accurate until the next time the code is edited at which point anything can be going on and that comment will quickly become misleading. Personally I'd suggest always using a history tool such as git gui blame to work out the issues that have affected the code. Cheers Sam
          Hide
          Aparup Banerjee added a comment -

          Hi Sam,
          That's a nice big patch , I've gone through it and here's my notes:

          • enrol/authorize/locallib.php ln 57=>66, 70 and elsewhere where $coursecontext is used is actually CONTEXT_USER - seems there's some mix up between contexts there and the variable seems misnamed too.
          • /lib/db/upgrade.php , comment '100 characters if practically useless' has typo.
          • /mod/assignment/lib.php ln 125, incorrect context variable used.
          • /mod/chat/gui_header_js/index.php ln 67 & /mod/chat/gui_sockets/index.php ln 63: whitespace before format_string()
          • mod/feedback/analysis_course.php ln 131 looks like incorrect variable used
          • /mod/feedback/mapcourse.php ln 119 has incorrect context variable

          cheers,
          Aparup

          Show
          Aparup Banerjee added a comment - Hi Sam, That's a nice big patch , I've gone through it and here's my notes: enrol/authorize/locallib.php ln 57=>66, 70 and elsewhere where $coursecontext is used is actually CONTEXT_USER - seems there's some mix up between contexts there and the variable seems misnamed too. /lib/db/upgrade.php , comment '100 characters if practically useless' has typo. /mod/assignment/lib.php ln 125, incorrect context variable used. /mod/chat/gui_header_js/index.php ln 67 & /mod/chat/gui_sockets/index.php ln 63: whitespace before format_string() mod/feedback/analysis_course.php ln 131 looks like incorrect variable used /mod/feedback/mapcourse.php ln 119 has incorrect context variable cheers, Aparup
          Hide
          Sam Hemelryk added a comment -

          Good spotting thanks Apu!
          I've made an additional commit to fix those, have cherry-picked it to all branches, and have pushed them up.
          All ready to go again

          Cheers
          SAm

          Show
          Sam Hemelryk added a comment - Good spotting thanks Apu! I've made an additional commit to fix those, have cherry-picked it to all branches, and have pushed them up. All ready to go again Cheers SAm
          Hide
          Sam Hemelryk added a comment -

          Linking to MDL-29240.

          Just a quick rundown on this issue I choose to format course.shortname consistently for this patch and in the mast branch increase the field length from 100 => 255 chars.

          Before this patch course shortname is formatted 50% of the time, and 50% not. Choosing to not format it would mean we have to find a solution for the moodle_page set_title + set_heading, and navigation_node::add which automatically format their input.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Linking to MDL-29240 . Just a quick rundown on this issue I choose to format course.shortname consistently for this patch and in the mast branch increase the field length from 100 => 255 chars. Before this patch course shortname is formatted 50% of the time, and 50% not. Choosing to not format it would mean we have to find a solution for the moodle_page set_title + set_heading, and navigation_node::add which automatically format their input. Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          My +1 for master and 21_STABLE (I would keep 20_STABLE out). It is a monster, I know. But we already had dozes of places supporting course->shortname being formatted. So why not extend it everywhere?

          About MDL-29240 it will need to be more carefully decided. I'm divided about who should have the responsibility of formatting. Let's discuss there and @ HQ.

          Ciao

          PS: Same applies to sister issues 100% (course->fullname, sections...)

          Show
          Eloy Lafuente (stronk7) added a comment - - edited My +1 for master and 21_STABLE (I would keep 20_STABLE out). It is a monster, I know. But we already had dozes of places supporting course->shortname being formatted. So why not extend it everywhere? About MDL-29240 it will need to be more carefully decided. I'm divided about who should have the responsibility of formatting. Let's discuss there and @ HQ. Ciao PS: Same applies to sister issues 100% (course->fullname, sections...)
          Hide
          Aparup Banerjee added a comment -

          This has been integrated, woot!
          Eloy, I've kept it out of 20 purely based on your +1, not even understood your reason. :-D

          Show
          Aparup Banerjee added a comment - This has been integrated, woot! Eloy, I've kept it out of 20 purely based on your +1, not even understood your reason. :-D
          Hide
          Rajesh Taneja added a comment -

          Works Great
          Thanks for fixing this Sam.

          Show
          Rajesh Taneja added a comment - Works Great Thanks for fixing this Sam.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          YTC !

          (aka, yay, thanks and ciao ) Closing.

          Show
          Eloy Lafuente (stronk7) added a comment - YTC ! (aka, yay, thanks and ciao ) Closing.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: