Moodle
  1. Moodle
  2. MDL-28488

Import requires is too integrated with backup/restore permissions

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.3, 2.1, 2.2
    • Fix Version/s: 2.0.5, 2.1.2
    • Component/s: Backup
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      Ideally, test this issue under 21_STABLE

      There are various things to test here (each one corresponds to a separate commit so should be easy to revert partially if necessary. I hope it won't be necessary). The commit hash is the master one in the instructions below.

      Important note: You may get one warning about "uasort" (in textlib) when executing any backup/restore/import operation. Ignore it, has been addressed as MDL-29319) and does not affect testing this.

      TESTING 8645a28f:

      1. Create 2 courses, "source" and "target" with some activities.
      2. Logged as (editing) teacher, go to "source" course and enable edition.
      3. TEST: The teacher must see the "x2" icon on each activity. Pick one and verify the activity becomes duplicated ok.
      4. In other browser, as admin, prevent 'moodle/backup:backuptargetimport' capability to the (editing) teacher in that course.
      5. TEST: The teacher cannot see the "x2" icon anymore, hence cannot duplicate activities.

      TESTING 77c2ca69:

      1. TEST: In the "source" course page, the teacher gets, in the settings block access to "backup", "restore" and "import". Try one operation of each type and check it ends ok.
      2. As admin, prevent any of the 'moodle/backup:backupcourse' and/or 'moodle/restore:restorecourse' capabilities to the (editing) teacher in that course.
      3. TEST: In the "source" course page, the teacher doesn't get anymore the "backup" and "restore" options. Only "import" remains. Try one import and check it ends ok.
      4. As admin, prevent and/or 'moodle/restore:restoretargetimport' capability to the (editing) teacher in that course.
      5. TEST: In the "source" course page, the teacher doesn't get the "import" option anymore.
      6. TEST: Go to "target" course as teacher. Pick import. In the list of courses you should see the "target" course itself but not the "source" course (because we have prevented it to be used as source for import).

      TESTING 9530e1ed:

      1. As admin, go to admin -> courses -> backup -> general backup defaults: Annotate the "include users" and "include logs" help strings.
      2. admin -> courses -> backup -> automated backup setup
      3. TEST: Verify that the help strings for those 2 settings are 100% the same. It was using different/old strings before.

      TESTING 2a4ba40f:

      1. As admin, prevent the 'moodle/backup:configure' and 'moodle/restore:configure' capabilities to the (editing) teacher in the "target" course.
      2. As (editing) teacher, try one import operation in the "target" course picking the same "target" course as source.
      3. TEST: The teacher sees the "import activities/blocks/filters" settings after picking the course.
      4. TEST: The teacher sees the list of sections and activities to select / unselect them
      5. TEST: The import operation finishes ok.

      TESTING 59fc0cbd:

      1. As admin, go to admin -> courses -> backup -> general backup defaults. Check and lock this setting: "include users" and uncheck and lock this setting: "include blocks".
      2. As (editing teacher) try any import operation.
      3. TEST: No error happens (it used to cause one backup exception after picking the source course)
      4. TEST: The "include blocks" setting is shown checked and unlocked (you can uncheck it) by default.
      5. TEST: The import operation ends ok.
      1. TEST: Run "backup" unit tests. 0 failures expected.
      Show
      Ideally, test this issue under 21_STABLE There are various things to test here (each one corresponds to a separate commit so should be easy to revert partially if necessary. I hope it won't be necessary). The commit hash is the master one in the instructions below. Important note: You may get one warning about "uasort" (in textlib) when executing any backup/restore/import operation. Ignore it, has been addressed as MDL-29319 ) and does not affect testing this. TESTING 8645a28f: Create 2 courses, "source" and "target" with some activities. Logged as (editing) teacher, go to "source" course and enable edition. TEST: The teacher must see the "x2" icon on each activity. Pick one and verify the activity becomes duplicated ok. In other browser, as admin, prevent 'moodle/backup:backuptargetimport' capability to the (editing) teacher in that course. TEST: The teacher cannot see the "x2" icon anymore, hence cannot duplicate activities. TESTING 77c2ca69: TEST: In the "source" course page, the teacher gets, in the settings block access to "backup", "restore" and "import". Try one operation of each type and check it ends ok. As admin, prevent any of the 'moodle/backup:backupcourse' and/or 'moodle/restore:restorecourse' capabilities to the (editing) teacher in that course. TEST: In the "source" course page, the teacher doesn't get anymore the "backup" and "restore" options. Only "import" remains. Try one import and check it ends ok. As admin, prevent and/or 'moodle/restore:restoretargetimport' capability to the (editing) teacher in that course. TEST: In the "source" course page, the teacher doesn't get the "import" option anymore. TEST: Go to "target" course as teacher. Pick import. In the list of courses you should see the "target" course itself but not the "source" course (because we have prevented it to be used as source for import). TESTING 9530e1ed: As admin, go to admin -> courses -> backup -> general backup defaults: Annotate the "include users" and "include logs" help strings. admin -> courses -> backup -> automated backup setup TEST: Verify that the help strings for those 2 settings are 100% the same. It was using different/old strings before. TESTING 2a4ba40f: As admin, prevent the 'moodle/backup:configure' and 'moodle/restore:configure' capabilities to the (editing) teacher in the "target" course. As (editing) teacher, try one import operation in the "target" course picking the same "target" course as source. TEST: The teacher sees the "import activities/blocks/filters" settings after picking the course. TEST: The teacher sees the list of sections and activities to select / unselect them TEST: The import operation finishes ok. TESTING 59fc0cbd: As admin, go to admin -> courses -> backup -> general backup defaults. Check and lock this setting: "include users" and uncheck and lock this setting: "include blocks". As (editing teacher) try any import operation. TEST: No error happens (it used to cause one backup exception after picking the source course) TEST: The "include blocks" setting is shown checked and unlocked (you can uncheck it) by default. TEST: The import operation ends ok. TEST: Run "backup" unit tests. 0 failures expected.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      18234

      Description

      Currently a user could see the Import link in their course, but if they aren't given full backupcourse and restorecourse capabilities they can't take activities from their CourseA to their CourseB. In our setup, we don't want our teachers to do full course backup and restores, but transferring their own course activities from one course to another should be allowed. In 1.9 this worked, but now a user must have full backup/restore permissions to do Imports.

      Other improvements that need to be made:
      Select all, Deselect all in the import area. If a user is importing a few activities from a full course they must manually uncheck all the things they don't want.

      Best case scenario:
      Have a course:import capability that allows users to Import to and from courses where they have that capability or maybe use manageactivities capability?. They do not need backupcourse or restorecourse capabilities.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for suggesting this.

          Adding more detail to your suggestion will make it easier to work on.

          If you can propose a code solution, that will increase the chance of this improvement/feature coming about sooner.

          Show
          Michael de Raadt added a comment - Thanks for suggesting this. Adding more detail to your suggestion will make it easier to work on. If you can propose a code solution, that will increase the chance of this improvement/feature coming about sooner.
          Hide
          Ryan Smith added a comment -

          This is an unexpected loss of capability going from Moodle 1.9.x to Moodle 2.x.

          On our site, we cannot give our users the ability to backup and restore. Backups are too resource intensive and we like to have full control on what gets restored.

          On 1.9.x this worked great, teachers could still import activities from course to course. Now there is no import functionality since we've upgrade to Moodle 2.1.

          In my opinion, this priority should be increase since it is lost functionality between versions.

          Show
          Ryan Smith added a comment - This is an unexpected loss of capability going from Moodle 1.9.x to Moodle 2.x. On our site, we cannot give our users the ability to backup and restore. Backups are too resource intensive and we like to have full control on what gets restored. On 1.9.x this worked great, teachers could still import activities from course to course. Now there is no import functionality since we've upgrade to Moodle 2.1. In my opinion, this priority should be increase since it is lost functionality between versions.
          Hide
          Scott Ritzler added a comment -

          We just upgraded to Moodle 2.1 and discovered this issue. It definitely is a loss of a Moodle 1.9 feature that we weren't expecting. Does anybody have any quick workarounds? I hope this can be fixed soon.

          Show
          Scott Ritzler added a comment - We just upgraded to Moodle 2.1 and discovered this issue. It definitely is a loss of a Moodle 1.9 feature that we weren't expecting. Does anybody have any quick workarounds? I hope this can be fixed soon.
          Hide
          Javier Martinez added a comment -

          Hello:

          Is there an update to this issue? My group upgraded to Moodle 2.1 and now our teachers cannot import activities from their other courses. We cannot give them backup and restore permissions.

          This worked perfectly in Moodle 1.9.

          Show
          Javier Martinez added a comment - Hello: Is there an update to this issue? My group upgraded to Moodle 2.1 and now our teachers cannot import activities from their other courses. We cannot give them backup and restore permissions. This worked perfectly in Moodle 1.9.
          Hide
          Tim Simmons added a comment -

          I just found this bug and have to post! We've been pulling our hair out trying to figure out why our users couldn't import between courses. In Moodle 1.9 users could import activities and whatnot between their different courses even though we have backup and restore disabled for their role of teacher.

          We upgraded to Moodle 2, then to 2.1 about a month ago. Now teachers cannot import and there is no role capability for it! Please fix this ASAP, we have many users upset about this problem. There was nothing in the upgrade documentation for Moodle 2 that this would be changing.

          Show
          Tim Simmons added a comment - I just found this bug and have to post! We've been pulling our hair out trying to figure out why our users couldn't import between courses. In Moodle 1.9 users could import activities and whatnot between their different courses even though we have backup and restore disabled for their role of teacher. We upgraded to Moodle 2, then to 2.1 about a month ago. Now teachers cannot import and there is no role capability for it! Please fix this ASAP, we have many users upset about this problem. There was nothing in the upgrade documentation for Moodle 2 that this would be changing.
          Hide
          Betty Fitzgerald added a comment -

          I'm thrilled I found others with this same problem! Our teachers cannot import activities either. They could in Moodle 1.9.

          Can this be fixed soon?

          Show
          Betty Fitzgerald added a comment - I'm thrilled I found others with this same problem! Our teachers cannot import activities either. They could in Moodle 1.9. Can this be fixed soon?
          Hide
          Cindy Duggan added a comment -

          We are having the same problem. Teachers are trying to move stuff between a test and Fall Production server. School starts today.

          Show
          Cindy Duggan added a comment - We are having the same problem. Teachers are trying to move stuff between a test and Fall Production server. School starts today.
          Hide
          Henning Bostelmann added a comment -

          Note that a partial fix is in MDL-29072.

          Show
          Henning Bostelmann added a comment - Note that a partial fix is in MDL-29072 .
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hope to be over this along this week, I thought this was working as specified @ MDL-24518. Seems it isn't

          Thanks for the complete description here and potential fix @ MDL-29072.

          Stay tuned... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hope to be over this along this week, I thought this was working as specified @ MDL-24518 . Seems it isn't Thanks for the complete description here and potential fix @ MDL-29072 . Stay tuned... ciao
          Hide
          Javier Martinez added a comment -

          Eloy thank you for letting us know that you are working on it. Any progress made yet?

          Show
          Javier Martinez added a comment - Eloy thank you for letting us know that you are working on it. Any progress made yet?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ho,

          well I've been looking how capabilities were planned (MDL-21668) and related code in xxx_check_security(), and also one related issue has been sent to integration (thanks Henning for MDL-29072).

          Said that, I think the problem here is how those capabilities were expected to work. In my mind, when implementing the subsystem, I always thought about the backupcourse/section/activity caps like global switchers allowing/preventing roles to perform any operation. And later, I imagined the "target" permissions like sort of sub-capabilities to enable certain types of backup & restore operations (import, hub...).

          So, given that, obviously, in order to be able to perform one import from course X, the user needs to have backup permissions in that course and also targetimport permission there. And that's the way it's enforced by the security checks in backup/restore controllers.

          But it seems that the UI is more "relaxed" (faulty really if we accept the behavior above like the expected one), and it only checks for the targetimport capability, ignoring the backup one.

          So really, the use case described above (teachers not able to backup course X, but able to import from it) is not supported at all.

          Said that, I can see the point about that use-case, so perhaps we should be undoing that master switch/secondary permission behavior and, instead, use them separately so (let's call this "the separate way"):

          • backupcourse/section/activity caps will control "general" backups.
          • targetimport/hub will control import/hub backups.
          • (same applies for corresponding restore caps)

          In the other side... I continue thinking that the master/secondary behavior has sense, in fact, I don't understand why you're denying teachers perms to backup when the import operation is 100% the same, aka, one backup and one restore executed in a chained way). With the same risks, load for the server, etc... 100% the same. So let's call this (the current way), the "master/subcap" way.

          Said that, there are 2 alternatives, please, comment/discuss and agree (here and/or in the forums). I'll be happy to implement the any of them.

          My personal vote goes to the "master/subcap" one (the current one, that means fix the "relaxed" UI to enforce things like backup controller does). But it's only one more vote.

          But definitively, now it's a good time to decide, implement and document the final expected behavior once and for all.

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Ho, well I've been looking how capabilities were planned ( MDL-21668 ) and related code in xxx_check_security(), and also one related issue has been sent to integration (thanks Henning for MDL-29072 ). Said that, I think the problem here is how those capabilities were expected to work. In my mind, when implementing the subsystem, I always thought about the backupcourse/section/activity caps like global switchers allowing/preventing roles to perform any operation. And later, I imagined the "target" permissions like sort of sub-capabilities to enable certain types of backup & restore operations (import, hub...). So, given that, obviously, in order to be able to perform one import from course X, the user needs to have backup permissions in that course and also targetimport permission there. And that's the way it's enforced by the security checks in backup/restore controllers. But it seems that the UI is more "relaxed" (faulty really if we accept the behavior above like the expected one), and it only checks for the targetimport capability, ignoring the backup one. So really, the use case described above (teachers not able to backup course X, but able to import from it) is not supported at all. Said that, I can see the point about that use-case, so perhaps we should be undoing that master switch/secondary permission behavior and, instead, use them separately so (let's call this "the separate way"): backupcourse/section/activity caps will control "general" backups. targetimport/hub will control import/hub backups. (same applies for corresponding restore caps) In the other side... I continue thinking that the master/secondary behavior has sense, in fact, I don't understand why you're denying teachers perms to backup when the import operation is 100% the same, aka, one backup and one restore executed in a chained way). With the same risks, load for the server, etc... 100% the same. So let's call this (the current way), the "master/subcap" way. Said that, there are 2 alternatives, please, comment/discuss and agree (here and/or in the forums). I'll be happy to implement the any of them. My personal vote goes to the "master/subcap" one (the current one, that means fix the "relaxed" UI to enforce things like backup controller does). But it's only one more vote. But definitively, now it's a good time to decide, implement and document the final expected behavior once and for all. Thanks and ciao
          Hide
          Martin Dougiamas added a comment -

          My +1 for the ability to specify via a separate capability that import can work independently of backup/restore, since that seems to be a useful thing (and the fact it uses backup/restore in the background is not relevant to users). Obviously you need the capability enabled in source and destination courses for it to work.

          Show
          Martin Dougiamas added a comment - My +1 for the ability to specify via a separate capability that import can work independently of backup/restore, since that seems to be a useful thing (and the fact it uses backup/restore in the background is not relevant to users). Obviously you need the capability enabled in source and destination courses for it to work.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          So then, theses caps:

          • moodle/backup:[backup|restore][course|section|activity] will allow/prevent general and automated (course|section|activity) backups & restores.

          And these caps:

          • moodle/backup:[backup|restore]target[import|hub] will allow/prevent special (import|hub) backups & restores.

          Both groups of caps doing their job in a 100% independent way (i.e. only one of the groups will be checked and they won't work together any more.

          Sounds ok? Anybody disagree? Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - So then, theses caps: moodle/backup: [backup|restore] [course|section|activity] will allow/prevent general and automated (course|section|activity) backups & restores. And these caps: moodle/backup: [backup|restore] target [import|hub] will allow/prevent special (import|hub) backups & restores. Both groups of caps doing their job in a 100% independent way (i.e. only one of the groups will be checked and they won't work together any more. Sounds ok? Anybody disagree? Ciao
          Hide
          Matthew Davidson added a comment -

          I agree with separating the caps. Thank you Eloy.

          Show
          Matthew Davidson added a comment - I agree with separating the caps. Thank you Eloy.
          Hide
          Matthew Davidson added a comment -

          Should I open an improvement bug for the select all / deselect all activities button?

          Show
          Matthew Davidson added a comment - Should I open an improvement bug for the select all / deselect all activities button?
          Hide
          Ryan Smith added a comment -

          The separate caps sound great. Thanks Eloy!

          Show
          Ryan Smith added a comment - The separate caps sound great. Thanks Eloy!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Matthew, about the select all UI, surely MDL-26740 is the issue to vote / follow / comment.

          Back to this issue, I'm working on it right now. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Matthew, about the select all UI, surely MDL-26740 is the issue to vote / follow / comment. Back to this issue, I'm working on it right now. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Here there is one initial patch for Moodle 2.2dev providing:

          • better control of the "duplicate activity" in course edit mode.
          • separation of capabilities, so import will be controlled by the [backup|restore]targetimport ones exclusively without requiring anymore the backup[course|section|activity].

          While playing with this, I've found a few annoyances in the import UI, depending of other capabilities, so surely I'll add here some more commits fixing that, but the two commits above should be enough to test if behavior is the expected one.

          Patch: https://github.com/stronk7/moodle/compare/master...MDL-28488
          Download: https://github.com/stronk7/moodle/zipball/MDL-28488

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Here there is one initial patch for Moodle 2.2dev providing: better control of the "duplicate activity" in course edit mode. separation of capabilities, so import will be controlled by the [backup|restore] targetimport ones exclusively without requiring anymore the backup [course|section|activity] . While playing with this, I've found a few annoyances in the import UI, depending of other capabilities, so surely I'll add here some more commits fixing that, but the two commits above should be enough to test if behavior is the expected one. Patch: https://github.com/stronk7/moodle/compare/master...MDL-28488 Download: https://github.com/stronk7/moodle/zipball/MDL-28488 Ciao
          Hide
          Ryan Smith added a comment -

          Eloy,

          It works great so far! I downloaded your dev version of Moodle and installed it. I created a user and made two courses. I set the backup and restore course permissions to Prevent for teacher roles.

          I gave that user the teacher role in both courses. The Import link appears, and I see the list of the two courses. Small issue here, it shows (and allows) me to select the current course I'm importing into. Ideally it should only show my other course(s), and not the one I'm importing to.

          The import worked perfectly, I tested by importing a URL and a assignment activity.

          Thank you for your work on this. Our users will be very happy when this lands into the current 2.1 stable.

          Show
          Ryan Smith added a comment - Eloy, It works great so far! I downloaded your dev version of Moodle and installed it. I created a user and made two courses. I set the backup and restore course permissions to Prevent for teacher roles. I gave that user the teacher role in both courses. The Import link appears, and I see the list of the two courses. Small issue here, it shows (and allows) me to select the current course I'm importing into. Ideally it should only show my other course(s), and not the one I'm importing to. The import worked perfectly, I tested by importing a URL and a assignment activity. Thank you for your work on this. Our users will be very happy when this lands into the current 2.1 stable.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ho Ryan,

          thanks for the complete testing, yay!

          About the "current course" being available in the list, if I don't remember wrongly, that was done on purpose, to allow quick-duplication of n activities within the course using the import facility. Other selectors like the "restore in existing course" already prevent "current course", but on import we decided to leave it available.

          I'm reviewing some more minor (UI) bits here related with other capabilities and, hopefully this will land next week both into Moodle 2.1 and 2.2 (surely 2.0 will miss this feature, have to check it yet).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Ho Ryan, thanks for the complete testing, yay! About the "current course" being available in the list, if I don't remember wrongly, that was done on purpose, to allow quick-duplication of n activities within the course using the import facility. Other selectors like the "restore in existing course" already prevent "current course", but on import we decided to leave it available. I'm reviewing some more minor (UI) bits here related with other capabilities and, hopefully this will land next week both into Moodle 2.1 and 2.2 (surely 2.0 will miss this feature, have to check it yet). Ciao
          Hide
          Ryan Smith added a comment -

          Eloy,

          Isn't the duplication of items in Moodle 2.1 done with the x2 icon when you turn editing on? I don't believe that feature was in 2.0. This would accomplish the same result as importing an existing item into the same course.

          Show
          Ryan Smith added a comment - Eloy, Isn't the duplication of items in Moodle 2.1 done with the x2 icon when you turn editing on? I don't believe that feature was in 2.0. This would accomplish the same result as importing an existing item into the same course.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yes, yes, it's the same than the x2 icon in Moodle 2.1.

          The difference is that it allows you to duplicate n activities together instead of 1 by 1 (by importing from self).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Yes, yes, it's the same than the x2 icon in Moodle 2.1. The difference is that it allows you to duplicate n activities together instead of 1 by 1 (by importing from self). Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          One more commit added. Once applied, the 'moodle/[backup|restore]:configure' capabilities won't control anything in the import operations.

          By definition, one import operation must be able always to pick activities, so the capability above is nosense at all in this mode. It will continue working against the general/automated/hub backups.

          Show
          Eloy Lafuente (stronk7) added a comment - One more commit added. Once applied, the 'moodle/ [backup|restore] :configure' capabilities won't control anything in the import operations. By definition, one import operation must be able always to pick activities, so the capability above is nosense at all in this mode. It will continue working against the general/automated/hub backups.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And added one more commit (the last) preventing general defaults to be applied to import/hub backups. It could lead to wrong situations if some settings were forced to yes, while the mode itself imposes the opposite. For example setting in defaults users=yes (and locked) was leading to error because the security constraints try later to set the setting to users=no, but find it locked.

          Show
          Eloy Lafuente (stronk7) added a comment - And added one more commit (the last) preventing general defaults to be applied to import/hub backups. It could lead to wrong situations if some settings were forced to yes, while the mode itself imposes the opposite. For example setting in defaults users=yes (and locked) was leading to error because the security constraints try later to set the setting to users=no, but find it locked.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          I'm sending this for integration:

          • Backported to 20 and 21 stable.
          • The "duplicate module" functionality does not exist under 20_STABLE so that branch shows one less commit.
          • Note for integrators: Surely is better to see each commit individually. Each one fixes one different problem detected on import.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited I'm sending this for integration: Backported to 20 and 21 stable. The "duplicate module" functionality does not exist under 20_STABLE so that branch shows one less commit. Note for integrators: Surely is better to see each commit individually. Each one fixes one different problem detected on import. Ciao
          Hide
          Sam Hemelryk added a comment -

          Thanks guys, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks guys, this has been integrated now
          Hide
          Andrew Davis added a comment -

          8645a28f passed

          Show
          Andrew Davis added a comment - 8645a28f passed
          Hide
          Andrew Davis added a comment - - edited

          posted a screenshot in MDL-27919 as ran into that.

          77c2ca69 passed
          9530e1ed passed
          2a4ba40f passed although it seems weird that we allow the same course to be both the source and the target course for an import.
          59fc0cbd passed although if I click on backup instead of import I get an error "error/setting_locked_by_config". Does this require further attention?
          All backup unit tests pass.

          Show
          Andrew Davis added a comment - - edited posted a screenshot in MDL-27919 as ran into that. 77c2ca69 passed 9530e1ed passed 2a4ba40f passed although it seems weird that we allow the same course to be both the source and the target course for an import. 59fc0cbd passed although if I click on backup instead of import I get an error "error/setting_locked_by_config". Does this require further attention? All backup unit tests pass.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Re:

          2a4ba40f yeah, it's weird, surely we should end having one own capability for duplicating activities. But before the patch status was worse because you could be seeing the "x2" without really being able to duplicate (perms error in when clicked).

          59fc0cbd yeah too, there are sort of "conflict" between different "locking statuses". This is something I want to discuss with SamH, to see how we could solve it (surely stabilizing some sort of locking priority or so). I think you can ignore it for now.

          So I'd consider this passed. Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Re: 2a4ba40f yeah, it's weird, surely we should end having one own capability for duplicating activities. But before the patch status was worse because you could be seeing the "x2" without really being able to duplicate (perms error in when clicked). 59fc0cbd yeah too, there are sort of "conflict" between different "locking statuses". This is something I want to discuss with SamH, to see how we could solve it (surely stabilizing some sort of locking priority or so). I think you can ignore it for now. So I'd consider this passed. Thanks!
          Hide
          Andrew Davis added a comment -

          Passing. Leaving this to Sam and Eloy to follow up on any further weirdness

          Show
          Andrew Davis added a comment - Passing. Leaving this to Sam and Eloy to follow up on any further weirdness
          Hide
          Eloy Lafuente (stronk7) added a comment -

          YTC !

          (aka, yay, thanks and ciao ) Closing.

          Show
          Eloy Lafuente (stronk7) added a comment - YTC ! (aka, yay, thanks and ciao ) Closing.
          Hide
          Ryan Smith added a comment -

          We updated last night and the new code works great.

          We are still trying to diagnose http://tracker.moodle.org/browse/MDL-29357. The import fix from this bug worked fine for about 12 hours, now it gives us the same error as the x2 issue. A red screen with error/cannot_empty_backup_temp_dir. Very frustrating.

          Show
          Ryan Smith added a comment - We updated last night and the new code works great. We are still trying to diagnose http://tracker.moodle.org/browse/MDL-29357 . The import fix from this bug worked fine for about 12 hours, now it gives us the same error as the x2 issue. A red screen with error/cannot_empty_backup_temp_dir. Very frustrating.
          Hide
          Matthew Davidson added a comment - - edited

          I believe that the permissions for import work beautifully, however I have run into another problem. It seems that in restore_ui_components.php in the function search(), the process by which the backup/restore/import area creates the list of available courses is to grab 250 courses from the database and test them 1 by 1 to see if the user has the correct capabilities to proceed. Unfortunately, if their courses don't get grabbed in that 250, it still returns "No courses available". The SQL that gets the list of courses is incorrect, or the process is poorly designed. If I change the limit from 250 to 2000 (we have about 1500 courses), then the courses show up as expected, but the server had to check each course out individually. Probably needs to be a new bug. Also, this is why the course list seems to have no relevant sort.

          Opened as new bug: http://tracker.moodle.org/browse/MDL-29628

          Show
          Matthew Davidson added a comment - - edited I believe that the permissions for import work beautifully, however I have run into another problem. It seems that in restore_ui_components.php in the function search(), the process by which the backup/restore/import area creates the list of available courses is to grab 250 courses from the database and test them 1 by 1 to see if the user has the correct capabilities to proceed. Unfortunately, if their courses don't get grabbed in that 250, it still returns "No courses available". The SQL that gets the list of courses is incorrect, or the process is poorly designed. If I change the limit from 250 to 2000 (we have about 1500 courses), then the courses show up as expected, but the server had to check each course out individually. Probably needs to be a new bug. Also, this is why the course list seems to have no relevant sort. Opened as new bug: http://tracker.moodle.org/browse/MDL-29628
          Hide
          Gilles-Philippe Leblanc added a comment -

          In TEST 77c2ca69, I still see the course who have "moodle/restore:restoretargetimport'" capability removed for (editing) teacher. Tested with version 2.1.2 and 2.0.3 with the backported fix.

          Show
          Gilles-Philippe Leblanc added a comment - In TEST 77c2ca69, I still see the course who have "moodle/restore:restoretargetimport'" capability removed for (editing) teacher. Tested with version 2.1.2 and 2.0.3 with the backported fix.
          Hide
          Damyon Wiese added a comment -

          Sorry - wrong bug number.

          Show
          Damyon Wiese added a comment - Sorry - wrong bug number.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: