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

          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