The title says it.

Steps to test

  1. clean install of 8.x
  2. drush dl devel -y
  3. drush en devel_generate -y
  4. drush genc --types=page 13
  5. add new block via comment #6 or #7

Comments

oadaeh’s picture

I don't want to hold anyone up who may also be working on this, but I have started this, and I will have patch options as both a display of the curent admin/content view, and as its own view later today.

oadaeh’s picture

Issue tags:+VDC

Forgot the tag.

bdone’s picture

@oadaeh: still actively working on this? i'd like to try a pass, if you wouldn't mind.

oadaeh’s picture

@bdone: I haven't had the time available to do anything with this for the past two or three weeks, and I'm sure what I did do would need to be updated to the current state of core, so go for it.

bdone’s picture

Assigned:Unassigned» bdone
bdone’s picture

Status:Active» Needs work
StatusFileSize
new11.33 KB
PASSED: [[SimpleTest]]: [MySQL] 57,932 pass(es).
[ View ]

a few questions to get started...

  1. what view should the new block display go in?
    • a new content_recent view? (see attached patch)
    • or, the existing "content" view
  2. seems like the "More" link should be hidden, when users do not have the "Access the Content overview page" permission. how should access checks for the "More link" to /admin/content be handled?

steps to enable this block:

  1. enable new "Recent content" view at /admin/structure/views
  2. add block display: /admin/structure/block/add/views_block%3Acontent_recent-block_1/bartik
bdone’s picture

StatusFileSize
new10.52 KB
PASSED: [[SimpleTest]]: [MySQL] 57,846 pass(es).
[ View ]

alternate patch to add a block display to the existing "content" view, per comment #6 bullet 2.

steps to enable this block:

  1. add block display: /admin/structure/block/add/views_block%3Acontent-block_1/bartik
bdone’s picture

StatusFileSize
new204.08 KB
new248.14 KB

adding screenshots for #2020393-7: Convert "Recent content" block to a View...

as user 1:
2020393-7-auth-user.png

as anonymous:
2020393-7-anon-user.png

dawehner’s picture

Thank you very much on working on this issue.

views.view.content.yml is the admin view (admin/content/node) so I would suggest to create a separate view for the block.

dawehner’s picture

We can now override the items per page, let's get this issue up to speed again.

bdone’s picture

StatusFileSize
new17.75 KB
FAILED: [[SimpleTest]]: [MySQL] 57,923 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

this still needs work, but currently adds the following:

  • added filter "Content: Published status or admin user" to hide unpublished nodes as needed.
  • dropped RenderContentBlock and its theme related functions
  • added More link. since there is no page display for this block to link to, the "More link" is uses a custom URL under "Link display".

how should access checks for the "More link" to /admin/content be handled? it yields access denied for users without "access content overview". the non-views block used...

if (user_access('access content overview')) {
  $more_link = array(
    '#theme' => 'more_link',
    '#url' => 'admin/content',
    '#title' => t('Show more content'),
  );
  $output .= drupal_render($more_link);
}
bdone’s picture

regarding more links access, let's address that separately in #2053015: Add user access checks to 'more' link.

bdone’s picture

StatusFileSize
new14.4 KB

@dawehner, thank you for pointing out the option in #10. the patch in #11 works well with it, because it's a default.

Screen Shot 2013-07-29 at 4.03.53 PM.png

dawehner’s picture

I would personally say that removing the real more link here is ok, though yeah this needs feedback.

Currently on the recent content block there is a "more" link to admin/content which for sure needs an access check. Views does not have the concept of access check for the read more link.

There are three opportunities:

  • Skip the readmore link
  • Let the readmore link appear all the time
  • Build in a different plugin in views for just this special readmore link.
dawehner’s picture

Add bojhan for an oppinion.

Bojhan’s picture

I don't think it makes a whole lot of sense to include a link that goes to admin.

We can add a read more link but ideally that is an actual page that lists the recent content (somewhat like /node). It is what I would expect, and it could be a page view under this view. Given that this is the only block that does this, I think its up to committers if they really think its needed. I don't think so.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new17.75 KB
FAILED: [[SimpleTest]]: [MySQL] 57,627 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

Uploading the actual patch file.

Status:Needs review» Needs work

The last submitted patch, vdc-2020393-17.patch, failed testing.

oadaeh’s picture

Status:Needs work» Needs review
StatusFileSize
new23.14 KB
PASSED: [[SimpleTest]]: [MySQL] 58,875 pass(es).
[ View ]
new3.61 KB

Updated patch.

I also removed the node block test that was failing (due to the block being removed).

A screen shot is attached, too.

views-recent-content.png

oadaeh’s picture

Assigned:bdone» Unassigned

Unassigning.

oadaeh’s picture

Issue summary:View changes

bdone: adding steps for manual tests

dawehner’s picture

Issue summary:View changes
Status:Needs review» Reviewed & tested by the community
StatusFileSize
new24.51 KB
FAILED: [[SimpleTest]]: [MySQL] 59,472 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

Just a simple rerole.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 21: vdc-2020393.patch, failed testing.

pcambra’s picture

Status:Needs work» Needs review
StatusFileSize
new23.25 KB
PASSED: [[SimpleTest]]: [MySQL] 59,491 pass(es).
[ View ]

Not sure why the class SyndicateBlock is being removed in #21, here's a reroll without that

dawehner’s picture

Ups, I am sorry for that.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

back to RTBC

webchick’s picture

Status:Reviewed & tested by the community» Needs review

I left a big-ass review of a sister issue at #2020399-54: Convert "Who's online" block to a View, much of which may apply here, except...

I'm not even getting to step 1 in my review, because the block is not showing up in the block listing, even after a fresh install. :( Can anyone else reproduce?

webchick’s picture

Status:Needs review» Needs work
Issue tags:+alpha target

No longer applies. Would be great to get into the next alpha though, so tagging.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new23.24 KB
PASSED: [[SimpleTest]]: [MySQL] 59,173 pass(es).
[ View ]

just a straight reroll.

jibran’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC.

dawehner’s picture

StatusFileSize
new23.23 KB
PASSED: [[SimpleTest]]: [MySQL] 59,438 pass(es).
[ View ]
new443 bytes

doh.

The last submitted patch, 30: vdc-2020393.patch, failed testing.

webchick’s picture

Status:Reviewed & tested by the community» Needs work

Doing testbot's job for it...

olli’s picture

30: vdc-2020393.patch queued for re-testing.

olli’s picture

  1. +++ b/core/modules/node/config/views.view.content_recent.yml
    @@ -0,0 +1,461 @@
    +      link_url: admin/content

    I think this is not needed/used anymore.

  2. +++ b/core/modules/node/config/views.view.content_recent.yml
    @@ -0,0 +1,461 @@
    +      use_more: '1'

    Can we set the more link to "No"?

  3. +++ /dev/null
    index 70598ee..0000000
    --- a/core/modules/node/lib/Drupal/node/Tests/NodeBlockFunctionalTest.php

    --- a/core/modules/node/lib/Drupal/node/Tests/NodeBlockFunctionalTest.php
    +++ /dev/null

    Instead of removing this test completely, could we change it to test the views block?

  4. +++ /dev/null
    @@ -1,153 +0,0 @@
    -    // Enable the "Powered by Drupal" block only on article nodes.
    -    $block = $this->drupalPlaceBlock('system_powered_by_block', array(
    -      'visibility' => array(
    -        'node_type' => array(
    -          'types' => array(
    -            'article' => 'article',
    -          ),
    -        ),
    -      ),
    -    ));
    -    $visibility = $block->get('visibility');
    -    $this->assertTrue(isset($visibility['node_type']['types']['article']), 'Visibility settings were saved to configuration');
    -
    -    // Create a page node.
    -    $node5 = $this->drupalCreateNode(array('uid' => $this->adminUser->id(), 'type' => 'page'));
    -
    -    // Verify visibility rules.
    -    $this->drupalGet('');
    -    $label = $block->label();
    -    $this->assertNoText($label, 'Block was not displayed on the front page.');
    -    $this->drupalGet('node/add/article');
    -    $this->assertText($label, 'Block was displayed on the node/add/article page.');
    -    $this->drupalGet('node/' . $node1->id());
    -    $this->assertText($label, 'Block was displayed on the node/N when node is of type article.');
    -    $this->drupalGet('node/' . $node5->id());
    -    $this->assertNoText($label, 'Block was not displayed on nodes of type page.');

    Removing this does not seem related to me.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new19.68 KB
PASSED: [[SimpleTest]]: [MySQL] 59,720 pass(es).
[ View ]
new2.38 KB

I think this is not needed/used anymore.

We need this for the more link to point to "admin/content".

Can we set the more link to "No"?

As before, this was clearly part of the original code:

-    if (user_access('access content overview')) {
-      $more_link = array(
-        '#theme' => 'more_link',
-        '#url' => 'admin/content',
-        '#title' => t('Show more content'),
-      );
-      $output .= drupal_render($more_link);
-    }
-  }
-

Instead of removing this test completely, could we change it to test the views block?

Okay, let's try to keep this test coverage.

olli’s picture

Right, I misunderstood #14 / #16. Simplytesting #35: the link is not rendered and editing link display show that the link url is empty..?

dawehner’s picture

StatusFileSize
new20.26 KB
PASSED: [[SimpleTest]]: [MySQL] 60,166 pass(es).
[ View ]

Rerolled and opened a follow up after some discusion with xjm: #2162073: Switch the recent content block to use a list instead of a table.

xjm’s picture

Component:node.module» node system

(Merging "node system" and "node.module" components for 8.x; disregard.)

dawehner’s picture

37: vdc-2020393.patch queued for re-testing.

jibran’s picture

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeBlockFunctionalTest.php
@@ -100,13 +100,13 @@ public function testRecentNodeBlock() {
-    $this->assertTrue($this->xpath('//div[@id="block-test-block"]/div/table/tbody/tr[position() = 1]/td/div/a[text() = "' . $node3->label() . '"]'), 'Nodes were ordered correctly in block.');
+    $this->assertTrue($this->xpath('//div[@id="block-test-block"]//table/tbody/tr[position() = 1]/td/a[text() = "' . $node3->label() . '"]'), 'Nodes were ordered correctly in block.');

Seems like div structure has changed.

dawehner’s picture

So? Views is adding additional divs, that is no problem. If it is, fuckoff.

andypost’s picture

Suppose this should not be a straight conversion because usability and accessibility issues are closed|postponed on this

  1. +++ b/core/modules/node/config/views.view.content_recent.yml
    @@ -0,0 +1,461 @@
    +        edit_node:
    ...
    +        delete_node:

    We have no dashboard now, please drop this useless links, as was suggested in #849670: Recent content block usability and #2162073: Switch the recent content block to use a list instead of a table.

  2. +++ b/core/modules/node/node.module
    @@ -626,9 +620,6 @@ function node_preprocess_block(&$variables) {
    -      case 'node_recent_block':
    -        $variables['attributes']['role'] = 'navigation';
    -        break;

    Is there a way to add this via views?

dawehner’s picture

Regarding 2) No there is not and jessebeach was 100% okay with removing them.

We have no dashboard now, please drop this useless links, as was suggested in #849670: Recent content block usability and #2162073: Switch the recent content block to use a list instead of a table.

You can figure out later how we replaced this table by some other thing, though we have tests for that, which would blow up the patch and make the freaking thing harder to review.

klonos’s picture

Issue summary:View changes
Parent issue:» #1895160: Convert admin/content to a View, keep a non-views fallback with no bulk operations
Related issues:+#1823450: [Meta] Convert core listings to Views
jibran’s picture

So? Views is adding additional divs, that is no problem. If it is, fuckoff.

hehe. Sorry to offend you but I was just pointing it. And TBH it is not an issue.

olli’s picture

Nice work with the test, thanks!

We need this for the more link to point to "admin/content".

I could not see the more link in #36.

olli’s picture

Status:Needs review» Needs work

.. and does not apply anymore.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new19.61 KB
PASSED: [[SimpleTest]]: [MySQL] 59,363 pass(es).
[ View ]

Rerolled.

dawehner’s picture

48: vdc-2020393.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 48: vdc-2020393.patch, failed testing.

jibran’s picture

Status:Needs work» Needs review

48: vdc-2020393.patch queued for re-testing.

damiankloip’s picture

Status:Needs review» Reviewed & tested by the community

Looks good!

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Cool, validated that this does show up now. :) The block kinda looks like crap, but that's what HEAD looks like too. (I think this block was originally designed for the dashboard.)

Committed and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.