Moodle
  1. Moodle
  2. MDL-30669

'Sticky' site wide blocks in 2.2 can be deleted from the course page (by an admin)

    Details

    • Database:
      Any
    • Testing Instructions:
      Hide
      1. Create a site wide block.
      2. Set page contexts to 'Display throughout the entire site'.
        • Delete this block and check that the delete message contains information about the original location and the display on page types. Check that the information regarding the block location and page types are the following: ('System','Any page').
      3. Create another block on the front page and set the page context to 'Display on the front page and any pages added to the front page'.
        • Delete this block and check that the delete message contains information about the original location and the display on page types. Check that the information regarding the block location and page types are the same as what you configured ('Front page','Display on the front page and any pages added to the front page').
      4. Enter a course and create a course wide block.
      5. Set Display on page types to 'Any page'.
        • Delete this block and check that the delete message contains information about the original location and the display on page types. Check that the information regarding the block location and page types are the same as what you configured ('Course:XXXX','Any page').
      6. Try deleting a block in an activity. Check that no additional error message is displayed.
      Show
      Create a site wide block. Set page contexts to 'Display throughout the entire site'. Delete this block and check that the delete message contains information about the original location and the display on page types. Check that the information regarding the block location and page types are the following: ('System','Any page'). Create another block on the front page and set the page context to 'Display on the front page and any pages added to the front page'. Delete this block and check that the delete message contains information about the original location and the display on page types. Check that the information regarding the block location and page types are the same as what you configured ('Front page','Display on the front page and any pages added to the front page'). Enter a course and create a course wide block. Set Display on page types to 'Any page'. Delete this block and check that the delete message contains information about the original location and the display on page types. Check that the information regarding the block location and page types are the same as what you configured ('Course:XXXX','Any page'). Try deleting a block in an activity. Check that no additional error message is displayed.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-30669-master
    • Rank:
      33487

      Description

      After setting up a Front Page block set to 'Display throughout the site' (i.e. adding a sticky block), the block can be deleted from within a course (if the user is an admin) without any warning that this course-level editing is deleting a block from the entire site. This is in contrast to the 1.9 behaviour, where sticky blocks did not have editing buttons available within the course context.

      Replication steps:
      1. Log in as Admin.
      2. Add a block to the Front Page.
      3. Set block to be visible every where within the site.
      4. Browse to a course.
      5. Turn editing on.
      6. Click the 'delete' button on the site wide block
      -> Block disappears from the entire site.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Can I take your suggestion as an improvement, ie. a warning that a block is being deleted from higher contexts?

          Show
          Michael de Raadt added a comment - Can I take your suggestion as an improvement, ie. a warning that a block is being deleted from higher contexts?
          Hide
          Michael de Raadt added a comment -

          Actually, this really is a bug.

          Show
          Michael de Raadt added a comment - Actually, this really is a bug.
          Hide
          Adrian Greeve added a comment -

          I had a good talk with Aparup and we agreed on a couple of points:

          • When changing the visibility of the block, the parentcontextid (mdl_block_instances) should be updated to represent this change, and
          • When deleting these blocks with higher visibility they should be restricted to the parent context only.
          Show
          Adrian Greeve added a comment - I had a good talk with Aparup and we agreed on a couple of points: When changing the visibility of the block, the parentcontextid (mdl_block_instances) should be updated to represent this change, and When deleting these blocks with higher visibility they should be restricted to the parent context only.
          Hide
          Martin Dougiamas added a comment - - edited

          The block you are seeing in the course really IS the site-wide original block ... it's being displayed exactly as requested in the original. Deleting it as it does makes sense in the model of how blocks are designed.

          Solutions:

          A) Add a warning to the admin:

          "You are about to delete a block that appears everywhere below the original context X, are you sure you want to do this?"

          B) Don't show the delete button at all and force them to look in the block config to find the original context and go there to delete it. Needs to be done carefully to avoid regressions.

          Show
          Martin Dougiamas added a comment - - edited The block you are seeing in the course really IS the site-wide original block ... it's being displayed exactly as requested in the original. Deleting it as it does makes sense in the model of how blocks are designed. Solutions: A) Add a warning to the admin: "You are about to delete a block that appears everywhere below the original context X, are you sure you want to do this?" B) Don't show the delete button at all and force them to look in the block config to find the original context and go there to delete it. Needs to be done carefully to avoid regressions.
          Hide
          Mary Cooch added a comment -

          I prefer option A. As an admin you can access site admin settings from inside a course so why not be able to delete a sitewide block from inside a course (rather than having to go and find it?) With the explanatory warning I think that is enough.

          Show
          Mary Cooch added a comment - I prefer option A. As an admin you can access site admin settings from inside a course so why not be able to delete a sitewide block from inside a course (rather than having to go and find it?) With the explanatory warning I think that is enough.
          Hide
          Tim Hunt added a comment -

          When changing the visibility of the block, the parentcontextid (mdl_block_instances) should be updated to represent this change,

          That is completely wrong. It will cause serious breakage.

          If I could work out what misconception you have about the meaning of block_instances.parentcontextid I would try to explain, but I can't.

          If you describe what you think block_instances.parentcontextid means, then I will try to explain better.

          ------------------

          The OU was encountering this problem, and our response was to fix MDL-35964, which makes the 'Protect block' feature work better.

          ------------------

          I agree with option A). The are-you-sure message when you delete a block should make it clear exactly what you are deleting, to try to help people avoid mistakes. I think we should repeat these 2 settings from the configuration form in the are-you-sure message:

          Where this block appears
          Original block location: ...
          Display on page types: ...

          Show
          Tim Hunt added a comment - When changing the visibility of the block, the parentcontextid (mdl_block_instances) should be updated to represent this change, That is completely wrong. It will cause serious breakage. If I could work out what misconception you have about the meaning of block_instances.parentcontextid I would try to explain, but I can't. If you describe what you think block_instances.parentcontextid means, then I will try to explain better. ------------------ The OU was encountering this problem, and our response was to fix MDL-35964 , which makes the 'Protect block' feature work better. ------------------ I agree with option A). The are-you-sure message when you delete a block should make it clear exactly what you are deleting, to try to help people avoid mistakes. I think we should repeat these 2 settings from the configuration form in the are-you-sure message: Where this block appears Original block location: ... Display on page types: ...
          Hide
          Tim Hunt added a comment -

          Several problems here:

          1. get_class($parentcontext) == 'context_system' is wrong. $parentcontext->contextlevel = CONTEXT_SYSTEM.

          2. The if statement only covers 3 of the 5 possible context levels.

          3. But anyway, hard-coding the context levels is wrong. It would break if we ever added a new type of context in future. Can you change it to use a generic language string that uses context_helper::get_level_name or $parentcontext->get_context_name to fill in the necessary information?

          4. HTML like <strong> in language strings should be avoided if at all possible, and it seems to be possible here.

          5. Using <br /> is also not very semantic HTML. Would be better to wrap each bit of the message in <p> ... </p>.

          Show
          Tim Hunt added a comment - Several problems here: 1. get_class($parentcontext) == 'context_system' is wrong. $parentcontext->contextlevel = CONTEXT_SYSTEM. 2. The if statement only covers 3 of the 5 possible context levels. 3. But anyway, hard-coding the context levels is wrong. It would break if we ever added a new type of context in future. Can you change it to use a generic language string that uses context_helper::get_level_name or $parentcontext->get_context_name to fill in the necessary information? 4. HTML like <strong> in language strings should be avoided if at all possible, and it seems to be possible here. 5. Using <br /> is also not very semantic HTML. Would be better to wrap each bit of the message in <p> ... </p>.
          Hide
          Tim Hunt added a comment -

          I was thinking about this a bit more on my icy cycle in to work. Would it work better if the text that appears in the confirmation pop-up to describe where this block appears is closer to what appears on the block settings form? Those are the words that admins are already familiar with using in order to describe where the block appears.

          Of course, there is probably some advantage in making the warning 'more scary' if the block appears in lots of places, not just on the current page.

          Show
          Tim Hunt added a comment - I was thinking about this a bit more on my icy cycle in to work. Would it work better if the text that appears in the confirmation pop-up to describe where this block appears is closer to what appears on the block settings form? Those are the words that admins are already familiar with using in order to describe where the block appears. Of course, there is probably some advantage in making the warning 'more scary' if the block appears in lots of places, not just on the current page.
          Hide
          Adrian Greeve added a comment -

          I'm currently in the process of fixing up the other problems that you mentioned, but I do have a query about number two: Looking at the way things work at the moment, it seems that blocks created from the site, category, and course level are the only ones that change the showinsubcontexts field. Is it still necessary to do checks for my_moodle and module blocks?

          Show
          Adrian Greeve added a comment - I'm currently in the process of fixing up the other problems that you mentioned, but I do have a query about number two: Looking at the way things work at the moment, it seems that blocks created from the site, category, and course level are the only ones that change the showinsubcontexts field. Is it still necessary to do checks for my_moodle and module blocks?
          Hide
          Tim Hunt added a comment -

          Really, I think that comment 3 overrides comment 2. If at all possible you should avoid having and if-statemend relating to context level at all. You should just use $parentcontext->get_... method to get the information you need about the context - assuming that you can get sufficient information that way.

          Show
          Tim Hunt added a comment - Really, I think that comment 3 overrides comment 2. If at all possible you should avoid having and if-statemend relating to context level at all. You should just use $parentcontext->get_... method to get the information you need about the context - assuming that you can get sufficient information that way.
          Hide
          moodle.com added a comment -

          For those who might be following this issue... Adrian has gone on a short holiday and will return to this issue in a couple of weeks.

          Show
          moodle.com added a comment - For those who might be following this issue... Adrian has gone on a short holiday and will return to this issue in a couple of weeks.
          Hide
          Adrian Greeve added a comment -

          Hello Tim,

          I've greatly simplified the original code and removed the if statements as you suggested.

          Could you please have another look at this for me please?

          Show
          Adrian Greeve added a comment - Hello Tim, I've greatly simplified the original code and removed the if statements as you suggested. Could you please have another look at this for me please?
          Hide
          Tim Hunt added a comment -

          I can't look now, but will try to have the review done before you get into work tomorrow morning.

          Show
          Tim Hunt added a comment - I can't look now, but will try to have the review done before you get into work tomorrow morning.
          Hide
          Tim Hunt added a comment -

          1. print_context_name is deprecated. Use the $context->get_context_name method.

          2. $message .= '</p><p>' . $warning; is just evil. How do you know that will produce valid HTML?

          3. The new warning is not acutally true. Sticly blocks do not necessarily appear "everywhere below" a given context. I have already said how I think the warning should look. End of this comment: https://tracker.moodle.org/browse/MDL-30669?focusedCommentId=191150&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-191150. I suggest you discuss the wording of the message with someone like Helen or Mary, who can give the user's point of view.

          Show
          Tim Hunt added a comment - 1. print_context_name is deprecated. Use the $context->get_context_name method. 2. $message .= '</p><p>' . $warning; is just evil. How do you know that will produce valid HTML? 3. The new warning is not acutally true. Sticly blocks do not necessarily appear "everywhere below" a given context. I have already said how I think the warning should look. End of this comment: https://tracker.moodle.org/browse/MDL-30669?focusedCommentId=191150&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-191150 . I suggest you discuss the wording of the message with someone like Helen or Mary, who can give the user's point of view.
          Hide
          Helen Foster added a comment -

          Adrian, thanks for asking my opinion on language string wording. My suggestion is as follows:

          You are about to delete a block that appears elsewhere.

          Original block location: Course: Moodle Features Demo
          Display on page types: Any type of course main page

          Are you sure you want to continue?

          Show
          Helen Foster added a comment - Adrian, thanks for asking my opinion on language string wording. My suggestion is as follows: You are about to delete a block that appears elsewhere. Original block location: Course: Moodle Features Demo Display on page types: Any type of course main page Are you sure you want to continue?
          Hide
          Adrian Greeve added a comment -

          Thanks Helen for your help.

          Tim, if you would be so kind as to have another look at my latest patch I would be most grateful.

          At the moment I'm not all that happy with the empty 'p' tags that are displayed in the error message. Perhaps you might have some idea on how I could circumvent this problem.

          Thanks.

          Show
          Adrian Greeve added a comment - Thanks Helen for your help. Tim, if you would be so kind as to have another look at my latest patch I would be most grateful. At the moment I'm not all that happy with the empty 'p' tags that are displayed in the error message. Perhaps you might have some idea on how I could circumvent this problem. Thanks.
          Hide
          Tim Hunt added a comment -

          That is looking better. Just one problem:

          1. Does $messagestring->pagetype = get_string($pagetype, 'pagetype'); work in all cases? I don't think so. If you look at generate_page_type_patterns in blocklib.php, you will see it has to do all sorts of callbacks into different plugins (which makes sense, otherwise there would be no way to specify page type names for add-on plugins.) You probably need to add a new function to blocklib get_page_type_pattern_name($pagetype) to do the right thing.

          Also, the <p> tags in the lang string seem a bit ugly, but seem to be unavoidable. I asked in dev chat and David Mudrak couldn't think of a way to avoid them either.

          Show
          Tim Hunt added a comment - That is looking better. Just one problem: 1. Does $messagestring->pagetype = get_string($pagetype, 'pagetype'); work in all cases? I don't think so. If you look at generate_page_type_patterns in blocklib.php, you will see it has to do all sorts of callbacks into different plugins (which makes sense, otherwise there would be no way to specify page type names for add-on plugins.) You probably need to add a new function to blocklib get_page_type_pattern_name($pagetype) to do the right thing. Also, the <p> tags in the lang string seem a bit ugly, but seem to be unavoidable. I asked in dev chat and David Mudrak couldn't think of a way to avoid them either.
          Hide
          Adrian Greeve added a comment -

          Hello Tim,

          I've made some further alterations.
          I also made a change to the course_page_type_list() function. generate_page_type_patterns() accepts $currentcontext as an optional parameter. Calls to this function with a block that has a course page type pattern will result in an error if $currentcontext is not set. Also calling this function with a category page type (which uses course page types), regardless of whether the $currentcontext is set or not will result in an error.

          I do not think that I have changed the functionality of the function, but careful consideration should be made regarding the changes that I have made, so once again I ask for another peer review.

          Thanks.

          Show
          Adrian Greeve added a comment - Hello Tim, I've made some further alterations. I also made a change to the course_page_type_list() function. generate_page_type_patterns() accepts $currentcontext as an optional parameter. Calls to this function with a block that has a course page type pattern will result in an error if $currentcontext is not set. Also calling this function with a category page type (which uses course page types), regardless of whether the $currentcontext is set or not will result in an error. I do not think that I have changed the functionality of the function, but careful consideration should be made regarding the changes that I have made, so once again I ask for another peer review. Thanks.
          Hide
          Tim Hunt added a comment -

          "I do not think that I have changed the functionality of the function"

          It really should not be the peer reviewer's job to do your thinking for you.

          Does this method have unit tests? If not, could you add some. If so, can you add some more. That would be a good way to determine whether it was working or not.

          Show
          Tim Hunt added a comment - "I do not think that I have changed the functionality of the function" It really should not be the peer reviewer's job to do your thinking for you. Does this method have unit tests? If not, could you add some. If so, can you add some more. That would be a good way to determine whether it was working or not.
          Hide
          Adrian Greeve added a comment -

          Unit tests are now available.

          Show
          Adrian Greeve added a comment - Unit tests are now available.
          Hide
          Michael de Raadt added a comment -

          Shifting to the next sprint.

          Show
          Michael de Raadt added a comment - Shifting to the next sprint.
          Hide
          Adrian Greeve added a comment -

          The tested the unit tests for the alteration of the course_page_type_list() function. It came back with no errors. I then added one last test to check for the issue that I was having when generating a page type list.

          Show
          Adrian Greeve added a comment - The tested the unit tests for the alteration of the course_page_type_list() function. It came back with no errors. I then added one last test to check for the issue that I was having when generating a page type list.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          If you can amend it a bit to fix some alignment problems arounds line #3692,12 ...

          Also, I think it would be worth testing, on front page, the "on front page and pages added to front page". I think it's 100% equivalent to the "any page" on courses, but better verify it.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - If you can amend it a bit to fix some alignment problems arounds line #3692,12 ... Also, I think it would be worth testing, on front page, the "on front page and pages added to front page". I think it's 100% equivalent to the "any page" on courses, but better verify it. Ciao
          Hide
          Adrian Greeve added a comment -

          Thanks Eloy,

          I fixed up the formatting and expanded the testing instructions to include the front page configuration that you mentioned.

          Show
          Adrian Greeve added a comment - Thanks Eloy, I fixed up the formatting and expanded the testing instructions to include the front page configuration that you mentioned.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (23, 24 & master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (23, 24 & master), thanks!
          Hide
          Frédéric Massart added a comment -

          Passing. Thanks!

          Show
          Frédéric Massart added a comment - Passing. Thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Because

          A
          MARVELOUS
          A       U
          Z  YOU  P
          I  ARE  E
          N  PPL  R
          G       B
            TNKS! 
          

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Because A MARVELOUS A U Z YOU P I ARE E N PPL R G B TNKS! Closing, ciao
          Hide
          Mary Cooch added a comment -

          Just added a quick note about this in the 2.3 and 2.4 docs http://docs.moodle.org/24/en/Block_settings#.27Sticky_blocks.27

          Show
          Mary Cooch added a comment - Just added a quick note about this in the 2.3 and 2.4 docs http://docs.moodle.org/24/en/Block_settings#.27Sticky_blocks.27

            People

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

              Dates

              • Created:
                Updated:
                Resolved: