Dashboard blocks and regions should not appear on the main blocks configuration page

Superscribe! Thanks, & yes, good start. I imagine this to be linked as a secondary tab in the *customize* dashboard screen only:

I like this, subscribing.

For the record, I was against the initial implementation in the D7UX: Dashboard issue - because it creates a concept which says that on /admin/structure/blocks regions would live of other parts then your site. And also advised it as a solution in #601932: Allow dashboard to limit available blocks - to know where I am coming from.

Then onto this implementation, it seems good. Obviously its a bit confusing at first that disabling and enabling a block is a two step process, but David explained to me that this is kind of an edge case. Other then that, I would think this issue needs some reviews, since its holding back a critical issue.

Me and yoroy discussed this on irc, the only label change we perceive is "Manage dashboard blocks" rather then "more". This design concept, could very much be conceived as a UI hack for the sake of accessibility. Placing it in the description or other common places, would only cause more clutter.

Subscribing.

I'm confused by the screenshot.

1. It looks like we're on the manage dashboard block page, and then there is another link to 'manage dashboard blocks'?

2. It seems like incorrect use of sub-tabs.

@Dries yes, its a UI hack - we are still experimenting I would say, the functionality almost entirely for accessibility sake.

Subscribing

This is an interesting and somewhat complicated issue from a ux / accessibility perspective. There obviously needs to be an easy to understand and accessible method to rearrange blocks on the dashboard for users who cannot use drag-and-drop functionality.

I would be happy to discuss this in person in SF if anyone is interested.

#4: split-dashboard-blocks-761956-4.patch queued for re-testing.

this is kind of, sort of blocking critical (priority-major and webchick beta blocker) issue: #601932: Allow dashboard to limit available blocks

@David could you reroll a patch we can apply and test? I am keen on fixing this before the #601932: Allow dashboard to limit available blocks issue, it should be possible - where to place this link is to be discussed, but honestly shouldn't keep this issue from moving.

With a new patch this can be set to needs review and will keep it moving.

Issues that block criticals are critical themselves. A reroll of the patch would be appreciated. Lets get some of the last ux criticals moving again.

Intresting, with a small label change I think that could work - since we have to have the changes are automatically saved anyway since we didn't implement any coherent auto-save interaction/visual

Ok...

GOOD
-----
1) testbot is green
2) still applies
3) I like the screenshots

BAD
----
1) I found two verbs in the doc headers (process and preprocess) that need to be rewritten to processes and preprocesses

So...

Can someone summarize what has to be done to mark this rtbc?
As this is needed for another critical I think we should pay attention to this (pretty big) patch

From my perspective only fixing the copywriting a little.

configuration page = Dashboard blocks overview page.

Disabling a block makes it available on the main blocks administration page = I dont get what this means.

Either way, I would love if David could envision how this would work with the checkbox parent issue #601932: Allow dashboard to limit available blocks

Patch works as intended. My review focusses mostly on copywriting:

Lets not re-use the same flow of words of the normal blocks page description, makes it harder to tell the two apart. I tried to use simpler words.

__

- Suggest to use a more descriptive page title to differentiate it more clearly from 'Customize dashboard page'.
- Lets just call it a dashboard, and administrative is a very difficult word :-)
- I still wonder if we can do something else for the 'Disabled' label in the table itself. It's weird that disabling something actually also means enabling it somewhere else. I was thinking something like 'not available for dashboard', which is also a negative but only for the dashboard context.

@yoroy:

I like the wording changes you suggest. I haven't tested the patch, but this looks from the screenshots like it should be good.

Would "inactive" be better than "disabled"?

For the wording at the top of the table, you could say "Removing a block from the dashboard makes it available on the main blocks administration page."

+++ modules/block/block.admin.inc	9 Jul 2010 06:10:57 -0000
@@ -122,7 +172,7 @@ function block_admin_display_form($form,
 function block_admin_display_form_submit($form, &$form_state) {
...
-  foreach ($form_state['values'] as $block) {
+  foreach ($form_state['values']['blocks'] as $block) {

Love you for this change. Finally allows modules to extend that form! :)

+++ modules/dashboard/dashboard.module	9 Jul 2010 06:10:57 -0000
@@ -235,6 +268,86 @@ function dashboard_admin($launch_customi
+  require_once DRUPAL_ROOT . '/' . drupal_get_path('module', 'block') . '/block.admin.inc';

module_load_include() ?

+++ modules/dashboard/dashboard.module	9 Jul 2010 06:10:57 -0000
@@ -235,6 +268,86 @@ function dashboard_admin($launch_customi
+  // @todo: This assumes the current page is being displayed using the same
+  // theme that the dashboard is displayed in.

Proper syntax for @todo is explained on http://drupal.org/node/1354

+++ modules/dashboard/dashboard.module	9 Jul 2010 06:10:57 -0000
@@ -235,6 +268,86 @@ function dashboard_admin($launch_customi
+  $regions_to_remove = array_keys(array_diff_key(system_region_list($theme_key, REGIONS_VISIBLE), $dashboard_regions));
+  foreach ($blocks as $id => $block) {
+    if (in_array($block['region'], $regions_to_remove)) {

It looks like removing array_keys() for $regions_to_remove would allow to use the much simpler and faster isset() instead of in_array().

Aside from that, this looks ready to fly to me.

39 critical left. Go review some!

+++ modules/dashboard/dashboard.module	23 Jul 2010 04:31:41 -0000
@@ -18,6 +18,11 @@ function dashboard_help($path, $arg) {
       return $output;
+    case 'admin/structure/dashboard':
+      // @todo: This assumes the current page is being displayed using the same

Aside from a missing newline between case and return, and @todo still having a colon behind it, this looks ready to fly for me.

+++ modules/dashboard/dashboard.module	23 Jul 2010 04:31:41 -0000
@@ -235,6 +268,86 @@ function dashboard_admin($launch_customi
+function template_preprocess_dashboard_admin_display_form(&$variables) {
+  template_preprocess_block_admin_display_form($variables);
+}

I wonder why that other preprocess function isn't registered in hook_theme() instead, but maybe this is the correct way. (I have no idea)

Aforementioned issues are minor, may be changed in a follow-up, or not.

40 critical left. Go review some!

One second, I am reviewing it - I am feeling the placement in the IA is somewhat weird.

Ok looks good, I am not sure it belongs in Structure at all though. We might want to move it somewhere else later.

Re #34: There's an issue about moving the blocks system to Appearance: #567418: Move blocks page into appearance, although that may or may not be an appropriate place for this page, depending on whether or not we want "Appearance" to refer to user-facing settings only.

@amc that is not the appropriate place. I will comment in the other issue.

Looking through the code, this seems to be a pretty elegant solution to the problem. Great!

Unfortunately, the patch has gone stale, so I can't test it. :( Sorry. Would someone be able to do a quick re-roll?

Re-rolled against HEAD.

Previous patch did not remove the Dashboard regions from the block configuration form (for a single block).
Added dashboard_form_block_admin_configure_alter().
No other changes.

Odd. @David, please re-confirm this patch.

Can you make a patch that does work?

This issue been waiting for way to long to continue moving.

When I apply the last patch in #41, I get the following error when I go to admin/structure/dashboard:

PHP Fatal error:  Call to undefined function  block_admin_display_prepare_blocks() in /site_path/modules/dashboard/dashboard.module on line 296

It seems like this function does not exist. Is it supposed to be in this patch, or already in core?

drupal.split-dashboard-blocks.41.patch lost block_admin_display_prepare_blocks and changes to block_admin_display and block_admin_display_form

Applied 40.patch and copied the dashboard_form_block_admin_configure_alter from 41.

So, this patch looks and works good for me.

Thinking about #43, this is an interesting problem. Here are some points, questions:

* If someone disables the dashboard module, then those configured blocks need to be updated (have their region removed).
* If a module provides dashboard regions and gets disabled, then those regions need to be removed from the blocks that were configured appropriately.
* If someone sets a dashboard block for a specific theme with dashboard regions, and the theme is switched or disabled, this is not a problem as blocks are configured per theme.

Overall, what needs to happen is when a module is disabled (and implementes hook_dashboard_regions), then something needs to get fired to take into account that blocks could be configured in that region for any theme. This is technically a different issue than the original, s the original is actually just based around the UI for blocks, not about the dashboard_region functionality.

Addressing the problem of #43, here's the #47 patch with the added twist of hiding the dashboard regions for all themes but the current, which is assumed to be the admin theme.

So, thinking about this more. The way the block module handles this is not listening to a disable, but rehashing the blocks on the admin page in _block_rehash() which we call in block_admin_display_prepare_blocks() so this is good to go. Need someone with a little more exp to review as well.

So, more into this, what we now need to do is implement a hook_cache_flushes() similar to blocks:

function block_flush_caches() {
  // Rehash blocks for active themes. We don't use list_themes() here,
  // because if MAINTENANCE_MODE is defined it skips reading the database,
  // and we can't tell which themes are active.
  $themes = db_query("SELECT name FROM {system} WHERE type = 'theme' AND status = 1");
  foreach ($themes as $theme) {
    _block_rehash($theme->name);
  }

  return array('cache_block');
}

I believe this is the issue with what @linclark describes in #50.

After talking with @linclark, this problem seems to persist past what my previous problem describes. The _block_rehash() function must not be fully equipped for this. Looking into further.

The problem described in #50 is another issue, it's still a problem without this patch.

The tests are broken though.

Re-rolled to fix those test fails. The size difference from the last patch is due to my IDE fixing spacing issues in search.test.

The "(empty)" thing seems to be a feature of dashboard. The dashboard blocks are rendered by the normal block system, and then moved to the dashboard regions. If a block isn't rendered (because it is empty, or configured to not be shown for some reason), dashboard adds a empty placeholder.

As the missing comment module block isn't rendered because it doesn't exist anymore, dashboard adds it in.

Re #48

* If someone disables the dashboard module, then those configured blocks need to be updated (have their region removed).
* If a module provides dashboard regions and gets disabled, then those regions need to be removed from the blocks that were configured appropriately.

But people would expect it to work like the regular blocks: reenabling the module brings the blocks back the same places (if you haven't messed with block settings in the meanwhile). However, one could make a case for dashboards being something else...

I hope I'm not missing something (I'm a little new to reviewing patches).

I repeated the test described below.

I ran into the following problem. I am not sure whether this is exactly the steps, but want to post them now before we get kicked out.

1. I assign Recent comments to Sidebar First.
2. I assign Recent comments to Dashboard.
3. I disable Comment module and Dashboard. The Recent comments box disappears in Sidebar First (as expected).
4. I re-enable Comment module. The Recent comments block in Sidebar First does not return, even though it is in Sidebar First in my blocks list (NOT expected behavior).
5. I re-enable Dashboard. Recent blocks in Sidebar First appears again

Upon completing step number 4, re-enabling the comment module, I get the following error in the overlay:

FieldException: Attempt to create an instance of field comment_body on bundle comment_node_article that already has an instance of that field. in field_create_instance() (line 673 of /home/omar/web/drupal-7.x-dev/modules/field/field.crud.inc).

... and the "recent comments" are not displayed. Re-submitting the module form a second time (i.e. without changing anything) does *not* generate the error and the "recent comments" block appears. So I think that the result of step number 5 have nothing to do with the re-enabling of the dashboard module.

This behavior is the same whether or not the patch number 57 is applied.

I hope that this is helpful.

Have been playing around with this and there do seem to be some problems with caching.

On a clean install moving things around works ok but as soon as you disable and enable the dashboard module (or generally mess about as Lin describes in 58) things start behaving strangely. Blocks appears on both the dashboard and the main block page even though they are enabled for one or the other, etc.

There is also a set of messages saying that the blocks assigned to the Dashboard are assigned to invalid regions and have been disabled - this is not major but might be confusing for a user that things something went terribly wrong.

In order to get things back to some normality the cache needs to be cleared explicitly - so some more work on that needs to happen.

Is anyone actively working on this patch? I'd be happy to pitch in and add some cache_clear_all calls to retest. I think the problems described in #61 might also be related to cache (but perhaps not #58).

@jenlampton zzolo was working on this yesterday and looking at the comments above has been looking at caching issues - so would wait for him to chip in?

thanks @ronald_istos. I just did some testing of my own and could not recreate the caching problems.

1. I assign Recent comments to Sidebar First.
2. I assign Recent comments to Dashboard.
3. I disable Comment module
4. I disable Dashboard module.
5. I re-enable Comment module.
Result: Block reappears in sidebar first (as expected) (Note: the error message in #61 comes from #877388: FieldException when re-enabling comment module)
6. I re-enable Dashboard.
Result: blocks to NOT appear in previous locations on Dashboard.

I even tried futzing around with some other stuff in the UI like turning off and on dashboard again, and off and on comment, but everything works as expected for me.

I wonder if the dashboard placement should not also be preserved, but I don't think that should be critical. Perhaps time for a new issue?

New patch based on #57 with fixes for:

* If someone disables the dashboard module, then those configured blocks need to be updated (have their region removed).

dashboard_disable implements this

* If a module provides dashboard regions and gets disabled, then those regions need to be removed from the blocks that were configured appropriately.

dashboard_modules_disabled implemented. Someone please test.

can you reroll this patch quickly without the a and b? Will try a find and replace if not.

*EDIT*

1) I don't think this is working. After the dashboard module is disabled, the value for 'region' in the block table is -1. When blocks are removed from regions via the UI, the whole record is removed from the block table.

2) ...

Ah, git thingies, I'll figure out what others do and reroll.

I have a block table full of -1 blocks. I took it for being the canonical 'disabled' value. I can remove them instead.

a/b fixed.

I looked at block_admin_display_form_submit which has this:

    $block['status'] = (int) ($block['region'] != BLOCK_REGION_NONE);
    $block['region'] = $block['status'] ? $block['region'] : '';

So i changed the disable hooks to to set the region to ''.

This is better, but I still don't think it's enough. As well as updating the region, I think we should be updating the status. The block's status needs to change from 1 to 0.

In both cases where an enabled block can't go on the dashboard because it disappears, the block should become disabled, no?

(It would also be nice if we could preserve the region for if/when the module gets enabled again, too)

Can we change the status instead of the region?

(It would also be nice if we could preserve the region for if/when the module gets enabled again, too)

Can't. Because of:

      if (!empty($block['region']) && $block['region'] != BLOCK_REGION_NONE && !isset($regions[$block['region']])) {
        drupal_set_message(t('The block %info was assigned to the invalid region %region and has been disabled.', array('%info' => $block['info'], '%region' => $block['region'])), 'warning');
        // Disabled modules are moved into the BLOCK_REGION_NONE later so no
        // need to move the bock to another region.
        $block['status'] = 0;

in _block_rehash.

Status = 0 added...

Sorry, DrupalCon happened. I'll review/work on this probably on Friday during code sprint.

Yeah, the behavior of the block system is that if a region gets disabled (theme changes or module that offers regions gets disabled), then it just automatically updates the block table on cache flush or when going to block page.

Though, this may not be the best behavior, persay, it is alright for now, and it is important that the blocks get their region reset, so that they can be configured again (since the block system only allows a block to be enabled per theme per region).

So, in theory, we should not really have to handle this case, as it should already be handled for us. At least, thats the idea.

@Xen
After posting a patch, you need to change the status to "needs review" so that the bot can test it.

Let's do it now :)

Your patch indeed makes the dashboard regions disappear. However, the blocks currently in these regions disappear as well.
So if we have a list of recent content on our dashboard page, we can not add this block to other blocks in the Admin theme.

Is this supposed to work like this?

Something else that's buggy (as you said, nothing to do with this patch) is the drag&drop interface on the Dashboard page. I've made a new issue for that (#895246: Dragging & dropping disabled blocks is buggy).

Allright, if that is the purpose, it makes sense. Wouldn't it be nice to have a small message on the blocks page as in 'Blocks that have been added to the Dashboard page are not available.'?

What's strange in this setup is that the Dashboard page uses Seven as theme, with it's own regions not used outside the dashboard. We'd had better created a seven_dashboard child theme to use for the Dashboard. If we had done so, we don't have to use your patch at all, and blocks can be added to Seven AND the dashboard as well.

Anyhow, I think your patch looks good and does what it should do.

Making it a subtheme wouldn't be achievable I guess, as other themes can be a admin theme as well.
I tested it without Javascript and that also workes great.

I found one small bug: the link to the Dahboard page on admin/structure/dashboard points to the wrong url.

The "Add block" form didn't have dashboard regions hidden from its region select lists. I added a form_alter for that form. It simply calls the form_alter for the configure form as they're basically the same (block.admin.inc does the same thing when building the forms).

function dashboard_form_block_add_block_form_alter(&$form, &$form_state) {
  dashboard_form_block_admin_configure_alter($form, $form_state);
}

I also combined two nested if clauses in dashboard_form_block_admin_configure_alter().

Other than that, I agree with David that it's tempting to leave the @todos for a spin-off issue. It looks a bit tricky, but I don't think it's critical in itself, it feels like only an edge case could cause problems.

After all this work, can there be some screenshots describing what the proposed interaction is?

Haven't applied the most recent patch, but coming from earlier ones, I don't think much has changed visually; as in, no new visual elements have been introduced. On admin/build/block admin/structure/blocks, with the patch the dashboard regions are not visible, and with them all blocks that are on the dashboard. Also, if you click configure on one of the blocks, the select list to choose the region for the block does also not show the Dashboard regions. Only For the admin theme, which is assumed (see above) to be the theme the dashboard is displayed in.

I just reread the patch and tried it out extensively. I tried enabling and moving around normal blocks and dashboard blocks, with and without JS and disabling dashboard module. Nothing unexpected happened. Setting to RTBC.

For Bojhan I attached a screenshot of admin/structure/dashboard, because that's the only thing that's new. Note, that only people without JS ever go there because JS-users use the fancy drag-and-drop interface (of course they can navigate there manually through the toolbar, for instance). The screenshot is taken with JS (otherwise, the 'grippies' would not appear etc). In general it is really the same thing as admin/build/blocks (hey, it actually uses the same code...), so nothing special.

Great work, folks. Committed to HEAD.

No idea what happend here, should we reopen #601932: Allow dashboard to limit available blocks, Dashboard in structure?

The dashboard *settings* page is in structure, yeah. It's a fall-back in case the default drag-and-drop interface doesn't work.

I hate reopening this, but I noticed today that the dashboard regions are not showing up on the /admin/structure/dashboard page.

Bojhan confirmed this on IRC :(

This makes a bleen sad

this should do it

+++ modules/dashboard/dashboard.module	13 Sep 2010 00:52:58 -0000
@@ -313,15 +313,17 @@ function dashboard_admin_blocks() {
 function dashboard_form_block_admin_display_form_alter(&$form, &$form_state) {
...
+  if ($form['#form_id'] == 'block_admin_display_form') {

$form_id is passed as third argument and should be used here.

Overall, however, this is very odd and at least deserves a code comment, and I'm entirely not sure what's going on in Dashboard module.

Powered by Dreditor.

This patch has tests too ... I wasn't sure what the best way was to test this, but this definitely works.

...and this patch has tests as well as changes based on #91

+++ modules/dashboard/dashboard.module	13 Sep 2010 02:23:19 -0000
@@ -310,18 +310,22 @@ function dashboard_admin_blocks() {
+  // administration form and from the options list on that form. Because this
+  // function is called for both the dashboard block configuration form and the
+  // standard bloack configuration form, the form_id must be checked.

Thanks, but who is guilty for that double-calling? Is Dashboard module implementing hook_forms()?

Powered by Dreditor.

yup:

function dashboard_forms() {
  // Reroute the dashboard configuration form to the main blocks administration
  // form. This allows us to distinguish them by form ID in hook_form_alter().
  $forms['dashboard_admin_display_form'] = array(
    'callback' => 'block_admin_display_form',
  );

  return $forms;
}

I dont understand this well enough to suggest removing it ... though it seems like maybe we should? Thoughts?

Ah-ha, thanks! :) Now we want to cross-reference that knowledge in the added code comment; i.e., so that we don't simply state what happens, but instead why something happens. That's the essence of code comments. (Or in other words: It's obvious that by looking at the code we are in a hook_form_FORMID_alter() function and, although we are in a fom_id-specific alter hook, that the code somehow takes the actual $form_id into account, prior to performing any changes. We need to document the non-obvious.)

Thanks for driving this home, bleen18!

how bout this

+++ modules/dashboard/dashboard.module	14 Sep 2010 01:00:01 -0000
@@ -310,18 +310,23 @@ function dashboard_admin_blocks() {
+  // standard bloack configuration form so that both forms can share the same

Wonderful. Just that "bloack" somehow managed to get between your fingers ;)

+++ modules/dashboard/dashboard.test	14 Sep 2010 01:00:01 -0000
@@ -58,3 +58,45 @@ class DashboardAccessTestCase extends Dr
+class DashboardAdminTestCase extends DrupalWebTestCase {

Missing phpDoc. Do we really need an entire new test case for this? It doesn't seem to do anything special, so I suspect it could be merged into one of the existing.

Powered by Dreditor.

how bout this

wait .. makin a quick change to testname

Thanks!

Please wait for green light before commit.

this one

blaaarg!! found another typo

I meant to do (1) ... but just forgot. This patch covers all the points from #105

rerolled... will pass test and fixed spacing from #107

+++ modules/dashboard/dashboard.test	14 Sep 2010 15:36:28 -0000
@@ -6,11 +6,11 @@
-class DashboardAccessTestCase extends DrupalWebTestCase {
+class DashboarBlocksTestCase extends DrupalWebTestCase {

Still a typo here.

Powered by Dreditor.

blaaarg

Thanks! :)

Awesome!

Committed to HEAD. Thanks! :)