Moodle
  1. Moodle
  2. MDL-29400

Single file assignment displays 'File uploaded successfully' when no file uploaded

    Details

    • Testing Instructions:
      Hide
      1. The following test need to be run on 21, 22 and master
      2. create an assignments of type "Upload a single file" and "Advanced uploading of files"
      3. Go to upload a file on both.
      4. "Save changes" without choosing a file - the form should prevent you from saving changes
      5. Under "Settings/Assignment administration" you should NOT be able to see a message telling you that the assignment was submitted on a certain date, as can be seen in the attached screenshot [2012-03-07 12-08-31_Upload single file assignment - Mozilla Firefox.png].
      6. On both assignments go to upload a file again and this time actually upload a file. In the Advanced uploading of files, don't click "Send for marking" yet.
      7. Again under "Settings/Assignment administration" you should be able to see a message telling you that the assignment was submitted on a certain date, as can be seen in the attached screenshot. In the Advanced uploading of files assignment, the submission should be marked as a draft.
      8. Now, as a student, submit the Advanced upload of files
      9. Under "Settings/Assignment administration" you should see that the Advanced upload of files assignment submission is now complete.
      Show
      The following test need to be run on 21, 22 and master create an assignments of type "Upload a single file" and "Advanced uploading of files" Go to upload a file on both. "Save changes" without choosing a file - the form should prevent you from saving changes Under "Settings/Assignment administration" you should NOT be able to see a message telling you that the assignment was submitted on a certain date, as can be seen in the attached screenshot [2012-03-07 12-08-31_Upload single file assignment - Mozilla Firefox.png] . On both assignments go to upload a file again and this time actually upload a file. In the Advanced uploading of files, don't click "Send for marking" yet. Again under "Settings/Assignment administration" you should be able to see a message telling you that the assignment was submitted on a certain date, as can be seen in the attached screenshot. In the Advanced uploading of files assignment, the submission should be marked as a draft. Now, as a student, submit the Advanced upload of files Under "Settings/Assignment administration" you should see that the Advanced upload of files assignment submission is now complete.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Rank (Obsolete):
      18964

      Description

      Assignment file upload without choosing a file?
      Shouldn't an error like "Error: No file selected" or something come up?
      Now you see a submitted date and think you are done with this.

      /moodle/mod/assignment/type/upload/upload.php

      1. MDL-29400.patch
        4 kB
        Michael de Raadt
      1. 2012-03-07 12-08-31_Upload single file assignment - Mozilla Firefox.png
        89 kB
      2. Auswahl_047.png
        47 kB
      3. screenshot3.png
        101 kB
      4. student_view.jpg
        40 kB
      5. teacher_view.jpg
        46 kB

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting that. I was able to reproduce the problem where the assignment settings block showed that the student had submitted their assignment, even though they had not yet clicked the "Send for marking" button. This happens regardless of whether the student has uploaded files or not.

          I suspect the record in the database is needed for the submission, even before the assignment is sent, but the settings block should behave correctly and not show that the assignment has been submitted until it actually has.

          Show
          Michael de Raadt added a comment - Thanks for reporting that. I was able to reproduce the problem where the assignment settings block showed that the student had submitted their assignment, even though they had not yet clicked the "Send for marking" button. This happens regardless of whether the student has uploaded files or not. I suspect the record in the database is needed for the submission, even before the assignment is sent, but the settings block should behave correctly and not show that the assignment has been submitted until it actually has.
          Hide
          Stephan Kaftanski added a comment -

          2.1.2: problem still exists

          Show
          Stephan Kaftanski added a comment - 2.1.2: problem still exists
          Hide
          Benjamin Wolf added a comment -

          The easiest way to fix this would be to set an required rule for the filepicker in the upload_form.php. But because of MDL-25937 the rule dosen't work in 2.1.2. It will be fixed in 2.2 when i understand it correct. Than that fix will be possible in 2.2.

          Show
          Benjamin Wolf added a comment - The easiest way to fix this would be to set an required rule for the filepicker in the upload_form.php. But because of MDL-25937 the rule dosen't work in 2.1.2. It will be fixed in 2.2 when i understand it correct. Than that fix will be possible in 2.2.
          Hide
          Gerard Caulfield added a comment -

          @Benjamin Wolf: Just to save anybody chasing their tail, it's been a little while since I looked at this, but I seem to remember that the problem occurred before the form was submitted. There is a necessary database entry that is created upon loading the form page. It is this entry that the alert is seeing, so I do not believe what you are suggesting would fix this. There also appeared to be some other linked issues, but I can't be sure just yet.

          Show
          Gerard Caulfield added a comment - @Benjamin Wolf: Just to save anybody chasing their tail, it's been a little while since I looked at this, but I seem to remember that the problem occurred before the form was submitted. There is a necessary database entry that is created upon loading the form page. It is this entry that the alert is seeing, so I do not believe what you are suggesting would fix this. There also appeared to be some other linked issues, but I can't be sure just yet.
          Hide
          Michael de Raadt added a comment -

          Here are Gerry's comments from his task...

          Sorry for the delay, it took a little while for me to start to get the hang
          of things and get set up. In particular I think I need I'll need to go out
          and buy a lot more RAM.

          The problem is that the $instance->get_submission($formdata->userid,
          true); call is creating a submission and the display code is only checking
          that a submission exists before outputting the date and other links
          associated with a valid submission. Setting the creation argument of the
          get_submission call to false would fix this problem, but cause another. The
          other problem being that the upload form needs to receive a submission ID
          which it then passes on for the uploaded file to be linked to. Part 1 of
          the MDL-29400.patch file that I attached fixes this issue. I imagine it
          would be possible to generate this at the time the form is submitted and I
          originally started looking into this solution, however it was bringing me
          to more questions than answers and so I didn't feel confident enough to
          make a change like that without being able to ask the rest of the team some
          questions in person.

          This bug also seems to affect
          assignment_base::assignment_count_real_submissions() (and it's wrapper
          method) which counts submissions even when they have no files associated
          with them. Without first hand information on what constitutes a "real"
          submission I have assumed it is one that has files and as such I have added
          a check to the SQL in the previously mentioned method. This check can be
          seen in part 2 of the MDL-29400.patch file.

          assignment_base::display_submissions() and related functions may also need
          to be altered if we don't wish to display empty submissions on
          /mod/assignment/submissions.php

          I've also included a patch for slightly improving the print_object
          function. Normally when a value of false or NULL is passed to it, the
          function doesn't output anything that would be visible without looking at
          the source. For a debugging function, this could end up causing a lot of
          pain. I also included a call to print_location_comment() so that calls can
          be tracked down from their output. If there are currently any production
          calls to this function which previously didn't output anything due to their
          argument values being false or NULL, then this patch would cause an issue.
          For this reason I wanted to set the function to only display for
          developers, but the logic in the debugging() had not been abstracted out
          and I do not feel confident enough about my knowledge of the Moodle code
          base yet to do so. But I've passed along the patch anyway just in case you
          wished to do something with it.

          Show
          Michael de Raadt added a comment - Here are Gerry's comments from his task... Sorry for the delay, it took a little while for me to start to get the hang of things and get set up. In particular I think I need I'll need to go out and buy a lot more RAM. The problem is that the $instance->get_submission($formdata->userid, true); call is creating a submission and the display code is only checking that a submission exists before outputting the date and other links associated with a valid submission. Setting the creation argument of the get_submission call to false would fix this problem, but cause another. The other problem being that the upload form needs to receive a submission ID which it then passes on for the uploaded file to be linked to. Part 1 of the MDL-29400 .patch file that I attached fixes this issue. I imagine it would be possible to generate this at the time the form is submitted and I originally started looking into this solution, however it was bringing me to more questions than answers and so I didn't feel confident enough to make a change like that without being able to ask the rest of the team some questions in person. This bug also seems to affect assignment_base::assignment_count_real_submissions() (and it's wrapper method) which counts submissions even when they have no files associated with them. Without first hand information on what constitutes a "real" submission I have assumed it is one that has files and as such I have added a check to the SQL in the previously mentioned method. This check can be seen in part 2 of the MDL-29400 .patch file. assignment_base::display_submissions() and related functions may also need to be altered if we don't wish to display empty submissions on /mod/assignment/submissions.php I've also included a patch for slightly improving the print_object function. Normally when a value of false or NULL is passed to it, the function doesn't output anything that would be visible without looking at the source. For a debugging function, this could end up causing a lot of pain. I also included a call to print_location_comment() so that calls can be tracked down from their output. If there are currently any production calls to this function which previously didn't output anything due to their argument values being false or NULL, then this patch would cause an issue. For this reason I wanted to set the function to only display for developers, but the logic in the debugging() had not been abstracted out and I do not feel confident enough about my knowledge of the Moodle code base yet to do so. But I've passed along the patch anyway just in case you wished to do something with it.
          Hide
          Gerard Caulfield added a comment -

          I couldn't find any documentation on the fields used, so I've based my fix on observations made during the various stages of uploading a file in both the advanced file uploader and the simple uploader.

          Show
          Gerard Caulfield added a comment - I couldn't find any documentation on the fields used, so I've based my fix on observations made during the various stages of uploading a file in both the advanced file uploader and the simple uploader.
          Hide
          Andrew Davis added a comment -

          The code changes look good. Just improve the test instructions a little. After step 3 explain what the tester should see.

          Show
          Andrew Davis added a comment - The code changes look good. Just improve the test instructions a little. After step 3 explain what the tester should see.
          Hide
          Gerard Caulfield added a comment -

          Thanks for Peer reviewing both my patches Andrew. Updated the testing instructions.

          Show
          Gerard Caulfield added a comment - Thanks for Peer reviewing both my patches Andrew. Updated the testing instructions.
          Hide
          Gerard Caulfield added a comment -

          BTW, just to be safe I'll mention that the patch attachment is not the correct patch. The correct patch is the one in the github repo.

          Show
          Gerard Caulfield added a comment - BTW, just to be safe I'll mention that the patch attachment is not the correct patch. The correct patch is the one in the github repo.
          Hide
          Martin Dougiamas added a comment -

          Note this needs patches for master, 2.2 and 2.1

          Show
          Martin Dougiamas added a comment - Note this needs patches for master, 2.2 and 2.1
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Integrated, thanks!

          Note this has been backported to 20, 21 and 22 STABLE branches (by cherry-picking, applied clean).

          Some comments:

          • When sending issues to integration it's better to provide as many branches as possible OR, if cherry-picking is ok, add one note to integrators: "To integrators: this should be backported to XX, YY and ZZ" or "To integrators: this should be backported to YY, not need for XX".
          • Testing instructions may not only cover one of the cases (the user does not pick a file), but also the opposite (the user picks one file). And in this patch we need to cover both (coz we have changed the condition controlling that). And not only for one of the assignment types, but for both, as far as the conditions are different too.
          • Also, ideally, testing instructions should include the list of branches where testing is needed. In this case, 20, 21 and 22 (master === 22 right now).

          Please amend the instructions...TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Integrated, thanks! Note this has been backported to 20, 21 and 22 STABLE branches (by cherry-picking, applied clean). Some comments: When sending issues to integration it's better to provide as many branches as possible OR, if cherry-picking is ok, add one note to integrators: "To integrators: this should be backported to XX, YY and ZZ" or "To integrators: this should be backported to YY, not need for XX". Testing instructions may not only cover one of the cases (the user does not pick a file), but also the opposite (the user picks one file). And in this patch we need to cover both (coz we have changed the condition controlling that). And not only for one of the assignment types, but for both, as far as the conditions are different too. Also, ideally, testing instructions should include the list of branches where testing is needed. In this case, 20, 21 and 22 (master === 22 right now). Please amend the instructions...TIA and ciao
          Hide
          Gerard Caulfield added a comment - - edited

          I already said a little of this Eloy over chat, but I'll elaborate here for posterity.

          Sorry about not supplying those other patches. I was actually mid way through it yesterday when I had to run off. I should have changed the status back to development while I worked on it. There's a small issue with the hamachi server at the moment (Jordan is already onto it) so finishing it off from home wasn't really an option.

          Testing instructions may not only cover one of the cases (the user does not pick a file), but also the opposite (the user picks one file). And in this patch we need to cover both (coz we have changed the condition controlling that). And not only for one of the assignment types, but for both, as far as the conditions are different too.
          Also, ideally, testing instructions should include the list of branches where testing is needed. In this case, 20, 21 and 22 (master === 22 right now).

          I wanted to make the test instructions as simple as possible so I could be sure that people would actually do the test instead of just saying they did. Ideally the test would test that there the the problem exists and is fixed (2 branch types, stable and integration) on all branches (4 branches) setting 2 of each type of file upload assignment (2 single uploads, 2 multiple file uploads), one with an upload and one without. However this would be 32 steps (2*4*4) over 8 different environments, not including any the substeps. I was trying to remove the parts where you can possibly make an assumption. I believe that if we actually want this level of testing then it would be better to write a test case for every bug (either by the submitter or the assignee) and then have it automatically run on all of the various branches.

          I will write the test as you have requested though.

          Also thanks for the feedback, doing the cherry-picking and integrating.

          Show
          Gerard Caulfield added a comment - - edited I already said a little of this Eloy over chat, but I'll elaborate here for posterity. Sorry about not supplying those other patches. I was actually mid way through it yesterday when I had to run off. I should have changed the status back to development while I worked on it. There's a small issue with the hamachi server at the moment (Jordan is already onto it) so finishing it off from home wasn't really an option. Testing instructions may not only cover one of the cases (the user does not pick a file), but also the opposite (the user picks one file). And in this patch we need to cover both (coz we have changed the condition controlling that). And not only for one of the assignment types, but for both, as far as the conditions are different too. Also, ideally, testing instructions should include the list of branches where testing is needed. In this case, 20, 21 and 22 (master === 22 right now). I wanted to make the test instructions as simple as possible so I could be sure that people would actually do the test instead of just saying they did. Ideally the test would test that there the the problem exists and is fixed (2 branch types, stable and integration) on all branches (4 branches) setting 2 of each type of file upload assignment (2 single uploads, 2 multiple file uploads), one with an upload and one without. However this would be 32 steps (2*4*4) over 8 different environments, not including any the substeps. I was trying to remove the parts where you can possibly make an assumption. I believe that if we actually want this level of testing then it would be better to write a test case for every bug (either by the submitter or the assignee) and then have it automatically run on all of the various branches. I will write the test as you have requested though. Also thanks for the feedback, doing the cherry-picking and integrating.
          Hide
          Gerard Caulfield added a comment -

          Updating the test in the way that Eloy has recommended

          Show
          Gerard Caulfield added a comment - Updating the test in the way that Eloy has recommended
          Hide
          Rajesh Taneja added a comment -

          Sorry guys,
          It fails... It will be good to put check as !empty($submission->numfiles), as you can't get property on a non-object

          Steps to reproduce:

          1. Create an assignments of type "upload single"
          2. click save and display
          3. you will see this error:
          Notice: Trying to get property of non-object in /usr/local/www/moodleintegration/mod/assignment/type/uploadsingle/assignment.class.php on line 322 Call Stack: 0.0031 679808 1. {main}() /usr/local/www/moodleintegration/mod/assignment/view.php:0 0.1875 42617632 2. assignment_uploadsingle->view() /usr/local/www/moodleintegration/mod/assignment/view.php:51 0.2006 43234312 3. assignment_base->view_header() /usr/local/www/moodleintegration/mod/assignment/type/uploadsingle/assignment.class.php:70 0.2009 43235752 4. bootstrap_renderer->header() /usr/local/www/moodleintegration/mod/assignment/lib.php:194 0.2009 43236144 5. bootstrap_renderer->__call() /usr/local/www/moodleintegration/lib/setuplib.php:194 0.2043 43328800 6. call_user_func_array() /usr/local/www/moodleintegration/lib/setuplib.php:1363 0.2043 43329176 7. core_renderer->header() /usr/local/www/moodleintegration/lib/setuplib.php:1363 0.2149 43889352 8. core_renderer->render_page_layout() /usr/local/www/moodleintegration/lib/outputrenderers.php:637 0.2151 43986776 9. include('/usr/local/www/moodleintegration/theme/base/layout/general.php') /usr/local/www/moodleintegration/lib/outputrenderers.php:685 0.2979 47917792 10. block_manager->region_has_content() /usr/local/www/moodleintegration/theme/base/layout/general.php:6 0.2979 47917792 11. block_manager->ensure_content_created() /usr/local/www/moodleintegration/lib/blocklib.php:349 0.2980 47918104 12. block_manager->create_block_contents() /usr/local/www/moodleintegration/lib/blocklib.php:978 0.3649 48168656 13. block_base->get_content_for_output() /usr/local/www/moodleintegration/lib/blocklib.php:926 0.3651 48171504 14. block_base->formatted_contents() /usr/local/www/moodleintegration/blocks/moodleblock.class.php:232 0.3651 48171504 15. block_settings->get_content() /usr/local/www/moodleintegration/blocks/moodleblock.class.php:280 0.3658 48243320 16. moodle_page->__get() /usr/local/www/moodleintegration/lib/pagelib.php:132 0.3658 48243472 17. moodle_page->magic_get_settingsnav() /usr/local/www/moodleintegration/lib/pagelib.php:617 0.3659 48252544 18. settings_navigation->initialise() /usr/local/www/moodleintegration/lib/pagelib.php:601 0.3659 48252688 19. settings_navigation->load_module_settings() /usr/local/www/moodleintegration/lib/navigationlib.php:2867 0.3818 48626008 20. assignment_extend_settings_navigation() /usr/local/www/moodleintegration/lib/navigationlib.php:3458 0.3870 48644664 21. assignment_uploadsingle->extend_settings_navigation() /usr/local/www/moodleintegration/mod/assignment/lib.php:3906 
          
          Show
          Rajesh Taneja added a comment - Sorry guys, It fails... It will be good to put check as !empty($submission->numfiles), as you can't get property on a non-object Steps to reproduce: Create an assignments of type "upload single" click save and display you will see this error: Notice: Trying to get property of non-object in /usr/local/www/moodleintegration/mod/assignment/type/uploadsingle/assignment.class.php on line 322 Call Stack: 0.0031 679808 1. {main}() /usr/local/www/moodleintegration/mod/assignment/view.php:0 0.1875 42617632 2. assignment_uploadsingle->view() /usr/local/www/moodleintegration/mod/assignment/view.php:51 0.2006 43234312 3. assignment_base->view_header() /usr/local/www/moodleintegration/mod/assignment/type/uploadsingle/assignment.class.php:70 0.2009 43235752 4. bootstrap_renderer->header() /usr/local/www/moodleintegration/mod/assignment/lib.php:194 0.2009 43236144 5. bootstrap_renderer->__call() /usr/local/www/moodleintegration/lib/setuplib.php:194 0.2043 43328800 6. call_user_func_array() /usr/local/www/moodleintegration/lib/setuplib.php:1363 0.2043 43329176 7. core_renderer->header() /usr/local/www/moodleintegration/lib/setuplib.php:1363 0.2149 43889352 8. core_renderer->render_page_layout() /usr/local/www/moodleintegration/lib/outputrenderers.php:637 0.2151 43986776 9. include('/usr/local/www/moodleintegration/theme/base/layout/general.php') /usr/local/www/moodleintegration/lib/outputrenderers.php:685 0.2979 47917792 10. block_manager->region_has_content() /usr/local/www/moodleintegration/theme/base/layout/general.php:6 0.2979 47917792 11. block_manager->ensure_content_created() /usr/local/www/moodleintegration/lib/blocklib.php:349 0.2980 47918104 12. block_manager->create_block_contents() /usr/local/www/moodleintegration/lib/blocklib.php:978 0.3649 48168656 13. block_base->get_content_for_output() /usr/local/www/moodleintegration/lib/blocklib.php:926 0.3651 48171504 14. block_base->formatted_contents() /usr/local/www/moodleintegration/blocks/moodleblock.class.php:232 0.3651 48171504 15. block_settings->get_content() /usr/local/www/moodleintegration/blocks/moodleblock.class.php:280 0.3658 48243320 16. moodle_page->__get() /usr/local/www/moodleintegration/lib/pagelib.php:132 0.3658 48243472 17. moodle_page->magic_get_settingsnav() /usr/local/www/moodleintegration/lib/pagelib.php:617 0.3659 48252544 18. settings_navigation->initialise() /usr/local/www/moodleintegration/lib/pagelib.php:601 0.3659 48252688 19. settings_navigation->load_module_settings() /usr/local/www/moodleintegration/lib/navigationlib.php:2867 0.3818 48626008 20. assignment_extend_settings_navigation() /usr/local/www/moodleintegration/lib/navigationlib.php:3458 0.3870 48644664 21. assignment_uploadsingle->extend_settings_navigation() /usr/local/www/moodleintegration/mod/assignment/lib.php:3906
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yeah, Rajesh, well spotted, my fault!

          Not sure why but I remember myself assuming that surely the empty() check was not necessary there because it originally had not it. Obviously I'm idiot and my logic an epic fail so yes, the empty() check is needed there to handle non-objects.

          Gerard, are you on it?

          Show
          Eloy Lafuente (stronk7) added a comment - Yeah, Rajesh, well spotted, my fault! Not sure why but I remember myself assuming that surely the empty() check was not necessary there because it originally had not it. Obviously I'm idiot and my logic an epic fail so yes, the empty() check is needed there to handle non-objects. Gerard, are you on it?
          Hide
          Gerard Caulfield added a comment - - edited

          No my fault and my epic fail. Yes I'm on this, although unfortunately can't currently connect from home to sort this right now. edit: I mean I could make the change, but I want to see why I never encountered the error and make sure there are no others. edit 2: And strangely enough my original patch added an isset() on the properties. slaps forhead

          Show
          Gerard Caulfield added a comment - - edited No my fault and my epic fail. Yes I'm on this, although unfortunately can't currently connect from home to sort this right now. edit: I mean I could make the change, but I want to see why I never encountered the error and make sure there are no others. edit 2: And strangely enough my original patch added an isset() on the properties. slaps forhead
          Hide
          Gerard Caulfield added a comment - - edited

          Despite having my debugging config as shown in the attached screenshot, I was not able to reproduce the bug Rajesh encountered until I changed the error reporting level just above the if statement.

          error_reporting(E_ALL);
          if ($submission->numfiles) {
          

          If I comment out the error_reporting call then the bug does not show up in my php_error log. I'm guessing I'll have to submit this as a bug.

          Also on top of this despite having "Display debug messages" enabled I have to set ini_set('display_errors', 1); before the notice is displayed for me. Very strange.

          Strange that this has not been discovered before now. Perhaps it is something to do with the fact I have xdebug installed, however I don't believe I should have to set error_reporting manually.

          Either way I'll make the change now that I know how to see the errors.

          Show
          Gerard Caulfield added a comment - - edited Despite having my debugging config as shown in the attached screenshot, I was not able to reproduce the bug Rajesh encountered until I changed the error reporting level just above the if statement. error_reporting(E_ALL); if ($submission->numfiles) { If I comment out the error_reporting call then the bug does not show up in my php_error log. I'm guessing I'll have to submit this as a bug. Also on top of this despite having "Display debug messages" enabled I have to set ini_set('display_errors', 1); before the notice is displayed for me. Very strange. Strange that this has not been discovered before now. Perhaps it is something to do with the fact I have xdebug installed, however I don't believe I should have to set error_reporting manually. Either way I'll make the change now that I know how to see the errors.
          Hide
          Gerard Caulfield added a comment - - edited

          @Eloy I've made the changes. I did them as a second commit to make thing easier for you as the first is already in integration (after asking Aparup if it would make things easier). I used isset instead of empty as we are already testing the value, it would be like writing:

          if(isset($submission->data2) AND $submission->data2 != '' AND $submission->data2 == 'submitted')

          If it doesn't make it into this release, just say the word and I can rebase it into a single commit.

          Cheers

          Gerry

          Show
          Gerard Caulfield added a comment - - edited @Eloy I've made the changes. I did them as a second commit to make thing easier for you as the first is already in integration (after asking Aparup if it would make things easier). I used isset instead of empty as we are already testing the value, it would be like writing: if (isset($submission->data2) AND $submission->data2 != '' AND $submission->data2 == 'submitted') If it doesn't make it into this release, just say the word and I can rebase it into a single commit. Cheers Gerry
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Thanks Gerard!

          The new commit has been sent to all branches, so looking forward a new testing round...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Thanks Gerard! The new commit has been sent to all branches, so looking forward a new testing round...ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sorry, but I think this cannot be considered complete as far as both the teacher (teacher_view attachment) and the student (student_view attachment) continue getting feedback about one existing submission.

          I'm failing-testing this again. The fix applied does not hurt (as far as the student does not see the submission in the block anymore) so I won't be reverting it in code base, but it is incomplete.

          So I'm reopening it but, as said, keeping the changes up to now applied.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Sorry, but I think this cannot be considered complete as far as both the teacher (teacher_view attachment) and the student (student_view attachment) continue getting feedback about one existing submission. I'm failing-testing this again. The fix applied does not hurt (as far as the student does not see the submission in the block anymore) so I won't be reverting it in code base, but it is incomplete. So I'm reopening it but, as said, keeping the changes up to now applied. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Testing failed & reopenend

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Testing failed & reopenend
          Hide
          Michael de Raadt added a comment -

          Well done for getting this far, Gerry.

          Please launch a new issue for the teacher count and link it to this issue. Then this solution should be able to be integrated for the current problem.

          Show
          Michael de Raadt added a comment - Well done for getting this far, Gerry. Please launch a new issue for the teacher count and link it to this issue. Then this solution should be able to be integrated for the current problem.
          Hide
          Gerard Caulfield added a comment -

          @Eloy and Michael: I already launched and supplied a fix for the teacher count error MDL-30724 (although please see my last comment on MDL-30724), however I didn't spot the other date field on the student view, so I will still have to fix that.

          Show
          Gerard Caulfield added a comment - @Eloy and Michael: I already launched and supplied a fix for the teacher count error MDL-30724 (although please see my last comment on MDL-30724 ), however I didn't spot the other date field on the student view, so I will still have to fix that.
          Hide
          Gerard Caulfield added a comment -

          With the issue of the header date on the student view problem was quite different in both the solution and severity, so I split it off into another issue as suggested: MDL-30957 This issue can probably go to integration testing now if there are no objections?

          Show
          Gerard Caulfield added a comment - With the issue of the header date on the student view problem was quite different in both the solution and severity, so I split it off into another issue as suggested: MDL-30957 This issue can probably go to integration testing now if there are no objections?
          Hide
          Michael de Raadt added a comment -

          Carrying this over to the next STABLE Sprint.

          Show
          Michael de Raadt added a comment - Carrying this over to the next STABLE Sprint.
          Hide
          Jean-Philippe Gaudreau added a comment -

          Hi everyone,

          Don't know if it helps but here's the patch we made to fix this problem for "Upload a single file" assignments :
          https://github.com/StudiumDevel/moodle/commit/ad4df03ae0be301ad4b62c0e5b206558cd2c6f03

          I think personnally that it's a better idea to avoid adding submissions if there's no file uploaded than trying to hide submissions that don't have any file.

          Still, it seems more difficult to fix this problem with "Advanced uploading of files" assignments because the submission is created before uploading any files when clicking on "Upload files" button (mod/assignment/type/upload/upload.php). I don't have that much time to put on this task but i'm sure there's a way to apply a similar pattern for the "Advanced uploading of files" assignments.

          Let me know what you think about this! Thx!

          Show
          Jean-Philippe Gaudreau added a comment - Hi everyone, Don't know if it helps but here's the patch we made to fix this problem for "Upload a single file" assignments : https://github.com/StudiumDevel/moodle/commit/ad4df03ae0be301ad4b62c0e5b206558cd2c6f03 I think personnally that it's a better idea to avoid adding submissions if there's no file uploaded than trying to hide submissions that don't have any file. Still, it seems more difficult to fix this problem with "Advanced uploading of files" assignments because the submission is created before uploading any files when clicking on "Upload files" button (mod/assignment/type/upload/upload.php). I don't have that much time to put on this task but i'm sure there's a way to apply a similar pattern for the "Advanced uploading of files" assignments. Let me know what you think about this! Thx!
          Hide
          Gerard Caulfield added a comment -

          I originally wanted to do that. I agree it's a more ideal solution. However I ran into the same problem with advanced upload complexities vs time it would take to solve the issue. I'll take a second look though, when I next get a chance. Thanks for the patch.

          Show
          Gerard Caulfield added a comment - I originally wanted to do that. I agree it's a more ideal solution. However I ran into the same problem with advanced upload complexities vs time it would take to solve the issue. I'll take a second look though, when I next get a chance. Thanks for the patch.
          Hide
          Jean-Philippe Gaudreau added a comment -

          Here's what I have for "advanced uploading of files" assignment activities that checks if files exist before adding the submission :
          https://github.com/StudiumDevel/moodle/commit/ae38f631fe16f0d3ad583cb2af0c26017d8212f6

          I had to change a bit of code in upload.php so that the submission isn't created when you click on "Upload files" button but only when uploading the files (function upload_file in assignment.class.php).

          This fix is still incomplete in my opinion. There should be another validation that checks if the submission is modified and saved with no files after a first submission is done. If so, the submission should be deleted (Submission in draft mode).

          Can someone tell me what is the use of the function upload_responsefile in assignment,class.php? I think a fix is needed there too but I can't find a way to test it in Moodle...

          Show
          Jean-Philippe Gaudreau added a comment - Here's what I have for "advanced uploading of files" assignment activities that checks if files exist before adding the submission : https://github.com/StudiumDevel/moodle/commit/ae38f631fe16f0d3ad583cb2af0c26017d8212f6 I had to change a bit of code in upload.php so that the submission isn't created when you click on "Upload files" button but only when uploading the files (function upload_file in assignment.class.php). This fix is still incomplete in my opinion. There should be another validation that checks if the submission is modified and saved with no files after a first submission is done. If so, the submission should be deleted (Submission in draft mode). Can someone tell me what is the use of the function upload_responsefile in assignment,class.php? I think a fix is needed there too but I can't find a way to test it in Moodle...
          Hide
          Gerard Caulfield added a comment -

          Hey Aparup, I've added you to this as it seems you were the last to touch that function in both mod/assignment/type/uploadsingle/assignment.class.php and the last to touch the function header in mod/assignment/type/upload/assignment.class.php so I figured you might have some idea of it's purpose.

          Show
          Gerard Caulfield added a comment - Hey Aparup, I've added you to this as it seems you were the last to touch that function in both mod/assignment/type/uploadsingle/assignment.class.php and the last to touch the function header in mod/assignment/type/upload/assignment.class.php so I figured you might have some idea of it's purpose.
          Hide
          Aparup Banerjee added a comment -

          I understand from Gerry, he's referring to upload_responsefile().

          From what i recall, response files are the files a user grading a submission wants to show in 'response' during the grading of a submission. Its not the best of terms here I'd agree.

          Show
          Aparup Banerjee added a comment - I understand from Gerry, he's referring to upload_responsefile(). From what i recall, response files are the files a user grading a submission wants to show in 'response' during the grading of a submission. Its not the best of terms here I'd agree.
          Hide
          Jean-Philippe Gaudreau added a comment -

          aaahh ok thx Aparup it's now clear to me where the function is call . At first sight, I would say we don't have to modify this function.

          Show
          Jean-Philippe Gaudreau added a comment - aaahh ok thx Aparup it's now clear to me where the function is call . At first sight, I would say we don't have to modify this function.
          Hide
          Gerard Caulfield added a comment -

          @Jean-Philipee
          In order to get the major issue resolved for most people as soon as possible I've submitted my patches in MDL-30957 and MDL-30724.

          Your changes very good and worth pursuing. I did however encountered an issue where on advanced file uploads the Revert to Draft button stops working. I have created a separate issue (MDL-31545) so that work on this can still continue, including the other change which you mentioned about removing a submission which is re-submitted without files.

          Show
          Gerard Caulfield added a comment - @Jean-Philipee In order to get the major issue resolved for most people as soon as possible I've submitted my patches in MDL-30957 and MDL-30724 . Your changes very good and worth pursuing. I did however encountered an issue where on advanced file uploads the Revert to Draft button stops working. I have created a separate issue ( MDL-31545 ) so that work on this can still continue, including the other change which you mentioned about removing a submission which is re-submitted without files.
          Hide
          moodle.com added a comment -

          Assigning to Dan to wrap up.

          Show
          moodle.com added a comment - Assigning to Dan to wrap up.
          Hide
          Gerard Caulfield added a comment -

          Dan, just so you know, there was one change already integrated with the MDL-29400 label. Another part was fixed by MDL-30957 and the rest is fixed by MDL-30724 as soon as it passes integration and testing. So after MDL-30724 gets through, this can be closed.

          Show
          Gerard Caulfield added a comment - Dan, just so you know, there was one change already integrated with the MDL-29400 label. Another part was fixed by MDL-30957 and the rest is fixed by MDL-30724 as soon as it passes integration and testing. So after MDL-30724 gets through, this can be closed.
          Hide
          Dan Poltawski added a comment -

          thanks Gerry

          Show
          Dan Poltawski added a comment - thanks Gerry
          Hide
          Michael de Raadt added a comment -

          I've just attached a screenshot I captured while testing MDL-30724. In a single upload assignment, as a student, after clicking Save without adding any files, the assignment record was produced and the submission appeared in the submissions table. The count is recognised correctly (which was the issue related to MDL-30724), but I'm not sure if the submission should appear in the submissions table.

          Show
          Michael de Raadt added a comment - I've just attached a screenshot I captured while testing MDL-30724 . In a single upload assignment, as a student, after clicking Save without adding any files, the assignment record was produced and the submission appeared in the submissions table. The count is recognised correctly (which was the issue related to MDL-30724 ), but I'm not sure if the submission should appear in the submissions table.
          Hide
          Gerard Caulfield added a comment -

          Hi Michael

          Good point. I actually spoke to you to find out if drafts in advanced file uploads should still show up in this list and you said that they should, however I don't believe I'd looked at that table while having a single upload assignment with no attached files. Now that I think of it, it's quite weird that the form allows saving of a single file submission without first attaching any files. Anyway my hands are off this, just wanted to add some input in case it helps.

          Show
          Gerard Caulfield added a comment - Hi Michael Good point. I actually spoke to you to find out if drafts in advanced file uploads should still show up in this list and you said that they should, however I don't believe I'd looked at that table while having a single upload assignment with no attached files. Now that I think of it, it's quite weird that the form allows saving of a single file submission without first attaching any files. Anyway my hands are off this, just wanted to add some input in case it helps.
          Hide
          Dan Poltawski added a comment -

          I have updated the summary to what seems to be the remaining issue.

          Show
          Dan Poltawski added a comment - I have updated the summary to what seems to be the remaining issue.
          Hide
          Dan Poltawski added a comment -

          OK i've fixed the final remaining issue in this by making the upload file form field a required field. Submitting for peer review.

          Show
          Dan Poltawski added a comment - OK i've fixed the final remaining issue in this by making the upload file form field a required field. Submitting for peer review.
          Hide
          Andrew Davis added a comment -

          Code change looks fine. Do we want to supply a message for the second parameter to addRule()?

          Also, make a quick pass over the testing instructions. They talk about an assignment then "both assignments".

          Show
          Andrew Davis added a comment - Code change looks fine. Do we want to supply a message for the second parameter to addRule()? Also, make a quick pass over the testing instructions. They talk about an assignment then "both assignments".
          Hide
          Dan Poltawski added a comment -

          Actually i've just noticed that the form for advanced upload lets you submit the upload a file form without actually choosing any file. This is far from ideal from a UI point of view despite not creating a record so fixing that one too. Reverting to Michael's testing instructions.

          Show
          Dan Poltawski added a comment - Actually i've just noticed that the form for advanced upload lets you submit the upload a file form without actually choosing any file. This is far from ideal from a UI point of view despite not creating a record so fixing that one too. Reverting to Michael's testing instructions.
          Hide
          Dan Poltawski added a comment -

          Andrew, could you review my change again, thanks

          Show
          Dan Poltawski added a comment - Andrew, could you review my change again, thanks
          Hide
          Andrew Davis added a comment -

          Looks good.

          Just to clarify the branch information mentioned in the test instructions. This will most likely be going into master, 2.2 stable and 2.1 stable. The period for fixing general bugs in 2.0 has ended (http://docs.moodle.org/dev/Releases#Moodle_2.0)

          Show
          Andrew Davis added a comment - Looks good. Just to clarify the branch information mentioned in the test instructions. This will most likely be going into master, 2.2 stable and 2.1 stable. The period for fixing general bugs in 2.0 has ended ( http://docs.moodle.org/dev/Releases#Moodle_2.0 )
          Hide
          Dan Poltawski added a comment -

          Thanks, submitting for integration.

          Integrators - this patch can be cherry-picked into 21_STABLE and master.

          Show
          Dan Poltawski added a comment - Thanks, submitting for integration. Integrators - this patch can be cherry-picked into 21_STABLE and master.
          Hide
          Sam Hemelryk added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Sam Hemelryk added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Sam Hemelryk added a comment -

          Thanks all, this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks all, this has been integrated now.
          Hide
          Michael de Raadt added a comment -

          After consulting Dan, I've ammended the testing instructions to match the current fix.

          Show
          Michael de Raadt added a comment - After consulting Dan, I've ammended the testing instructions to match the current fix.
          Hide
          Michael de Raadt added a comment -

          Test result: Success.

          Tested in 2.1, 2.2 and master.

          I encountered the error described in MDL-26969 where the Advanced upload of files assignment creates an entry in the submissions table as soon as a student clicks the Upload files button.

          Show
          Michael de Raadt added a comment - Test result: Success. Tested in 2.1, 2.2 and master. I encountered the error described in MDL-26969 where the Advanced upload of files assignment creates an entry in the submissions table as soon as a student clicks the Upload files button.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this has landed upstream, finally! Yay!

          תודה רבה && شكرا جزيلا



          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this has landed upstream, finally! Yay! תודה רבה && شكرا جزيلا Closing, ciao

            People

            • Votes:
              13 Vote for this issue
              Watchers:
              14 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: