Moodle
  1. Moodle
  2. MDL-29563

Duplicate runs on modules that do not support backup/restore

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.1.1, 2.2, 2.3
    • Fix Version/s: 2.2.5, 2.3.2
    • Component/s: Backup
    • Labels:
    • Testing Instructions:
      Hide
      1. Download and install a contrib module without backup2 support, for example: http://moodle.org/plugins/view.php?plugin=mod_videochat (throws a few notices but it's ok for this testing)
      2. Create / goto a course and add an assignment and a instance of the "module without backup2 support"
      3. Only the assignment instance should have the duplicate icon
      4. Copy the URL of the duplicate action of the assignment instance and change the instance id for the "module without backup2 support" instance id
      5. At the next screen click continue
      6. There should appear an error message explaining the situation and a continue button to return to the course view page
      Show
      Download and install a contrib module without backup2 support, for example: http://moodle.org/plugins/view.php?plugin=mod_videochat (throws a few notices but it's ok for this testing) Create / goto a course and add an assignment and a instance of the "module without backup2 support" Only the assignment instance should have the duplicate icon Copy the URL of the duplicate action of the assignment instance and change the instance id for the "module without backup2 support" instance id At the next screen click continue There should appear an error message explaining the situation and a continue button to return to the course view page
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-29563_master

      Description

      Some modules may not support backup/restore. If they do not, then you get a fatal error about it's backup class does not exist at backup/util/factories/backup_factory.class.php on line 107

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Mark Nielsen added a comment -

            The attached patch suppresses the copy icon in the course icons and also displays a nice error on modduplicate.php. You may want to move the error to mod.php (display it before the confirmation of duplication).

            Show
            Mark Nielsen added a comment - The attached patch suppresses the copy icon in the course icons and also displays a nice error on modduplicate.php. You may want to move the error to mod.php (display it before the confirmation of duplication).
            Hide
            Michael de Raadt added a comment -

            Thanks for spotting that and providing a patch.

            We'll need to find a contributed module that does not have backup support to test this. Do you have any suggestions, Mark?

            Show
            Michael de Raadt added a comment - Thanks for spotting that and providing a patch. We'll need to find a contributed module that does not have backup support to test this. Do you have any suggestions, Mark?
            Hide
            Mark Nielsen added a comment -

            Sorry I don't

            Show
            Mark Nielsen added a comment - Sorry I don't
            Hide
            David Monllaó added a comment -

            Thanks for the patch Mark. Adapting the patch to 2.2, 2.3 and master and adding pull branches

            Show
            David Monllaó added a comment - Thanks for the patch Mark. Adapting the patch to 2.2, 2.3 and master and adding pull branches
            Hide
            Rajesh Taneja added a comment -

            Thanks for the patch Mark and David,

            Patch looks nice, although you might want to consider using print_error at https://github.com/dmonllao/moodle/compare/master...MDL-29563_master#L1R64

            As we don't expect this to execute in normal case (Duplicate icon will not rendered after this patch), IMO this should be handled by print_error.

            Rest all look good.

            Show
            Rajesh Taneja added a comment - Thanks for the patch Mark and David, Patch looks nice, although you might want to consider using print_error at https://github.com/dmonllao/moodle/compare/master...MDL-29563_master#L1R64 As we don't expect this to execute in normal case (Duplicate icon will not rendered after this patch), IMO this should be handled by print_error. Rest all look good.
            Hide
            David Monllaó added a comment -

            Thanks for the comments Raj, I've updated the pull branches with the proposed change. Requesting peer review again

            Show
            David Monllaó added a comment - Thanks for the comments Raj, I've updated the pull branches with the proposed change. Requesting peer review again
            Hide
            Rajesh Taneja added a comment -

            Thanks David,
            Patch looks good now, pushing for integration review.

            Show
            Rajesh Taneja added a comment - Thanks David, Patch looks good now, pushing for integration review.
            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
            David Monllaó added a comment -

            Hi Dan,

            Branches rebased

            Show
            David Monllaó added a comment - Hi Dan, Branches rebased
            Hide
            Sam Hemelryk added a comment -

            Awesome thanks David, this has been integrated now.

            Show
            Sam Hemelryk added a comment - Awesome thanks David, this has been integrated now.
            Hide
            Adrian Greeve added a comment -

            Tested with 2.2, 2.3 and master. The patch works on all the listed versions.
            No problems encountered.
            Test Passed

            Show
            Adrian Greeve added a comment - Tested with 2.2, 2.3 and master. The patch works on all the listed versions. No problems encountered. Test Passed
            Hide
            Aparup Banerjee added a comment -

            yay, it works!

            This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week.

            Thank you all for taking the time to get us here.

            cheers!

            Show
            Aparup Banerjee added a comment - yay, it works! This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week. Thank you all for taking the time to get us here. cheers!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: