Moodle
  1. Moodle
  2. MDL-36644

Course upload limit option appears last in the list of available upload limit options when editing an assignment

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.2
    • Fix Version/s: 2.3.4, 2.4.1
    • Component/s: Assignment
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide
      1. Change the site upload limit to 2MB (Site administration / Security / Site policies
        )
      2. Change the upload limit for a course to 2MB (Course settings)
      3. Create a new assignment in the course
      4. Verify that the list of options under "Maximum submission size" lists "Course upload limit (2MB)" as the first option
      Show
      Change the site upload limit to 2MB (Site administration / Security / Site policies ) Change the upload limit for a course to 2MB (Course settings) Create a new assignment in the course Verify that the list of options under "Maximum submission size" lists "Course upload limit (2MB)" as the first option
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-36644-master
    • Rank:
      46139

      Description

      When editing the submission settings of an assignment, the Course upload limit option appears as the last item in the list of available options. Generally, the course upload limit will be largest available option and should therefore be positioned at the begining of the list - given that the list is already sorted by available sizes.

      Steps to reproduce:

      1. Login as a teacher and modify the maximum upload limit of a course. Set the value to 1MB.
      2. Create an Assignment activity within the same course.
      3. In the Submission Settings area, click on the drop down list for "Maximum submission size". The option for "Course Upload limit (1MB)" is present as the last available item in the list, and not in a consistent order with the other options.

      I would expect the "Course Upload limit" option to be the first item in the list of options as it is the largest value available for selection.

        Issue Links

          Activity

          Hide
          Damyon Wiese added a comment -

          Thanks for reporting this and providing a solution. I agree with the suggestion and have marked this issue for peer review.

          Show
          Damyon Wiese added a comment - Thanks for reporting this and providing a solution. I agree with the suggestion and have marked this issue for peer review.
          Hide
          Damyon Wiese added a comment - - edited

          [Y] Syntax
          [-] Output
          [Y] Whitespace
          [Y] Language
          [-] Databases
          [Y] Testing
          [-] Security
          [-] Documentation
          [N] Git
          [Y] Sanity check

          Hi Cathal,

          Thanks for submitting this patch. It is almost perfect - the only thing that needs changing is the git commit messages before we accept this.

          The first two lines of the git commit messages should be in this format:
          <tracker number> <Component>: <message>
          <empty line>

          e.g.

          MDL-36644 Assignment: prepend the site upload limit to the options array
          
          More detail...
          

          And the first line should ideally be less than 80 chars (not a strict requirement). You can put more detail after the first 2 lines.

          If you can update your branches and add a comment I'll re-review this patch.

          Regards, Damyon

          Show
          Damyon Wiese added a comment - - edited [Y] Syntax [-] Output [Y] Whitespace [Y] Language [-] Databases [Y] Testing [-] Security [-] Documentation [N] Git [Y] Sanity check Hi Cathal, Thanks for submitting this patch. It is almost perfect - the only thing that needs changing is the git commit messages before we accept this. The first two lines of the git commit messages should be in this format: <tracker number> <Component>: <message> <empty line> e.g. MDL-36644 Assignment: prepend the site upload limit to the options array More detail... And the first line should ideally be less than 80 chars (not a strict requirement). You can put more detail after the first 2 lines. If you can update your branches and add a comment I'll re-review this patch. Regards, Damyon
          Hide
          Damyon Wiese added a comment -

          One more thing while we are looking at these lines of code.

          $choices = array(0=>get_string('siteuploadlimit', 'assignsubmission_file')) + $choices;

          Is not only prepending the site limit to the list of download options - it is also replacing the option for 0=>"0 bytes" (because that is how the php array union operator works).

          It has the desired effect - but I think this is not obvious from reading the code.

          I would prefer if we did this explicitly like this:

          $choices = get_max_upload_sizes...
          // Remove the option for 0 bytes.
          unset($choices[0]);
          
          Show
          Damyon Wiese added a comment - One more thing while we are looking at these lines of code. $choices = array(0=>get_string('siteuploadlimit', 'assignsubmission_file')) + $choices; Is not only prepending the site limit to the list of download options - it is also replacing the option for 0=>"0 bytes" (because that is how the php array union operator works). It has the desired effect - but I think this is not obvious from reading the code. I would prefer if we did this explicitly like this: $choices = get_max_upload_sizes... // Remove the option for 0 bytes. unset($choices[0]);
          Hide
          Cathal O'Riordan added a comment -

          Thanks Damyon. I've updated the commits as per your suggestions and pushed upto Github. Can you let me know if its okay?

          Show
          Cathal O'Riordan added a comment - Thanks Damyon. I've updated the commits as per your suggestions and pushed upto Github. Can you let me know if its okay?
          Hide
          Damyon Wiese added a comment -

          Hi Cathal

          Your git branch has both the old commits and some new ones with the updated comments and then a merge. We need to clean this up before we can integrate this patch.

          In case you are not familiar with git I'll put some instructions:

          We can use an interactive rebase for this:

          git rebase -i 8ccaa296fa906d1fb4cf75068d8afe5e5e68c031 (the commit hash from just before your patches)

          This will open an editor with one line for each commit, we change the first word of each line to tell git what we want to do with each commit:

          Make it like this:

          pick 944ef56 MDL-36644 Assignment: ... 
          squash 99f71bf append course ...
          pick a5cd42b MDL-36644 Assignment: ...
          squash 7243ced append the site upload ...  
          squash 82621e0 append the site upload ...
          

          This will then open an editor to set the commit message for the first commit - clean it up (Remove any duplication in the comments) save and quit

          This will then open an editor to set the commit message for the second commit - clean it up (Remove any duplication in the comments) save and quit

          You should now have a clean branch with only 2 commits - repush that to git and you are done (You will need to use --force to push it as we are re-writing history).

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Hi Cathal Your git branch has both the old commits and some new ones with the updated comments and then a merge. We need to clean this up before we can integrate this patch. In case you are not familiar with git I'll put some instructions: We can use an interactive rebase for this: git rebase -i 8ccaa296fa906d1fb4cf75068d8afe5e5e68c031 (the commit hash from just before your patches) This will open an editor with one line for each commit, we change the first word of each line to tell git what we want to do with each commit: Make it like this: pick 944ef56 MDL-36644 Assignment: ... squash 99f71bf append course ... pick a5cd42b MDL-36644 Assignment: ... squash 7243ced append the site upload ... squash 82621e0 append the site upload ... This will then open an editor to set the commit message for the first commit - clean it up (Remove any duplication in the comments) save and quit This will then open an editor to set the commit message for the second commit - clean it up (Remove any duplication in the comments) save and quit You should now have a clean branch with only 2 commits - repush that to git and you are done (You will need to use --force to push it as we are re-writing history). Thanks, Damyon
          Hide
          Cathal O'Riordan added a comment -

          Hi Damyon,

          Thanks for the tip - I'm still getting to grips with git

          The branches should be tidied up now.

          cheers,
          Cathal.

          Show
          Cathal O'Riordan added a comment - Hi Damyon, Thanks for the tip - I'm still getting to grips with git The branches should be tidied up now. cheers, Cathal.
          Hide
          Damyon Wiese added a comment -

          I fixed the format of the php comment in the last commit and rebased to latest integration.

          Show
          Damyon Wiese added a comment - I fixed the format of the php comment in the last commit and rebased to latest integration.
          Hide
          Damyon Wiese added a comment -

          Thanks Cathal for submitting this patch and helping to improve it.

          Thumbs up from me!

          Show
          Damyon Wiese added a comment - Thanks Cathal for submitting this patch and helping to improve it. Thumbs up from me!
          Hide
          Dan Poltawski added a comment -

          It took me a while to understand this is about the difference between a php array and 'dictionary' (for want of a better word) and the ordering which the entires enter it.

          Show
          Dan Poltawski added a comment - It took me a while to understand this is about the difference between a php array and 'dictionary' (for want of a better word) and the ordering which the entires enter it.
          Hide
          Dan Poltawski added a comment -

          Integrated to master 24 and 23, thanks!

          (BTW I rebased the two commits into one as they had the same commit message and it made life simpler to apply to 24)

          Show
          Dan Poltawski added a comment - Integrated to master 24 and 23, thanks! (BTW I rebased the two commits into one as they had the same commit message and it made life simpler to apply to 24)
          Hide
          Michael de Raadt added a comment -

          Test result: Success!

          Tested in 2.3 and master.

          I found a fault in the limits while I was testing this. I will look for an existing issue or report a new one.

          Show
          Michael de Raadt added a comment - Test result: Success! Tested in 2.3 and master. I found a fault in the limits while I was testing this. I will look for an existing issue or report a new one.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: