Details

    • Testing Instructions:
      Hide

      1. As admin/teacher, log into a Moodle 1.9 course
      2. Create a course with an assignment (eg, Online text)
      3. Save the assignment and edit the settings
      4. Note the kind of editor used
      5. Backup the course
      6. Restore the course to a 2.x site and open the course
      7. Edit the settings for the assignment
      8. Note the format of the Description editor

      Show
      1. As admin/teacher, log into a Moodle 1.9 course 2. Create a course with an assignment (eg, Online text) 3. Save the assignment and edit the settings 4. Note the kind of editor used 5. Backup the course 6. Restore the course to a 2.x site and open the course 7. Edit the settings for the assignment 8. Note the format of the Description editor
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-34586-master

      Description

      The assignment format field isn't conveyed properly during a 1.9->2+ backup/restore scenario.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            mudrd8mz David Mudrak added a comment -

            Thanks for the patch Charles. However I'm not sure this is correct. We can't simply set the format without making sure that the text in in real HTML format. Why not to follow the other modules that do something like:

            if ($CFG->texteditors !== 'textarea') {
                $data['intro'] = text_to_html($data['intro'], false, false, true);
                $data['introformat'] = FORMAT_HTML;
            }

            Charles, was there any special reason for your approach?

            Show
            mudrd8mz David Mudrak added a comment - Thanks for the patch Charles. However I'm not sure this is correct. We can't simply set the format without making sure that the text in in real HTML format. Why not to follow the other modules that do something like: if ($CFG->texteditors !== 'textarea') { $data['intro'] = text_to_html($data['intro'], false, false, true); $data['introformat'] = FORMAT_HTML; } Charles, was there any special reason for your approach?
            Hide
            mudrd8mz David Mudrak added a comment -

            Hmm, by the way. Note that generally, the moodle1 conversion logic should follow/copy the upgrade path (so that both ways lead to same results). While in other modules, the upgrade 1.9->2.0 code deals with intro and introformat, I did not find anything in mod/assignment. I guess that is why we did not implement the introformat conversion into moodle1.

            Show
            mudrd8mz David Mudrak added a comment - Hmm, by the way. Note that generally, the moodle1 conversion logic should follow/copy the upgrade path (so that both ways lead to same results). While in other modules, the upgrade 1.9->2.0 code deals with intro and introformat, I did not find anything in mod/assignment. I guess that is why we did not implement the introformat conversion into moodle1.
            Hide
            cfulton Charles Fulton added a comment -

            @David, no particular reason and your approach is much better. I don't think I even spot-checked another module when going forward. Rewritten.

            Show
            cfulton Charles Fulton added a comment - @David, no particular reason and your approach is much better. I don't think I even spot-checked another module when going forward. Rewritten.
            Hide
            mudrd8mz David Mudrak added a comment -

            Yup, looking good now. Thanks for the patch Charles!

            Show
            mudrd8mz David Mudrak added a comment - Yup, looking good now. Thanks for the patch Charles!
            Hide
            poltawski Dan Poltawski added a comment -

            Sending for integration based on comments on the other issue.

            Show
            poltawski Dan Poltawski added a comment - Sending for integration based on comments on the other issue.
            Hide
            poltawski Dan Poltawski 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
            poltawski Dan Poltawski 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
            poltawski Dan Poltawski added a comment -

            Hi Charles,

            This seems to have trailing whitespace in it, and I guess it also needs backporting.

            I'd fix/cherry-pick it myself, but with the combination of both problems its a bit tricky, so if I could reopen it so you can address it,

            cheers

            Show
            poltawski Dan Poltawski added a comment - Hi Charles, This seems to have trailing whitespace in it, and I guess it also needs backporting. I'd fix/cherry-pick it myself, but with the combination of both problems its a bit tricky, so if I could reopen it so you can address it, cheers
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            cfulton Charles Fulton added a comment -

            Hi Dan, thanks for noticing that (gah, how did I miss it?) Rebased and cherry-picked for 2.2 and 2.3.

            Show
            cfulton Charles Fulton added a comment - Hi Dan, thanks for noticing that (gah, how did I miss it?) Rebased and cherry-picked for 2.2 and 2.3.
            Hide
            cfulton Charles Fulton added a comment -

            Rebased all branches.

            Show
            cfulton Charles Fulton added a comment - Rebased all branches.
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            A very important fix (from Teacher's POV) please see if you can integrate it

            Show
            nadavkav Nadav Kavalerchik added a comment - A very important fix (from Teacher's POV) please see if you can integrate it
            Hide
            nadavkav Nadav Kavalerchik added a comment -

            Please integrate.

            Show
            nadavkav Nadav Kavalerchik added a comment - Please integrate.
            Hide
            mudrd8mz David Mudrak added a comment -

            The logic of the patch has had my +1 (see above). Is Charles Fulton willing to prepare a freshly rebased brances for 2.4, 2.5 and 2.6?

            Show
            mudrd8mz David Mudrak added a comment - The logic of the patch has had my +1 (see above). Is Charles Fulton willing to prepare a freshly rebased brances for 2.4, 2.5 and 2.6?
            Hide
            cfulton Charles Fulton added a comment -

            Branches refreshed.

            Show
            cfulton Charles Fulton added a comment - Branches refreshed.
            Hide
            mudrd8mz David Mudrak added a comment -

            Looks good to me, sending this for integration now. Thanks Charles.

            Show
            mudrd8mz David Mudrak added a comment - Looks good to me, sending this for integration now. Thanks Charles.
            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
            damyon Damyon Wiese added a comment -

            Thanks Charles,

            This patch looks good to me. Integrated to master, 25 and 24. We should finish the job by fixing the upgrade steps in Moodle 2.2 to match. I have created a bug for that: MDL-42322.

            Note: We have said we will still perform 2.2 updates only for 1.9 upgrade issues if required.

            Show
            damyon Damyon Wiese added a comment - Thanks Charles, This patch looks good to me. Integrated to master, 25 and 24. We should finish the job by fixing the upgrade steps in Moodle 2.2 to match. I have created a bug for that: MDL-42322 . Note: We have said we will still perform 2.2 updates only for 1.9 upgrade issues if required.
            Hide
            poltawski Dan Poltawski added a comment -

            Ping, Jerome.

            Show
            poltawski Dan Poltawski added a comment - Ping, Jerome.
            Hide
            jerome Jérôme Mouneyrac added a comment - - edited

            Error 1:
            When trying to restore the 1.9 backup on master (just after clicking on the button Continue on the page where we indicate we want to retore it in a new course), I have this error:

            Coding error detected, it must be fixed by a programmer: progress() without start_progress

            More information about this error

            Debug info:
            Error code: codingerror
            Stack trace:
            line 175 of /backup/util/progress/core_backup_progress.class.php: coding_exception thrown
            line 100 of /backup/moodle2/restore_plan_builder.class.php: call to core_backup_progress->progress()
            line 73 of /backup/util/plan/restore_plan.class.php: call to restore_plan_builder::build_plan()
            line 483 of /backup/controller/restore_controller.class.php: call to restore_plan->build()
            line 445 of /backup/controller/restore_controller.class.php: call to restore_controller->load_plan()
            line 56 of /backup/restore.php: call to restore_controller->convert()

            Error 2:
            I tested on 2.4, I don't have the error but the assignment wasn't restore at all.

            In both case the file generated by Moodle 1.9, when trying to restor on master/2.4, the restore process displays:

            The selected file is not a standard Moodle backup file. The restore process will try to convert the backup file into the standard format and then restore it.

            Summary:
            I understand these blocker issues are not related to the fix but I can't process the step instructions.

            Show
            jerome Jérôme Mouneyrac added a comment - - edited Error 1: When trying to restore the 1.9 backup on master (just after clicking on the button Continue on the page where we indicate we want to retore it in a new course), I have this error: Coding error detected, it must be fixed by a programmer: progress() without start_progress More information about this error Debug info: Error code: codingerror Stack trace: line 175 of /backup/util/progress/core_backup_progress.class.php: coding_exception thrown line 100 of /backup/moodle2/restore_plan_builder.class.php: call to core_backup_progress->progress() line 73 of /backup/util/plan/restore_plan.class.php: call to restore_plan_builder::build_plan() line 483 of /backup/controller/restore_controller.class.php: call to restore_plan->build() line 445 of /backup/controller/restore_controller.class.php: call to restore_controller->load_plan() line 56 of /backup/restore.php: call to restore_controller->convert() Error 2: I tested on 2.4, I don't have the error but the assignment wasn't restore at all. In both case the file generated by Moodle 1.9, when trying to restor on master/2.4, the restore process displays: The selected file is not a standard Moodle backup file. The restore process will try to convert the backup file into the standard format and then restore it. Summary: I understand these blocker issues are not related to the fix but I can't process the step instructions.
            Hide
            damyon Damyon Wiese added a comment -

            Added a new issue for the fail on master.

            Show
            damyon Damyon Wiese added a comment - Added a new issue for the fail on master.
            Hide
            damyon Damyon Wiese added a comment -

            The 1.9 restore issue found here has been fixed (integrated). Back to testing.

            Show
            damyon Damyon Wiese added a comment - The 1.9 restore issue found here has been fixed (integrated). Back to testing.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I follow the exact test instructions and when I restore the asignment is not restored in 2.4 or master (didn't try 2.5 but I guess it's the same). However I can restore this backup with the assignment in 1.9. I'm attaching the culprit file.

            Show
            jerome Jérôme Mouneyrac added a comment - I follow the exact test instructions and when I restore the asignment is not restored in 2.4 or master (didn't try 2.5 but I guess it's the same). However I can restore this backup with the assignment in 1.9. I'm attaching the culprit file.
            Hide
            cfulton Charles Fulton added a comment -

            I was able to restore the provided backup in my test 2.6 instance (which is vanilla except for MDL-42411). The backup restored with the assignment and the assignment has the rich-text editor. I'm puzzled that the assignment wouldn't restore at all for you.

            Show
            cfulton Charles Fulton added a comment - I was able to restore the provided backup in my test 2.6 instance (which is vanilla except for MDL-42411 ). The backup restored with the assignment and the assignment has the rich-text editor. I'm puzzled that the assignment wouldn't restore at all for you.
            Hide
            damyon Damyon Wiese added a comment -

            Jerome - I think I worked out what is going on. Can you enable the Assignment 2.2 and rerun the tests?

            Show
            damyon Damyon Wiese added a comment - Jerome - I think I worked out what is going on. Can you enable the Assignment 2.2 and rerun the tests?
            Hide
            jerome Jérôme Mouneyrac added a comment - - edited

            Yes that's it, I see the assignments on 2.4 and master The last test instruction say: " Note the format of the Description editor" - I haven't see this format field for long time.

            Show
            jerome Jérôme Mouneyrac added a comment - - edited Yes that's it, I see the assignments on 2.4 and master The last test instruction say: " Note the format of the Description editor" - I haven't see this format field for long time.
            Hide
            jerome Jérôme Mouneyrac added a comment - - edited

            I'm going to pass the test looking at the code. However the test instructions lack some... "testing"

            Show
            jerome Jérôme Mouneyrac added a comment - - edited I'm going to pass the test looking at the code. However the test instructions lack some... "testing"
            Hide
            poltawski Dan Poltawski added a comment -

            Hurrah! Thanks for your contribution - this fix is part of Moodle.

            Show
            poltawski Dan Poltawski added a comment - Hurrah! Thanks for your contribution - this fix is part of Moodle.

              People

              • Votes:
                2 Vote for this issue
                Watchers:
                8 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  11/Nov/13