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

In the default course settings, numsections is not limited to maxsections

    Details

    • Testing Instructions:
      Hide

      Test steps

      1. Log in as the administrator.
      2. Go to [Settings ► Site administration ► Courses ► Course default settings].
      3. Set 'Maximum for number of weeks/topics' to 0 and save changes.
        • Check that 'Number of weeks/topics' is set to 0 and that this is the only option.
      4. Set 'Maximum for number of weeks/topics' to nothing and save changes.
        • Check that 'Number of weeks/topics' now has the option to go up to 52.
      5. Create a new course [Settings ► Site administration ► Courses ► Add/edit courses].
      6. Select 'Weekly format'.
        • In the section 'Formatting options for Weekly format' check that 'Number of weeks/topics' has the option to go up to 52.
      7. Change the format to 'Topics format'.
        • In the section 'Formatting options for Topics format' check that 'Number of weeks/topics' has the option to go up to 52.
      Show
      Test steps Log in as the administrator. Go to [Settings ► Site administration ► Courses ► Course default settings] . Set 'Maximum for number of weeks/topics' to 0 and save changes. Check that 'Number of weeks/topics' is set to 0 and that this is the only option. Set 'Maximum for number of weeks/topics' to nothing and save changes. Check that 'Number of weeks/topics' now has the option to go up to 52. Create a new course [Settings ► Site administration ► Courses ► Add/edit courses] . Select 'Weekly format'. In the section 'Formatting options for Weekly format' check that 'Number of weeks/topics' has the option to go up to 52. Change the format to 'Topics format'. In the section 'Formatting options for Topics format' check that 'Number of weeks/topics' has the option to go up to 52.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-36795-master

      Description

      When controlling the maximum number of sections allowed in a course and the default number of sections in a new course, the number of sections should be limited by the maximum value.

      Reproduction steps:

      1. Log in as admin
      2. Navigate to "Course default settings" (Settings -> Site administration -> Courses -> Course default settings)
      3. Change "Maximum for number of weeks/topics moodlecourse | maxsections" to 0
      4. Click save changes
      5. Check "Number of weeks/topics moodlecourse | numsections" drop-down

      Expected result: The number of weeks/topics should only show 0

      Actual result: The number of weeks/topics shows numbers from 0 to 52

      Currently the function that controls this value is in lib/adminlib.php...

      lib/adminlib.php, lines 3813 to 3829

      class admin_settings_num_course_sections extends admin_setting_configselect {
          public function __construct($name, $visiblename, $description, $defaultsetting) {
              parent::__construct($name, $visiblename, $description, $defaultsetting, array());
          }
       
          /** Lazy-load the available choices for the select box */
          public function load_choices() {
              $max = get_config('moodlecourse', 'maxsections');
              if (empty($max)) {
                  $max = 52;
              }
              for ($i = 0; $i <= $max; $i++) {
                  $this->choices[$i] = "$i";
              }
              return true;
          }
      }

      The check to see if the maximum value is set fails to allow for a value of zero. Changing the check to use !isset() instead of empty resolves this.

      if (!isset($max)) {

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Thanks Adrian,

              Patch makes sense, but it will be nice to check if maxsection is not a number rather then empty string https://github.com/abgreeve/moodle/compare/moodle:master...wip-MDL-36795-master#L0R217

              As config can save maxsection as string, it might be hard to debug for any user who has space stored there.

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks Adrian, Patch makes sense, but it will be nice to check if maxsection is not a number rather then empty string https://github.com/abgreeve/moodle/compare/moodle:master...wip-MDL-36795-master#L0R217 As config can save maxsection as string, it might be hard to debug for any user who has space stored there.
              Hide
              abgreeve Adrian Greeve added a comment -

              Thanks Raj,

              I've changed the check to just look at whether the value is numeric or not.

              Show
              abgreeve Adrian Greeve added a comment - Thanks Raj, I've changed the check to just look at whether the value is numeric or not.
              Hide
              abgreeve Adrian Greeve added a comment -

              After further talks with Raj, I've put back the check to see if the variable is set, just to be safe.

              Show
              abgreeve Adrian Greeve added a comment - After further talks with Raj, I've put back the check to see if the variable is set, just to be safe.
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Thanks Adrian,

              Patch looks good.

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks Adrian, Patch looks good.
              Hide
              abgreeve Adrian Greeve added a comment -

              I've created branches for 2.3 and 2.2.
              They are different from master so they must be tested if they are integrated.

              Show
              abgreeve Adrian Greeve added a comment - I've created branches for 2.3 and 2.2. They are different from master so they must be tested if they are integrated.
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Thanks Adrian, this has been integrated now.

              Show
              samhemelryk Sam Hemelryk added a comment - Thanks Adrian, this has been integrated now.
              Hide
              andyjdavis Andrew Davis added a comment -

              Works as described. Passing.

              Show
              andyjdavis Andrew Davis added a comment - Works as described. Passing.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Y E S !

              Closing as fixed, many thanks!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Y E S ! Closing as fixed, many thanks!

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    14/Jan/13