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

Actually, this really is a bug.

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.

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.

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.

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: ...

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>.

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.

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?

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.

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.

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?

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

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.

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?

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.

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.

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.

"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.

Unit tests are now available.

Shifting to the next sprint.

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.

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

Thanks Eloy,

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

Integrated (23, 24 & master), thanks!

Passing. Thanks!

Because

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

Closing, ciao

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