Correct whitespace issue for 'recent content block' in dashboard

I like this idea. Off hand, the only issue I can think of is that, during initial site creation, your content list might be skewed toward "about" pages and "contact" pages and other static pages that usually get stuck in a primary or secondary menu (and not highlighted otherwise). Those would roll off, though, as new content was added -- and until they did, the block would be a handy way to get to them.

Plus, you've got a patch, so changing status to CNW based on your comment in #1.

There's also #301902: Allow more users to see the node admin page - which gives you a 'view content' link leading to admin/content/node for users with some content permissions. Seems like this would be a good solution too - and no reason they couldn't both go in.

Nice ... You could future proof the SELECT query by making it dynamic and adding a node_access tag. Not a big deal though.

Okay, here is a new patch.

I moved the install bit into node_install (doesn't seem to belong in system since the code is in node). Also changed the block hooks to the new format. Tests pass AFAICT, and the block works :)

The .install doesn't look quite right - we should be using a defined constant for the block caching mode.

I would also have the entire block content generated by a theme function, rather than building the whole list in hook_block. Also, these sorts of loops I thought we were intended to use foreach($result...) rather than while(...?

I don't know why you would use foreach instead of while, I believe it is the same performance wise and looks about the same. Is there a style guideline on that? I typically use while when using an iterator (like this) and foreach when iterating an actual array, but obviously, it doesn't matter much either way.

In re: the theming, I agree here, but not sure what you propose. The list is already pushed through "theme_item_list". Are you thinking we make a new theme function so that each item can be pushed through it individually and then have the resultant HTML pushed through theme_item_list? Is there an existing theme hook which fits the bill?

Thanks for the review!
Jacob

Yes, we moved installing blocks to the default.profile. Also linking to this issue for http://www.drupalusability.org/node/150.

This patch did what it is supposed to. It looks good to me. I wonder why this hasn't been done sooner.

This needs a screenshot so someone from the usability team can properly review this.

- Does this patch make the block enabled by default? It should be to make it really work for the new user / fresh install
Yes it does from reading the initial issue :)

- "Recently posted" doesn't sound quite right to me. Let's use the word "content" here, after all that's what we're referring to.

"Latest content" or "Recent content"?

Feel like the tests could be cleaned up a lot and might be a tad bit of overkill. I'll review tomorrow when I get back home and on reliable wifi.

The patch itself looks good, but after looking closely at the problem we are trying to solve "not finding content after created" I believe that it is only a quick fix to a larger problem. As the usability testing of UB2008 and UB2009 seem to have shown was the user not confused by finding his content in admin/content he was soley confused by the fact that once he created a page, it did not initialy show on his homepage or anywhere (if he didn't came across administer yet).

It seems that issue, http://drupal.org/node/395968 takes better care of the "not finding content after created" issue which is a initial experience confusion that is caused by "page" and is not a longer experience kind of issue, which by adding this block we tend to imply.

With the dashboard looming, this should be something that's available to put there, so re-opening.

tag

subscribe

An attempt at a straight reroll for the bot's benefit.

Now with patch attached!

Tests should pass now.

Hm, test failures I can't reproduce locally. Spiffy. I'll see what I can do when I get a minute to work on this.

In the meantime, I'm wondering if we shouldn't add more content to this block (author, content type, ...) so it better matches the mockup at http://drupal.org/node/544360#comment-1923218 (see the block at the bottom left). I'm not entirely sure what the workflow buttons in that block would do though.

Bumping to critical as per #569190: D7UX: Dashboard contents (this is a must-have for a useful default dashboard)
Overall dashboard issue is @ #544360: D7UX dashboard module

Content-wise, yes, let's do a quick round of sketching ideas for what really should be in here. Not able to read a patch, could you tell me what it displays now?

no really. it's critical :-)

Oh sorry. Screenshot attached.

Since this is intended for the dashboard, we should remove it from the default install profile here, then the node.test hunk could go, and the recent content block test would need to explicitly enable it. With a bit of luck that'll stop the tests from breaking too.

Postponing it for a bit on the overall design discussion for 'recent stuff widgets' on the dashboard. Re-open here once we settle on a design direction in #614410: Patterns for recent-items-blocks on the dashboard. It's a bit jumpy but needs to be considered in a broader context first.

We haz direction (see #614410: Patterns for recent-items-blocks on the dashboard for the design process)

More better image:

Tested this patch - works nice.

I fixed up a call to theme('feed_icon') (in node_block_view(), which was preventing the patch from applying - after making this change, the patch applied successfully (with offsets).

Also fixed the call in the new block to theme('item_list'), which was affected by the theme() only takes 2 arguments recent API change.

This patch fix is courtesy of the Sydney Drupal Users Group code sprint. Yay!!! (Let's not mention how long it took a room full of geeks to fix one line in a patch... oops, I just did...)

Re-rolled patch attached.

Sorry, last patch was missing file markers. New one attached.

Just want to see which ones still are broken so I can help out with fixing them.

Currently working on it, but I have to go sleep now. I'll continue tomorrow.

Hm dont , I will post it... Bojhan / webchick asked for it. What I see is that all is well but the test is not adjusted for titles as translateable fields.

Well, almost all titles were, just one assert slipped through and a link pointed to an admin screen that moved meanwhile. Easy fix.

So what about this?

Here are screenshots:

Recent Content Block:

Recent Content Block settings page:

Ok great, we've got a functional patch again. Now let's compare it to the proposed design:

- Author name is missing, should link to account page (when you have permission to do so
- The 'time ago' info is less important, we can remove it.
- 'Show all' link to the content page is missing
- Header wants to have a linked 'content' , to the same content listing in there.

I think enough of the scaffolding is here that we could give this to a newish contributor to finish up. Not quite a "novice" patch, but should be doable by anyone with a little bit of PHP skills.

I'll be moving this forward. Back in a couple hours with results.

Based on http://drupal.org/files/issues/recent_content_0.patch

Done:

• Removed time ago.
• Added author name, ran through theme_username(). May want to look at selecting user in query instead of calling load_user() if there is a difference in performance (see node_block_view()), although I prefer to use functions available if there isn't a difference for maintainability.

Still to do:

• 'Show all' link to the content page is missing
• Header wants to have a linked 'content' , to the same content listing in there.

I could really use some clarification on what "Header wants to have a linked 'content' , to the same content listing in there." means.

Test submitted patch...

CVS settings in Eclipse were set to platform default line endings - this should be fixed

That vague error usually means that your checkout of Drupal 7 is out of date, and things have moved too fast and it no longer knows how to apply your patch. Could you try updating your source tree (somewhere under the Team menu in Eclipse) and try again?

Do we really need this block to be enabled on Garland by default? I would sleep better without those ugly:

+    // Remove the "recent content" block because it will mess up our assertions
+    // later on if enabled.
+    db_delete('block')
+      ->condition('module', 'node')
+      ->condition('delta', 'recent')
+      ->execute();

I agree.

Pulled all of the hacks from the other hooks and removed the addition to the install profile so that the block will not be shown by default in Garland.

Last patch did not have root directory set correctly.

Need to modify test cases.

cwgordon: Since then, I committed the patch to allow multiple users to see the admin/content page, based on a permission. I believe that addresses the "where did my content go?" problem better than this block. I'd advocate removing it from being visible by default.

Unfortunately, we don't have a lot in the way of user testing of Drupal 7's UI yet. But admin/content is *way* more prevalent than admin/content/node used to be, so chances are much higher they will see it.

We really only have the "Where did my content go?" problem for page nodes. I would like to solve that with a toggle in the content type settings to make "Provide a menu item" default to TRUE on certain content types. Not for this issue, though.

webchick, cwgordon: this block is specifically intended as a *dashboard* block, enabled in the default install profile. It's not specifically about fixing 'where's my content' anymore.

Changing title to clarify what we are making this block for.

Edit: and this is the design phase that got us here: #614410: Patterns for recent-items-blocks on the dashboard

Block is missing a lot of functionality.

Todo:

• Modify default install profile to set block to 'Dashboard main' below 'Management' block in Seven theme
• Format node items in a table instead of unordered list
• Append 'new' flag to node titles for content new to the user
• Add edit and delete links for each node based on user privileges
• Use JavaScript to hide edit and delete links and show upon hover of corresponding row?
• Add 'Show all content' link after item nodes
  Note: 'Thingnames' support was dropped from specs see webchick's comment
• Completely rewrite tests

Update

Todo:

• Modify default install profile to set block to 'Dashboard main' below 'Management' block in Seven theme
• Format node items in a table instead of unordered list
  Could really use some feedback based on the current table layout
• Append 'new' flag to node titles for content new to the user
  Note: Also applies updated via theme_mark()
• Add edit and delete links for each node based on user privileges
• Use JavaScript to hide edit and delete links and show upon hover of corresponding row?
• Add 'Show all content' link after item nodes
  Note: 'Thingnames' support was dropped from specs see webchick's comment
• Completely rewrite tests
  Note: Put off for further review as to which tests should be written

Todo after above is complete:

• Abstract functionality in to separate functions for future use, most likely by contributed modules.
• Make themeable by placing output functionality in to a theme function.

Screen shot of block attached to get feedback on what I should do about the themeing.

Todo:

• Format node items in a table instead of unordered list
  Could really use some feedback based on the current table layout
  http://drupal.org/files/issues/recent-block-screenshot.png
• Add edit and delete links for each node based on user privileges
• Use JavaScript to hide edit and delete links and show upon hover of corresponding row
• Abstract functionality in to separate functions for future use, most likely by contributed modules.
• Make themeable by placing output functionality in to a theme function.
• Completely rewrite tests
  Note: Put off for further review as to which tests should be written

I really like where this is going. Tagging it as a favorite.

I'd love to see us do a similar block for recent comments too.

# Use JavaScript to hide edit and delete links and show upon hover of corresponding row

I think this would look rather weird in a small width with long titles, either way you do it:
- if you make it retain it's width and space in the table, half of the table will be filled with empty space
- if you don't make it retain it's width and space in the table, and you put the hover on the row (not the cell because you aren't able to hover it then), then the text will wrap and not wrap on mouseenter/mouseleave

+          // TODO: Output is currently UGLY! Need to have "table" attached to heading
+          // to match http://img.skitch.com/20091028-mu8je3f7i9f5giyhbyyiaeg35s.png
+          // TODO: Valign top edit and delete columns 

to attach the table, you're probably going to have to do some ugly stuff if you want to avoid overriding default padding on the block content area
quick stab:

#dashboard #block-node-recent div.content {
  padding: 0 0 5px 1px;
}

that makes it look like this on FF & webkit and should hold up on IE, but didn't try

it certainly beats something like

#dashboard #block-node-recent table {
  margin-top: -10px;
  position: absolute;
  left: 1px;
  width: 99.9%;
}

that's just ugly and doesn't work on small widths

question is, where does it go?

also tested permissions, and they looked good, I just noticed how messed up the dashboard page looks to a user with only that permission, but that's entirely unrelated to this issue :P

oopsie, didn't mean to deprive you of your dreams

#dashboard #block-node-recent div.content {
  padding: 0 0 5px 1px;
}

seutje, the code above looks great, I tested in IE8. I added the code to dashboard.css at the end of the file - as there are no other *specific* block overrides. The reason I chose to add the code to dashboard.css is that the .content padding is defined within the dashboard.css and not within a theme. Thus it will be up to themes to account for the styling of this block should their global settings not result in an aesthetically pleasing result.

Spurred by the input of seutje, I am debating whether the operation links (edit and delete) should be hidden with JavaScript and shown on hover. I see a lot of potential hangups with this sort of display. The most critical being that I think this would actually reduce usability as you would have to hover the line to see if you can act on the node - kind of like navigation that doesn't list what it is until you hover it (circa 2000 web design).

Todo

• Format node items in a table instead of unordered list
• Finalize whether JavaScript should be used to hide edit and delete links and show upon hover of corresponding row
• Abstract functionality in to separate functions for future use, most likely by contributed modules.
• Make themeable by placing output functionality in to a theme function.
• Completely rewrite tests
  Note: Put off for further review as to which tests should be written

Totally unrelated: /me feels special that Dries favorites my work

Note: for RTL display you might want to reverse padding: 0 0 5px 1px; to padding: 0 1px 5px 0; but I'm not entirely sure on that

+    case 'recent':
+      if (user_access('access content')) {
+        $block['subject'] = t('Recent content');
+        $block['content'] = theme('node_recent_block');
+      }

wouldn't it be better to have the block content callback load the data and pass it as a part of the argument passed to theme_node_recent_block? It feels weird right now, as the theming functioning is also responsible for fetching the data...

+  foreach (node_get_recent($number) as $node) {
+    $item = array(l($node->title, 'node/' . $node->nid)
+      . theme('mark', array('type' => node_mark($node->nid, $node->changed)))
+      . '<br />' . theme('username', array('account' => user_load($node->uid))));
+    $item[] = node_access('update', $node) ? l(t('edit'), 'node/' . $node->nid . '/edit') : '';
+    $item[] = node_access('delete', $node) ? l(t('delete'), 'node/' . $node->nid . '/delete') : '';
+    $items[] = $item;
+  }

Also, can we drop that <br />? just wrap the node title and the username in divs if you must, BRs are annoying to style and enforce height, DIVs would be much nicer and easier to override with some padding or margin...

seutje, there isn't a dashboard-rtl.css so I do not know if there is a separate style to apply for right to left support.

I agree on both themeing points and will have them in the next patch. Should we apply any classes to the title and/or username divs? I hate to add any classes when I am not positive about their properness so that I do not make things more difficult for themers down the road.

Does anyone have any input as to whether JavaScript should be used to show operation links (edit and delete) on hover?

I'd advise against it, it makes no sense to me and can only introduce problems

if u do go for a hover thingy, u might have to rethink the whole "using a table without a thead" thing

New patch based on feedback and discussions with seutje and yoroy, also further fleshes out themeing functions.

Todo

• Finalize whether JavaScript should be used to hide edit and delete links and show upon hover of corresponding row
  Operations shown on row hover will not be added for usability reasons
• Finish out a few todos in the patch
• Completely rewrite tests
  Note: Put off for further review as to which tests should be written

Portions of code that I need input/feedback on to finish up functionality, read comments in each code block:

1: Node retrieval

/**
 * Find the most recent nodes that are available to the current user.
 *
 * @param integer $number
 *   (optional) The maximum number of nodes to find. Defaults to 10.
 * @return
 *   An array of partial node objects or an empty array if there are no recent
 *   nodes visible to the current user.
 */
function node_get_recent($number = 10) {
  $nodes = db_select('node', 'n')
    // Are there any other fields which should be returned for this?
    ->fields('n', array('uid', 'nid', 'type', 'status', 'title', 'changed'))
    // Should we set status as a condition? Isn't the point of the
    // module that users do not "lose" content they have created...
    ->condition('status', NODE_PUBLISHED)
    ->orderBy('created', 'DESC')
    ->range(0, $number)
    ->addTag('node_access')
    ->execute()
    ->fetchAll(PDO::FETCH_OBJ);
  return $nodes ? $nodes : array();
}

2: Class for 'Show all content' link - we should apply a class name even if it doesn't provide right alignment so that themers can control it without overriding the theme function

  if ($rows) {
    $output = theme('table', array('rows' => $rows));
    
    // TODO?: Apply a class to this link to match spec 
    // see: http://drupal.org/node/337947#comment-2294824
    $output .= l(t('Show all content'), 'admin/content');
  } 

3: Classes applied to div tags of themed node names and respective user

  // TODO: Add classes to div blocks to allow themers to modify without 
  // overriding theme function, need input on class names to use
  $output = '<div>';
  $output .= l($node->title, 'node/' . $node->nid);
  $output .= theme('mark', array('type' => node_mark($node->nid, $node->changed)));
  $output .= '</div><div>';
  $output .= theme('username', array('account' => user_load($node->uid)));
  $output .= '</div>';

Ok this should be one the final todos.

Todo

1. Get feedback and implement items needed - see http://drupal.org/node/337947#comment-2313988
2. Decide which tests should be written - need feedback and recommendations
3. Write tests

Attached is an updated patch. It is functionally identical to the last (http://drupal.org/node/337947#comment-2313748) but I took it upon myself to clear out some of the todos within the patch as I hadn't received any feedback. I also modified the text for the Recent Content Block Configuration page to make it more understandable and simple. A few comments were also removed.

looks pretty decent to me, one little remark:

+  foreach ($variables['nodes'] as $node) {
+    $cells = array();
+    $cells[] = theme('node_recent_content', array('node' => $node));
+    $cells[] = node_access('update', $node) ? l(t('edit'), 'node/' . $node->nid . '/edit') : '';
+    $cells[] = node_access('delete', $node) ? l(t('delete'), 'node/' . $node->nid . '/delete') : '';
+    $rows[] = $cells;
+  }

so when the user has no access to edit or delete any nodes, he will have 2 empty columns in the table, or am I tripping again?

Nope you are right, I didn't think that part through. New patch to come.

-- Edit --
After reviewing only showing cells for edit and delete if the user has at least one permission for each one, I have found that a somewhat complex loop would be needed to enforce that only the correct cells exist for the theme_table() call.

As a result there are two options that I see:

1. Leave current functionality as is, where cells are shown even if the user does not have the permission for any of the nodes displayed.
2. Remove operation cells and use a horizontal list floated right for edit and delete operations.

I say go for option 1, we did similar with the links on the modules page: #598758: Modules page: add link to its settings page for each module
It allows for consistent layout and alignment even for different permissions

yoroy, thanks for the feedback, we'll go with that option.

I'm also in need of feedback on the three points from http://drupal.org/node/337947#comment-2313988. Once that is received I will only be finishing of up tests and we can commit and close the issue.

With regards to question 1 in #104 I think it would be better to not filter by status. As a site administrator a draft that I create but have not published yet should show up in my recent content block allowing me to quickly find the nodes that I have been working on recently regardless of wether or not they are published.

I suppose making sure that it is apparent in the UI which nodes are not-published would be a good idea.

+++ modules/dashboard/dashboard.css	28 Nov 2009 03:36:30 -0000
@@ -113,3 +113,7 @@
\ No newline at end of file

This should not happen. Please always add a newline.

+++ modules/node/node.module	28 Nov 2009 03:36:30 -0000
@@ -1994,6 +2000,9 @@
+  
+  $blocks['recent']['info'] = t('Recent content');
+  

(and elsewhere) Trailing white-space here (blank lines should be blank).

+++ modules/node/node.module	28 Nov 2009 03:36:30 -0000
@@ -2001,13 +2010,130 @@
+ * Implementation of hook_block_configure().

(and elsewhere) The new standard for hook summaries is:

Implements hook_foo().

+++ modules/node/node.module	28 Nov 2009 03:36:30 -0000
@@ -2001,13 +2010,130 @@
+function node_block_configure($delta = '') {
+  $form = array();
+    if ($delta == 'recent') {

Strange indentation here.

+++ modules/node/node.module	28 Nov 2009 03:36:30 -0000
@@ -2001,13 +2010,130 @@
+        '#title' => t('Number of recent content nodes'),

We don't use the term "node" in user-facing text.

+++ modules/node/node.module	28 Nov 2009 03:36:30 -0000
@@ -2001,13 +2010,130 @@
+ * @param integer $number
+ *   (optional) The maximum number of nodes to find. Defaults to 10.
+ * @return

(perhaps also elsewhere) There should be a blank PHPDoc line between @param and @return.

+++ modules/node/node.module	28 Nov 2009 03:36:30 -0000
@@ -2001,13 +2010,130 @@
+  $nodes = db_select('node', 'n')
+    // Are there any other fields which should be returned for this?
+    ->fields('n', array('uid', 'nid', 'type', 'status', 'title', 'changed'))
...
+    ->execute()
+    ->fetchAll(PDO::FETCH_OBJ);

You only should define 'nid' here and do ->fetchCol() after execute() instead.

Afterwards, you do

$nodes = node_load_multiple($nids);

+++ modules/node/node.module	28 Nov 2009 03:36:30 -0000
@@ -2001,13 +2010,130 @@
+    // Should we set status as a condition? Isn't the point of the
+    // module that users do not "lose" content they have created...
+    ->condition('status', NODE_PUBLISHED)

We need to dynamically apply this condition depending on whether the current user has the "by-pass node access" permission.

+++ modules/node/node.module	28 Nov 2009 03:36:30 -0000
@@ -2001,13 +2010,130 @@
+    ->orderBy('created', 'DESC')

I believe this condition should be 'changed', not 'created'.

+++ modules/node/node.module	28 Nov 2009 03:36:30 -0000
@@ -2001,13 +2010,130 @@
+    $cells = array();
+    $cells[] = theme('node_recent_content', array('node' => $node));
+    $cells[] = node_access('update', $node) ? l(t('edit'), 'node/' . $node->nid . '/edit') : '';
+    $cells[] = node_access('delete', $node) ? l(t('delete'), 'node/' . $node->nid . '/delete') : '';
+    $rows[] = $cells;

The common standard in core is:

$row = array();
$row[] = ...;
$row[] = ...;
$rows[] = $row;

+++ modules/node/node.module	28 Nov 2009 03:36:30 -0000
@@ -2001,13 +2010,130 @@
+    // TODO?: Apply a class to this link to match spec 
+    // see: http://drupal.org/node/337947#comment-2294824
+    $output .= l(t('Show all content'), 'admin/content');

Off-hand, I don't know of another link in core that is similar to this.

The most comparable link class is probably ".read-more", but that's entirely different.

I'd suggest to use list-all.

+++ modules/node/node.module	28 Nov 2009 03:36:30 -0000
@@ -2001,13 +2010,130 @@
+  else {
+    $output = t('No content available.');
+  }

This should not happen. Instead, node_block_view() should not return anything, so no block is displayed if there is no content.

+++ modules/node/node.module	28 Nov 2009 03:36:30 -0000
@@ -2001,13 +2010,130 @@
+  // TODO: Add classes to div blocks to allow themers to modify without 
+  // overriding theme function, need input on class names to use
+  $output = '<div>';

.node-title

.node-author

This review is powered by Dreditor.

Updated patch with all of sun's inputs corrected.

I believe all that is left are tests. Please review and let me know if anything is missing or needs correction.

+++ modules/dashboard/dashboard.css	30 Nov 2009 06:06:45 -0000
@@ -113,3 +113,7 @@
+#dashboard #block-node-recent div.content {
+  padding: 0 0 5px 1px;
+}

That is some strange padding. Are you sure this is required? If it really is, then I'd say that we have a styling problem elsewhere in the dashboard.

+++ modules/node/node.module	30 Nov 2009 06:06:46 -0000
@@ -1016,30 +1022,30 @@
-  
+

ok, hehe... I only wanted _you_ to not introduce new trailing white-space, not really to fix the entire file. ;)

That is, because it's more likely now that your patch will fail to be applied when another patch changing another part of node.module will get in.

I hope we are lucky and this won't happen. If it will, then you'll need to re-roll. ;)

+++ modules/node/node.module	30 Nov 2009 06:06:46 -0000
@@ -2006,13 +2015,140 @@
+    case 'recent':
+      if (user_access('access content')) {
+        $block['subject'] = t('Recent content');
+        $block['content'] = theme('node_recent_block', array(
+          'nodes' => node_get_recent(variable_get('node_recent_block_count', 10)),
+        ));
+      }
+      break;

This does not skip the block output if there are no nodes to display.

if (user_access('access content') && ($nodes = node_get_recent(variable_get('node_recent_block_count', 10)))) {
  $block['subject'] = t('Recent content');
  $block['content'] = theme('node_recent_block', array(
    'nodes' => $nodes,
  ));
}

+++ modules/node/node.module	30 Nov 2009 06:06:46 -0000
@@ -2006,13 +2015,140 @@
+ * @param integer $number

We do not specify data types in PHPDoc.

+++ modules/node/node.module	30 Nov 2009 06:06:46 -0000
@@ -2006,13 +2015,140 @@
+  if (!user_access('bypass node access')) {
+    // If the user is able to view their own unpublished nodes, allow them
+    // to see these in addition to published nodes. Check that they actually
+    // have some unpublished nodes to view before adding the condition.
+    if (user_access('view own unpublished content') && $own_unpublished = db_query('SELECT nid FROM {node} WHERE uid = :uid AND status = :status', array(':uid' => $GLOBALS['user']->uid, ':status' => NODE_NOT_PUBLISHED))->fetchCol()) {
+      $query->condition(db_or()
+        ->condition('n.status', NODE_PUBLISHED)
+        ->condition('n.nid', $own_unpublished, 'IN')
+      );
+    }
+    else {
+      // If not, restrict the query to published nodes.
+      $query->condition('n.status', NODE_PUBLISHED);
+    }
+  }

oh nice! Copied from node_admin_content(), I guess :)

This really cries for a helper function...

+++ modules/node/node.module	30 Nov 2009 06:06:46 -0000
@@ -2006,13 +2015,140 @@
+  $rows = array();
...
+  if ($rows) {
+    $output = theme('table', array('rows' => $rows));
...
+  return $output;

$output is not initialized.

+++ modules/node/node.module	30 Nov 2009 06:06:46 -0000
@@ -2006,13 +2015,140 @@
+  $l_options = array('query' => array('destination' => $_GET['q']));

Instead of defining the value for 'query' yourself, you use http://api.drupal.org/api/function/drupal_get_destination/7

This review is powered by Dreditor.

and back to needs work… :)

"That is some strange padding. Are you sure this is required? If it really is, then I'd say that we have a styling problem elsewhere in the dashboard."

I agree. The reason we needed to do this was because the table is supposed to touch the heading. The dashboard.css file defines padding on "#dashboard div.block div.content" that we have to unset.

---

"ok, hehe... I only wanted _you_ to not introduce new trailing white-space, not really to fix the entire file. ;)"

Dries latest commit introduced this white space, I believe. Before I had forgot configure removal of trailing white space in my Eclipse editor on save (recently reinstalled). So if I need to remove this portion I will have to manually remove it. I do update my files before creating patches so that should limit the chance of the patch not being applicable.

---

"This does not skip the block output if there are no nodes to display."

I made all of the changes then had an issue with my merge from the updated CVS file and forgot to add that specific logic back in when I rewrote :\ - corrected in patch

---

"We do not specify data types in PHPDoc."

We do define datatype in phpDoc, it's Drupal's Doxygen that doesn't, I'm having hard time transitioning... - corrected in patch

--

"oh nice! Copied from node_admin_content(), I guess :)"

I totally agree that this needs a helper, and yes - no need to reinvent? :D

---

"$output is not initialized."

Corrected in patch

---

"Instead of defining the value for 'query' yourself..."

Corrected in patch

Forgot to set status to 'needs review'

So, in actually testing this patch, I can't get any content show up in the block.
- Added it to the dashboard
- Ran cron to be sure…
- Created some articles
- The new content is not listed in this block
- Ran cron again…
- No content shown.

Logged in as user/1 on an otherwise unpatched checkout of head. Can somebody duplicate?

@sun

That is some strange padding. Are you sure this is required? If it really is, then I'd say that we have a styling problem elsewhere in the dashboard.

o wow, as I was typing the reason I introduced this I had an epiphany, reset.css and defaults.css both set border-collapse:collapse; but if I disable both of those and have FF fall back to the user-agent default, which is border-collapse:separate;, the issue disappears and the nasty 1px padding is no longer necessary
test and source

so is this an issue in the reset.css and defaults.css and should it be fixed there, or should we override it for this? I'd say the former, but I'm no expert on resets

Should be fixed over there. There's enough weirdness in Seven's CSS that needs clean up, lets not introduce fixes for that somewhere else (like in here :-)

Actually, I can't see why this patch shouldn't work.... didn't test though.

Speaking of tests.... no tests? yoroy wouldn't even have to test something if this patch would ship with its own tests.

+++ modules/dashboard/dashboard.css	30 Nov 2009 21:30:51 -0000
@@ -113,3 +113,7 @@
+#dashboard #block-node-recent div.content {
+  padding: 0 0 5px 1px;
+}

So we want to remove this.

+++ modules/node/node.module	30 Nov 2009 21:30:51 -0000
@@ -2006,13 +2015,141 @@
+        $block['content'] = theme('node_recent_block', array(
+          'nodes' => $nodes,
+        ));

No need to write this on multiple lines now.

This review is powered by Dreditor.

Due to debate of whether the fix to dashboard.css should be included, especially considering UI freeze 12/1, attached are two patches and two screenshots. Also the whitespace fixes from HEAD in node.module (seen in last patch) have been removed.

@sun

So we want to remove this.

actually, if we want the table to touch the header, we might still want to override the left, right and top padding of the content area as this is the default that dashboard uses:

#dashboard div.block div.content {
  padding:10px 5px 5px;
}

which seems reasonable, and if we drop the left, right and top padding defaults, every block on the dashboard will stick to the header, which I don't think is something we want to default to

So I take it this is dead now that UI freeze is done?

No, this is critical for a viable dashboard implementation. We have some extra room for selected issues, this will be one of them. Do you need people testing it before you continue?

+++ modules/node/node.module	1 Dec 2009 04:44:41 -0000
@@ -2006,13 +2015,141 @@
+      '#options' => drupal_map_assoc(array(2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 25, 30)),
+    );

Do we have exactly these numbers elsewhere? Recent comments block maybe? Could be a helper perhaps but not important.

+++ modules/node/node.module	1 Dec 2009 04:44:41 -0000
@@ -2006,13 +2015,141 @@
+/**
+ * Implementation of hook_block_save().
+ */

"Implements".

+++ modules/node/node.module	1 Dec 2009 04:44:41 -0000
@@ -2006,13 +2015,141 @@
+/**
+ * Returns a formatted list of recent nodes to be displayed in the recent content block.
+ *

Exceeds 80 chars. I'd just drop the last 8 words (end with 'nodes').

Screenshot looks lovely. Agreed with sun that this could use some tests - ought to be as little as creating some content then visiting the dashboard.

This review is powered by Dreditor.

Do we have exactly these numbers elsewhere? Recent comments block maybe? Could be a helper perhaps but not important.

Yes, these numbers are from comment.module.

I'll fix the comments.

Also, I absolutely want to write the tests for this. I have a short contract in progress right now so I will need some time to get the tests written.

I'll try to start to write tests for this.

This patch uses the 'without_dashboard_css' as its base as I think that it's a separate issue altogether. I also think that the helper for the common numbers is a separate issue, so I didn't work on that in this patch. The test is *largely* based off the recent comments test, so it probably will fail with this patch.

Here's something that should fix it. Also takes care of the extraneous whitespace.

Okay, 3rd time lucky. This time, the patch passes on my local machine (all but one of the "Node (not) found in block" messages fail). Looks like it's just a matter of fixing the assert statements. I think I've taken care of all the syntax/typo errors. Also takes into account the change made by #635094: Unify "language neutral" language codes. I need some sleep now, so anybody's welcome to continue on this.

    // Test that a user without the 'access content' permission can not see the block.
    $this->drupalLogout();
    $this->drupalGet('');
    $this->assertNoText($block['title'], t('Block was not found.'));

I think this test fails because the anonymous user has 'access content' permission. You need to create a user without that permission. Also, the comment in that block needs to wrap at 80 characters.

// Test the only the 2 latest nodes are shown and in the proper order.

Should be "Test that ...".

Still shows up as 7 fails on my local machine, but let's see what the test bot has to say. Also, fixed both of the comments.

Let's see how this does now that I figured out how to revoke the 'access content' privilege from anonymous users.

In your patch, this $node1->title['zxx'][0]['value'] should be this $node1->title[LANGUAGE_NONE][0]['value'] in a number of places.

Patch contains a number of debug statements.
+ debug('Node "'.$node1->title['zxx'][0]['value'].' created.');

// Test that only the 2 latest nodes are shown and in the proper order.
$this->assertNoText($node1->title['zxx'][0]['value'], t('Node not found in block.'));
$this->assertText($node2->title['zxx'][0]['value'], t('Node found in block.'));
$this->assertText($node3->title['zxx'][0]['value'], t('Node found in block.'));
$this->assertTrue(strpos($this->drupalGetContent(), $node3->title['zxx'][0]['value']) < strpos($this->drupalGetContent(), $node2->title['zxx'][0]['value']), t('Nodes were ordered correctly in block.'));

I believe the problem with this set of tests is that all of your nodes are getting created with the exact same $node->changed timestamp. You might be able to do something like $default_settings['changed'] => time() - 100; , though I'm not sure if that will work or not.

Yeah it seemed that the failing tests were due to the fact that the "changed" time of the nodes are all the same so ordering could not be tested properly.

I added the following for node2 and node3. I was attempting to set it in the $default_settings variable but it seems that "drupalCreateNode" overrides the changed value. :(

db_query('UPDATE {node} SET changed = :changed WHERE nid = :nid', array(':changed' => $node1->changed + 100, ':nid' => $node2->nid));
db_query('UPDATE {node} SET changed = :changed WHERE nid = :nid', array(':changed' => $node1->changed + 200, ':nid' => $node3->nid));

Also the last set of tests were testing on "admin/structure/block/manage/node/recent/configure" instead of a page where the block would be displayed.

These tests now pass although I do not know what the proper solutions for the changed date would be I would think that we should be able to override the changed value from the passed in $default_settings.

** Made suggested changes from #139 http://drupal.org/node/337947#comment-2341644

Thanks,

Jonathan

Just a quick question. Would using db_update() be better in this case since it's really a pretty simple thing or is db_query more efficient?

db_query() is only supposed to be used for select queries. Any time you're updating or deleting, you have to use the query builder. There's pretty good documentation, fortunately.

Changing "changed" time for node to db_update instead of db_query.

+    db_update('node')
+      ->fields(array(
+        'changed' => $node1->changed + 100,
+      ))
+      ->condition('nid', $node2->nid, '=')
+      ->execute();
...
+    db_update('node')
+      ->fields(array(
+        'changed' => $node1->changed + 200,
+      ))
+      ->condition('nid', $node3->nid, '=')
+      ->execute();

Thanks,

Jonathan

Looks good to me, but I'll won't mark this as RTBC it since I'm a collaborator of this patch.

I glanced quickly over the code and it looks good. I tried it out and it works fine. I am wondering about the styling... maybe it's a Seven thing, but it seems odd to have a border around the bottom and sides but not the top. Here's a screenshot of it as it appears on the dashboard:

+++ modules/node/node.test	8 Dec 2009 14:10:35 -0000
@@ -1170,3 +1170,107 @@ class NodeFeedTestCase extends DrupalWeb
+  protected $admin_user;
+  protected $web_user;
+  protected $anon_user;
+  

These are technically not required.

Also note the trailing white-space here.

+++ modules/node/node.test	8 Dec 2009 14:10:35 -0000
@@ -1170,3 +1170,107 @@ class NodeFeedTestCase extends DrupalWeb
+      ->condition('nid', $node2->nid, '=')
...
+      ->condition('nid', $node3->nid, '=')

'=' can be removed here.

+++ modules/node/node.test	8 Dec 2009 14:10:35 -0000
@@ -1170,3 +1170,107 @@ class NodeFeedTestCase extends DrupalWeb
+    $node3 = $this->drupalCreateNode($default_settings);
+    // Change the changed time for node so that we can test ordering.

(and elsewhere) We could squeeze some blank lines in here to increase readability in the test flow.

+++ modules/node/node.test	8 Dec 2009 14:10:35 -0000
@@ -1170,3 +1170,107 @@ class NodeFeedTestCase extends DrupalWeb
+    //$this->assertNoText($block['title'], t('Block was not found.'));

?

+++ modules/node/node.test	8 Dec 2009 14:10:35 -0000
@@ -1170,3 +1170,107 @@ class NodeFeedTestCase extends DrupalWeb
+    $this->assertTrue(strpos(strip_tags($this->drupalGetContent()), $node3->title[LANGUAGE_NONE][0]['value']) < strpos(strip_tags($this->drupalGetContent()), $node2->title[LANGUAGE_NONE][0]['value']), t('Nodes were ordered correctly in block.'));

You want to use xpath here instead.

I'm on crack. Are you, too?

@axyjo and @grndlvl

+++ modules/node/node.test	8 Dec 2009 14:10:35 -0000
@@ -1170,3 +1170,107 @@ class NodeFeedTestCase extends DrupalWeb
+    //$this->assertNoText($block['title'], t('Block was not found.'));

Looks like we have one test that wasn't finished up. To do this we will need to make a role that does not have the 'access content' permission. This will require changing the permissions of the Anonymous User role. As part of this the $anon_user property can be entirely removed... or we can completely remove this test as I'm not sure it is needed.

// Test that block is not visible without nodes
$this->drupalGet('');
$this->assertNoText($block['title'], t('Block was not found.'));

Additionally we can add the above code to the test prior to nodes being added within testRecentNodeBlock().

Thank you two for picking up with these tests. I am under a contract that is finishing up in the next few days and this patch would not be at this stage yet without your help.

@ksenzee The styling issue of the block is due to padding being added to content within dashboard.css. See http://drupal.org/comment/reply/337947#comment-2324118 and previous comments regarding this issue.

This review is powered by Dreditor.

I'll try to reroll it with the suggestions after school today (unless grndlvl beats me to it). Thank you, ksenzee, sun and CodyCraven.

I have applied the suggestions from #147 && #148.

- removed unnecessary protected variable definitions.
- cleaned up
- removed equal signs from condition method.
- replacing check for order of nodes in block to xpath

xpath check

<?php
$this->assertTrue($this->xpath('//div[@id="block-node-recent"]/div/table/tbody/tr[position() = 1]/td/div/a[text() = "' . $node3->title[LANGUAGE_NONE][0]['value'] . '"]'), t('Nodes were ordered correctly in block.'));
?>

not getting any node titles

+function theme_node_recent_content($variables) {
+  $node = $variables['node'];
+
+  $output = '<div class="node-title">';
+  $output .= l($node->title[LANGUAGE_NONE][0]['safe'], 'node/' . $node->nid);
+  $output .= theme('mark', array('type' => node_mark($node->nid, $node->changed)));
+  $output .= '</div><div class="node-author">';
+  $output .= theme('username', array('account' => user_load($node->uid)));
+  $output .= '</div>';
+
+  return $output;
+}

$node->title[LANGUAGE_NONE][0]['safe'] isn't defined, changed this to $node->title[LANGUAGE_NONE][0]['safe_value']

also, I noticed that all dashboard blocks got a border by #633086: Dashboard visual design improvements

does this mean we're dropping the border on the table, or should we just add a proper thead with like "title" "author" "operations" or w/e?
coz right now it looks pretty funky with the double border that doesn't span the top of the table

attached patch just changes $node->title[LANGUAGE_NONE][0]['safe'] to $node->title[LANGUAGE_NONE][0]['safe_value']

Definately getting node titles now. Liking it. Any chance we can link the word 'content' in the 'Recent content' h2?

As for the styling: why have the borders show at all?

Attached patch should make it look like the above. Bluntly added these styles to dashboard.css because this is a rather dashboard specific override. Happy to be told otherwise ;)

yea see, much better without the borders

but uhm, why did u move the "show all content" to the right?

Any chance we can link the word 'content' in the 'Recent content' h2?

I think that would require some serious fiddling and it would effectively put the same link on this page 3 times, lil over the top if u ask me :P

I'm also not entirely sure on where to put these styles, as node.module is required and dashboard.module isn't, it would seem a bit silly to be loading these styles all the time, so I guess dashboard.css does seem like the better place to put it as those styles are only loaded on the dashboard

granted, with aggregation, it rly wouldn't matter

Well, both Mark's as my design have links in the header and bottom right in the design:
http://drupal.org/node/337947#comment-2208774
http://www.flickr.com/photos/mboulton/3748580107/sizes/o/

For the header I suspected it would be tricky, so we'll let it slide for now :)
For the right aligned one underneath, the idea was to make the link relate to the operation links.

oh ok, makes sense to me :)

come to think of it, linking a part of the title wouldn't be that hard, just change this to include a link:

+        $block['subject'] = t('Recent content');

I was going to test this patch, but it no longer applies for me. Needs a reroll?

Dries, I just patched up my D7 to match HEAD and yoroy's patch applied fine. Is there some sort of discrepancy between the test server(s) and HEAD?

Here is what I get:

deimos:drupal-cvs dries$ patch -p0 < f.p
patching file modules/node/node.module
Hunk #2 succeeded at 2034 (offset 3 lines).
Hunk #3 succeeded at 2044 (offset 3 lines).
patching file modules/node/node.test
patching file profiles/default/default.install
patching file modules/dashboard/dashboard.css
patch unexpectedly ends in middle of line
patch: **** malformed patch at line 335

yeh I'm getting the same

reroll attempt

Applied patch, but none of my content is displaying. :/ See screenshot.

Assuming it's not me causing the above problem, I think standard D7 phrasing for empty text is "No content available."

FYI:

patching file modules/dashboard/dashboard.css
Hunk #1 succeeded at 113 (offset -33 lines).
patching file modules/node/node.module
Hunk #1 succeeded at 157 (offset -13 lines).
Hunk #2 succeeded at 1978 (offset -56 lines).
Hunk #3 succeeded at 1988 with fuzz 1 (offset -56 lines).
patching file modules/node/node.test
Hunk #1 succeeded at 1072 (offset -98 lines).
patching file profiles/default/default.install
Hunk #1 succeeded at 121 (offset -75 lines).

This is one of those patches you want to apply to a fresh checkout of head *before* installing, only then can the system know it's there.

- Patch in #161 applies, thanks seutje.
- It works as expected. I added some content and it looked like this:

I reviewed the patch and the formatting looks good and the code (logically) makes sense. So I guess we will just wait on the testing system to stop postponing and hopefully it will still apply if there are any commits prior to this patch being tested.

bash-3.2$ patch -p0 < ../recentcontent-reroll.patch
patching file modules/dashboard/dashboard.css
patching file modules/node/node.module
patching file modules/node/node.test
patching file profiles/default/default.install
Hunk #1 succeeded at 186 (offset -10 lines).

... o.O

reroll to get rid of offset I suppose

I broketeth it? :(

I don't rly see how I could of broken filefield uploads though... help?

I think this might be to an issues that got committed and not an issue w/ this patch because all the latest patches have failed on w/ the same test failing on testing.drupal.org.

definitely a bad commit

RTOTHEBC

Applying #167 adds the recent content block as described, but content only appears after the caches have been cleared.

Steps to reproduce:
1. apply patch #167
2. add 'recent content' block to dashboard
3. create some content
3. visit dashboard

expected: see recently created items in 'recent content' block
actual: 'recent content' block remains empty
workaround: clear caches

In contrast: the 'recent comments' block added to the dashboard *does* immediately show recent comments without needing to clear the caches.

The admin dashboard should not be cached?

Like:
/admin/content/node
/admin/content/comment/new

errr, these are blocks and thus act as such

and any block to any module's hook_block and it won't appear until the cache is cleared afaik

as stated by yoroy in #163, one should apply patches prior to installing, pretending it were in the package as downloaded

Sorry, folks but this will need some re-roll action after #571654: Revert node titles as fields.

This is also the next "biggie" patch on my radar to get in before alpha. Could someone re-roll?

I think it's because we haven't looked at color usage here yet :)

rerolled

peximo, your last patch was perfect thank you.

Due to the display of "(empty)" in Dashboard when no content has been created in the system yet, a UX decision has been made by yoroy to output the text "No content available."

The output of this message creates consistency with admin/content which outputs the same message when no content has yet been created.

Attached is the modified patch including a new test case to replace the block not found assertion.

I know you're never supposed to tell a designer that there's too much whitespace, but I think there's too much whitespace. ;) Or at least those gaps on the sides of the table look weird.

As loathe as I am to introduce more obvious visual bugs though, it's also an obvious visual bug for the Dashboard to look so... empty... by default. I also have a feeling these issues may be caused by/fixed in #563390: Seven theme lacks formatting on common html elements such as lists, paragraphs & <code>, and this poor patch has been through quite the hassle on its way towards being committed.

I looked through the code one more time and didn't spot anything obviously horrible, and it comes with tests to prove it works, so w00t!

Committed to HEAD. Marking back to code needs work to sort out the spacing bugs.

Thinking about this while away from computer (currently on BlackBerry)...

Since Garland and Seven are both affected by the weird whitespace in Dashboard for this block I am thinking that the issue is within dashboard.css and not either theme. If this is the case then it will be easy to add a little code to dashboard.css setting the H2 and .content elements to remove the padding that was recently added.

I will be at my computer in a couple hours and check on this if no one beats me to it.

dashboard.css is the culprit.

Attached is a patch which sets the padding on .content for the recent content block to 0.

Oops, had a path issue within the patch

Bump, just need a quick review by UX team and a yay or nay on the patch so we can close this out.

No screenshot?

Sorry, screen shot attached.

When the issue first arose I believe there was more whitespace, but I may be mistaken.

The "list-all" class introduced by this patch should be "more-link" to be consistent with other blocks that Drupal provides. Please consider changing this.

Here's a screenshot that demonstrates 3 other core blocks that use .more-link:

1. Active forum topics
2. New forum topics
3. Recent blog posts

Go, Jacine, go! :)

No idea what you are RTBC'n, there are like 4 diffrent patches in this issue. It seems Jacine only touches a small part of that,

Here's an updated patch that also removes the CSS the dashboard is providing for this. There is styling for .more-link in system.css that takes care of aligning the text right, and "magically" fixes the whitespace issue. Anything else that is needed to achieve Seven-specific styling should be done in the Seven theme.

Here's a screenshot showing the side-by-side comparison of before and after my patch: http://www.flickr.com/photos/jacine-rodriguez/4354240042/sizes/l/

Sorry, I have a bad habit of deleting the last line at the end of files. Use this patch instead.

There's another problem with this block.

When a user does not have access to edit or delete the nodes in the list, the result is empty table cells. This isn't immediately apparent because neither Seven nor Garland have vertical borders on their tables, but it's not acceptable.

I've updated the patch to fix this too.

EDIT: ignore this patch, see the one in the next comment.

Jeez. Sorry for the inbox spam/bad patch. Please ignore the last one and use this.

Jacine,

Your last patch does not address one important point. If a user has permissions to edit or delete one node, but not another the formatting of your table can very easily be messed up. The reason being is that your check decides row by row to to output the number of td tags and makes no adjustments in either appending td tags necessary or setting the colspan attribute; this will result in rows with varying numbers of cells.

I decided against the functionality with yoroy as the loop needed would be very complex to reside in a theme hook.

See comments #107 and #108 above.

ok, merged both patches, and can be please be done with this issue now? :P ;)

Incorporated #206. Now for real!