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 Master Branch:

      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.)

        Gliffy Diagrams

          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:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: