Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-44018

variant field of question_attempts table is not getting backed up by Moodle backup

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 2.3.11
    • Fix Version/s: 2.4.9, 2.5.5, 2.6.2
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide
      1. Create a test course.
      2. Add a quiz.
      3. Create a calculated simple question with a few dataset items (e.g. 3).
      4. Attempt the quiz once as a student.
      5. As teacher/admin, look at Quiz admin -> Results -> Statistics. (Following MDL-41757 this should let you see which variant the student got of each question.)
      6. Backup the course with user data.
      7. Restore it as a new course.
      8. Go to the Quiz statistics in the new course, and verify that the variant of each question has been correctly preserved.

      (If you cannot easily see this in the quiz statistics, then you may have to look in the database, question_attempts.variant columns.)

      Show
      Create a test course. Add a quiz. Create a calculated simple question with a few dataset items (e.g. 3). Attempt the quiz once as a student. As teacher/admin, look at Quiz admin -> Results -> Statistics. (Following MDL-41757 this should let you see which variant the student got of each question.) Backup the course with user data. Restore it as a new course. Go to the Quiz statistics in the new course, and verify that the variant of each question has been correctly preserved. (If you cannot easily see this in the quiz statistics, then you may have to look in the database, question_attempts.variant columns.)
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      It seems to have been omitted from the list of fields to backup.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            timhunt Tim Hunt added a comment -

            Ouch! That is bad. I will look into this.

            Show
            timhunt Tim Hunt added a comment - Ouch! That is bad. I will look into this.
            Hide
            timhunt Tim Hunt added a comment -

            I think this is the right patch, but I have not had time to test it.

            Since this is potential dataloss, I think we should make an exception and backport to 2.4.

            Not that 2.4 and 2.5 patch is slightly different to 2.6/master due to the introduction of maxfraction in 2.6.

            Show
            timhunt Tim Hunt added a comment - I think this is the right patch, but I have not had time to test it. Since this is potential dataloss, I think we should make an exception and backport to 2.4. Not that 2.4 and 2.5 patch is slightly different to 2.6/master due to the introduction of maxfraction in 2.6.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-44018

            • Error: the repository field is empty. Nothing was checked.
            Show
            cibot CiBoT added a comment - Results for MDL-44018 Error: the repository field is empty. Nothing was checked.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-44018

            • Remote repository: git://github.com/timhunt/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-44018 Remote repository: git://github.com/timhunt/moodle.git Remote branch MDL-44018 _24 to be integrated into upstream MOODLE_24_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1121 Warning: The MDL-44018 _24 branch at git://github.com/timhunt/moodle.git has not been rebased recently (>20 days ago). Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1121/artifact/work/smurf.html Remote branch MDL-44018 _25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1122 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1122/artifact/work/smurf.html Remote branch MDL-44018 _26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1123 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1123/artifact/work/smurf.html Remote branch MDL-44018 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1124 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1124/artifact/work/smurf.html
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Tim,

            This looks good and makes sense to me. I'll leave it up to Integrators to decide upon 2.4 but I agree that it should be included on this occasion owing to the data loss concern.

            Submitting for integration.

            Cheers,

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Tim, This looks good and makes sense to me. I'll leave it up to Integrators to decide upon 2.4 but I agree that it should be included on this occasion owing to the data loss concern. Submitting for integration. Cheers, Andrew
            Hide
            stronk7 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
            stronk7 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
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            damyon Damyon Wiese added a comment -

            Who would have thought question would be so hard to spell (the typo in the commit message is not even the same as the typo in the description of the column in install.xml)?

            Show
            damyon Damyon Wiese added a comment - Who would have thought question would be so hard to spell (the typo in the commit message is not even the same as the typo in the description of the column in install.xml)?
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Tim,

            This has been integrated to 25, 26 and master. Note we discussed the backport to 24 and we decided to stick to the policy about backports to the security branch - noise on this branch is not helpful.

            Show
            damyon Damyon Wiese added a comment - Thanks Tim, This has been integrated to 25, 26 and master. Note we discussed the backport to 24 and we decided to stick to the policy about backports to the security branch - noise on this branch is not helpful.
            Hide
            timhunt Tim Hunt added a comment -

            Thanks for integrating this Damyon.

            However, please can I ask you to reconsider the 2.4 issue. Data-loss is a security issue. (http://docs.moodle.org/dev/Security#Common_types_of_security_vulnerability). Also, since the problem is in the backup code, not the restore code, it is no good telling people who what to get data out of a 2.4 site to upgrade. Well, it would be OK to tell them to upgrade to the latest 2.4.x release. That is relatively easy. Expecting them to do a major version upgrade when we have a fix that is already backported, but because it is against some pre-convieved policy we have is unacceptable in my view.

            So, anyway, please can you discuss it again.

            Show
            timhunt Tim Hunt added a comment - Thanks for integrating this Damyon. However, please can I ask you to reconsider the 2.4 issue. Data-loss is a security issue. ( http://docs.moodle.org/dev/Security#Common_types_of_security_vulnerability ). Also, since the problem is in the backup code, not the restore code, it is no good telling people who what to get data out of a 2.4 site to upgrade. Well, it would be OK to tell them to upgrade to the latest 2.4.x release. That is relatively easy. Expecting them to do a major version upgrade when we have a fix that is already backported, but because it is against some pre-convieved policy we have is unacceptable in my view. So, anyway, please can you discuss it again.
            Hide
            timhunt Tim Hunt added a comment -

            Failing testing until integrators have re-considered whether to integrate into 2.4 branch.

            Show
            timhunt Tim Hunt added a comment - Failing testing until integrators have re-considered whether to integrate into 2.4 branch.
            Hide
            damyon Damyon Wiese added a comment -

            Hi Tim,

            I'll raise this at the dev meeting today - I really do not like exceptions to the policy - so maybe we need to clarify the policy for the security branch.

            Show
            damyon Damyon Wiese added a comment - Hi Tim, I'll raise this at the dev meeting today - I really do not like exceptions to the policy - so maybe we need to clarify the policy for the security branch.
            Hide
            timhunt Tim Hunt added a comment -

            Damyon, policy is just a tool. Normally, it helps us make the right decision in routine situations without a lot of thought. This is good. However, the policy is not god, and we were not necessarily infallible when we wrote it. I am going to start a thread in the security forum to try to get my ideas in order.

            Show
            timhunt Tim Hunt added a comment - Damyon, policy is just a tool. Normally, it helps us make the right decision in routine situations without a lot of thought. This is good. However, the policy is not god, and we were not necessarily infallible when we wrote it. I am going to start a thread in the security forum to try to get my ideas in order.
            Hide
            timhunt Tim Hunt added a comment -
            Show
            timhunt Tim Hunt added a comment - Forum post: https://moodle.org/mod/forum/discuss.php?d=254514
            Hide
            damyon Damyon Wiese added a comment -

            As noted in dev meeting - I'll push this for 24 and make a comment in the docs somewhere about our new random backports policy. You never know what you are going to get!

            Show
            damyon Damyon Wiese added a comment - As noted in dev meeting - I'll push this for 24 and make a comment in the docs somewhere about our new random backports policy. You never know what you are going to get!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            I only can disagree with this decision.

            1) Tim's link to security article is completely misleading. One think is a security vulnerability leading to data loss and another thing is a bug leading to data loss. The later is a bug, blocker or critical, whatever you want, but totally unrelated with security.

            2) There is a solution for everybody, deeply embed in the "release often schedule" decission. It's called upgrade. There are TWO good candidates to upgrade (25 and 26) already including the fix.

            3) We are breaking a clear, simple and understandable rule. Replacing it by the unknown. Which one will be the next one, lowering the limit?

            4) I'm not going to ask the Community here, nor participate in any forum discussion about this. Don't infer any negativity of my feelings about the Community with this. A plan for releases and support already was decided and agreed. With its consequences. Let's be 100% sticky to it.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - I only can disagree with this decision. 1) Tim's link to security article is completely misleading. One think is a security vulnerability leading to data loss and another thing is a bug leading to data loss. The later is a bug, blocker or critical, whatever you want, but totally unrelated with security. 2) There is a solution for everybody, deeply embed in the "release often schedule" decission. It's called upgrade. There are TWO good candidates to upgrade (25 and 26) already including the fix. 3) We are breaking a clear, simple and understandable rule. Replacing it by the unknown. Which one will be the next one, lowering the limit? 4) I'm not going to ask the Community here, nor participate in any forum discussion about this. Don't infer any negativity of my feelings about the Community with this. A plan for releases and support already was decided and agreed. With its consequences. Let's be 100% sticky to it. Ciao
            Hide
            timhunt Tim Hunt added a comment -

            Thanks Damyon.

            Eloy and I just had a discussion in developer chat, and agreed that we both had a logical point of view that was different, and we weren't going to resolve it.

            Show
            timhunt Tim Hunt added a comment - Thanks Damyon. Eloy and I just had a discussion in developer chat, and agreed that we both had a logical point of view that was different, and we weren't going to resolve it.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Just noting we are not "peers" in this discussion (not said in any "wrong" way at all, at all).

            It's under my umbrella (integration) to accept/reject this problematic issue (and ultimately decide), no matter you (developer) push for it everywhere.

            And for the safety and consistency of the process and the releases I continue thinking this has been a wrong decision and will continue opposing to it as loudly as possible. Of course we don't have to agree, nor convince the other. And, of course, this is not a win/lose competition.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Just noting we are not "peers" in this discussion (not said in any "wrong" way at all, at all). It's under my umbrella (integration) to accept/reject this problematic issue (and ultimately decide), no matter you (developer) push for it everywhere. And for the safety and consistency of the process and the releases I continue thinking this has been a wrong decision and will continue opposing to it as loudly as possible. Of course we don't have to agree, nor convince the other. And, of course, this is not a win/lose competition. Ciao
            Hide
            timhunt Tim Hunt added a comment -

            We have different roles, and different responsibilities, but we are peers in that we are both Moodlers that want what is best for Moodle.

            And having the discussion can help us both clarify our understanding of what is best for Moodle.

            Thank you for taking the time to discuss this with me.

            Of course, taking too much time to discuss every little things means that nothing ever gets done, which is not what is best for Moodle, but I think this was a helpful discussion.

            Show
            timhunt Tim Hunt added a comment - We have different roles, and different responsibilities, but we are peers in that we are both Moodlers that want what is best for Moodle. And having the discussion can help us both clarify our understanding of what is best for Moodle. Thank you for taking the time to discuss this with me. Of course, taking too much time to discuss every little things means that nothing ever gets done, which is not what is best for Moodle, but I think this was a helpful discussion.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            (agree, thanks Tim)

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - (agree, thanks Tim)
            Hide
            damyon Damyon Wiese added a comment -

            Tester - please test on 24 as well as this has been backported there.

            Show
            damyon Damyon Wiese added a comment - Tester - please test on 24 as well as this has been backported there.
            Hide
            marina Marina Glancy added a comment -

            The testing instructions are extremely confusing. There is no such thing as "dataset" on the question edit form. I would guess that you have meant the sets of wildcards but there is no possibility to create 3 of them, the dropdown has options 1,10,...

            Also there is no explanation of what is a "variant" and what should I be looking for in the backup/restore process. Reference to another issue sounds like "I am too lazy to type, google it" - testing instructions in that issue explain nothing. Searching by the word "variant" on the page does not help either. Screenshots are too big and it's impossible to see anything on them when they are scaled.

            I can see the variants in "question structure report" in master but can not see them in 2.6
            I fail the test because the testing instructions are not clear and also this error that always appears when creating a question. And I do not care that it is unrelated. You have no respect to the time of other people.

            Notice: Undefined index: number in /home/marina/repositories/int_master/moodle/question/type/calculatedsimple/edit_calculatedsimple_form.php on line 588
             
            Warning: Invalid argument supplied for foreach() in /home/marina/repositories/int_master/moodle/question/type/calculatedsimple/edit_calculatedsimple_form.php on line 589
            

            Show
            marina Marina Glancy added a comment - The testing instructions are extremely confusing. There is no such thing as "dataset" on the question edit form. I would guess that you have meant the sets of wildcards but there is no possibility to create 3 of them, the dropdown has options 1,10,... Also there is no explanation of what is a "variant" and what should I be looking for in the backup/restore process. Reference to another issue sounds like "I am too lazy to type, google it" - testing instructions in that issue explain nothing. Searching by the word "variant" on the page does not help either. Screenshots are too big and it's impossible to see anything on them when they are scaled. I can see the variants in "question structure report" in master but can not see them in 2.6 I fail the test because the testing instructions are not clear and also this error that always appears when creating a question. And I do not care that it is unrelated. You have no respect to the time of other people. Notice: Undefined index: number in /home/marina/repositories/int_master/moodle/question/type/calculatedsimple/edit_calculatedsimple_form.php on line 588   Warning: Invalid argument supplied for foreach() in /home/marina/repositories/int_master/moodle/question/type/calculatedsimple/edit_calculatedsimple_form.php on line 589
            Hide
            timhunt Tim Hunt added a comment -

            Marina, the teasing instructions were written on the assumption that you would know how to create a calculated question. If not, see http://docs.moodle.org/26/en/Calculated_question_type. Would you expect me to write detailed step-by-steps instructions every time I say 'create a quiz'? I don't, and no-one ever complains about that.

            The link to the other issue was nothing to do with laziness. That is something that was only integrated last week, after I wrote the testing instructions. The points was that if that had failed integration, these testing instructions would not work. It also makes it clear that this way of verifying that the fix worked is only possible in master.

            Did you look at the diff? It is a really small change, but this is the shortest way of testing it. I was trying to be as respectful as possible of the time of the tester.

            I have already done this testing. If you are too busy, you could bend the rules and pass this with me as tester.

            Show
            timhunt Tim Hunt added a comment - Marina, the teasing instructions were written on the assumption that you would know how to create a calculated question. If not, see http://docs.moodle.org/26/en/Calculated_question_type . Would you expect me to write detailed step-by-steps instructions every time I say 'create a quiz'? I don't, and no-one ever complains about that. The link to the other issue was nothing to do with laziness. That is something that was only integrated last week, after I wrote the testing instructions. The points was that if that had failed integration, these testing instructions would not work. It also makes it clear that this way of verifying that the fix worked is only possible in master. Did you look at the diff? It is a really small change, but this is the shortest way of testing it. I was trying to be as respectful as possible of the time of the tester. I have already done this testing. If you are too busy, you could bend the rules and pass this with me as tester.
            Hide
            marina Marina Glancy added a comment -

            Tim, everybody complains that your testing instructions are very difficult to follow.
            Again, you were using the words such as "dataset" that do not appear in this doc or on edit page. You did not provide instructions on how to check that variant has been restored. On 2.6 and below the word "variant" never appears on the Statistics page.

            The issue still remains failed because of the warning above. If you believe that it was introduced by another issue - please redirect to it.

            Show
            marina Marina Glancy added a comment - Tim, everybody complains that your testing instructions are very difficult to follow. Again, you were using the words such as "dataset" that do not appear in this doc or on edit page. You did not provide instructions on how to check that variant has been restored. On 2.6 and below the word "variant" never appears on the Statistics page. The issue still remains failed because of the warning above. If you believe that it was introduced by another issue - please redirect to it.
            Hide
            timhunt Tim Hunt added a comment -

            I have not seen the notice before. Reported as MDL-44288.

            Show
            timhunt Tim Hunt added a comment - I have not seen the notice before. Reported as MDL-44288 .
            Hide
            poltawski Dan Poltawski added a comment -

            Looks like thiss can be passed now

            Show
            poltawski Dan Poltawski added a comment - Looks like thiss can be passed now
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            I claim to be a simple individual
            liable to err like any other fellow mortal.
            I own, however, that I have humility enough
            to confess my errors and to retrace my steps.

            Mahatma Gandhi

            Your awesome code has met upstream, closing, thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - I claim to be a simple individual liable to err like any other fellow mortal. I own, however, that I have humility enough to confess my errors and to retrace my steps. Mahatma Gandhi Your awesome code has met upstream, closing, thanks!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  10/Mar/14