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

      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.

        Gliffy Diagrams

          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: