Moodle
  1. Moodle
  2. MDL-34640

Question types using uploaded files in responses cannot be automatically graded

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.4, 2.3.1, 2.4
    • Fix Version/s: 2.5
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide
      1. Run all the unit tests.
      2. Create a quiz containing an essay questions that allows files in the HTML editor, and attachments.
      3. Attempt the quiz as a student, and make sure you can embed an image in the editor, and attach a file, and then Submit all and finish without errors.
      4. Make sure that when you review the quiz as teacher, you can see both the image and the attached file.
      Show
      Run all the unit tests. Create a quiz containing an essay questions that allows files in the HTML editor, and attachments. Attempt the quiz as a student, and make sure you can embed an image in the editor, and attach a file, and then Submit all and finish without errors. Make sure that when you review the quiz as teacher, you can see both the image and the attached file.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      43084

      Description

      There is now way to get the files in the grade_responses method, either initially when the student submits their response, or later when the question is re-graded.

      See http://moodle.org/mod/forum/discuss.php?d=208009#p906877.

      Two things need to be done:

      1. add a get_files method to question_file_saver
      2. in question_attempt_step::load_from_records, we need to create an instance of a new class question_file_loader, which also implements the get_files and __toString methods, for those data values that have files.

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          There is a partial solution here: https://github.com/timhunt/moodle/compare/master...MDL-34640. It is incomplete because I could not be bothered to solve the problem of how to get qtype and contextid into question_attempt_step::load_from_records. For now, if you comment out the changes in that file, everything should work except re-grading.

          Show
          Tim Hunt added a comment - There is a partial solution here: https://github.com/timhunt/moodle/compare/master...MDL-34640 . It is incomplete because I could not be bothered to solve the problem of how to get qtype and contextid into question_attempt_step::load_from_records. For now, if you comment out the changes in that file, everything should work except re-grading.
          Hide
          Mathieu Petit-Clair added a comment -

          Thanks, I've applied this to my 2.2.3 checkout and improved it as much as I could today. You can find this here : https://github.com/scyrma/moodle/compare/4efe6df0745ffa006430ef8a466357a5749c3be6...3ffb785698844e76120a65d16225cfd306f3e508

          I partially solved the qtype and contextid issue, but there are at least two cases left:

          • load_question_attempt_step($stepid) in datalib.php (should we change the sql query to get the information?)
          • the tests files

          Question: I wonder if (as I've done in the patch) it's an acceptable idea to only return the response files when $qtype and $contextid are provided.

          Show
          Mathieu Petit-Clair added a comment - Thanks, I've applied this to my 2.2.3 checkout and improved it as much as I could today. You can find this here : https://github.com/scyrma/moodle/compare/4efe6df0745ffa006430ef8a466357a5749c3be6...3ffb785698844e76120a65d16225cfd306f3e508 I partially solved the qtype and contextid issue, but there are at least two cases left: load_question_attempt_step($stepid) in datalib.php (should we change the sql query to get the information?) the tests files Question: I wonder if (as I've done in the patch) it's an acceptable idea to only return the response files when $qtype and $contextid are provided.
          Hide
          Mathieu Petit-Clair added a comment -

          It seems like the question_engine_data_mapper::load_question_attempt_step() method is not called anywhere in Moodle.

          Show
          Mathieu Petit-Clair added a comment - It seems like the question_engine_data_mapper::load_question_attempt_step() method is not called anywhere in Moodle.
          Hide
          Mathieu Petit-Clair added a comment - - edited

          Here's some more work done on the initial patch, based on Tim's initial patch.
          https://github.com/scyrma/moodle/tree/MDL-34640

          My first attempt was on 2.2.4 (which we are using for the current project over here...), but this is based on moodle/master.

          Hopefully I will have time to add some tests to this later on.

          Show
          Mathieu Petit-Clair added a comment - - edited Here's some more work done on the initial patch, based on Tim's initial patch. https://github.com/scyrma/moodle/tree/MDL-34640 My first attempt was on 2.2.4 (which we are using for the current project over here...), but this is based on moodle/master. Hopefully I will have time to add some tests to this later on.
          Hide
          Tim Hunt added a comment -

          I will try to look at this soon.

          Show
          Tim Hunt added a comment - I will try to look at this soon.
          Hide
          Tim Hunt added a comment -

          Sorry, I still have not looked at this, and I am on holiday next week. I have not forgotten about this however.

          Show
          Tim Hunt added a comment - Sorry, I still have not looked at this, and I am on holiday next week. I have not forgotten about this however.
          Hide
          Mathieu Petit-Clair added a comment -

          No worries - There's been delays here too (server issues last week and public holidays, provincial elections next week)!

          Show
          Mathieu Petit-Clair added a comment - No worries - There's been delays here too (server issues last week and public holidays, provincial elections next week)!
          Hide
          Dan Poltawski added a comment -

          Sending all 'waiting for peer review' issues to integration before freeze, as agreed in Integrators Meeting 19/10/12. We are doing this to ensure any 'integratable issues' will not got missed before freeze..

          Show
          Dan Poltawski added a comment - Sending all 'waiting for peer review' issues to integration before freeze, as agreed in Integrators Meeting 19/10/12. We are doing this to ensure any 'integratable issues' will not got missed before freeze..
          Hide
          Tim Hunt added a comment -

          Note, this genuinely requires peer review, and perhaps some more work, so don't integrate this until you see another comment from me saying it is OK.

          Show
          Tim Hunt added a comment - Note, this genuinely requires peer review, and perhaps some more work, so don't integrate this until you see another comment from me saying it is OK.
          Hide
          Dan Poltawski added a comment -

          Sending this back to waiting for peer review state, as requested.

          Show
          Dan Poltawski added a comment - Sending this back to waiting for peer review state, as requested.
          Hide
          Tim Hunt added a comment -

          I'm finally getting around to looking at this again. This being https://github.com/scyrma/moodle/compare/470d47f512...MDL-34640.

          Show
          Tim Hunt added a comment - I'm finally getting around to looking at this again. This being https://github.com/scyrma/moodle/compare/470d47f512...MDL-34640 .
          Hide
          Tim Hunt added a comment -

          1. We have some whitespace errors: https://github.com/scyrma/moodle/compare/470d47f512...MDL-34640#L0R1335

          2. Can we not pass the question_type object to load_from_records, rather than the string qtype name? I think yes.

          3. Now that this core code exists, I think I should improve the essay qtype to use the new API, since it is better, and core code should be a good example to plugin developers.

          I think I should be able to do all that. (Looking at this code again, I am surprised at how simple the changes are really. Therefore, I think this should get integrated early in the 2.5 cycle. Is that soon enough for you Mathieu?

          Show
          Tim Hunt added a comment - 1. We have some whitespace errors: https://github.com/scyrma/moodle/compare/470d47f512...MDL-34640#L0R1335 2. Can we not pass the question_type object to load_from_records, rather than the string qtype name? I think yes. 3. Now that this core code exists, I think I should improve the essay qtype to use the new API, since it is better, and core code should be a good example to plugin developers. I think I should be able to do all that. (Looking at this code again, I am surprised at how simple the changes are really. Therefore, I think this should get integrated early in the 2.5 cycle. Is that soon enough for you Mathieu?
          Hide
          Mathieu Petit-Clair added a comment -

          Hi Tim,

          Sorry for the long delay on my part.

          I updated my MDL-34640 branch to address #2. I am not sure what is the whitespace error you were referring to in #1 and (since I rebased, I guess) the link is not working anymore.

          The current diff can be seen on https://github.com/scyrma/moodle/compare/master...MDL-34640

          Hopefully this will help move this forward...

          Show
          Mathieu Petit-Clair added a comment - Hi Tim, Sorry for the long delay on my part. I updated my MDL-34640 branch to address #2. I am not sure what is the whitespace error you were referring to in #1 and (since I rebased, I guess) the link is not working anymore. The current diff can be seen on https://github.com/scyrma/moodle/compare/master...MDL-34640 Hopefully this will help move this forward...
          Hide
          Tim Hunt added a comment -

          I keep nearly getting around to looking at this, and then something else comes up. It would be nice to get this into 2.5.

          Show
          Tim Hunt added a comment - I keep nearly getting around to looking at this, and then something else comes up. It would be nice to get this into 2.5.
          Hide
          Tim Hunt added a comment -

          OK, I think these final changes deal with the remaining issues. Submitting for integration.

          I created MDL-38810 to remind myself to update qtype_essay to take advantage of the new API, and also to write some unit tests.

          To INTEGRATORS: because of one minor merge conflict, I had to rebase this on top of MDL-38538, and so that issue needs to be integrated first.

          Show
          Tim Hunt added a comment - OK, I think these final changes deal with the remaining issues. Submitting for integration. I created MDL-38810 to remind myself to update qtype_essay to take advantage of the new API, and also to write some unit tests. To INTEGRATORS: because of one minor merge conflict, I had to rebase this on top of MDL-38538 , and so that issue needs to be integrated first.
          Hide
          Dan Poltawski added a comment -

          Integrated, thanks Tim.

          Show
          Dan Poltawski added a comment - Integrated, thanks Tim.
          Hide
          Mark Nelson added a comment -

          Thanks Tim. Works as expected. Passing.

          Show
          Mark Nelson added a comment - Thanks Tim. Works as expected. Passing.
          Hide
          Dan Poltawski added a comment -

          Did you remember to call thankDevelopers() for 'this_weeks_work'? Defaulting to PARAM_SHODDY thanking.

          line 1289 of \lib\changes.php: call to debugging()
          line 281 of \lib\are.php: call to moodleform->detectMissingThanks()
          line 202 of \lib\now.php: call to moodleform->_is_poor_form()
          line 73 of \course\upstream.php: call to moodleform->forgetingToThank()

          Show
          Dan Poltawski added a comment - Did you remember to call thankDevelopers() for 'this_weeks_work'? Defaulting to PARAM_SHODDY thanking. line 1289 of \lib\changes.php: call to debugging() line 281 of \lib\are.php: call to moodleform->detectMissingThanks() line 202 of \lib\now.php: call to moodleform->_is_poor_form() line 73 of \course\upstream.php: call to moodleform->forgetingToThank()
          Hide
          Mathieu Petit-Clair added a comment -

          Thanks!

          Show
          Mathieu Petit-Clair added a comment - Thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: