Moodle
  1. Moodle
  2. MDL-39764

Course maxsections cannot be exceeded when changing an existing course with a greater number of sections.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.7, 2.4.4, 2.5, 2.6
    • Fix Version/s: 2.3.8, 2.4.5, 2.5.1
    • Component/s: Course
    • Labels:
      None
    • Testing Instructions:
      Hide
      1. Create a course in topics format with 14 sections (do not use existing course, it is important to test creating a course)
      2. In Home -> Site administration -> Courses -> Course default settings set the maximum number of sections to '12'
      3. Edit the course settings and observe that the number of sections combo-box is set at '14', even though this is greater than the maximum '12' and that values in the range 0-14 are shown.
      4. Do not change this field and press save
      5. Make sure all 14 sections are shown.

      Repeat test for weeks format

      Ensure this is tested in 23_STABLE, solution there is different. Then any of 24/25/master should be enough.

      Show
      Create a course in topics format with 14 sections (do not use existing course, it is important to test creating a course) In Home -> Site administration -> Courses -> Course default settings set the maximum number of sections to '12' Edit the course settings and observe that the number of sections combo-box is set at '14', even though this is greater than the maximum '12' and that values in the range 0-14 are shown. Do not change this field and press save Make sure all 14 sections are shown. Repeat test for weeks format Ensure this is tested in 23_STABLE, solution there is different. Then any of 24/25/master should be enough.
    • Workaround:
      Hide

      None!

      Show
      None!
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
      wip-MDL-39764_M24_3
    • Pull 2.5 Branch:
      wip-MDL-39764_M25_3
    • Pull Master Branch:
      wip-MDL-39764_master_3
    • Rank:
      50504

      Description

      When editing a course you can add new sections using an icon at the bottom of the section list. Currently, this icon is shown even when the default max sections has been reached (as set up in Home /Site administration /Courses /Course default settings - moodlecourse | maxsections setting) which controls the population of the numsections combo-box in the course settings.

      Therefore it is possible for the user to add new sections beyond the maximum and add activities / resources to them. But when they edit the course settings, the new number of sections in the course is greater than the maximum value populated in the combo-box so it reverts to '0'. Then the user only sees the '0' section, goes back to the course settings to set the number of sections to the maximum, which is still less than has been added previously through the icon, only to find that there are now orphaned activities at the bottom of the course.

      Looking at the code in '/course/format/renderer.php' method 'print_multiple_section_page':

                  // Increase number of sections.
                  $straddsection = get_string('increasesections', 'moodle');
                  $url = new moodle_url('/course/changenumsections.php',
                      array('courseid' => $course->id,
                            'increase' => true,
                            'sesskey' => sesskey()));
                  $icon = $this->output->pix_icon('t/switch_plus', $straddsection);
                  echo html_writer::link($url, $icon.get_accesshide($straddsection), array('class' => 'increase-sections'));
      

      there is no 'guard' to check the maxsections. Therefore the code should be:

                  $max = get_config('moodlecourse', 'maxsections');
                  if (!isset($max) || !is_numeric($max)) {
                      $max = 52;
                  }
                  if ($course->numsections < $max) {
                      // Increase number of sections.
                      $straddsection = get_string('increasesections', 'moodle');
                      $url = new moodle_url('/course/changenumsections.php',
                                      array('courseid' => $course->id,
                                           'increase' => true,
                                           'sesskey' => sesskey()));
                      $icon = $this->output->pix_icon('t/switch_plus', $straddsection);
                      echo html_writer::link($url, $icon . get_accesshide($straddsection), array('class' => 'increase-sections'));
                  }
      

      where I have borrowed the principle setting retrieval code of:

      $max = get_config('moodlecourse', 'maxsections');
      if (!isset($max) || !is_numeric($max)) {
          $max = 52;
      }
      

      from 'admin_settings_num_course_sections' in '/lib/adminlib.php'.

      This arose from this thread in the course formats forum: https://moodle.org/mod/forum/discuss.php?d=228468

        Issue Links

          Activity

          Hide
          Dan Poltawski added a comment -

          Wow, how long have we had maxsections for?

          Show
          Dan Poltawski added a comment - Wow, how long have we had maxsections for?
          Hide
          Gareth J Barnard added a comment -

          Hi Dan Poltawski,

          I've tracked down the setting of maxsections to MDL-25357 integrated back on the 7th March 2011. Before that the maximum was hard coded at '52' and I think that has been since the beginning of known time - there are no known records 'BM' or 'Before Moodle' . You added the increase icon code in MDL-32508 on the 21st November 2011.

          I'll work on the patch today.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Hi Dan Poltawski , I've tracked down the setting of maxsections to MDL-25357 integrated back on the 7th March 2011. Before that the maximum was hard coded at '52' and I think that has been since the beginning of known time - there are no known records 'BM' or 'Before Moodle' . You added the increase icon code in MDL-32508 on the 21st November 2011. I'll work on the patch today. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          P.S. Snag is, there are four branches to update in core. But! I have four contributed plugins that also have the same code as I overrode that method and I think they all have versions going back to M2.3 - this is going to take a while!

          Show
          Gareth J Barnard added a comment - P.S. Snag is, there are four branches to update in core. But! I have four contributed plugins that also have the same code as I overrode that method and I think they all have versions going back to M2.3 - this is going to take a while!
          Hide
          Marina Glancy added a comment -

          Quite stupid parameter isn't it?
          I think Gareth's patch looks ok, in this approach we would also need to add this check in /course/changenumsections.php, but....

          imagine when some course already has 40 sections and some smart admin comes and changes the config variable to 10. After that if teacher tries to edit the course setting he'll be offered a dropdown select 1-10 number of sections and even if he wanted to change another field all his sections will be screwed. So I would change the course edit form (/course/edit_form.php) to populate the number of sections dropdown up to maximum of config var and existing number of sections (even though this is not really related to this issue).

          Also to be honest I don't see a problem with creating more sections that defined in config. Especially since the description to it says "The maximum value in the number of sections dropdown menu (applies to certain course formats only)." It does not say that this is the maximum number of section a course can have.

          Show
          Marina Glancy added a comment - Quite stupid parameter isn't it? I think Gareth's patch looks ok, in this approach we would also need to add this check in /course/changenumsections.php, but.... imagine when some course already has 40 sections and some smart admin comes and changes the config variable to 10. After that if teacher tries to edit the course setting he'll be offered a dropdown select 1-10 number of sections and even if he wanted to change another field all his sections will be screwed. So I would change the course edit form (/course/edit_form.php) to populate the number of sections dropdown up to maximum of config var and existing number of sections (even though this is not really related to this issue). Also to be honest I don't see a problem with creating more sections that defined in config. Especially since the description to it says "The maximum value in the number of sections dropdown menu (applies to certain course formats only)." It does not say that this is the maximum number of section a course can have.
          Hide
          Marina Glancy added a comment -

          PS Moodle 2.4+ instead of /course/edit_form.php this dropdown is in format_xxx::course_format_options() (xxx = legacy, topics, weeks)

          Show
          Marina Glancy added a comment - PS Moodle 2.4+ instead of /course/edit_form.php this dropdown is in format_xxx::course_format_options() (xxx = legacy, topics, weeks)
          Hide
          Marina Glancy added a comment -

          PPS actually it was exactly the case in this forum post https://moodle.org/mod/forum/discuss.php?d=228468 - user changed some other course setting and his number of sections got reset

          Show
          Marina Glancy added a comment - PPS actually it was exactly the case in this forum post https://moodle.org/mod/forum/discuss.php?d=228468 - user changed some other course setting and his number of sections got reset
          Hide
          Gareth J Barnard added a comment -

          Thanks Marina Glancy,

          So as this patch pertains to the icon, should the '/course/changenumsections.php' check be another issue?

          And it was https://moodle.org/mod/forum/discuss.php?d=228468 that sparked this

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Thanks Marina Glancy , So as this patch pertains to the icon, should the '/course/changenumsections.php' check be another issue? And it was https://moodle.org/mod/forum/discuss.php?d=228468 that sparked this Cheers, Gareth
          Hide
          Marina Glancy added a comment -

          Gareth,
          do you insist on your solution?

          I still think that maxsections is what it says in description: "The maximum value in the number of sections dropdown menu (applies to certain course formats only)."
          It should not affect the +/- icons visibility or logic in /course/changenumsections.php

          What we should do is change the edit form so it does not get broken when real number of sections is more than 'maxsections'

          Show
          Marina Glancy added a comment - Gareth, do you insist on your solution? I still think that maxsections is what it says in description: "The maximum value in the number of sections dropdown menu (applies to certain course formats only)." It should not affect the +/- icons visibility or logic in /course/changenumsections.php What we should do is change the edit form so it does not get broken when real number of sections is more than 'maxsections'
          Hide
          Gareth J Barnard added a comment -

          Dear Marina Glancy,

          I'm not insisting on anything, just applying common practice guard boundary conditions to set limits. If 'maxsections' is ambiguous then this is a design flaw and needs to be clarified by possibly the new 'front end team'. If the administrator / managers wish to exercise control over the their Moodle site and implement site policy through the use of this admin setting then they should be allowed to do so rather than mayhem and inconsistency. Therefore, the setting needs to be pinned down as a 'maximum' and if is changed locally by an administrator / manager then it is up to them to be locally responsible. A 'maximum' is a 'maximum' not 'its a maximum only when it suits us'.

          If a 'smart admin' makes the change and all hell breaks loose then its a local issue in the realms of policy.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Marina Glancy , I'm not insisting on anything, just applying common practice guard boundary conditions to set limits. If 'maxsections' is ambiguous then this is a design flaw and needs to be clarified by possibly the new 'front end team'. If the administrator / managers wish to exercise control over the their Moodle site and implement site policy through the use of this admin setting then they should be allowed to do so rather than mayhem and inconsistency. Therefore, the setting needs to be pinned down as a 'maximum' and if is changed locally by an administrator / manager then it is up to them to be locally responsible. A 'maximum' is a 'maximum' not 'its a maximum only when it suits us'. If a 'smart admin' makes the change and all hell breaks loose then its a local issue in the realms of policy. Cheers, Gareth
          Hide
          Marina Glancy added a comment -

          Dan, what do you think?

          Show
          Marina Glancy added a comment - Dan, what do you think?
          Hide
          Dan Poltawski added a comment -

          I'd prefer that we didn't have the setting at all, so in that sense, I prefer your suggestion Marina. The intent of the original issue (MDL-25357) seems to be to allow more sections in the dropdown UI (without forcing everyone to have a really huge dropdown list), rather than a setting to restrict the number of sections.

          Show
          Dan Poltawski added a comment - I'd prefer that we didn't have the setting at all, so in that sense, I prefer your suggestion Marina. The intent of the original issue ( MDL-25357 ) seems to be to allow more sections in the dropdown UI (without forcing everyone to have a really huge dropdown list), rather than a setting to restrict the number of sections.
          Hide
          Gareth J Barnard added a comment -

          Ok girls and boys, what would you like me to do? I can close this one down, create another and implement the dropdown UI fix as suggested as there is still the outstanding issue of a bug where the current functionality is causing confusion.

          And perhaps change some of the definition wording of 'maxsections' to reflect the intent of MDL-25357?

          Let me know and I'll play ball.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Ok girls and boys, what would you like me to do? I can close this one down, create another and implement the dropdown UI fix as suggested as there is still the outstanding issue of a bug where the current functionality is causing confusion. And perhaps change some of the definition wording of 'maxsections' to reflect the intent of MDL-25357 ? Let me know and I'll play ball. Cheers, Gareth
          Hide
          Marina Glancy added a comment -

          My vote is for another issue (or another wording of this issue) that makes sure that the course edit form does not reset the current number of sections if it is greater than maxsections.

          Show
          Marina Glancy added a comment - My vote is for another issue (or another wording of this issue) that makes sure that the course edit form does not reset the current number of sections if it is greater than maxsections.
          Hide
          Gareth J Barnard added a comment -

          Dear Marina Glancy and Dan Poltawski,

          I've changed the issue name to reflect a more positive outlook and to reduce admin faff gone for Marina's suggestion of rewording this issue.

          Please could you have a look at a proposed fix for M2.4+ in https://github.com/gjb2048/moodle/compare/master...wip-MDL-39764_master_2 (M2.3 will do something similar). I will of course need to replicate to the weeks format and update the testing instructions. It is interesting to note that I had to get the courseid from the URL parameter in order to get the course object containing the numsections data as an examination of the '$this' object in the 'course_format_options' method revealed that all it knew was:

          format_topics Object
          (
              [courseid:protected] => 0
              [format:protected] => topics
              [course:protected] => 
              [formatoptions:protected] => Array
                  (
                  )
          
          )
          

          And there was no information in the '$foreditform' variable. If this is ok, what additional code do I need if the courseid ('id') parameter is not there?

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Marina Glancy and Dan Poltawski , I've changed the issue name to reflect a more positive outlook and to reduce admin faff gone for Marina's suggestion of rewording this issue. Please could you have a look at a proposed fix for M2.4+ in https://github.com/gjb2048/moodle/compare/master...wip-MDL-39764_master_2 (M2.3 will do something similar). I will of course need to replicate to the weeks format and update the testing instructions. It is interesting to note that I had to get the courseid from the URL parameter in order to get the course object containing the numsections data as an examination of the '$this' object in the 'course_format_options' method revealed that all it knew was: format_topics Object ( [courseid: protected ] => 0 [format: protected ] => topics [course: protected ] => [formatoptions: protected ] => Array ( ) ) And there was no information in the '$foreditform' variable. If this is ok, what additional code do I need if the courseid ('id') parameter is not there? Cheers, Gareth
          Hide
          Marina Glancy added a comment -

          Hi Gareth J Barnard, I'm afraid it's way too tricky this way. First I doubt it would work in case of creating the course. Second, function get_course() calls function course_format_options() and should not be used inside it.
          I would recommend overwriting function create_edit_form_elements() and there we already have instance of $mform. We can just add in the end something like:

          if (!$forsection && $mform->getElementValue('numsections') > get_config('moodlecourse', 'numsections'))

          { $mform->getElement('numsections')->addOption(...); }
          Show
          Marina Glancy added a comment - Hi Gareth J Barnard , I'm afraid it's way too tricky this way. First I doubt it would work in case of creating the course. Second, function get_course() calls function course_format_options() and should not be used inside it. I would recommend overwriting function create_edit_form_elements() and there we already have instance of $mform. We can just add in the end something like: if (!$forsection && $mform->getElementValue('numsections') > get_config('moodlecourse', 'numsections')) { $mform->getElement('numsections')->addOption(...); }
          Hide
          Gareth J Barnard added a comment -

          Dear Marina Glancy,

          Thank you for reviewing the code, making observations and suggesting an alternative. I had not considered the course format object instance issues and effective timeline thereof. But then it is always better to ask the person who created it.

          I'll update with your alternate solution as soon as I can.

          Kind regards,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Marina Glancy , Thank you for reviewing the code, making observations and suggesting an alternative. I had not considered the course format object instance issues and effective timeline thereof. But then it is always better to ask the person who created it. I'll update with your alternate solution as soon as I can. Kind regards, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Marina Glancy and Dan Poltawski,

          All branches updated with latest code. Submitting for peer review.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Marina Glancy and Dan Poltawski , All branches updated with latest code. Submitting for peer review. Cheers, Gareth
          Hide
          Marina Glancy added a comment -

          Awesome Gareth J Barnard, thanks a lot. Code looks fine to me

          Show
          Marina Glancy added a comment - Awesome Gareth J Barnard , thanks a lot. Code looks fine to me
          Hide
          Marina Glancy added a comment -

          Gareth, I edited your testing instructions, there is no need to test that it was broken before the fix Thanks again for spotting this

          Show
          Marina Glancy added a comment - Gareth, I edited your testing instructions, there is no need to test that it was broken before the fix Thanks again for spotting this
          Hide
          Gareth J Barnard added a comment -

          Dear Marina Glancy,

          No worries - I see that you had set the status of 'continuing development' - as you say you are happy with the code I've set it back to 'Waiting for peer review' as I cannot progress it any further to send to integration etc.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Marina Glancy , No worries - I see that you had set the status of 'continuing development' - as you say you are happy with the code I've set it back to 'Waiting for peer review' as I cannot progress it any further to send to integration etc. Cheers, Gareth
          Hide
          Marina Glancy added a comment -

          grrr, this status was set automatically when I clicked "finish peer review" and I did not look at it. sorry

          Show
          Marina Glancy added a comment - grrr, this status was set automatically when I clicked "finish peer review" and I did not look at it. sorry
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Gareth J Barnard, I just would recommend to add some comments within the code to explain what's being done and why. Surely that would help other developers building custom course formats.

          For your consideration, I'll keep this on hold for some hours before proceeding to integrate it.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Gareth J Barnard , I just would recommend to add some comments within the code to explain what's being done and why. Surely that would help other developers building custom course formats. For your consideration, I'll keep this on hold for some hours before proceeding to integrate it. Ciao
          Hide
          Gareth J Barnard added a comment -

          Dear Eloy Lafuente (stronk7),

          Good point , but now 22:55 here and on my Netbook so not in an easy position to update and rebase / squash all four branches until tomorrow - say 11 hours time.

          What would you like done? I'd be happy to comment / add to the course format's developer page.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Eloy Lafuente (stronk7) , Good point , but now 22:55 here and on my Netbook so not in an easy position to update and rebase / squash all four branches until tomorrow - say 11 hours time. What would you like done? I'd be happy to comment / add to the course format's developer page. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          P.S. I'll have a bash though and see if the Netbook holds together.

          Show
          Gareth J Barnard added a comment - P.S. I'll have a bash though and see if the Netbook holds together.
          Hide
          Gareth J Barnard added a comment -

          Dear Eloy Lafuente (stronk7),

          Comment added to all branches, rebased and squashed into one commit. Moodle 2.3 comment the same as the rest as although the code is different as it still reads correctly for the situation it fixes.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Eloy Lafuente (stronk7) , Comment added to all branches, rebased and squashed into one commit. Moodle 2.3 comment the same as the rest as although the code is different as it still reads correctly for the situation it fixes. Cheers, Gareth
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Thanks Gareth! Reviewing and pushing...

          Show
          Eloy Lafuente (stronk7) added a comment - Thanks Gareth! Reviewing and pushing...
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Oh, my. Comment is perfect, just PHPDoc style is not allowed there .

          You should use inline (//) PHP comments as the correct alternative.

          Edited: nvm, performing the change here...

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Oh, my. Comment is perfect, just PHPDoc style is not allowed there . You should use inline (//) PHP comments as the correct alternative. Edited: nvm, performing the change here...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (23, 24, 25 and master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (23, 24, 25 and master), thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          To tester: Ensure this is tested in 23_STABLE, solution there is different. Then any of 24/25/master should be enough.

          Show
          Eloy Lafuente (stronk7) added a comment - To tester: Ensure this is tested in 23_STABLE, solution there is different. Then any of 24/25/master should be enough.
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Eloy Lafuente (stronk7),

          Ok , I thought

          '/**'

          is documentation comment but

          '/*'

          is multi-line like Java and if I use '//' then Code Checker will complain about no periods at the end of lines when the sentence overflows because of the 132 character line limit!

          I hope the edit still mean's I credited with the fix in the log

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Eloy Lafuente (stronk7) , Ok , I thought '/**' is documentation comment but '/*' is multi-line like Java and if I use '//' then Code Checker will complain about no periods at the end of lines when the sentence overflows because of the 132 character line limit! I hope the edit still mean's I credited with the fix in the log Cheers, Gareth
          Hide
          Petr Škoda added a comment -

          works fine, thanks

          Show
          Petr Škoda added a comment - works fine, thanks
          Hide
          Dan Poltawski added a comment -
          Feature: Thanks to our superb contributors
            In order to make Moodle better
            As an integrator
            I need to thank all our contributors
          
            Scenario: Dan thanks you all
              Given I log in as "dan"
              And I see "lots of fixed issues"
              When I follow "Close integrated issues"
              Then I should see "Lots of thanks to all our contributors"
          

          Your changes are upstream

          Show
          Dan Poltawski added a comment - Feature: Thanks to our superb contributors In order to make Moodle better As an integrator I need to thank all our contributors Scenario: Dan thanks you all Given I log in as "dan" And I see "lots of fixed issues" When I follow "Close integrated issues" Then I should see "Lots of thanks to all our contributors" Your changes are upstream
          Hide
          Gareth J Barnard added a comment -

          Dear all,

          Just a side note to this - tested the other day with a maximum number of sections set at 65536, then set the course to 65536 sections with my Collapsed Topics. Moodle fell over when trying to display the page. Setting the number of sections back failed via the GUI, so did so in the database. This then presented a database error when trying to create new rows in the course_sections table. So only solution was to manually delete the extra rows. Discovered that the heights of 27364 sections had been reached before MySQL connector waived a white flag. So, should there be an upper 'maxsections' limit within the overall configuration to prevent 'muppet' actions? As it was quite tricky to recover from and not via the GUI.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear all, Just a side note to this - tested the other day with a maximum number of sections set at 65536, then set the course to 65536 sections with my Collapsed Topics. Moodle fell over when trying to display the page. Setting the number of sections back failed via the GUI, so did so in the database. This then presented a database error when trying to create new rows in the course_sections table. So only solution was to manually delete the extra rows. Discovered that the heights of 27364 sections had been reached before MySQL connector waived a white flag. So, should there be an upper 'maxsections' limit within the overall configuration to prevent 'muppet' actions? As it was quite tricky to recover from and not via the GUI. Cheers, Gareth

            People

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

              Dates

              • Created:
                Updated:
                Resolved: