Moodle
  1. Moodle
  2. MDL-27595

Course import fails in some conditions

    Details

    • Testing Instructions:
      Hide

      Prerequisites:

      • Moodle in standard configuration.
      • Two courses, C1 and C2, where C1 contains at least one activity or resource.
      • C2 must have at least one role override for the "Manager" role. For example, in C2, click "Settings -> Users -> Permissions", select "Manager", and set "Configure cohort instances" to "Allow".
      • One user, U1, who is assigned the teacher role in both C1 and C2.

      Test case:

      • Log in as U1.
      • Go to course C1 and click "Settings -> Course administration -> Import".
      • Select C2 as the course to import from.
      • Click through the following screens without changing the default settings, until the import is being performed.
      • Expected behaviour: the following message is being displayed.
        "Warning

      The Manager role in the backup file cannot be mapped to any of the roles that you are allowed to assign.

      Import complete. Click continue to return to the course."

      • Click "Continue".
      • Check expected behaviour: Activities/Resources from C2 now appear in C1.
      Show
      Prerequisites: Moodle in standard configuration. Two courses, C1 and C2, where C1 contains at least one activity or resource. C2 must have at least one role override for the "Manager" role. For example, in C2, click "Settings -> Users -> Permissions", select "Manager", and set "Configure cohort instances" to "Allow". One user, U1, who is assigned the teacher role in both C1 and C2. Test case: Log in as U1. Go to course C1 and click "Settings -> Course administration -> Import". Select C2 as the course to import from. Click through the following screens without changing the default settings, until the import is being performed. Expected behaviour: the following message is being displayed. "Warning The Manager role in the backup file cannot be mapped to any of the roles that you are allowed to assign. Import complete. Click continue to return to the course." Click "Continue". Check expected behaviour: Activities/Resources from C2 now appear in C1.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull 2.4 Branch:
      MDL-27595-2_24
    • Pull 2.5 Branch:
      MDL-27595-2_25
    • Pull Master Branch:
    • Rank:
      17259

      Description

      If any warnings occur during a course import, then the import fails, although Moodle reports that it has been completed successfully. (An example of a warning would be that the backup file contains references to a role that the present user cannot assign - even if it's not actually necessary to assign it for performing the import.)

        Activity

        Hide
        Andreas Grabs added a comment -

        Hi Michael, why did you switched the priority to Minor? It is very important that the import functionality works. In many installations a teacher has no rights to assign students in its course but he has to be enabled to import content from another course.
        Best regards
        Andreas

        Show
        Andreas Grabs added a comment - Hi Michael, why did you switched the priority to Minor? It is very important that the import functionality works. In many installations a teacher has no rights to assign students in its course but he has to be enabled to import content from another course. Best regards Andreas
        Hide
        Damyon Wiese added a comment -

        There are lots of reasons the import can fail, the problem is that if the restore_controller returns warnings instead of errors they are not displayed in the interface, but the import process aborts.

        To fix this change line 116 in backup/import.php

        from this:

        // Execute prechecks
        if (!$rc->execute_precheck()) {
        $precheckresults = $rc->get_precheck_results();
        if (is_array($precheckresults) && !empty($precheckresults['errors'])) {
        fulldelete($tempdestination);

        to this:

        // Execute prechecks
        if (!$rc->execute_precheck()) {
        $precheckresults = $rc->get_precheck_results();
        if (is_array($precheckresults) && (!empty($precheckresults['errors']) || !empty($precheckresults['warnings']))) {
        fulldelete($tempdestination);

        Regards, Damyon

        Show
        Damyon Wiese added a comment - There are lots of reasons the import can fail, the problem is that if the restore_controller returns warnings instead of errors they are not displayed in the interface, but the import process aborts. To fix this change line 116 in backup/import.php from this: // Execute prechecks if (!$rc->execute_precheck()) { $precheckresults = $rc->get_precheck_results(); if (is_array($precheckresults) && !empty($precheckresults ['errors'] )) { fulldelete($tempdestination); to this: // Execute prechecks if (!$rc->execute_precheck()) { $precheckresults = $rc->get_precheck_results(); if (is_array($precheckresults) && (!empty($precheckresults ['errors'] ) || !empty($precheckresults ['warnings'] ))) { fulldelete($tempdestination); Regards, Damyon
        Hide
        Eloy Lafuente (stronk7) added a comment -

        This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue.

        For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

        Show
        Eloy Lafuente (stronk7) added a comment - This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment
        Hide
        Henning Bostelmann added a comment -

        This issue persists in the current master. As others have said, the root cause is that the import process is always cancelled when warnings occur; but Moodle nevertheless reports that it has been completed successfully.

        It is a bit hard to reproduce this bug, since it is not so easy to provoke a warning message in this context. One occasion on which a warning occurs is when the user has the Teacher role in this course, and the backup file from the source course contains reference to the Manager role (which teachers cannot normally assign). See test case for details.

        It's not quite evident from the present code what the expected behaviour is. I suggest the following solution: Even when a warning occurs, the import should go ahead, but the warning messages should be displayed to the user alongside the completion message.

        Patch to follow.

        Show
        Henning Bostelmann added a comment - This issue persists in the current master. As others have said, the root cause is that the import process is always cancelled when warnings occur; but Moodle nevertheless reports that it has been completed successfully. It is a bit hard to reproduce this bug, since it is not so easy to provoke a warning message in this context. One occasion on which a warning occurs is when the user has the Teacher role in this course, and the backup file from the source course contains reference to the Manager role (which teachers cannot normally assign). See test case for details. It's not quite evident from the present code what the expected behaviour is. I suggest the following solution: Even when a warning occurs, the import should go ahead, but the warning messages should be displayed to the user alongside the completion message. Patch to follow.
        Hide
        Henning Bostelmann added a comment -

        Sending my patch to peer review. For now, I've provided a patch against master only, but the change should be easy to cherry-pick into 2.3-2.5 as well; will provide these for integration.

        Show
        Henning Bostelmann added a comment - Sending my patch to peer review. For now, I've provided a patch against master only, but the change should be easy to cherry-pick into 2.3-2.5 as well; will provide these for integration.
        Hide
        CiBoT added a comment -

        Results for MDL-27595

        Show
        CiBoT added a comment - Results for MDL-27595 Branch MDL-27595 -2 to be integrated into upstream master Executed job http://ci.stronk7.com/job/Precheck%20remote%20branch/526 Execution status: 0 Details: http://ci.stronk7.com/job/Precheck%20remote%20branch/526/artifact/work/smurf.html
        Hide
        Dan Poltawski added a comment -

        Hi Henning,

        Thanks a lot for the patch - it makes sense to me.

        There were a few whitespace issues and a minor coding style issue detected which would be great to be fixed up and then I can sent it for integration:
        http://ci.stronk7.com/job/Precheck%20remote%20branch/526/artifact/work/smurf.html

        Show
        Dan Poltawski added a comment - Hi Henning, Thanks a lot for the patch - it makes sense to me. There were a few whitespace issues and a minor coding style issue detected which would be great to be fixed up and then I can sent it for integration: http://ci.stronk7.com/job/Precheck%20remote%20branch/526/artifact/work/smurf.html
        Hide
        Henning Bostelmann added a comment -

        Hi Dan,

        thanks for reviewing; and sorry for the whitespace issues, it seems I had misconfigured my Eclipse. I have amended the patch, it's now fine by the results of local/codechecker.

        Backports for Moodle 2.3, 2.4, 2.5 provided as well.

        Show
        Henning Bostelmann added a comment - Hi Dan, thanks for reviewing; and sorry for the whitespace issues, it seems I had misconfigured my Eclipse. I have amended the patch, it's now fine by the results of local/codechecker. Backports for Moodle 2.3, 2.4, 2.5 provided as well.
        Hide
        Dan Poltawski added a comment -

        Thanks Henning - sending for integration

        Show
        Dan Poltawski added a comment - Thanks Henning - sending for integration
        Hide
        Damyon Wiese added a comment -

        Thanks Henning. This obviously annoyed me 2 years ago but I didn't follow through with a nice patch. This one looks great.

        Integrated to master, 24 and 25.

        Show
        Damyon Wiese added a comment - Thanks Henning. This obviously annoyed me 2 years ago but I didn't follow through with a nice patch. This one looks great. Integrated to master, 24 and 25.
        Hide
        Rossiani Wijaya added a comment -

        Hi Henning,

        The importing process for the activity is working great except that I didn't get warning message for mapping the role.

        Testing instruction: 
        
        Expected behaviour: the following message is being displayed.
        "Warning
        The Manager role in the backup file cannot be mapped to any of the roles that you are allowed to assign.
        

        Could you provide some feedback on this?

        Thanks.

        Show
        Rossiani Wijaya added a comment - Hi Henning, The importing process for the activity is working great except that I didn't get warning message for mapping the role. Testing instruction: Expected behaviour: the following message is being displayed. "Warning The Manager role in the backup file cannot be mapped to any of the roles that you are allowed to assign. Could you provide some feedback on this? Thanks.
        Hide
        Henning Bostelmann added a comment -

        Hi Rossiani,

        thanks for testing. Are you sure that all prerequisites are set up as described (including the role override in the course context)? Also, the user U1 must have only the teacher role and no more; if you do the procedure under an admin or manager account, the warning will not appear.

        Show
        Henning Bostelmann added a comment - Hi Rossiani, thanks for testing. Are you sure that all prerequisites are set up as described (including the role override in the course context)? Also, the user U1 must have only the teacher role and no more; if you do the procedure under an admin or manager account, the warning will not appear.
        Hide
        Rossiani Wijaya added a comment -

        Hi Henning,

        Yes, I followed the steps as describe in testing instructions.

        I login as U1 to import the c1 and the role override is done in course context.

        I'm attaching my c1 and c2 courses for your review purpose.

        Thank you for commenting.

        Show
        Rossiani Wijaya added a comment - Hi Henning, Yes, I followed the steps as describe in testing instructions. I login as U1 to import the c1 and the role override is done in course context. I'm attaching my c1 and c2 courses for your review purpose. Thank you for commenting.
        Hide
        Dan Poltawski added a comment -

        Hmm, i'm not getting the warnings following the testing instructions either.

        Show
        Dan Poltawski added a comment - Hmm, i'm not getting the warnings following the testing instructions either.
        Hide
        Dan Poltawski added a comment -

        OK, I do get the warning now - I was being lazy and using loginas rather than logging in as proper user.

        Show
        Dan Poltawski added a comment - OK, I do get the warning now - I was being lazy and using loginas rather than logging in as proper user.
        Hide
        Rossiani Wijaya added a comment -

        Hi Dan,

        I was doing the proper login when I tested this.

        Also Dan's suggested to import c2 into c1, instead for c1 into c2 as instructed in testing instructions, the warning appears. So we are assuming that it is a typo in the testing instructions. I'm updating the testing instructions.

        Henning,
        Please let me know if this is incorrect.

        Thanks Dan & Henning.

        Show
        Rossiani Wijaya added a comment - Hi Dan, I was doing the proper login when I tested this. Also Dan's suggested to import c2 into c1, instead for c1 into c2 as instructed in testing instructions, the warning appears. So we are assuming that it is a typo in the testing instructions. I'm updating the testing instructions. Henning, Please let me know if this is incorrect. Thanks Dan & Henning.
        Hide
        Rossiani Wijaya added a comment -

        This is now working as expected.

        Tested for 2.4, 2.5 and master.

        Test passed.

        Show
        Rossiani Wijaya added a comment - This is now working as expected. Tested for 2.4, 2.5 and master. Test passed.
        Hide
        Dan Poltawski added a comment -

        (oops, not sure how I ended up assigning this issue to me)

        Show
        Dan Poltawski added a comment - (oops, not sure how I ended up assigning this issue to me)
        Hide
        Damyon Wiese added a comment -

        Thanks for your efforts. This issue is one of the outstanding issues that passed all our testing and was accepted into Moodle this week.

        Hurray!

        Show
        Damyon Wiese added a comment - Thanks for your efforts. This issue is one of the outstanding issues that passed all our testing and was accepted into Moodle this week. Hurray!
        Hide
        Henning Bostelmann added a comment -

        Thanks Dan and Rossiani for completing the test, and sorry for not responding earlier - I was out of the office for some days.

        Show
        Henning Bostelmann added a comment - Thanks Dan and Rossiani for completing the test, and sorry for not responding earlier - I was out of the office for some days.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: