Moodle
  1. Moodle
  2. MDL-39118

Database file upload fails when file limit is set to course upload limit

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Labels:
      None
    • Rank:
      49710

      Description

      Found while testing MDLQA-5538

      When the File limit for picture and file fields are set to course upload limit, file uploads fail with the message: The file is larger than the space remaining in this area.

      Steps to reproduce:

      As a teacher:
      Create a database activity using the default settings.
      Add a file field named "file" be sure that the maximum size is set to course upload limit
      Add a picture field named "photo" be sure that the maximum size is set to course upload limit

      As a student:
      Navigate to the database activity and click "Add entry"
      Attempt to upload a file to the field named "file" be sure that the file is smaller than the course upload limit (For my testing I used a 5K file and the course limit was set to 2MB)
      save these changes

      Expected result: the file should upload successfully:
      Actual result: The following error is displayed: The file is larger than the space remaining in this area.

      Repeat the process with the field named Photo.

      Note this issue does not appear if the file limit is NOT set to "Course upload limit"

        Issue Links

          Activity

          Hide
          Adrian Greeve added a comment -

          I believe that issues that arise from QA testing don't need testing instructions here as they will be retested in QA. If I'm wrong then I'll provide some testing instructions here.

          Show
          Adrian Greeve added a comment - I believe that issues that arise from QA testing don't need testing instructions here as they will be retested in QA. If I'm wrong then I'll provide some testing instructions here.
          Hide
          Rajesh Taneja added a comment -

          Hello Adrian,

          Just wondering if you have considered using get_user_max_upload_file_size() or get_max_upload_file_size() to get proper upload size.

          Looking at picture/mod.html, and how param3 drop-down is created, I will probably go with checking course->maxbytes with isset and not empty(), as it can be 0 and there is no need to fetch this from db.

          if (!isset($PAGE->course->maxbytes)) {
              $coursebytes = $DB->get_field('course', 'maxbytes', array('id'=>$this->data->course));
          }
          get_user_max_upload_file_size($PAGE->context, $CFG->maxbytes, $coursebytes, $this->field->param3);
          

          This will make sure:

          1. DB activity maxbytesize is always less then course maxbytessize (Possible if course maxbytesize is reduced after creating database activity)
          2. Course maxbytes is 0, this will happen if site maxbytes is reduced after creating a course with maxbytesize more then site limit.
          3. Respect moodle/course:ignorefilesizelimits capability for users.
          Show
          Rajesh Taneja added a comment - Hello Adrian, Just wondering if you have considered using get_user_max_upload_file_size() or get_max_upload_file_size() to get proper upload size. Looking at picture/mod.html, and how param3 drop-down is created, I will probably go with checking course->maxbytes with isset and not empty(), as it can be 0 and there is no need to fetch this from db. if (!isset($PAGE->course->maxbytes)) { $coursebytes = $DB->get_field('course', 'maxbytes', array('id'=>$this->data->course)); } get_user_max_upload_file_size($PAGE->context, $CFG->maxbytes, $coursebytes, $this->field->param3); This will make sure: DB activity maxbytesize is always less then course maxbytessize (Possible if course maxbytesize is reduced after creating database activity) Course maxbytes is 0, this will happen if site maxbytes is reduced after creating a course with maxbytesize more then site limit. Respect moodle/course:ignorefilesizelimits capability for users.
          Hide
          Adrian Greeve added a comment -

          Hi Raj,

          I had a look at your suggestion and it is better than my solution, so I've made those changes. If you could have another look just to be sure that I got it right, that would be great.

          Thanks.

          Show
          Adrian Greeve added a comment - Hi Raj, I had a look at your suggestion and it is better than my solution, so I've made those changes. If you could have another look just to be sure that I got it right, that would be great. Thanks.
          Hide
          Rajesh Taneja added a comment -

          Thanks Adrian,

          Patch looks good to me.

          I am not sure, if you would like to add more testing instructions to cover different scenarios, like if site maxbytesize is less or user have capability etc.

          Show
          Rajesh Taneja added a comment - Thanks Adrian, Patch looks good to me. I am not sure, if you would like to add more testing instructions to cover different scenarios, like if site maxbytesize is less or user have capability etc.
          Hide
          Adrian Greeve added a comment - - edited

          The testing instructions have been altered in Moodle QA. Submitting for integration.

          Please note that both MDLQA-5538 and MDLQA-5539 will have to be reset once this patch is integrated.

          Show
          Adrian Greeve added a comment - - edited The testing instructions have been altered in Moodle QA. Submitting for integration. Please note that both MDLQA-5538 and MDLQA-5539 will have to be reset once this patch is integrated.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          Before this gets integrated I quick question, really a decision that I'll let you make.
          This solution solves the problem, however there is an easier solution available.
          In understanding this issue it appears the issue comes because of a difference in processing of filemanager options maxbytes and areamaxbytes.
          The maxbytes option is internally run through get_user_max_upload_file_size just as you are doing now.
          The areamaxbytes option however is left as is.
          So when field->param3 is set to 0 the maxbytes option gets set correctly and the areamaxbytes option doesn't.
          Now that may be a bug in itself perhaps and probably we should create a bug to improve that.
          As for this specific issue what I noticed is that we also set maxfiles to one for both of these fields. Because of that areamaxbytes is irrelevant anyway and you could just remove it from field.class.php.
          My personal preference would be towards removing that option from these two fields. In this particular case I don't think its relevant and I can't see a request coming through to allow multiple files in a file field within the database activity. Even if it did that would potentially represent a new setting and bridge best crossed at the time.
          Either way however I am really happy, so I'll leave it up to you.

          This will remain in integration review in process

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Before this gets integrated I quick question, really a decision that I'll let you make. This solution solves the problem, however there is an easier solution available. In understanding this issue it appears the issue comes because of a difference in processing of filemanager options maxbytes and areamaxbytes. The maxbytes option is internally run through get_user_max_upload_file_size just as you are doing now. The areamaxbytes option however is left as is. So when field->param3 is set to 0 the maxbytes option gets set correctly and the areamaxbytes option doesn't. Now that may be a bug in itself perhaps and probably we should create a bug to improve that. As for this specific issue what I noticed is that we also set maxfiles to one for both of these fields. Because of that areamaxbytes is irrelevant anyway and you could just remove it from field.class.php. My personal preference would be towards removing that option from these two fields. In this particular case I don't think its relevant and I can't see a request coming through to allow multiple files in a file field within the database activity. Even if it did that would potentially represent a new setting and bridge best crossed at the time. Either way however I am really happy, so I'll leave it up to you. This will remain in integration review in process Many thanks Sam
          Hide
          Adrian Greeve added a comment -

          Hi Sam,

          I think that removing areamaxbytes makes sense. At the moment the picture and file fields are only for uploading one file. We don't really need the areamaxbytes in this situation and as you pointed out if a demand for additional uploads comes about then we can deal with the problem at that point in time.

          I've created a new patch. Should I upload that on top of the last one?

          Show
          Adrian Greeve added a comment - Hi Sam, I think that removing areamaxbytes makes sense. At the moment the picture and file fields are only for uploading one file. We don't really need the areamaxbytes in this situation and as you pointed out if a demand for additional uploads comes about then we can deal with the problem at that point in time. I've created a new patch. Should I upload that on top of the last one?
          Hide
          Sam Hemelryk added a comment -

          Thanks Adrian - this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Adrian - this has been integrated now
          Hide
          Mark Nelson added a comment -

          Went through both QA instructions, they work as expected. Passing.

          Show
          Mark Nelson added a comment - Went through both QA instructions, they work as expected. Passing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I feel myself really alone tonight! So was time to push your fixes upstream!

          "Lest we forget. We will remember them."

          Thanks and ciao!

          Show
          Eloy Lafuente (stronk7) added a comment - I feel myself really alone tonight! So was time to push your fixes upstream! "Lest we forget. We will remember them." Thanks and ciao!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: