Details

    • Rank:
      43019

      Description

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

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          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
          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
          David Mudrak added a comment -

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

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

          Sending for integration based on comments on the other issue.

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

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

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          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
          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
          Charles Fulton added a comment -

          Rebased all branches.

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

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

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

          Please integrate.

          Show
          Nadav Kavalerchik added a comment - Please integrate.
          Hide
          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
          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
          Charles Fulton added a comment -

          Branches refreshed.

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

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

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

          Ping, Jerome.

          Show
          Dan Poltawski added a comment - Ping, Jerome.
          Hide
          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
          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 Wiese added a comment -

          Added a new issue for the fail on master.

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

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

          Show
          Damyon Wiese added a comment - The 1.9 restore issue found here has been fixed (integrated). Back to testing.
          Hide
          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
          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
          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
          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 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 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
          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
          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
          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
          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
          Dan Poltawski added a comment -

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

          Show
          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: