Moodle
  1. Moodle
  2. MDL-19125

New addinstance capabilities for activity modules to replace the old complicated course restricted modules

    Details

    • Testing Instructions:
      Hide

      We are mostly testing the new functionality in 2.3, but part of that is testing that data is upgraded and restored properly from 2.2.

      1. With default settings, go to any course and ensure you can add any type of activity.

      2. Change the role definition for the Teacher role, to remove mod/chat:addinstance, and make sure you cannot add a Chat in any course when logged in as a Teacher.

      3. Using a Moodle 2.2 site, turn on the Module security feature, and set it to restrict modules for all courses, with a default list. (Using the old http://docs.moodle.org/22/en/Managing_activities#Module_security feature)

      4. Go into a course in this site, go to the course settings, make sure module security is turned on there, and that the list of allowed modules is right, then Save the form.

      5. Backup this course, and restore it into your Moodle 2.3 dev install. Verify that role overrides are created to prevent Teachers from adding the disallowed modules.

      6. As for steps 4. and 5. but set things up on, and backup and restore from, a Moodle 1.9 site.

      7. Upgrade the Moodle 2.2 site that you used in steps 4. and 5. to the latest master. Verify that the correct role overrides are created; and that the old course_allowed_modules table, and course.retrictmodules column and restrictmodulesfor, restrictbydefault and defaultallowedmodules config settings are dropped.)

      8. Test a fresh install just to be sure

      Show
      We are mostly testing the new functionality in 2.3, but part of that is testing that data is upgraded and restored properly from 2.2. 1. With default settings, go to any course and ensure you can add any type of activity. 2. Change the role definition for the Teacher role, to remove mod/chat:addinstance, and make sure you cannot add a Chat in any course when logged in as a Teacher. 3. Using a Moodle 2.2 site, turn on the Module security feature, and set it to restrict modules for all courses, with a default list. (Using the old http://docs.moodle.org/22/en/Managing_activities#Module_security feature) 4. Go into a course in this site, go to the course settings, make sure module security is turned on there, and that the list of allowed modules is right, then Save the form. 5. Backup this course, and restore it into your Moodle 2.3 dev install. Verify that role overrides are created to prevent Teachers from adding the disallowed modules. 6. As for steps 4. and 5. but set things up on, and backup and restore from, a Moodle 1.9 site. 7. Upgrade the Moodle 2.2 site that you used in steps 4. and 5. to the latest master. Verify that the correct role overrides are created; and that the old course_allowed_modules table, and course.retrictmodules column and restrictmodulesfor, restrictbydefault and defaultallowedmodules config settings are dropped.) 8. Test a fresh install just to be sure
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-19125_module_security
    • Rank:
      5260

      Description

      Currently the way this works is:

      • Enforce settings (as defaults) for newly created courses.
      • Per course, use a whitelist to determine which modules can be added.

      This is kind of useless, as an empty whitelist is ambiguous. It could either mean "use the site default", or it could mean "i don't want to allow anything at all".

      One way to get around this is a new setting (maybe course.restrictedmodules) which indicates that the course has overridden the site default.

      Another way is to add a capability to each module 'canaddtocourse' or similar, and override it for a role at a course context. I think the latter is probably more correct in a 'moodle sense' but has the following drawbacks:

      1. Third party module authors must implement this capability, for their module to implement this setting (stupid)
      2. Eh, roles are complicated enough as it is, and Howard will kill me if we must override things further.

      The former is probably simpler (and easier to implement) but means adding yet another field to course. Do courses have extra settings? Maybe we could have a course_preferences table or something.

      1. Alter roles by capability.bmml
        2 kB
        Tim Hunt
      2. Alter roles by capability 2.bmml
        7 kB
        Tim Hunt
      3. Alter roles by capability 2.bmml
        7 kB
        Tim Hunt
      4. Alter roles by capability 2.bmml
        2 kB
        Tim Hunt
      5. Possible admin interface.bmml
        9 kB
        Tim Hunt
      6. Possible admin interface.bmml
        5 kB
        Tim Hunt
      7. Possible course settings UI.bmml
        3 kB
        Tim Hunt
      8. Possible course settings UI.bmml
        5 kB
        Tim Hunt
      9. Possible course settings UI.bmml
        5 kB
        Tim Hunt
      10. sam proposed - course settings UI.bmml
        3 kB
        Sam Marshall
      1. Alter roles by capability.png
        15 kB
      2. Alter roles by capability 2.png
        36 kB
      3. Possible admin interface.png
        57 kB
      4. Possible course settings UI.png
        16 kB
      5. sam proposed - course settings UI.png
        23 kB

        Issue Links

          Activity

          Hide
          Penny Leach added a comment -

          MDL-19110 has fixed the code to 'work as expected' , however as mentioned in this bug, i don't think 'as expected' is that helpful

          Show
          Penny Leach added a comment - MDL-19110 has fixed the code to 'work as expected' , however as mentioned in this bug, i don't think 'as expected' is that helpful
          Hide
          Adrian Schlegel added a comment -

          'restrictmodulesfor' has also the following issue:
          If you change it to 'none', the restricted flag on courses is not removed. This results in the modules in this course still being restricted and not even an administrator can change this since the corresponding setting is no longer available in the course settings.

          Show
          Adrian Schlegel added a comment - 'restrictmodulesfor' has also the following issue: If you change it to 'none', the restricted flag on courses is not removed. This results in the modules in this course still being restricted and not even an administrator can change this since the corresponding setting is no longer available in the course settings.
          Hide
          Tim Hunt added a comment -

          Added UI Mockup: <Possible admin interface>

          Show
          Tim Hunt added a comment - Added UI Mockup: <Possible admin interface>
          Hide
          Tim Hunt added a comment -

          Added UI Mockup: <Possible course settings UI>

          Show
          Tim Hunt added a comment - Added UI Mockup: <Possible course settings UI>
          Hide
          Tim Hunt added a comment -

          Edited UI Mockup <Possible admin interface>: New improved UI. Thanks sam.

          Show
          Tim Hunt added a comment - Edited UI Mockup <Possible admin interface>: New improved UI. Thanks sam.
          Hide
          Tim Hunt added a comment -

          Added UI Mockup: <Alter roles by capability>

          Show
          Tim Hunt added a comment - Added UI Mockup: <Alter roles by capability>
          Hide
          Tim Hunt added a comment -

          Added UI Mockup: <Alter roles by capability 2>

          Show
          Tim Hunt added a comment - Added UI Mockup: <Alter roles by capability 2>
          Hide
          Tim Hunt added a comment -

          So, the current plan is to implement "proposal two" from http://docs.moodle.org/dev/Module_security_improvements#Proposal_two

          The necessary work is:

          1. Make the 'Add an activity' code in Moodle do has_capability('mod/whatever:addinstance', $coursecontext) - if that capability exists.
          2. Add that capability to all core modules.
          3. Add debugging output to warn about modules that do not define this capability.
          4. Remove the old code for the 'Module security' feature.

          This work is planned to be done for Moodle 2.3.

          Regarding 3. Where should this notice appear? Options are:
          a. only during install/upgrade
          b. on any course page when editing is on
          c. something else.

          Do we need to add an extra task:
          3.5. Write upgrade code to convert the old module security settings into permission overrides?
          I really hope not. I think it is sufficient to warn about these changes in the Moodle 2.3 release notes.

          Show
          Tim Hunt added a comment - So, the current plan is to implement "proposal two" from http://docs.moodle.org/dev/Module_security_improvements#Proposal_two The necessary work is: 1. Make the 'Add an activity' code in Moodle do has_capability('mod/whatever:addinstance', $coursecontext) - if that capability exists. 2. Add that capability to all core modules. 3. Add debugging output to warn about modules that do not define this capability. 4. Remove the old code for the 'Module security' feature. This work is planned to be done for Moodle 2.3. Regarding 3. Where should this notice appear? Options are: a. only during install/upgrade b. on any course page when editing is on c. something else. Do we need to add an extra task: 3.5. Write upgrade code to convert the old module security settings into permission overrides? I really hope not. I think it is sufficient to warn about these changes in the Moodle 2.3 release notes.
          Hide
          Davo Smith added a comment -

          Tim - it might be worth putting the code for 1. at the top of the 'course_allowed_module' in course/lib.php as that should cover all situations where there needs to be a check if a module can be added (at least that is what I've done when I was asked to implement a similar feature on a 1.9 and a 2.2 site yesterday).

          For 3. I'd, personally, want to add a developer only debug message to the course page, to nag them into fixing it, but default to allowing the module to be added if the capability does not exist ( if (!get_capability_info($capabilityname)) ).

          And as an aside, the suggestion in the forums to automatically add the capability to modules where it doesn't exist turns out to be a bad idea in terms of lang files - visiting the 'define roles' page produces a long list of developer warnings as the 'MODNAME:addinstance' strings don't exist in the modules concerned.

          Show
          Davo Smith added a comment - Tim - it might be worth putting the code for 1. at the top of the 'course_allowed_module' in course/lib.php as that should cover all situations where there needs to be a check if a module can be added (at least that is what I've done when I was asked to implement a similar feature on a 1.9 and a 2.2 site yesterday). For 3. I'd, personally, want to add a developer only debug message to the course page, to nag them into fixing it, but default to allowing the module to be added if the capability does not exist ( if (!get_capability_info($capabilityname)) ). And as an aside, the suggestion in the forums to automatically add the capability to modules where it doesn't exist turns out to be a bad idea in terms of lang files - visiting the 'define roles' page produces a long list of developer warnings as the 'MODNAME:addinstance' strings don't exist in the modules concerned.
          Hide
          Tim Hunt added a comment -

          Thanks Davo for the tip. That saved me some time.

          Is that really enough to cover all code paths I wonder? (e.g. backup/restore, web services etc. I guess it should be.)

          I agree about maximum nagging. The most natural place to add the debug code is in course_allowed_module - I just need to check where that makes the debug message appear.

          And I agree about not doing it automatically.

          Show
          Tim Hunt added a comment - Thanks Davo for the tip. That saved me some time. Is that really enough to cover all code paths I wonder? (e.g. backup/restore, web services etc. I guess it should be.) I agree about maximum nagging. The most natural place to add the debug code is in course_allowed_module - I just need to check where that makes the debug message appear. And I agree about not doing it automatically.
          Hide
          Tim Hunt added a comment -

          I think this is a working implementation. Can anyone peer review it for me please?

          Note that course_allowed_module is used on all relevant code paths. (There are currently no web services that add an activity of any type, and Eloy confirmed that we should not be calling it from restore code.)

          And, in the end, I did implement upgrade.

          Show
          Tim Hunt added a comment - I think this is a working implementation. Can anyone peer review it for me please? Note that course_allowed_module is used on all relevant code paths. (There are currently no web services that add an activity of any type, and Eloy confirmed that we should not be calling it from restore code.) And, in the end, I did implement upgrade.
          Hide
          Tim Hunt added a comment -

          Docs required:

          QA tests required:

          • Remove any existing test cases for 'Module security'
          • Add a new test case, checking that turning off this capability for any module stops a teacher adding the module.
          Show
          Tim Hunt added a comment - Docs required: Add to release notes In the 2.3 equivalent of http://docs.moodle.org/22/en/Managing_activities#Module_security , change it to say use the capabilities instead. Document the new capabilities in the user docs. In the developer docs for writing a new mod, mention the new capability. QA tests required: Remove any existing test cases for 'Module security' Add a new test case, checking that turning off this capability for any module stops a teacher adding the module.
          Hide
          Dan Poltawski added a comment -

          Hi Tim,

          Looks good! I can't really find fault as usual..

          Some comments:

          • You are not checking a capabilities existence when calling assign_capability() to prevent access on upgrade/restore. As we are pretty certain there are going to be third-party modules without the addinstance capability have you checked if that has any impact? Does assign_capabilty just ignore it?
          • I think $string['imscp:addinstance'] should be 'Add a new IMS content package'
          • It might be nice to phpdoc $legacyrestrictmodules and $legacyallowedmodules in restore_course_structure_step
          Show
          Dan Poltawski added a comment - Hi Tim, Looks good! I can't really find fault as usual.. Some comments: You are not checking a capabilities existence when calling assign_capability() to prevent access on upgrade/restore. As we are pretty certain there are going to be third-party modules without the addinstance capability have you checked if that has any impact? Does assign_capabilty just ignore it? I think $string ['imscp:addinstance'] should be 'Add a new IMS content package' It might be nice to phpdoc $legacyrestrictmodules and $legacyallowedmodules in restore_course_structure_step
          Hide
          Tim Hunt added a comment -

          Thanks for the quick review Dan. (Can I just say that when reviewing, it is better to use numbered lists, not bullet lists. That makes it easier to respond to individual points.)

          2. and 3. are good changes. I will do them today.

          1. is the key one. I know I am not checking that capabilities exist before calling assign_capability. There are two reasons for this:

          a. During upgrade, system is done before mods, so actually at this point in the upgrade script, the capabilities don't exist in the DB yet!

          b. Even if the upgrade order was different, consider this scenario: site with complex module security restrictions and custom mods is upgraded to 2.3; only then do they see the warnings about needing a new capability and add them to their mods. With the current code, the overrides that correspond to the old module security settings will already be there, and will start working. If we did not do it the way I have done, there would be dataloss.

          It seems that sometimes there are advantages to not having referential integrity enforced

          Show
          Tim Hunt added a comment - Thanks for the quick review Dan. (Can I just say that when reviewing, it is better to use numbered lists, not bullet lists. That makes it easier to respond to individual points.) 2. and 3. are good changes. I will do them today. 1. is the key one. I know I am not checking that capabilities exist before calling assign_capability. There are two reasons for this: a. During upgrade, system is done before mods, so actually at this point in the upgrade script, the capabilities don't exist in the DB yet! b. Even if the upgrade order was different, consider this scenario: site with complex module security restrictions and custom mods is upgraded to 2.3; only then do they see the warnings about needing a new capability and add them to their mods. With the current code, the overrides that correspond to the old module security settings will already be there, and will start working. If we did not do it the way I have done, there would be dataloss. It seems that sometimes there are advantages to not having referential integrity enforced
          Hide
          Michael Aherne added a comment -

          Works great! The only (very minor) issue I noticed is a couple of typos in course/lib.php (lines 3313 & 3318).

          Show
          Michael Aherne added a comment - Works great! The only (very minor) issue I noticed is a couple of typos in course/lib.php (lines 3313 & 3318).
          Hide
          Tim Hunt added a comment -

          Thanks for looking at it Michael.

          Branch amended to fix the issues that Dan and Michael found.

          Show
          Tim Hunt added a comment - Thanks for looking at it Michael. Branch amended to fix the issues that Dan and Michael found.
          Hide
          Tim Hunt added a comment -

          I have rebased this.

          Submitting for integration now.

          To INEGRATORS: please be warned that this was a horrible rebase to have to do, because of the UNSIGNED chagnes last week. I hope everything has been sorted out OK. If not, please accept my apologies.

          Show
          Tim Hunt added a comment - I have rebased this. Submitting for integration now. To INEGRATORS: please be warned that this was a horrible rebase to have to do, because of the UNSIGNED chagnes last week. I hope everything has been sorted out OK. If not, please accept my apologies.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Some hours ago...

          the main moodle.git repository has 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 - Some hours ago... the main moodle.git repository has 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
          Eloy Lafuente (stronk7) added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.
          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

          Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          Hide
          Martin Dougiamas added a comment -

          +1 to move to 100% capabilities for control of adding each activity module.

          Show
          Martin Dougiamas added a comment - +1 to move to 100% capabilities for control of adding each activity module.
          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
          Tim Hunt added a comment -

          Rebased (again).

          Show
          Tim Hunt added a comment - Rebased (again).
          Hide
          Sam Hemelryk added a comment -

          Hi Tim,

          Awesome effort, this has just been integrated now.
          During my review I noted a handful of things and after integration added a commit to fix up two inline commets (typo, and relevance).
          I also noted the following, while not blocking integration I would like to get your thoughts on them.

          1. Lang string modulesecurity,admin appears to be no longer used. Can it be removed now.
          2. Argument change to course_allowed_module should be mentioned somewhere
          3. I am wondering what the effect of calling assign_capability is for a capability that doesn't exist. For instance when upgrading a site with restricted modules that have not yet defined the addinstance capability. Looking though code it appears the record will be created and will be an orphan. I suppose this is the desired effect in this case as eventually all modules should be updated (and thanks to the debugging notice will be)

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Tim, Awesome effort, this has just been integrated now. During my review I noted a handful of things and after integration added a commit to fix up two inline commets (typo, and relevance). I also noted the following, while not blocking integration I would like to get your thoughts on them. Lang string modulesecurity,admin appears to be no longer used. Can it be removed now. Argument change to course_allowed_module should be mentioned somewhere I am wondering what the effect of calling assign_capability is for a capability that doesn't exist. For instance when upgrading a site with restricted modules that have not yet defined the addinstance capability. Looking though code it appears the record will be created and will be an orphan. I suppose this is the desired effect in this case as eventually all modules should be updated (and thanks to the debugging notice will be) Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Just reading back I've seen the conversation Dan and yourself had about point 3, as its been discussed good as gold!

          Show
          Sam Hemelryk added a comment - Just reading back I've seen the conversation Dan and yourself had about point 3, as its been discussed good as gold!
          Hide
          Sam Hemelryk added a comment -

          Made a very quick commit just now, there was a typo in upgrade.php (retrictmodules) ... oops!
          Now fixed, anyone who has upgraded in the last hour will still have the coure.restrictmodules field.

          Show
          Sam Hemelryk added a comment - Made a very quick commit just now, there was a typo in upgrade.php (retrictmodules) ... oops! Now fixed, anyone who has upgraded in the last hour will still have the coure.restrictmodules field.
          Hide
          Tim Hunt added a comment -

          1. Yes, those lang strings are now dead, and should be been removed. Do you want to create a new MDL issue?

          2. I really don't think so. This API is only used in a few places in core code, and only one of them was using numerical arguments. I don't believe anyone outside core would be calling this API. Well, they might, but it is really unlikely, so I think it is over-the-top to add it to the release notes, etc.

          3. You saw the above discussion.

          4. Oops. That is the problem with our if (column_exists) logic in upgrade scripts. It hides errors like this. This one is odd. I am sure I tested the upgrade, including looking at the updated DB table definitions.

          Thanks for sorting out the mess.

          Show
          Tim Hunt added a comment - 1. Yes, those lang strings are now dead, and should be been removed. Do you want to create a new MDL issue? 2. I really don't think so. This API is only used in a few places in core code, and only one of them was using numerical arguments. I don't believe anyone outside core would be calling this API. Well, they might, but it is really unlikely, so I think it is over-the-top to add it to the release notes, etc. 3. You saw the above discussion. 4. Oops. That is the problem with our if (column_exists) logic in upgrade scripts. It hides errors like this. This one is odd. I am sure I tested the upgrade, including looking at the updated DB table definitions. Thanks for sorting out the mess.
          Hide
          Petr Škoda added a comment -
          +    if ($oldversion < 2012031500.01) {
          +        // Upgrade old course_allowed_modules data to be permission overrides.
          +        if ($CFG->restrictmodulesfor = 'all') {
          +            $courses = $DB->get_records_menu('course', array(), 'id', 'id, 1');
          +        } else if ($CFG->restrictmodulesfor = 'requested') {
          +            $courses = $DB->get_records_menu('course', array('retrictmodules' => 1), 'id', 'id, 1');
          +        } else {
          +            $courses = array();
          +        }
          

          I suppose the assignment in "ifs" is a sloppy typo, right?

          Show
          Petr Škoda added a comment - + if ($oldversion < 2012031500.01) { + // Upgrade old course_allowed_modules data to be permission overrides. + if ($CFG->restrictmodulesfor = 'all') { + $courses = $DB->get_records_menu('course', array(), 'id', 'id, 1'); + } else if ($CFG->restrictmodulesfor = 'requested') { + $courses = $DB->get_records_menu('course', array('retrictmodules' => 1), 'id', 'id, 1'); + } else { + $courses = array(); + } I suppose the assignment in "ifs" is a sloppy typo, right?
          Hide
          Petr Škoda added a comment -

          Next time it might be better to not use assign_capability(), get_roles_with_cap_in_context() and contexts in general in core upgrade (it is ok in plugins because roles must be fully upgraded there). The recommended way is to use low level SQL. The reason is if we change roles internals all this could break, hopefully we will not change rewrite accesslib anytime soon...

          Show
          Petr Škoda added a comment - Next time it might be better to not use assign_capability(), get_roles_with_cap_in_context() and contexts in general in core upgrade (it is ok in plugins because roles must be fully upgraded there). The recommended way is to use low level SQL. The reason is if we change roles internals all this could break, hopefully we will not change rewrite accesslib anytime soon...
          Hide
          Sam Hemelryk added a comment -

          Thanks for looking Petr, I've made an additional commit now (6f3570f) to fix the assignment typo.

          Show
          Sam Hemelryk added a comment - Thanks for looking Petr, I've made an additional commit now (6f3570f) to fix the assignment typo.
          Hide
          Tim Hunt added a comment -

          Bum! how many different people reviewed the code (especially me) and did not see that.

          Thank you Petr and Sam.

          And, good point about being careful with API functions.

          Show
          Tim Hunt added a comment - Bum! how many different people reviewed the code (especially me) and did not see that. Thank you Petr and Sam. And, good point about being careful with API functions.
          Hide
          Rossiani Wijaya added a comment -

          Hi Tim,

          Question with step 2 from testing instruction. Should it be done from 2.2 or 2.3?

          If i'm not mistaken, mod/chat:addinstance only occurs in 2.3. The next question is adding database. could you be more specific on this? I'm pretty sure it is not the database activity correct? If it is database activity, then the test is failed because with mod/chat:addinstance disabled I still can add db activity.

          Show
          Rossiani Wijaya added a comment - Hi Tim, Question with step 2 from testing instruction. Should it be done from 2.2 or 2.3? If i'm not mistaken, mod/chat:addinstance only occurs in 2.3. The next question is adding database. could you be more specific on this? I'm pretty sure it is not the database activity correct? If it is database activity, then the test is failed because with mod/chat:addinstance disabled I still can add db activity.
          Hide
          Tim Hunt added a comment -

          All this code is master-only. Look at the fix version.

          Testing instructions updated to fix the silly typo, and make them a bit clearer.

          Show
          Tim Hunt added a comment - All this code is master-only. Look at the fix version. Testing instructions updated to fix the silly typo, and make them a bit clearer.
          Hide
          Rossiani Wijaya added a comment -

          Hi Tim,

          I'm failing this issue because restoring from 1.9 to 2.x doesn't limit teacher's capability to add module. (I tried restoring from 1.9 to 2.0/2.1/2.2/2.3, none of them work).

          The rest of testing instructions are good.

          Show
          Rossiani Wijaya added a comment - Hi Tim, I'm failing this issue because restoring from 1.9 to 2.x doesn't limit teacher's capability to add module. (I tried restoring from 1.9 to 2.0/2.1/2.2/2.3, none of them work). The rest of testing instructions are good.
          Hide
          Tim Hunt added a comment -

          Does backup and restore from a 2.0 or 2.1 site work?

          The problem going from 1.9 is most likely to be with the change of backup format between 1.9 and 2.0, not with my changes. If so, that is a separate bug.

          Show
          Tim Hunt added a comment - Does backup and restore from a 2.0 or 2.1 site work? The problem going from 1.9 is most likely to be with the change of backup format between 1.9 and 2.0, not with my changes. If so, that is a separate bug.
          Hide
          Tim Hunt added a comment -

          Looking at moodle1_course_header_handler in backup/converter/moodle1/handlerlib.php it seems to hard-code 'restrictmodules' => 0.

          So, that looks like a restore-from-1,9 bug to me. Not a bug with my new code. I suggest you file a new but for that "Module security settings lost when restoring from 1.9 -> 2.0". Steps 4. and 5. of the testing instructions show that restore works from 2.0/1/2 -> 2.3.

          Show
          Tim Hunt added a comment - Looking at moodle1_course_header_handler in backup/converter/moodle1/handlerlib.php it seems to hard-code 'restrictmodules' => 0. So, that looks like a restore-from-1,9 bug to me. Not a bug with my new code. I suggest you file a new but for that "Module security settings lost when restoring from 1.9 -> 2.0". Steps 4. and 5. of the testing instructions show that restore works from 2.0/1/2 -> 2.3.
          Hide
          Rossiani Wijaya added a comment -

          Hi Tim,

          Thank for the feedback. I spoke with Eloy and suggested to fix the restore issue on new issue.

          The new issue to address the restore issue is available at: MDL-32158.

          I will pass this issue once it back to testing.

          Show
          Rossiani Wijaya added a comment - Hi Tim, Thank for the feedback. I spoke with Eloy and suggested to fix the restore issue on new issue. The new issue to address the restore issue is available at: MDL-32158 . I will pass this issue once it back to testing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          reset

          Show
          Eloy Lafuente (stronk7) added a comment - reset
          Hide
          Rossiani Wijaya added a comment -

          Passing test.

          Thanks everyone.

          Show
          Rossiani Wijaya added a comment - Passing test. Thanks everyone.
          Hide
          Tim Hunt added a comment -

          Yay! Thanks everyone who helped with this.

          Show
          Tim Hunt added a comment - Yay! Thanks everyone who helped with this.
          Hide
          Sam Hemelryk added a comment -

          Congratulations are in order, you've made it, or at least your code has!
          It's now part of Moodle and both the git and cvs repositories have been updated.

          This issue is being marked as fixed and closed.

          Thank you.

          Show
          Sam Hemelryk added a comment - Congratulations are in order, you've made it, or at least your code has! It's now part of Moodle and both the git and cvs repositories have been updated. This issue is being marked as fixed and closed. Thank you.
          Hide
          Helen Foster added a comment -

          Just noting that on http://qa.moodle.net the roles of manager and teacher needed resetting to defaults in order for the add a resource and add an activity drop down menus to appear.

          Show
          Helen Foster added a comment - Just noting that on http://qa.moodle.net the roles of manager and teacher needed resetting to defaults in order for the add a resource and add an activity drop down menus to appear.
          Hide
          Tim Hunt added a comment -

          I am worried about this. The definitions of the new capabilities looks like

              'mod/assignment:addinstance' => array(
                  'riskbitmask' => RISK_XSS,
          
                  'captype' => 'write',
                  'contextlevel' => CONTEXT_COURSE,
                  'archetypes' => array(
                      'editingteacher' => CAP_ALLOW,
                      'manager' => CAP_ALLOW
                  ),
                  'clonepermissionsfrom' => 'moodle/course:manageactivities'
              ),
          

          I thought that meant that on upgrade, the mod/assignment:addinstance capability would be set to the same thing as the moodle/course:manageactivities capability, for all roles.

          However, we observed the same problem when upgrading the OU's site. No roles got the :addinstance capabilities, and we had to edit the roles by hand.

          I am afraid that I don't have time to investigate this now, but it would be really good if someone could.

          Show
          Tim Hunt added a comment - I am worried about this. The definitions of the new capabilities looks like 'mod/assignment:addinstance' => array( 'riskbitmask' => RISK_XSS, 'captype' => 'write', 'contextlevel' => CONTEXT_COURSE, 'archetypes' => array( 'editingteacher' => CAP_ALLOW, 'manager' => CAP_ALLOW ), 'clonepermissionsfrom' => 'moodle/course:manageactivities' ), I thought that meant that on upgrade, the mod/assignment:addinstance capability would be set to the same thing as the moodle/course:manageactivities capability, for all roles. However, we observed the same problem when upgrading the OU's site. No roles got the :addinstance capabilities, and we had to edit the roles by hand. I am afraid that I don't have time to investigate this now, but it would be really good if someone could.
          Hide
          Petr Škoda added a comment -

          this is caused by the fact that you can clone in one plugin only, I am going to add possibility to clone from core too - see MDL-32477

          Show
          Petr Škoda added a comment - this is caused by the fact that you can clone in one plugin only, I am going to add possibility to clone from core too - see MDL-32477
          Hide
          Mary Cooch added a comment -

          This was documented, so removing the label.

          Show
          Mary Cooch added a comment - This was documented, so removing the label.
          Hide
          Tim Hunt added a comment -

          Just updating the bug summary so this is actually findable.

          Show
          Tim Hunt added a comment - Just updating the bug summary so this is actually findable.
          Hide
          nitin sale added a comment -

          HI....how r u ?

          Show
          nitin sale added a comment - HI....how r u ?

            People

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

              Dates

              • Created:
                Updated:
                Resolved: