Moodle
  1. Moodle
  2. MDL-35207

Workshop submission attachment not being upgraded from 1.9 to 2.0, 2.1 and 2.2

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.1.7, 2.2.4
    • Fix Version/s: 2.1.9, 2.2.6
    • Component/s: Workshop
    • Labels:
      None
    • Testing Instructions:
      Hide

      Testing difficulty: Medium/Hard (requires 1.9 installation and checking values in the DB)

      • Prepare a Moodle 1.9 site with the Workshop module enabled
      • Create a workshop instance
      • Submit some submissions with attachments into the workshop. (Hint: choose names of the attachments in a way that will allow you to match it with the submission later).
      • Delete some submissions and submit some more again - we need non-linear ids in the workshop submissions table. (If unable to delete submissions, use multiple instances of workshops and then delete some instance. Make sure there are "holes" in the sequence of the ids in the workshop submissions table).
      • Upgrade the site to 2.2 or 2.1 (or eventually 2.0 if integrators decide to backport this there, too).
      • TEST: Make sure that after the upgrade, the attachments submissions are all present and they match the submissions.
      Show
      Testing difficulty: Medium/Hard (requires 1.9 installation and checking values in the DB) Prepare a Moodle 1.9 site with the Workshop module enabled Create a workshop instance Submit some submissions with attachments into the workshop. (Hint: choose names of the attachments in a way that will allow you to match it with the submission later). Delete some submissions and submit some more again - we need non-linear ids in the workshop submissions table. (If unable to delete submissions, use multiple instances of workshops and then delete some instance. Make sure there are "holes" in the sequence of the ids in the workshop submissions table). Upgrade the site to 2.2 or 2.1 (or eventually 2.0 if integrators decide to backport this there, too). TEST: Make sure that after the upgrade, the attachments submissions are all present and they match the submissions.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Rank:
      43846

      Description

      It seems that submission attachments of workshop module are not being migrated from 1.9 to 2.0, 2.1 and 2.2 version of moodle.

      The patch attached seems to solve the problem. Please review it.

      The directory that the code uses to find submissions uses the new submission id not the old one, so no submission attachment is ever found.

      Regards.

      1. fix_workshop.php
        3 kB
        Juan Segarra Montesinos
      2. fix_workshop.php
        3 kB
        Juan Segarra Montesinos
      3. mdl_35207.patch
        1 kB
        Juan Segarra Montesinos
      4. MDL-35207-patch2.diff
        2 kB
        David Mudrak

        Activity

        Hide
        David Mudrak added a comment -

        Very well spotted Juan! This is pretty nasty data-loss risk bug. I believe it survived testing because all testers had linear sequence of old submission ids, thence the new id matched the old id.

        Thanks a lot for tracking down the problematic code. I just modified it a bit so that it does not execute another SQL within the foreach loop. Can you please review and eventually test my solution attached as MDL-35207-patch2.diff? Thanks a lot in advance.

        Show
        David Mudrak added a comment - Very well spotted Juan! This is pretty nasty data-loss risk bug. I believe it survived testing because all testers had linear sequence of old submission ids, thence the new id matched the old id. Thanks a lot for tracking down the problematic code. I just modified it a bit so that it does not execute another SQL within the foreach loop. Can you please review and eventually test my solution attached as MDL-35207 -patch2.diff? Thanks a lot in advance.
        Hide
        David Mudrak added a comment -

        Increasing severity due to data-loss risk during the upgrade.

        Show
        David Mudrak added a comment - Increasing severity due to data-loss risk during the upgrade.
        Hide
        David Mudrak added a comment -

        I believe the patch is correct even without confirmation. Submitting for integration.

        DEAR INTEGRATORS: Please consider breaking the rulez (yay!) and eventually cherry-pick this to 2.0 too. Note there is no change for 2.3 and master as this is an 1.9 -> 2.x upgrade code.

        Show
        David Mudrak added a comment - I believe the patch is correct even without confirmation. Submitting for integration. DEAR INTEGRATORS: Please consider breaking the rulez (yay!) and eventually cherry-pick this to 2.0 too. Note there is no change for 2.3 and master as this is an 1.9 -> 2.x upgrade code.
        Hide
        Juan Segarra Montesinos added a comment -

        Sorry David, we've just migrated to 2.3... I'll try the path once I've time.

        Show
        Juan Segarra Montesinos added a comment - Sorry David, we've just migrated to 2.3... I'll try the path once I've time.
        Hide
        Dan Poltawski added a comment -

        Hi David,

        Thanks, I've integrated this to 21 and 22.

        I'm afraid I didn't backport to 20 as this branch is completely 'dead' from an integration perspective. It doesn't get security fixes and it isn't part of the release process so it wouldn't get sent to production git without special measures. I think that updating the 20 branch now might give a false sense of security with updated timestamp implying that fixes are being made to it, so from a security perspective its best not to do it either.

        Show
        Dan Poltawski added a comment - Hi David, Thanks, I've integrated this to 21 and 22. I'm afraid I didn't backport to 20 as this branch is completely 'dead' from an integration perspective. It doesn't get security fixes and it isn't part of the release process so it wouldn't get sent to production git without special measures. I think that updating the 20 branch now might give a false sense of security with updated timestamp implying that fixes are being made to it, so from a security perspective its best not to do it either.
        Hide
        Juan Segarra Montesinos added a comment -

        Hi guys, It's relatively easy to program a small command that fixes the issue. I've attached a fix_workshop.php CLI_SCRIPT that I used to fix the problem once updated.

        Show
        Juan Segarra Montesinos added a comment - Hi guys, It's relatively easy to program a small command that fixes the issue. I've attached a fix_workshop.php CLI_SCRIPT that I used to fix the problem once updated.
        Hide
        Frédéric Massart added a comment -

        Tested on 2.1 and 2.2 and everything looked fine. Thanks!

        Show
        Frédéric Massart added a comment - Tested on 2.1 and 2.2 and everything looked fine. Thanks!
        Hide
        Dan Poltawski added a comment -

        Congratulations, you've done it!

        Nf n erjneq sbe fhpprfshy vagrtengvba vagb guvf jrrxf eryrnfr, V pna abj qvfpybfr gb lbh gur rkvfgnapr bs shapgvba fge_ebg13(), gb tb va lbhe gbbyxvg nybat jvgu uggc://cuc.arg/znahny/ra/shapgvba.tmtrgff.cuc

        Cyrnfr qb abg nyybj guvf vasbezngvba gb cnff shegure.

        Show
        Dan Poltawski added a comment - Congratulations, you've done it! Nf n erjneq sbe fhpprfshy vagrtengvba vagb guvf jrrxf eryrnfr, V pna abj qvfpybfr gb lbh gur rkvfgnapr bs shapgvba fge_ebg13(), gb tb va lbhe gbbyxvg nybat jvgu uggc://cuc.arg/znahny/ra/shapgvba.tmtrgff.cuc Cyrnfr qb abg nyybj guvf vasbezngvba gb cnff shegure.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: