Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-39833

Change course Topic "Not available" string to something more informative (require token change)

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.7, 2.4.4, 2.5
    • Fix Version/s: 2.7
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide

      Here is the testing instruction.

      1. Create a course.

      2. login as admin & Go to course/view.

      3. In the course settings set the course for "display hidden section as not available"

      4. turn editing on, and hide view sections/topics.

      5. login as student and see how they appears.

      They should have information about section or topic not available e.g. "PHP is not available".

      Show
      Here is the testing instruction. 1. Create a course. 2. login as admin & Go to course/view. 3. In the course settings set the course for "display hidden section as not available" 4. turn editing on, and hide view sections/topics. 5. login as student and see how they appears. They should have information about section or topic not available e.g. "PHP is not available".
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      When a teacher set the course for "display hidden section as not available", the students see a "Not available" message on the course front page which is not so clear to them. (as seen on the attached screen capture)

      I am suggesting to make it more clear by (maybe) changing the message along with the string token. Since get_string('notavailable') is used on other situations across moodle in which the context is correct and need not to change.

      Current code (course/format/renderer.php - 559):

      $o.= get_string('notavailable');
      

      Some suggestions:
      "Section 4 - Not available"

      $o.= get_string('section').' '.$sectionno.' - '.get_string('notavailable');
      

      "Section XYZ - Not available"

      // populate $section using $sectionnum
      $o.= get_string('section').' '.$section->name.' - '.get_string('notavailable');
      

      "Section XYZ is Not available at the moment"

      // populate $section using $sectionnum
      $o.= get_string('sectionnotavailable','mooodle',array(sectionname=>$section->name));
      

        Gliffy Diagrams

          Activity

          Hide
          marina Marina Glancy added a comment -

          Hi
          thanks for reporting that. We have a function that returns the proper section name, it should be used here: get_section_name($course, $section)
          Also it is very not recommended to use strings concatenation because we must always think about non-English languages.

          I would suggest the availability message to look very similar to the availability message of the module,
          html_writer::tag('div', get_string('notavailable'), array('class' => 'availabilityinfo'));

          This will result with:
          <div class="availabilityinfo">Not available</div>

          Marina

          Show
          marina Marina Glancy added a comment - Hi thanks for reporting that. We have a function that returns the proper section name, it should be used here: get_section_name($course, $section) Also it is very not recommended to use strings concatenation because we must always think about non-English languages. I would suggest the availability message to look very similar to the availability message of the module, html_writer::tag('div', get_string('notavailable'), array('class' => 'availabilityinfo')); This will result with: <div class="availabilityinfo">Not available</div> Marina
          Hide
          nadavkav Nadav Kavalerchik added a comment -

          Hi
          I accept the technical recommendations, but I think the string is too simplified. It should also include more information about what is not available. Like section's title or number or week or dates. Example:

          <div class="availabilityinfo">Section "Homework" Not available</div>

          Otherwise, when several sections are "not available" it looks unclear to the students which get confused.
          It looks like:

          Not available
          Not available
          Not available
          Not available
          Not available

          And the students think it is a system malfunction.

          Show
          nadavkav Nadav Kavalerchik added a comment - Hi I accept the technical recommendations, but I think the string is too simplified. It should also include more information about what is not available. Like section's title or number or week or dates. Example: <div class="availabilityinfo">Section "Homework" Not available</div> Otherwise, when several sections are "not available" it looks unclear to the students which get confused. It looks like: Not available Not available Not available Not available Not available And the students think it is a system malfunction.
          Hide
          marina Marina Glancy added a comment - - edited

          Nadav, I did not mean that "Not available" will be instead of section name. Rather below, like it is for unavailable modules. Currently one of our GSoC students is trying to work on this project so my previous message was more for him.

          Show
          marina Marina Glancy added a comment - - edited Nadav, I did not mean that "Not available" will be instead of section name. Rather below, like it is for unavailable modules. Currently one of our GSoC students is trying to work on this project so my previous message was more for him.
          Hide
          marina Marina Glancy added a comment -

          I confirm that this is a bug introduced in 2.3. In Moodle 2.2 the section name is displayed

          Show
          marina Marina Glancy added a comment - I confirm that this is a bug introduced in 2.3. In Moodle 2.2 the section name is displayed
          Hide
          nadavkav Nadav Kavalerchik added a comment -

          Thank you.

          Do you have a link to the GSOC project?
          Or should I suggest a quick fix?

          Show
          nadavkav Nadav Kavalerchik added a comment - Thank you. Do you have a link to the GSOC project? Or should I suggest a quick fix?
          Hide
          shashitechno Shashikant vaishnav added a comment - - edited

          Thank you Ma'am

          Looking over the Discussion its clear that using html_writer is the efficient way for outputting HTML tags rather then using string concatenation.

          It can be done something like this:-

          html_writer::tag('div',get_section_name($course, $section), array('class' => 'notavailable'));
          

          Show
          shashitechno Shashikant vaishnav added a comment - - edited Thank you Ma'am Looking over the Discussion its clear that using html_writer is the efficient way for outputting HTML tags rather then using string concatenation. It can be done something like this:- html_writer::tag('div',get_section_name($course, $section), array('class' => 'notavailable'));
          Hide
          marina Marina Glancy added a comment -

          Hi Shashikant, CSS class 'notavailable' is not used in Moodle.
          Leave the string 'Not available' and make the CSS class for it 'availabilityinfo'
          See how it looks for not available modules

          Show
          marina Marina Glancy added a comment - Hi Shashikant, CSS class 'notavailable' is not used in Moodle. Leave the string 'Not available' and make the CSS class for it 'availabilityinfo' See how it looks for not available modules
          Hide
          shashitechno Shashikant vaishnav added a comment -

          Just fixed the issue, Here is the git diff for moodle 25 stable release.
          https://github.com/shashirepo/moodle/compare/master...MDL-39833_m25

          Show
          shashitechno Shashikant vaishnav added a comment - Just fixed the issue, Here is the git diff for moodle 25 stable release. https://github.com/shashirepo/moodle/compare/master...MDL-39833_m25
          Hide
          cibot CiBoT added a comment -

          Results for MDL-39833

          • Error: the repository field is empty. Nothing was checked.
          Show
          cibot CiBoT added a comment - Results for MDL-39833 Error: the repository field is empty. Nothing was checked.
          Hide
          cibot CiBoT added a comment -

          Results for MDL-39833

          Show
          cibot CiBoT added a comment - Results for MDL-39833 Remote repository: https://github.com/shashirepo/moodle Remote branch MDL-39833 _m25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1594 Error: The MDL-39833 _m25 branch at https://github.com/shashirepo/moodle is very old (>60 days ago). Please rebase against current MOODLE_25_STABLE.
          Hide
          marina Marina Glancy added a comment -

          Shashikant, you can not change the status to "Peer review in progress" if you are not a peer reviewer.
          I do not volunteer to peer review this issue.

          Show
          marina Marina Glancy added a comment - Shashikant, you can not change the status to "Peer review in progress" if you are not a peer reviewer. I do not volunteer to peer review this issue.
          Hide
          shashitechno Shashikant vaishnav added a comment -

          Sorry, I was not aware of that.

          Show
          shashitechno Shashikant vaishnav added a comment - Sorry, I was not aware of that.
          Hide
          cibot CiBoT added a comment -

          Results for MDL-39833

          Show
          cibot CiBoT added a comment - Results for MDL-39833 Remote repository: https://github.com/shashirepo/moodle Remote branch MDL-39833 _m24 to be integrated into upstream MOODLE_24_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1610 Warning: The MDL-39833 _m24 branch at https://github.com/shashirepo/moodle has not been rebased recently (>20 days ago). Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1610/artifact/work/smurf.html Remote branch MDL-39833 _m25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1611 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1611/artifact/work/smurf.html
          Hide
          shashitechno Shashikant vaishnav added a comment -

          Hello, Just fixed the coding convention issues !

          Show
          shashitechno Shashikant vaishnav added a comment - Hello, Just fixed the coding convention issues !
          Hide
          shashitechno Shashikant vaishnav added a comment -

          Hello there,

          Just rebased & updated the git diff URLS for affected branches along with master.

          thanks !

          Show
          shashitechno Shashikant vaishnav added a comment - Hello there, Just rebased & updated the git diff URLS for affected branches along with master. thanks !
          Hide
          poltawski Dan Poltawski added a comment -

          Please stop adding me as a peer reviewer to this issue.

          Show
          poltawski Dan Poltawski added a comment - Please stop adding me as a peer reviewer to this issue.
          Hide
          andyjdavis Andrew Davis added a comment -

          Hello. It would be better to avoid the string concatenation. Instead put the space in the string itself and pass the result of get_section_name() into get_string(). In the lang file look for {$a} to see how this is done in the string.

          Show
          andyjdavis Andrew Davis added a comment - Hello. It would be better to avoid the string concatenation. Instead put the space in the string itself and pass the result of get_section_name() into get_string(). In the lang file look for {$a} to see how this is done in the string.
          Hide
          shashitechno Shashikant vaishnav added a comment -

          Hello Andrew,

          Thanks for pointing out this, I am afraid, don’t it will effect another components that are using same lang string.

          lang/en/moodle

              // Change the lang string to take arguments.
              $string['notavailable'] = '{$a} is Not available'; 
              

          Or should I create an another lang string for "course not availability info".

          Regards
          Shashikant

          Show
          shashitechno Shashikant vaishnav added a comment - Hello Andrew, Thanks for pointing out this, I am afraid, don’t it will effect another components that are using same lang string. lang/en/moodle // Change the lang string to take arguments. $string['notavailable'] = '{$a} is Not available'; Or should I create an another lang string for "course not availability info". Regards Shashikant
          Hide
          nadavkav Nadav Kavalerchik added a comment -

          I agree with Shashikant, the $string['notavailable'] is used in 29 places in the code. (mainly in: mod/* and course/format/* folders)
          It is better to simply concatenate it or create a new string token and advise developers of course format "tabtopics", "topcoll" and "calendar" to fix their code.

          Show
          nadavkav Nadav Kavalerchik added a comment - I agree with Shashikant, the $string ['notavailable'] is used in 29 places in the code. (mainly in: mod/* and course/format/* folders) It is better to simply concatenate it or create a new string token and advise developers of course format "tabtopics", "topcoll" and "calendar" to fix their code.
          Hide
          andyjdavis Andrew Davis added a comment -

          In that case, create a new string.

          Show
          andyjdavis Andrew Davis added a comment - In that case, create a new string.
          Hide
          shashitechno Shashikant vaishnav added a comment - - edited

          Hello Andrew,

          Just updated all the git urls by amending the fix with last commit, Fixed!

          Thanks!

          Show
          shashitechno Shashikant vaishnav added a comment - - edited Hello Andrew, Just updated all the git urls by amending the fix with last commit, Fixed! Thanks!
          Show
          cibot CiBoT added a comment - Results for MDL-39833 Remote repository: https://github.com/shashirepo/moodle Remote branch MDL-39833 _m24 to be integrated into upstream MOODLE_24_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2069 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2069/artifact/work/smurf.html Remote branch MDL-39833 _m25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2070 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2070/artifact/work/smurf.html Remote branch MDL-39833 _m26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2071 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2071/artifact/work/smurf.html Remote branch MDL-39833 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2072 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2072/artifact/work/smurf.html
          Hide
          andyjdavis Andrew Davis added a comment -

          Sorry, two more minor things.

          It would be worth adding something to the testing instructions to get the tester to check the "Hidden sections" setting in the course settings.

          In the string Not should not be capitalized.

          Once those are done, put this up for integration.

          Show
          andyjdavis Andrew Davis added a comment - Sorry, two more minor things. It would be worth adding something to the testing instructions to get the tester to check the "Hidden sections" setting in the course settings. In the string Not should not be capitalized. Once those are done, put this up for integration.
          Hide
          shashitechno Shashikant vaishnav added a comment -

          Hello Andrew,

          Just fixed these issues, Have a quick look.

          Thanks!

          Show
          shashitechno Shashikant vaishnav added a comment - Hello Andrew, Just fixed these issues, Have a quick look. Thanks!
          Hide
          andyjdavis Andrew Davis added a comment -

          Looks good so I have taken the liberty of submitting this for integration.

          Show
          andyjdavis Andrew Davis added a comment - Looks good so I have taken the liberty of submitting this for integration.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          cibot CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          samhemelryk Sam Hemelryk added a comment - - edited

          Hi Shashikant,

          First up thank you for working on this!

          I think your solution is in the right direction, there is just one thing that I think needs to be addressed before this can be integrated.
          The use of optional_param within API (section_hidden) rings alarm bells for me.
          Should any situation arise where that renderer is used on a page that uses the id param for something other than a course ID then we potentially will be showing the wrong information or the user will see an error.
          Should the ID match a course by chance it could end up showing the user information they couldn't see otherwise, making it a trivial security issue, but a security issue none the less.

          I think the correct solution is to add $course as an argument to the section_hidden method.
          In master we can require it, however in the stable branches we can't change API without making it backwards compatible so you would have to do something like:

          // Master.
          protected function section_hidden($sectionno, $courseorid) {...}
           
          // Stable branches.
          protected function section_hidden($sectionno, $courseorid = null) {
              if (is_null($courseorid)) {
                   debugging('Please update your call to section_hidden', DEBUG_DEVELOPER);
                   $courseorid = optional_param('id', 0, PARAM_INT);
              }
              ...
          }
          

          The rest looks spot on thank you.

          Regards
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - - edited Hi Shashikant, First up thank you for working on this! I think your solution is in the right direction, there is just one thing that I think needs to be addressed before this can be integrated. The use of optional_param within API (section_hidden) rings alarm bells for me. Should any situation arise where that renderer is used on a page that uses the id param for something other than a course ID then we potentially will be showing the wrong information or the user will see an error. Should the ID match a course by chance it could end up showing the user information they couldn't see otherwise, making it a trivial security issue, but a security issue none the less. I think the correct solution is to add $course as an argument to the section_hidden method. In master we can require it, however in the stable branches we can't change API without making it backwards compatible so you would have to do something like: // Master. protected function section_hidden($sectionno, $courseorid) {...}   // Stable branches. protected function section_hidden($sectionno, $courseorid = null) { if (is_null($courseorid)) { debugging('Please update your call to section_hidden', DEBUG_DEVELOPER); $courseorid = optional_param('id', 0, PARAM_INT); } ... } The rest looks spot on thank you. Regards Sam
          Hide
          cibot CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          shashitechno Shashikant vaishnav added a comment - - edited

          Hello Sam,

          Thanks for pointing this out. What if we just make the courseorid optional argument. otherwise all calls to hidden_section will need to be modified ?

          something like what you suggested for stable branches ?

          Show
          shashitechno Shashikant vaishnav added a comment - - edited Hello Sam, Thanks for pointing this out. What if we just make the courseorid optional argument. otherwise all calls to hidden_section will need to be modified ? something like what you suggested for stable branches ?
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Hi Shashikant,

          Really it is that default behaviour that is the problem. We don't want to guess the course ID using optional_param unless we cannot avoid it.
          As such really we need to modify all calls to hidden_section as well, for both stable branches and master.

          The more I think about this the more I wonder if it should be a master only change.
          Our usual policy is that only bugs get backported to stable, and this is really feeling like an improvement to me (improvements don't get backported).
          How do you feel about this landing to master only Shashikant, or is this something you must have in 2.5|2.6?

          Cheers
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Hi Shashikant, Really it is that default behaviour that is the problem. We don't want to guess the course ID using optional_param unless we cannot avoid it. As such really we need to modify all calls to hidden_section as well, for both stable branches and master. The more I think about this the more I wonder if it should be a master only change. Our usual policy is that only bugs get backported to stable, and this is really feeling like an improvement to me (improvements don't get backported). How do you feel about this landing to master only Shashikant, or is this something you must have in 2.5|2.6? Cheers Sam
          Hide
          nadavkav Nadav Kavalerchik added a comment -

          If it will be available in 2.7, It is good enough for me.

          Show
          nadavkav Nadav Kavalerchik added a comment - If it will be available in 2.7, It is good enough for me.
          Hide
          shashitechno Shashikant vaishnav added a comment -

          Hello Sam, Nadav

          I agree. That looks like an improvement. Lets modify the method calls for master only.

          I will update the fixes in few hours.

          Cheers
          Shashikant

          Show
          shashitechno Shashikant vaishnav added a comment - Hello Sam, Nadav I agree. That looks like an improvement. Lets modify the method calls for master only. I will update the fixes in few hours. Cheers Shashikant
          Hide
          shashitechno Shashikant vaishnav added a comment -

          Hello Sam,

          Just updates the master and other branches fixes, have look !

          Thanks !

          Show
          shashitechno Shashikant vaishnav added a comment - Hello Sam, Just updates the master and other branches fixes, have look ! Thanks !
          Hide
          shashitechno Shashikant vaishnav added a comment -

          Fix for other branches to kept backward compatibility.

          Show
          shashitechno Shashikant vaishnav added a comment - Fix for other branches to kept backward compatibility.
          Show
          cibot CiBoT added a comment - Results for MDL-39833 Remote repository: https://github.com/shashirepo/moodle Remote branch MDL-39833 _m25 to be integrated into upstream MOODLE_25_STABLE Executed job http://social.srv.in.moodle.com/job/Precheck%20remote%20branch/2429 Details: http://social.srv.in.moodle.com/job/Precheck%20remote%20branch/2429/artifact/work/smurf.html Remote branch MDL-39833 _m26 to be integrated into upstream MOODLE_26_STABLE Executed job http://social.srv.in.moodle.com/job/Precheck%20remote%20branch/2430 Details: http://social.srv.in.moodle.com/job/Precheck%20remote%20branch/2430/artifact/work/smurf.html Remote branch MDL-39833 to be integrated into upstream master Executed job http://social.srv.in.moodle.com/job/Precheck%20remote%20branch/2431 Details: http://social.srv.in.moodle.com/job/Precheck%20remote%20branch/2431/artifact/work/smurf.html
          Hide
          andyjdavis Andrew Davis added a comment -

          Squash those two commits into as single commit. Also, if the other versions (2.6, 2.5) are not going to be updated just remove the branch and diff information from this issue.

          Once that is done, submit this for integration when you are ready.

          Show
          andyjdavis Andrew Davis added a comment - Squash those two commits into as single commit. Also, if the other versions (2.6, 2.5) are not going to be updated just remove the branch and diff information from this issue. Once that is done, submit this for integration when you are ready.
          Show
          cibot CiBoT added a comment - Results for MDL-39833 Remote repository: https://github.com/shashirepo/moodle Remote branch MDL-39833 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2628 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2628/artifact/work/smurf.html
          Hide
          cibot CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks Shashikant this has been integrated now.
          I made a small commit to add a note to course/format/upgrade.txt about this API change and to add a default value to the API after discussing this change with Eloy.

          Many thanks
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks Shashikant this has been integrated now. I made a small commit to add a note to course/format/upgrade.txt about this API change and to add a default value to the API after discussing this change with Eloy. Many thanks Sam
          Hide
          fred Frédéric Massart added a comment -

          Passing the test assuming you mean "Hidden sections: Hidden sections are shown in collapsed form". Thanks!

          Show
          fred Frédéric Massart added a comment - Passing the test assuming you mean "Hidden sections: Hidden sections are shown in collapsed form". Thanks!
          Hide
          shashitechno Shashikant vaishnav added a comment -

          Thanks everyone

          Show
          shashitechno Shashikant vaishnav added a comment - Thanks everyone
          Hide
          poltawski Dan Poltawski added a comment -

          Thanks for your efforts, this change is now part of Moodle!

          Show
          poltawski Dan Poltawski added a comment - Thanks for your efforts, this change is now part of Moodle!

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                12/May/14