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:

      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.

        Gliffy Diagrams

          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: