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:

      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

          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