Moodle
  1. Moodle
  2. MDL-31393

The contents of the field feedback in essay questions disappear after an update

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.1.2, 2.2
    • Fix Version/s: 2.1.7, 2.2.4
    • Component/s: Questions
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      1. In a Moodle 2.0 (or 1.9) site, create some essay questions with various combinations of General feedback and Feedback.

      2. Upgrade the site to Moodle 2.1 or 2.2.

      3. Verify that the General feedback and Feedback have been concatenated into the General feedback field during the upgrade.

      For extra credit:

      A. Test this on all supported databases.

      B. Test with various combinations of text format for General feedback and Feedback. (e.g. FORMAT_MOODLE, FORMAT_HTML.)

      Show
      1. In a Moodle 2.0 (or 1.9) site, create some essay questions with various combinations of General feedback and Feedback. 2. Upgrade the site to Moodle 2.1 or 2.2. 3. Verify that the General feedback and Feedback have been concatenated into the General feedback field during the upgrade. For extra credit: A. Test this on all supported databases. B. Test with various combinations of text format for General feedback and Feedback. (e.g. FORMAT_MOODLE, FORMAT_HTML.)
    • Workaround:
      Hide

      A solution that we considered would be to copy the contents of the field feedback from the field to the table mdl_question_answer generalfeedback table mdl_question ex:

      MySQL Query
      UPDATE mdl_question_answers a JOIN mdl_question b ON b.id = a.question AND b.qtype = "essay" AND LENGTH(a.feedback) > 0
      JOIN mdl_quiz_question_instances c ON c.question = b.id
      JOIN mdl_quiz d ON d.id = c.quiz 
      JOIN mdl_question e ON e.id = a.question
      SET e.generalfeedback = CONCAT(e.generalfeedback, a.feedback)
      
      Show
      A solution that we considered would be to copy the contents of the field feedback from the field to the table mdl_question_answer generalfeedback table mdl_question ex: MySQL Query UPDATE mdl_question_answers a JOIN mdl_question b ON b.id = a.question AND b.qtype = "essay" AND LENGTH(a.feedback) > 0 JOIN mdl_quiz_question_instances c ON c.question = b.id JOIN mdl_quiz d ON d.id = c.quiz JOIN mdl_question e ON e.id = a.question SET e.generalfeedback = CONCAT(e.generalfeedback, a.feedback)
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      37910

      Description

      We recently updated our production environment to move from version 2.0.3 to version 2.1.2and we no longer see the feedback contents of essay questions.

      Digging a bit, we noticed that with the new version, there is simply no more entries in the table "mdl_question_answer" created for questions of this type.
      Its a normal behaviour but the bug I think is that the old fieldback field content is now unavailable.

        Issue Links

          Activity

          Hide
          Juan Leyva (CV&A) added a comment -

          Hi Gilles,

          the solution you considered works fine for you?

          We have the same problem

          Regards

          Show
          Juan Leyva (CV&A) added a comment - Hi Gilles, the solution you considered works fine for you? We have the same problem Regards
          Hide
          Gilles-Philippe Leblanc added a comment -

          Hi, Indeed the solutions worked for us in our production environment.
          You should try it in a test environment before but it seems OK to us.

          You could also question yourself if you want to concatenate the both feedback field or just replace it.

          In our case, the vast majority of our users using feedback chose the "feedback" and left the field "general feedback" empty. The few teachers who used the two fields were filled with the same information. In this context, a concatenation would create redundancy so we chose to replace the data I would say in general, it is safer to concatenate fields.

          It's up to you depending on the context.

          Show
          Gilles-Philippe Leblanc added a comment - Hi, Indeed the solutions worked for us in our production environment. You should try it in a test environment before but it seems OK to us. You could also question yourself if you want to concatenate the both feedback field or just replace it. In our case, the vast majority of our users using feedback chose the "feedback" and left the field "general feedback" empty. The few teachers who used the two fields were filled with the same information. In this context, a concatenation would create redundancy so we chose to replace the data I would say in general, it is safer to concatenate fields. It's up to you depending on the context.
          Hide
          Juan Leyva (CV&A) added a comment -

          Thanks gilles, it works fine

          I think this field have been removed because it doesn't make sense in essay questions

          General feedback makes sense, but feedback when the answer is wrong doesn't (essay question are not auto-grade questions)

          Show
          Juan Leyva (CV&A) added a comment - Thanks gilles, it works fine I think this field have been removed because it doesn't make sense in essay questions General feedback makes sense, but feedback when the answer is wrong doesn't (essay question are not auto-grade questions)
          Hide
          Brandon Horn added a comment -

          I don't care about not being about to use the field in the future, but I had a significant amount of data in the field that was lost during the upgrade. If this can't be implemented immediately, a warning needs to be displayed so that people don't lose data. What's worse is that the lost data may not be immediately evident.

          Show
          Brandon Horn added a comment - I don't care about not being about to use the field in the future, but I had a significant amount of data in the field that was lost during the upgrade. If this can't be implemented immediately, a warning needs to be displayed so that people don't lose data. What's worse is that the lost data may not be immediately evident.
          Hide
          Tim Hunt added a comment -

          Right, I think this fixes it, but I really need someone to test it for real.

          Note that, although the data is not used after the upgrade to 2.1+, it is still in the database, so if you run this upgrade now, it should combine question.generalfeedback and question_answers.feedback for all your essay questions.

          So far, I have only tested on MySQL and Posgres, but I think this code should work on Oracle and MSSQL server too.

          Show
          Tim Hunt added a comment - Right, I think this fixes it, but I really need someone to test it for real. Note that, although the data is not used after the upgrade to 2.1+, it is still in the database, so if you run this upgrade now, it should combine question.generalfeedback and question_answers.feedback for all your essay questions. So far, I have only tested on MySQL and Posgres, but I think this code should work on Oracle and MSSQL server too.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi it looks spotty, 3 minor thoughts:

          • Do we need any type of handling of files present in the qa.feedback? Surely no, just to be sure.
          • Can this be loooong? If so perhaps we would need to increase upgrade time.
          • What happens if somebody running 2.2.[0|1|2] upgrades to 2.3 ?

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi it looks spotty, 3 minor thoughts: Do we need any type of handling of files present in the qa.feedback? Surely no, just to be sure. Can this be loooong? If so perhaps we would need to increase upgrade time. What happens if somebody running 2.2. [0|1|2] upgrades to 2.3 ? Ciao
          Hide
          Tim Hunt added a comment -

          1. I suppose theoretically we should handle files, but if I have to take more time to do that, this bug will probably never get fixed. Better to commit this solution, which is 99% of what is needed, now. If anyone really needs file handling, then we can add that later.

          2. I don't think this will every be very long. Few Moodle sites have more than ~1000 essays, most of which probably don't have feedback. However, if you can tell me the right thing to use in the tool to prevent time-outs (a progress bar?) I will add it.

          3. Nothing happens - the get_recordset just returns 0 rows.

          Show
          Tim Hunt added a comment - 1. I suppose theoretically we should handle files, but if I have to take more time to do that, this bug will probably never get fixed. Better to commit this solution, which is 99% of what is needed, now. If anyone really needs file handling, then we can add that later. 2. I don't think this will every be very long. Few Moodle sites have more than ~1000 essays, most of which probably don't have feedback. However, if you can tell me the right thing to use in the tool to prevent time-outs (a progress bar?) I will add it. 3. Nothing happens - the get_recordset just returns 0 rows.
          Hide
          Tim Hunt added a comment -

          I can't do a progress bar, since a recordset cannot tell you how many records there are, so I will just add a call to upgrade_set_timeout in the loop.

          Show
          Tim Hunt added a comment - I can't do a progress bar, since a recordset cannot tell you how many records there are, so I will just add a call to upgrade_set_timeout in the loop.
          Hide
          Tim Hunt added a comment -

          OK, Commits amended. Any more comments before I submit this for integration? Thanks.

          Show
          Tim Hunt added a comment - OK, Commits amended. Any more comments before I submit this for integration? Thanks.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          1. fair enough

          2. yes the timeout was the thing, just to be in the safe side.

          3. what I mean is that if I've one site running current 2.2.2+ and I never update it to any future 2.2... but only straight to 2.3 next month... that site will continue having the feedbacks in the 2 places, isn't it? In other words... don't we need to add the same upgrade step to current master too?

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - 1. fair enough 2. yes the timeout was the thing, just to be in the safe side. 3. what I mean is that if I've one site running current 2.2.2+ and I never update it to any future 2.2... but only straight to 2.3 next month... that site will continue having the feedbacks in the 2 places, isn't it? In other words... don't we need to add the same upgrade step to current master too? Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          grrr, forgot to move this out from p2preview, sorry.

          Show
          Eloy Lafuente (stronk7) added a comment - grrr, forgot to move this out from p2preview, sorry.
          Hide
          Tim Hunt added a comment -

          The 22 commit exactly cherry-picked to Master, so I did that. Let me know if you want me to change anything (e.g. change version number).

          Submitting for integration now.

          Show
          Tim Hunt added a comment - The 22 commit exactly cherry-picked to Master, so I did that. Let me know if you want me to change anything (e.g. change version number). Submitting for integration now.
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Dan Poltawski added a comment -

          Thanks Tim.

          I've integrated this now.

          Show
          Dan Poltawski added a comment - Thanks Tim. I've integrated this now.
          Hide
          Adrian Greeve added a comment -

          I tested this in Mysql and this works as expected. General feedback and feedback are concatenated into the one field.

          Postgres on the other hand didn't concatenate both fields. It seems that the feedback field is not added to general feedback.

          Sorry. I haven't had the chance to test this with oracle or mssql (I don't have those installed on this machine)

          Show
          Adrian Greeve added a comment - I tested this in Mysql and this works as expected. General feedback and feedback are concatenated into the one field. Postgres on the other hand didn't concatenate both fields. It seems that the feedback field is not added to general feedback. Sorry. I haven't had the chance to test this with oracle or mssql (I don't have those installed on this machine)
          Hide
          Dan Poltawski added a comment -

          Hmm, looking at this (both Tim and Adrian are away now unfortunately!). Seems curious that this would fail on postgres as I believe this is what Tim general develops against.

          Show
          Dan Poltawski added a comment - Hmm, looking at this (both Tim and Adrian are away now unfortunately!). Seems curious that this would fail on postgres as I believe this is what Tim general develops against.
          Hide
          Dan Poltawski added a comment -

          Hmm, well i'm not getting the same result as adrian here on pg.

          Testing on oracle and mssql now.

          Show
          Dan Poltawski added a comment - Hmm, well i'm not getting the same result as adrian here on pg. Testing on oracle and mssql now.
          Hide
          Dan Poltawski added a comment -

          Tested on oracle with all different formats and worked fine.

          Show
          Dan Poltawski added a comment - Tested on oracle with all different formats and worked fine.
          Hide
          Dan Poltawski added a comment -

          And working here on mssql too.

          I suspect Adrian made a mistake during testing (hard to confirm now) Passing.

          Show
          Dan Poltawski added a comment - And working here on mssql too. I suspect Adrian made a mistake during testing (hard to confirm now) Passing.
          Hide
          Dan Poltawski added a comment -

          Congratulations!

          Your work has made into the latest Moodle release!

          You are only authorised to celebrate after testing 15 Moodle 2.3 QA tests, thanks!

          Show
          Dan Poltawski added a comment - Congratulations! Your work has made into the latest Moodle release! You are only authorised to celebrate after testing 15 Moodle 2.3 QA tests, thanks!

            People

            • Votes:
              3 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: