Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.1
    • Fix Version/s: 2.4
    • Component/s: Blocks
    • Testing Instructions:
      Hide
      1. Create a 2.3 Moodle site.
      2. Create a new role called 'Manage blocks role ALL' with no archetype.
      3. Grant the following capabilities 'moodle/my:manageblocks' and 'moodle/site:manageblocks'.
      4. Create another role called 'Manage blocks role MY' with no archetype.
      5. Grant the capability 'moodle/my:manageblocks'.
      6. Create another role called 'Manage blocks role SITE' with no archetype.
      7. Grant the capability 'moodle/site:manageblocks'.
      8. Upgrade to the latest integration master branch.
      9. Check that 'Manage blocks role ALL' has allow for the myaddinstance and addinstance capability for each block.
      10. Check that 'Manage blocks role MY' has allow for only the myaddinstance capability for each block.
      11. Check that 'Manage blocks role SITE' has allow for only the addinstance capability for each block.
      12. Log in as a student.
      13. Visit the My Moodle page.
      14. Check that you can add/remove blocks.
      15. Remove some blocks, but not all.
      16. Log in as an administrator on separate browser.
      17. Go to Users -> Permissions -> Define roles and select authenticated users.
      18. Disallow the user to add the blocks you deleted as the student by unselecting allow for the myaddinstance capability, eg. block/<blockname>:myaddinstance.
      19. Go back to the user's My Moodle page and confirm you can not add these blocks.
      20. Visit a course as this student and ensure they can not add a block.
      21. As the administrator visit Users -> Permissions -> Define roles and select authenticated users.
      22. Allow the user to add some blocks by selecting allow for the addinstance capability (note. different than myaddinstance), eg. block/<blockname>:addinstance.
      23. Allow the user to 'Edit a block's settings' (moodle/block:edit) and 'Manage blocks on a page' (moodle/site:manageblocks).
      24. Go back to the user and visit that course.
      25. Check that you can add blocks to the course, but only those you specified.

      Note, if you remove the myaddinstance capability for a block currently on the users page, then they will not be able to remove it. This seems unavoidable without creating a dirty hack.

      Show
      Create a 2.3 Moodle site. Create a new role called 'Manage blocks role ALL' with no archetype. Grant the following capabilities 'moodle/my:manageblocks' and 'moodle/site:manageblocks'. Create another role called 'Manage blocks role MY' with no archetype. Grant the capability 'moodle/my:manageblocks'. Create another role called 'Manage blocks role SITE' with no archetype. Grant the capability 'moodle/site:manageblocks'. Upgrade to the latest integration master branch. Check that 'Manage blocks role ALL' has allow for the myaddinstance and addinstance capability for each block. Check that 'Manage blocks role MY' has allow for only the myaddinstance capability for each block. Check that 'Manage blocks role SITE' has allow for only the addinstance capability for each block. Log in as a student. Visit the My Moodle page. Check that you can add/remove blocks. Remove some blocks, but not all. Log in as an administrator on separate browser. Go to Users -> Permissions -> Define roles and select authenticated users. Disallow the user to add the blocks you deleted as the student by unselecting allow for the myaddinstance capability, eg. block/<blockname>:myaddinstance. Go back to the user's My Moodle page and confirm you can not add these blocks. Visit a course as this student and ensure they can not add a block. As the administrator visit Users -> Permissions -> Define roles and select authenticated users. Allow the user to add some blocks by selecting allow for the addinstance capability (note. different than myaddinstance), eg. block/<blockname>:addinstance. Allow the user to 'Edit a block's settings' (moodle/block:edit) and 'Manage blocks on a page' (moodle/site:manageblocks). Go back to the user and visit that course. Check that you can add blocks to the course, but only those you specified. Note, if you remove the myaddinstance capability for a block currently on the users page, then they will not be able to remove it. This seems unavoidable without creating a dirty hack.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-34270_master

      Description

      We love the possibilities created in MDL-19125 with mod/foo:addinstance - think it's required in blocks too though!

      When releasing new integrations, or new blocks, it's often handy to give access only to a pilot group of people and a block/foo:addinstance capability would fit perfectly there.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Sam Marshall added a comment -

            The OU would appreciate this feature - as described, but especially if not having 'addinstance' would also prevent you from REMOVING a particular block.

            (We have problems with people deleting blocks they shouldn't, and can't stop that in 2.2 without preventing them adding/deleting any block.)

            Show
            Sam Marshall added a comment - The OU would appreciate this feature - as described, but especially if not having 'addinstance' would also prevent you from REMOVING a particular block. (We have problems with people deleting blocks they shouldn't, and can't stop that in 2.2 without preventing them adding/deleting any block.)
            Hide
            Andrew Nicols added a comment -

            Do you propose an addinstance and a removeinstance, or do you think addinstance should also cover remove?

            Show
            Andrew Nicols added a comment - Do you propose an addinstance and a removeinstance, or do you think addinstance should also cover remove?
            Hide
            Michael de Raadt added a comment -

            Tim: You were involved in the revamp of course module restrictions so I thought you might be interested in this issue also.

            Show
            Michael de Raadt added a comment - Tim: You were involved in the revamp of course module restrictions so I thought you might be interested in this issue also.
            Hide
            Tim Hunt added a comment -

            Thanks Michael. I think one capability should control both add and remove. A good general principle is: don't give people permissions to make a mistake that they cannot fix. (Therefore, addinstance might not be the best name, but using the same name as for modules is good. Perhpas you should not be allowed to delete a module if you are no allowed to re-add a replacement of the same type, but that is another issue.)

            Show
            Tim Hunt added a comment - Thanks Michael. I think one capability should control both add and remove. A good general principle is: don't give people permissions to make a mistake that they cannot fix. (Therefore, addinstance might not be the best name, but using the same name as for modules is good. Perhpas you should not be allowed to delete a module if you are no allowed to re-add a replacement of the same type, but that is another issue.)
            Hide
            Mark Nelson added a comment -

            I am going to try and get this done before code freeze.

            Show
            Mark Nelson added a comment - I am going to try and get this done before code freeze.
            Hide
            Martin Dougiamas added a comment -

            My +1 for creating addinstance for blocks.

            However, we need to also be careful not to break legacy blocks ... if a block doesn't have the capability defined then we proceed as now and ALLOW people to add it.

            Obviously current manageblocks capabilities needs to continue to be respected as well.

            Show
            Martin Dougiamas added a comment - My +1 for creating addinstance for blocks. However, we need to also be careful not to break legacy blocks ... if a block doesn't have the capability defined then we proceed as now and ALLOW people to add it. Obviously current manageblocks capabilities needs to continue to be respected as well.
            Hide
            Tim Hunt added a comment -

            That is how mod addinstance worked. No capability => allow, but output a developer debug warning to encourage the developer to add the capability.

            And you need both mod/...:addinstance and course:manageactivities to add a given type of module.

            Show
            Tim Hunt added a comment - That is how mod addinstance worked. No capability => allow, but output a developer debug warning to encourage the developer to add the capability. And you need both mod/...:addinstance and course:manageactivities to add a given type of module.
            Hide
            Mark Nelson added a comment -

            Ok finished. Have to say this makes adding blocks very powerful. Not only does this address the issue with users adding certain blocks to their My Moodle page that they shouldn't, it also allows users to add only specified blocks to courses which I think is very cool.

            Show
            Mark Nelson added a comment - Ok finished. Have to say this makes adding blocks very powerful. Not only does this address the issue with users adding certain blocks to their My Moodle page that they shouldn't, it also allows users to add only specified blocks to courses which I think is very cool.
            Hide
            Mark Nelson added a comment -

            When reviewing please check the comments are correct as well as the capabilities names. I did do this myself but there are a lot of changes, so may have missed one in my eager state to finish.

            Eg. for blocks/myprofile/db/access.php

            Make sure the top comment is appropriate and the package is correct -

            My profile block caps.
            @package block_myprofile

            Check the capabilities are named correctly as well -

            block/myprofile:myaddinstance
            block/myprofile:addinstance

            Thanks!

            Show
            Mark Nelson added a comment - When reviewing please check the comments are correct as well as the capabilities names. I did do this myself but there are a lot of changes, so may have missed one in my eager state to finish. Eg. for blocks/myprofile/db/access.php Make sure the top comment is appropriate and the package is correct - My profile block caps. @package block_myprofile Check the capabilities are named correctly as well - block/myprofile:myaddinstance block/myprofile:addinstance Thanks!
            Hide
            Adrian Greeve added a comment -

            [Y] Syntax
            [Y] Output
            [Y] Whitespace
            [N] Language
            [-] Databases
            [N] Testing
            [Y] Security
            [Y] Documentation
            [Y] Git
            [Y] Sanity check

            Hi Mark,

            Everything in general looks really good. Just a few minor things:

            • blocks/tag_youtube/lang/en/block_tag_youtube.php
              line 46 string['tag_youtube:myaddinstance'] = 'Add a new flickr block to the My Moodle page'; You may want to replace flickr with youtube.
            • Testing instructions
              Manage blocks on a page:
              which capability? moodle/my:manageblocks, moodle/user:manageownblocks, moodel/user:manageblocks, moodle/site:manageblocks
              May not be immediately obvious.

            Thanks.

            Show
            Adrian Greeve added a comment - [Y] Syntax [Y] Output [Y] Whitespace [N] Language [-] Databases [N] Testing [Y] Security [Y] Documentation [Y] Git [Y] Sanity check Hi Mark, Everything in general looks really good. Just a few minor things: blocks/tag_youtube/lang/en/block_tag_youtube.php line 46 string ['tag_youtube:myaddinstance'] = 'Add a new flickr block to the My Moodle page'; You may want to replace flickr with youtube. Testing instructions Manage blocks on a page: which capability? moodle/my:manageblocks, moodle/user:manageownblocks, moodel/user:manageblocks, moodle/site:manageblocks May not be immediately obvious. Thanks.
            Hide
            Mark Nelson added a comment -

            Thanks Adrian for spotting that typo! I have made the changes you suggested.

            Show
            Mark Nelson added a comment - Thanks Adrian for spotting that typo! I have made the changes you suggested.
            Hide
            Aparup Banerjee 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
            Aparup Banerjee 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
            Dan Poltawski 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
            Dan Poltawski 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
            Dan Poltawski added a comment -

            Hi Mark,

            This looks good, there are just a few things i'm not sure about:

            1. has_add_block_capability looks like it needs a static $warned. Else that warned array will be reset each time its called and never do anything?
            2. Is has_add_block_capabilty the right name for this? Since it can be any capability name, it could be more generic than 'add'.
            3. The riskbitmap you are using is the same for each block and different for mymoodle and the normal add instance capabilities. I don't think its correct. (I'm happy for this to be corrected after integration, though)
            Show
            Dan Poltawski added a comment - Hi Mark, This looks good, there are just a few things i'm not sure about: has_add_block_capability looks like it needs a static $warned. Else that warned array will be reset each time its called and never do anything? Is has_add_block_capabilty the right name for this? Since it can be any capability name, it could be more generic than 'add'. The riskbitmap you are using is the same for each block and different for mymoodle and the normal add instance capabilities. I don't think its correct. (I'm happy for this to be corrected after integration, though)
            Hide
            Mark Nelson added a comment -

            Hi Dan,

            1 - I have fixed this, you make a valid point.
            2 - has_add_block_capability is only called in the function user_can_addto(), so the capability passed is either going to be 'block/<blockname>:addinstance' or 'block/<blockname>:myaddinstance', so I think this name is appropriate. However, I may have not understood what you meant so please correct me if I am wrong.
            3 - I copied the capabilities from lib/db/access.php for the capabilities 'moodle/site:manageblocks' and 'moodle/my:manageblocks'. I believe the riskbitmask is only present in the site:manageblocks because other users will be able to see the block, where as on the mymoodle page it is limited to that user - they are only going to be spamming themselves.

            Show
            Mark Nelson added a comment - Hi Dan, 1 - I have fixed this, you make a valid point. 2 - has_add_block_capability is only called in the function user_can_addto(), so the capability passed is either going to be 'block/<blockname>:addinstance' or 'block/<blockname>:myaddinstance', so I think this name is appropriate. However, I may have not understood what you meant so please correct me if I am wrong. 3 - I copied the capabilities from lib/db/access.php for the capabilities 'moodle/site:manageblocks' and 'moodle/my:manageblocks'. I believe the riskbitmask is only present in the site:manageblocks because other users will be able to see the block, where as on the mymoodle page it is limited to that user - they are only going to be spamming themselves.
            Hide
            Dan Poltawski added a comment -

            Thanks Mark. Regarding the function name, I think it could be more generic, but I don't think this is that important.

            I've integrated this to master now, go go testers!

            cheers

            Show
            Dan Poltawski added a comment - Thanks Mark. Regarding the function name, I think it could be more generic, but I don't think this is that important. I've integrated this to master now, go go testers! cheers
            Hide
            Dan Poltawski added a comment -

            Hi Mark,

            I realised over the weekend that these capabilities are missing clonepermissionsfrom definition in the capability.

            If someone has created a 'block manager role' which has moodle/site:manageblocks and isn't using an archetype, when they upgrade to 2.4 their role will be broken. We should make sure that things don't get broken and use clonepermissionsfrom to give the permisisons to any role which has moodle/site:manageblocks.

            Show
            Dan Poltawski added a comment - Hi Mark, I realised over the weekend that these capabilities are missing clonepermissionsfrom definition in the capability. If someone has created a 'block manager role' which has moodle/site:manageblocks and isn't using an archetype, when they upgrade to 2.4 their role will be broken. We should make sure that things don't get broken and use clonepermissionsfrom to give the permisisons to any role which has moodle/site:manageblocks.
            Hide
            Mark Nelson added a comment -

            Good point. I will be creating another commit today and will update the testing instructions. Cheers!

            Show
            Mark Nelson added a comment - Good point. I will be creating another commit today and will update the testing instructions. Cheers!
            Hide
            Mark Nelson added a comment -

            Hi Dan, all done. I tested the upgrade myself and all looks good.

            Show
            Mark Nelson added a comment - Hi Dan, all done. I tested the upgrade myself and all looks good.
            Hide
            Dan Poltawski added a comment -

            Perfect, thanks Mark - i've pulled that in now.

            Show
            Dan Poltawski added a comment - Perfect, thanks Mark - i've pulled that in now.
            Hide
            Ankit Agarwal added a comment -

            Hi Mark,
            As discussed am failing this.
            If a user doesnt have "myaddinstance" but has "addinstance" for a particular block. He is still able to add blocks to /my, which should not be possible.
            Thanks

            Show
            Ankit Agarwal added a comment - Hi Mark, As discussed am failing this. If a user doesnt have "myaddinstance" but has "addinstance" for a particular block. He is still able to add blocks to /my, which should not be possible. Thanks
            Hide
            Mark Nelson added a comment - - edited

            Ankit, nicely spotted. It is an issue with the previous code logic. If the user had the capability moodle/block:edit but not moodle/my:manageblocks they would still be able to add the block to the My Moodle page. I have pushed a commit for this to fix master, and will create a separate issue for 2.2 and 2.3.

            [EDIT]
            Not an issue in 2.2 or 2.3 - see MDL-36385

            Show
            Mark Nelson added a comment - - edited Ankit, nicely spotted. It is an issue with the previous code logic. If the user had the capability moodle/block:edit but not moodle/my:manageblocks they would still be able to add the block to the My Moodle page. I have pushed a commit for this to fix master, and will create a separate issue for 2.2 and 2.3. [EDIT] Not an issue in 2.2 or 2.3 - see MDL-36385
            Hide
            Mark Nelson added a comment -

            Hi Dan, when you have time can you pull in my last commit and set this back to testing for poor Ankit.

            Show
            Mark Nelson added a comment - Hi Dan, when you have time can you pull in my last commit and set this back to testing for poor Ankit.
            Hide
            Dan Poltawski added a comment -

            Thanks Mark, i've pulled that in. Ready for testing again.

            Show
            Dan Poltawski added a comment - Thanks Mark, i've pulled that in. Ready for testing again.
            Hide
            Ankit Agarwal added a comment -

            works now.
            Passing
            Thanks

            Show
            Ankit Agarwal added a comment - works now. Passing Thanks
            Hide
            Helen Foster added a comment -

            Just wondering what the point is of having a capability block/login:myaddinstance? Nobody (not even an admin) can add a Login block to My Moodle and it doesn't make sense to be able to do so.

            There are probably more blocks which can't be added to My Moodle, or don't make sense there. Let's check the list and not create new capabilities unnecessarily!

            Show
            Helen Foster added a comment - Just wondering what the point is of having a capability block/login:myaddinstance? Nobody (not even an admin) can add a Login block to My Moodle and it doesn't make sense to be able to do so. There are probably more blocks which can't be added to My Moodle, or don't make sense there. Let's check the list and not create new capabilities unnecessarily!
            Hide
            Dan Poltawski added a comment -

            Very good point, thanks Helen.

            I'm going to reopen this as I think we can fix this up in this round.

            Mark: Lets remove all the block capabilities where applicable_formats() doesn't allow adding on mymoodle?

            Show
            Dan Poltawski added a comment - Very good point, thanks Helen. I'm going to reopen this as I think we can fix this up in this round. Mark: Lets remove all the block capabilities where applicable_formats() doesn't allow adding on mymoodle?
            Hide
            Mark Nelson added a comment -

            Hi Helen, well spotted! You deserve a badge for that. I have added another commit to remove unnecessary capabilities, pull whenever you are ready.

            Show
            Mark Nelson added a comment - Hi Helen, well spotted! You deserve a badge for that. I have added another commit to remove unnecessary capabilities, pull whenever you are ready.
            Hide
            Dan Poltawski added a comment -

            Great job Mark, i've integrated this now.

            Don't think its necessary to re-test this in full, will just run install/upgrade tests.

            Show
            Dan Poltawski added a comment - Great job Mark, i've integrated this now. Don't think its necessary to re-test this in full, will just run install/upgrade tests.
            Hide
            Helen Foster added a comment -

            Thanks a lot Dan and Mark.

            Looking again at adding blocks to My Moodle, I realise there are quite a few blocks which can be added to the page, but make no sense, such as Course completion status and Mentees. I've reported this as MDL-36455. I guess it's rather a long shot to hope that MDL-36455 can be fixed and the unnecessary myaddinstance capabilities for these blocks removed in 2.4?

            Show
            Helen Foster added a comment - Thanks a lot Dan and Mark. Looking again at adding blocks to My Moodle, I realise there are quite a few blocks which can be added to the page, but make no sense, such as Course completion status and Mentees. I've reported this as MDL-36455 . I guess it's rather a long shot to hope that MDL-36455 can be fixed and the unnecessary myaddinstance capabilities for these blocks removed in 2.4?
            Hide
            Helen Foster added a comment -

            I'm removing the qa_test_required label as this issue now has some QA tests: MDLQA-4598, MDLQA-4599 (Note that these are QA test master copies which will be cloned for upcoming QA cycles.)

            If anyone thinks more QA tests are needed for this new feature, please comment and re-add the qa_test_required label.

            Show
            Helen Foster added a comment - I'm removing the qa_test_required label as this issue now has some QA tests: MDLQA-4598 , MDLQA-4599 (Note that these are QA test master copies which will be cloned for upcoming QA cycles.) If anyone thinks more QA tests are needed for this new feature, please comment and re-add the qa_test_required label.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Amazed. Inspired. Grateful. That’s how your generosity makes me feel.

            (not really)

            Closing, thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Amazed. Inspired. Grateful. That’s how your generosity makes me feel. (not really) Closing, thanks!
            Hide
            Helen Foster added a comment -

            Just noting an en lang string typo fix, which I've added to the en_fix lang pack:

            DEV [news_items:myaddinstance,block_news_items] 'Add a new latest news block to the My Moodle page' (was previously 'Add a new navigation block to the My Moodle page')

            (Though hoping that this unnecessary myaddinstance capability together with the others can be removed sooner rather than later...)

            Show
            Helen Foster added a comment - Just noting an en lang string typo fix, which I've added to the en_fix lang pack: DEV [news_items:myaddinstance,block_news_items] 'Add a new latest news block to the My Moodle page' (was previously 'Add a new navigation block to the My Moodle page') (Though hoping that this unnecessary myaddinstance capability together with the others can be removed sooner rather than later...)
            Hide
            Helen Foster added a comment -

            I'm removing the docs_required label as the new block capabilities are documented in various places:

            If I've missed anything out or got anything wrong, please shout!

            Show
            Helen Foster added a comment - I'm removing the docs_required label as the new block capabilities are documented in various places: http://docs.moodle.org/24/en/Capabilities/block/online_users:myaddinstance (and similar for all other capabilities) http://docs.moodle.org/24/en/Managing_blocks http://docs.moodle.org/24/en/My_Moodle If I've missed anything out or got anything wrong, please shout!
            Hide
            Juan Leyva added a comment -
            Show
            Juan Leyva added a comment - I think that this should be mentioned at https://github.com/moodle/moodle/blob/MOODLE_24_STABLE/blocks/upgrade.txt
            Hide
            Dan Poltawski added a comment -

            Juan, you are right. Sorry this has been missed. Mark, could you create an issue and do that?

            Show
            Dan Poltawski added a comment - Juan, you are right. Sorry this has been missed. Mark, could you create an issue and do that?
            Hide
            Mark Nelson added a comment -

            Issue created - MDL-37518

            Show
            Mark Nelson added a comment - Issue created - MDL-37518
            Hide
            Marina Glancy added a comment -
            Show
            Marina Glancy added a comment - FYI MDL-49151

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: