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

Forum maximum number of attachments invalid

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.9, 2.5.5, 2.6.2, 2.7
    • Fix Version/s: 2.5.7, 2.6.4, 2.7.1
    • Component/s: Forum
    • Labels:
    • Testing Instructions:
      Hide

      Before upgrade:

      1. Create a course.
      2. Create a forum. Set "Maximum number of attachments" to 20 (or 50 or 100).
      3. When you add a discussion topic you will notice that it says that only 11 (or 12 or 13) attachments are allowed.

      Upgrade:

      1. Go to your forum that you created above, make sure it now allows 20 (or 50 or 100) attachments.
      2. Create a forum. Set "Maximum number of attachments" to 20.
      3. Add a discussion topic to the forum. Drag 20 files into the Attachment uploader.
      Show
      Before upgrade: Create a course. Create a forum. Set "Maximum number of attachments" to 20 (or 50 or 100). When you add a discussion topic you will notice that it says that only 11 (or 12 or 13) attachments are allowed. Upgrade: Go to your forum that you created above, make sure it now allows 20 (or 50 or 100) attachments. Create a forum. Set "Maximum number of attachments" to 20. Add a discussion topic to the forum. Drag 20 files into the Attachment uploader.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
    • Pull 2.7 Branch:
      wip-MDL-45483-MOODLE_27_STABLE
    • Pull Master Branch:
      wip-MDL-45483-master

      Description

      I have set the maximum number of attachments in a standard forum to 20, but it only allows 11 attachments.

      It looks to be a basic error in the attachments dropdown code. Here's a snippet from the forum editing page:

      <select name="maxattachments" id="id_maxattachments">
      <option value="0">0</option>
      <option value="1">1</option>
      <option value="2">2</option>
      <option value="3">3</option>
      <option value="4">4</option>
      <option value="5">5</option>
      <option value="6">6</option>
      <option value="7">7</option>
      <option value="8">8</option>
      <option value="9">9</option>
      <option value="10">10</option>
      <option value="11" selected="selected">20</option>
      <option value="12">50</option>
      <option value="13">100</option>
      </select>

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            kwiliarty Kevin Wiliarty added a comment -

            I can see what the problem is here. The select box uses the array keys for the values, and in this case the keys are implicitly just ordered numbers. The display options start to make jumps, but the keys do not. We can fix this by specifying keys that match the values. I'll post a patch soon.

            Show
            kwiliarty Kevin Wiliarty added a comment - I can see what the problem is here. The select box uses the array keys for the values, and in this case the keys are implicitly just ordered numbers. The display options start to make jumps, but the keys do not. We can fix this by specifying keys that match the values. I'll post a patch soon.
            Hide
            cfulton Charles Fulton added a comment -

            Verified that this is broken in 2.7.

            Show
            cfulton Charles Fulton added a comment - Verified that this is broken in 2.7.
            Hide
            kwiliarty Kevin Wiliarty added a comment -

            I've written a fix for master, but I can also backport if the fix is right.

            Show
            kwiliarty Kevin Wiliarty added a comment - I've written a fix for master, but I can also backport if the fix is right.
            Hide
            cibot CiBoT added a comment -
            Show
            cibot CiBoT added a comment - Results for MDL-45483 Remote repository: https://github.com/kwiliarty/moodle.git Remote branch wip- MDL-45483 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3357 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3357/artifact/work/smurf.html
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Thanks Kevin for the patch,
            looks great. Just a minor code style issue http://docs.moodle.org/dev/Coding_style#Wrapping_Arrays

            It would be awesome if you can provide backport branches (please note that we don't bump the first 8 digits of the version in stable branches)

            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks Kevin for the patch, looks great. Just a minor code style issue http://docs.moodle.org/dev/Coding_style#Wrapping_Arrays It would be awesome if you can provide backport branches (please note that we don't bump the first 8 digits of the version in stable branches)
            Hide
            kwiliarty Kevin Wiliarty added a comment -

            Thanks for the speedy replay, Ankit.

            I've fixed the coding style for the associative arrays. In the meantime, master moved along with a lot of version bumps, so I incremented my version by 1 over what core currently has in master. I'm not sure whether that was the right approach, so just let me know if it ought to be something different. It's all rebased and force pushed.

            I also put together backports for 2.6 and 2.5 (against the latest from core). The version bumps on those are incremented by just 1.

            Show
            kwiliarty Kevin Wiliarty added a comment - Thanks for the speedy replay, Ankit. I've fixed the coding style for the associative arrays. In the meantime, master moved along with a lot of version bumps, so I incremented my version by 1 over what core currently has in master. I'm not sure whether that was the right approach, so just let me know if it ought to be something different. It's all rebased and force pushed. I also put together backports for 2.6 and 2.5 (against the latest from core). The version bumps on those are incremented by just 1.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Thanks Kevin, looks good to me.

            Pushing for integration.

            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks Kevin, looks good to me. Pushing for integration.
            Hide
            kwiliarty Kevin Wiliarty added a comment -

            Just added a pull branch name and diff URL for 2.7

            Show
            kwiliarty Kevin Wiliarty added a comment - Just added a pull branch name and diff URL for 2.7
            Hide
            marina Marina Glancy added a comment - - edited

            Thanks Kevin, this was integrated in 2.5, 2.6, 2.7 and master

            edited: we still integrate in 2.5 for two more weeks if the issue was submitted before the freeze with 2.5 branch

            Show
            marina Marina Glancy added a comment - - edited Thanks Kevin, this was integrated in 2.5, 2.6, 2.7 and master edited: we still integrate in 2.5 for two more weeks if the issue was submitted before the freeze with 2.5 branch
            Hide
            kwiliarty Kevin Wiliarty added a comment -

            Thanks, Marina and all.

            Show
            kwiliarty Kevin Wiliarty added a comment - Thanks, Marina and all.
            Hide
            fred Frédéric Massart added a comment -

            Passing, thanks.

            Show
            fred Frédéric Massart added a comment - Passing, thanks.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            So, finally, this landed, making Moodle better, many thanks!

            Be patient and understanding.
            Life is too short to be
            vengeful or malicious.

            Phillips Brooks

            Closing as fixed, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - So, finally, this landed, making Moodle better, many thanks! Be patient and understanding. Life is too short to be vengeful or malicious. Phillips Brooks Closing as fixed, ciao
            Hide
            kwiliarty Kevin Wiliarty added a comment -

            Show
            kwiliarty Kevin Wiliarty added a comment -

              People

              • Votes:
                9 Vote for this issue
                Watchers:
                8 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/Jul/14