Uploaded image for project: '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

      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.

        Gliffy Diagrams

        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
            mjollnir 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
            mjollnir 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
            adisch 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
            adisch 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
            timhunt Tim Hunt added a comment -

            Added UI Mockup: <Possible admin interface>

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

            Added UI Mockup: <Possible course settings UI>

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

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

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

            Added UI Mockup: <Alter roles by capability>

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

            Added UI Mockup: <Alter roles by capability 2>

            Show
            timhunt Tim Hunt added a comment - Added UI Mockup: <Alter roles by capability 2>
            Hide
            timhunt 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
            timhunt 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
            davosmith 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
            davosmith 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
            timhunt 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
            timhunt 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
            timhunt 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
            timhunt 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
            timhunt 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
            timhunt 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
            poltawski 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
            poltawski 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
            timhunt 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
            timhunt 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
            maherne 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
            maherne 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
            timhunt Tim Hunt added a comment -

            Thanks for looking at it Michael.

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

            Show
            timhunt Tim Hunt added a comment - Thanks for looking at it Michael. Branch amended to fix the issues that Dan and Michael found.
            Hide
            timhunt 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
            timhunt 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
            stronk7 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
            stronk7 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
            stronk7 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
            stronk7 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
            dougiamas Martin Dougiamas added a comment -

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

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

            Rebased (again).

            Show
            timhunt Tim Hunt added a comment - Rebased (again).
            Hide
            samhemelryk 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
            samhemelryk 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
            samhemelryk 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
            samhemelryk 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
            samhemelryk 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
            samhemelryk 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
            timhunt 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
            timhunt 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            samhemelryk Sam Hemelryk added a comment -

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

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks for looking Petr, I've made an additional commit now (6f3570f) to fix the assignment typo.
            Hide
            timhunt 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
            timhunt 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
            rwijaya 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
            rwijaya 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
            timhunt 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
            timhunt 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
            rwijaya 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
            rwijaya 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
            timhunt 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
            timhunt 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
            timhunt 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
            timhunt 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
            rwijaya 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
            rwijaya 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
            stronk7 Eloy Lafuente (stronk7) added a comment -

            reset

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

            Passing test.

            Thanks everyone.

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

            Yay! Thanks everyone who helped with this.

            Show
            timhunt Tim Hunt added a comment - Yay! Thanks everyone who helped with this.
            Hide
            samhemelryk 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
            samhemelryk 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
            tsala 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
            tsala 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
            timhunt 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
            timhunt 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            marycooch Mary Cooch added a comment -

            This was documented, so removing the label.

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

            Just updating the bug summary so this is actually findable.

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

            HI....how r u ?

            Show
            nitinsale139 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:
                  Fix Release Date:
                  25/Jun/12