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
    • Rank:
      42623

      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.

        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

            People

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

              Dates

              • Created:
                Updated:
                Resolved: