Allow more users to see the node admin page

--- INSTALL.txt	18 Jul 2008 07:24:29 -0000	1.70
+++ INSTALL.txt	30 Aug 2008 12:53:03 -0000
@@ -1,5 +1,7 @@
 // $Id: INSTALL.txt,v 1.70 2008/07/18 07:24:29 dries Exp $
 
+hey
+

So we discussed this at the drupalcon usability sprint and I was working on the patch with skilip.

The idea is to allow any user with create/edit $foo type to see admin/content/node - then progressively show more nodes and/or operations the more permissions they have. In this iteration, we only show users content which they have access to edit (maybe access to view if n.uid = u.uid as well) - so they get a list of all 'their' content - everything else, including the form and the mass operations checkboxes gets hidden unless you have administer nodes. A followup patch might allow for sticky/front page etc. split into distinct permissions, which would allow content editors to do bulk operations without administer nodes too.

Seems reasonable to me. Note, however, that on many sites users can create content which they are then unable to view/edit. I think rather than trying to come up with "reasonable" rules for which nodes a user should be able to see that this code should just respect the node access system which is more drupalish and should be easier to write since it leverages db_rewrite_sql (or whatever the equivalent for that is under D7).

I'm also not sure that it is "critical" but defer to those working on the patch.

db_rewrite_sql() seems reasonable - it's not retired yet, so presumably we could add it here without too much trouble and it'll get hit in the general conversion. In that case though, we might want an admin/content/node/%user so that people can find "their own stuff" - since on large sites, just view permissions is going to be very wide. I won't be able to do any work on this for at least a couple of days but any ideas welcome.yy
as to critical - users creating pages, then never being able to find them again we think is critical. Whether this solution is the best possible one I don't know, but it's definitely the least worst option re-using pages which are already in core (i.e. preferable to tracker.module).

Skilip: probably best to add a new comment with the updated patch and see if DrupalTestbedBot is still unhappy or not.

First of all, your code does not conform to coding standards: if space (condition) { newline.... newline } is what you wanted. coder module is your friend.

Second, because the page is no longer under admin/* the admin theme won't fire I think.

Added a better title. Feel free to edit it but the "where did my content go" is not really a good one... (despite it does solve that problem but still)

needs review

I started an issue on this subject a while back over at #239816: admin/content/node does not allow for proper permissions usage, great to see a push to get this changed.

A few thoughts on the patch:
1). After installing and setting up a user account to test this with, I found it pretty confusing that the list was initially empty. This is because although the user account I created had access to the content menu, it didn't have a "edit any %s content" permission set, and the nodes I generated weren't owned by it. This brings up two points... I know I said otherwise in the issue I linked above, but I've actually come around to thinking it should list all content, but only provide an edit link to nodes the current user can edit. Additionally, as long as we're making usability improvements to this page, a blank state screen would be nice.

2). Any reason not to leave the filtering options intact for everyone with access to this page? I'm not seeing it on the unprivileged account I created. this is because of the #access => user_access('administer nodes') on the fieldset. This is a must have for any site with a lot of content.

3). As chx said, with the page outside of the admin/ path, the admin theme won't pick up. I don't think it should be moved outside of the admin path, but if it were, it makes sense to take the rest of the "content management" section with it. It's confusing to have both a "content" and "content management" section - there's no clear distinction between the two.

We could probably add #access 'administer nodes' to the mass operations on that fieldset, and leave it open for the filtering. However I'd quite like to completely rework those filters and make the permissions for various bits more granular, I'm also not sure if the taxonomy (and etc.) filters are covered by db_rewrite_sql or not, this was the original motivation for hiding the entire fieldset there.

A blank state message is a good idea.

As to the path, I think we should leave it as admin/content/node - if you navigate to that path directly on HEAD as an authenticated user with 'administer nodes' permission it shows up just fine, there's just no link to it in the navigation menu. So we'd need an additional direct link from navigation which we can do with menu_link_save(). If we can get #301902: Allow more users to see the node admin page in then the duplication of the link wouldn't be a big deal.

If we show all content the user has access to (+ permissions based edit links) then it'd be good to have admin/content/node/user with just a list of content created by the user as well.

Had a few minutes to put into this tonight.

Here's what's included:

• Moved the screens back to admin/content/node
• Created a "My Content" tab, this is now the default local task. Moved "All Content" to admin/content/node/all
• Restored filtering permissions for non node-admins
• Removed the check to make sure the user owns or can edit the content being displayed, moved that check to the 'edit' link. So at this point on the "All content" tab, users can see all content they have permission to view (respecting db_rewrite_sql), but only have edit links for content which they can edit. This addresses greggles feedback in #3.
• added a ContentAdminPagesCase test to node.test, to 'prove' this is all working

Looking forward to feedback.

@skillip: I feel like it's more intuitive to have the separate tabs for those two lists. That being said, I'm totally willing to go with what the community thinks on the issue... or better yet what usability testing shows is the best route to go on the issue.. is the UTS far enough along we could even hope to quickly get an answer via testing on this sort of thing?

Secondly, I left the admin/content/node page permissions how you left off, the permission tweak I'm referring to is removing the check to make sure a user owns/can edit content before it's displayed on the Content screen. Specifically

 $edit = node_access('update', node_load($node->nid));
 if (!$edit && ($user->uid !== $node->uid)) {
   continue;
 }

I feel like if we're trying to solve the issue of "where did my content go" then we should make sure content editors can at least see content they have access to view listed on this screen. Is it not better to at least show them the content in the site without an edit link, than to not list the content at all? Again, this is something that I'm 'on board' with what the community thinks. I see now, from reading catch's comments above more closely, that perhaps that issue has already been decided on, and the check should be re-added to the patch. It's a sticky situation though, since in it's current form it throws pagination off, since editing isn't accounted for in the query as it currently stands. Tossing out results after the query has ran, so you have 50 - (number of items you can't edit) items listed on the page isn't the solution we're looking for, imho.

I think based on this discussion it's fairly obvious that the code needs some work.

Off topic:

Adding the menu-item under 'admin/..', would cause permission problems for users who has no administration rights

this reflects a deep misunderstanding about how the Drupal 6 menu system works -- I am worried about how widespread could be and what the consequences could be security wise. But in D6 and onwards, every page on it's own. In D5 you had access precalculated for all paths there was -- in D6 the access is calculated on runtime so calculating all parents would be very expensive. Edit: not to mention that calculating parents is not tirival either!

sorry :(

@catch: looking through the code it looks like node_filters() uses taxonomy_get_vocabularies() and taxonomy_get_tree() to build those taxonomy filters, both of those functions use db_rewrite_sql() to fetch a list of vocabularies/terms. Looks like we're already covered there.

beeradb: that's good to know. I'm also leaning towards the 'my content' tab, then the 'all the stuff I can see, with edit links if I can edit' doing what the current page does. I can get 'my issues' by going to http://drupal.org/project/issues/search - but it takes about four times as long to do that as going to project/issues/user. Could be a followup patch though, but IMO so should adding a filter by user to that page (and changing how the filters work in general).

@chx: while the page itself is accessible, the link to it is hidden - I was half expecting a 'content' link to show up in the navigation menu in HEAD as a user with only 'administer nodes' permission. Do we need to create an extra menu link for this in the default profile or is there another way around that?

Marking back to needs review since I think the approach to this needs discussion more than the patch itself, and we'll get more eyes on the issue that way.

The primary goal of this patch is to provide a clear overview of MY content, that is what the usability tests showed to be a Big Problem. All other additions we're discussing here are exactly that, additions.

I realise it's all related and it might be handy etc. but we;re trying to improve usabilty for novices here, let's not confront them with extra features they are initially not interested in.

Adding tabs and/or filters for 'all editable content' and 'all content' is nice but the first view should be a list of MY content.
Can we focus on getting this initial idea right first and discuss further options in follow-up patches or are these issues to much intertwined to do that?

Agreed, let's move on with the initial idea.

BTW -- any takers on "Fix our broken list filters and bring them to the "Smart Folder" era"?

So, I talked with beeradb in IRC a bit and I now have a better understanding of his concerns. It's important we do not cripple this page for users who DO have the 'administer nodes' permission. Meaning, we should not reduce this page to _only_ show own content. I do think this should be the initial view, but we have to keep the existing functionality of this page intact.

This could be solved by tabbing this page: [my content] and [all content]. Probably the easiest/quickest way?

Another option would be to add this distinction to the filtering options on the top of the page. Which, as kika points out, aren't really up to par at the moment and would certainly need it's own issue & patch.

So, adding the [my content] and [all content] tabs seems like the best idea and within scope of this issue, right?

Skilip: I'll have to actually test the patch before further commenting on it. Talking about it without actually seeing it was only adding to the confusion. If you could post before-after screenshots that explain what is happening that would be very useful.

Skilip, thank you for those! This confirms my suspicion that you are showing all nodes in the same list even when permissions are bigger then only 'own nodes'.

Skilip and moi discussed this a bit in IRC and we both think it would still be useful to show a list of only 'my nodes' for roles with wider permissions. I think that's why beeradb introduced a tab. It could also be added as a filter option or a sortable table header. The latter seems less useful if your name does not start with an 'A' or 'Z'… A tab would be least amount of clicks, just 1 (or none if we make 'own nodes' the default. Setting the filters is a lot more clicks.

So, based on the screengrabs, I'd say this patch is already a big improvement as is. Do we want to add the 'my stuff' tab/filter/tablesort for roles with wider permissions here? I think we should. We're back on track here!

Here's another stab at this. Rerolled to chase head, general code cleanup, and splitting the content into two different tabs. I left the content admin screen where it currently is, but if there's an agreement to move it out to the top level of the menu then we can. I've included a few screenshots to show the differences.

The general idea is this:
1). Any user with any of the permissions available in node_list_permissions for any content type has access to the menu item. Access administration pages is also required.
2). Users may list any nodes and taxonomy terms they have access to. This is handled through db_rewrite_sql as before. An edit link will only appear when a user has access to edit that content.
3). The default view for the admin content screen is "my content".

As in my previous patch, tests are included.

Actually, one quick note about why I didn't move the menu item out of the admin screens. Although I'm not opposed to moving it somewhere more visible, I think for this patch we should concentrate on improvements to the content admin screens, and if we decide it's better suited for another location, we can move it out with a followup patch, as it's really a separate issue.

Quite impressive improvement to the content admin page. I think that this general idea can be pushed elsewhere throughout the admin interface. Anyways, my tests turned up no bugs or errors, and I am generally happy with the way that things are laid out. My only concern is the lack of help. First of all, the help link (which is currently on the My Content tab and links to admin/help/node) disappears when on the "All Content" tab. Also, I think that a bit of information explaining what "My Content" is and what "All Content" is would be helpful.

Other than the help issue, I think that this is a great addition to the usability patches coming in. Nice work!

(Laving as cnr for now to encourage more to test this out)

Here's a re-roll with the help link on both pages, I also fixed a minor code style issue. Functionally this looks great, code looks fine too. I think we need two followup issues once this goes in:

1. Convert the node query builder to dbtng - it's barely touched by this patch, needs doing, but shouldn't hold this one up either.
2. Whether we move this page (or duplicate it) outside of /admin

some context for point 2:
#273137: Split Navigation to User and Administration menu
Is dependant on
#279399: Breadcrumbs are only taken from one menu
Just to say I agree on having that discussion somewhere else :)

Great work all. Hope to see this get a good review soon.

The functionality looks sane.

We should be able to tighten up the number of db queries. At the moment we are basically grabbing all the node objects (albeit slightly incomplete) and then looping through the results where we load each node object a second time using a node_load(). If we join that initial query to the node_revisions table in order to get the format, will that be enough for the access check?

Found a couple of test failures, one permission check missing, and needed updating for 'bypass node access' which affects the edit links.

Found a couple of test failures, one permission check missing, and needed updating for 'bypass node access' which affects the edit links.

Skillip has a point, although we've improved the admin content listings, we haven't opened this page up that much. This patch introduces a new permission 'access content listings'. Authenticated users will get this permission by default, but admins can still disable it to prevent users from viewing the page. Additionally I've updated default.profile to add a top level menu item in the navigation menu. This menu item is called "My content" and links directly to that content listing. This method will avoid having the link unintentionally added during site upgrades.

Since pictures are worth more than words, I've included a screenshot of the new default page after installation, showing the new "My content" menu item. Screenshots in #34 of the admin content pages are still accurate for the functionality of those pages.

now with executive summary:

Applies nicely, code is clean, and works as expected.. I am happy. And with comment #44, this should be good to go.

I'm still worried about all those node_loads() we have going on for the edit link. With a decent amount of content each request is going to contain 50 of them. 50 node_loads does not come cheap.

I don't think a node_load on 50 nodes is too bad for an admin page tbh. I also just posted a very initial patch which'd make node_load() a lot less expensive - http://drupal.org/node/324313

@skillip The user may still access 'My content' without the permission to 'access administration pages'. It requires that they be able to create or edit at least one content type, and have the 'access content listings' permission.

'access content listings' is too generic - sounds like /any/ content listing. We should call it 'access content overview' or 'access content overview listings' - neither of those are that great though...

@catch: I actually think 'access content overview' is good.. the only points of confusion I see there are people possibly confusing it with menus, or a sitemap. I think both of those possibilities are minimized by the permissions description.

With that in mind, here's another reroll....

Changes I've made in the patch:

1. 'My content' is a bit of a misnomer since you can still see 'All content' too. I propose 'View content' for the extra link.
2. Tabs should read 'My content' and 'All content' instead of 'My Content' and 'All Content' (i.e. no capital in second word).
3. The access check for the checkboxes were applied inconsistently to the header and actual checkboxes and theme function, causing misalignment woes.
4. If we are going to do a whole node_load() (which does seem necessary now I've looked into it), then we can tidy up the pager query to just grab the nid, and take everything else from the node object.

Other thoughts:

1. Some users are going to have access to this page but no edit permissions. This means they will be presented with an empty 'operations' column. Should that column disappear for them? Should they not have access to this page? Should we just rely on the site builder selecting sensible permissions?
2. I applied the multiple_node_load patch as a proof of concept here, and checked times with the devel module:
   1. With 50 nodes, before this patch query times were about 27ms, after they were about 62ms and with multiple_node_load around 42ms.
   2. Loaded up with taxonomy terms, I'm getting 28ms with no patches applied, 174ms with this issue's patch and 142ms with the addition of multi load.

   Although that's a big jump with taxonomy, I suppose it isn't awful. However, I wonder if we should be concerned by other modules slowing this page down by implementing hook_nodeapi_load()? I guess it is okay, since this would cause the rest of the site to slow down as well, but I thought I'd mention it given that this page is particularly heavy with the node_loads.

3. FYI, if you test taxonomy filtering on this page and get a notice, there is already an issue for that at #324432: Test for taxonomy filter on admin/content/node

subscribing

See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476

While we're in here, $form['operations'][$node->nid] should be split onto multiple lines.
The query which builds this page really ought to be converted to dbtng - but I'd hope we could do that in a followup patch, since there's a lot of query building going on, and it's only lightly touched by the patch.

'View content' sounds like a good change - especially since we have the tabs. Will try to do a more thorough review later on.

Re-rolled so the test applies.

Works for me.

⁣

Removed some cruft, also updates the queries performed by the node admin page to DBTNG. Talking to catch in irc we were thinking this could potentially be a blocker.

This patch could use a really thorough review :)

comments from IRC

[8:08pm] dmitrig01|afk: beeradb: + if (!user_access('access content')) { return FALSE; + if (!user_access('access content overview' return FALSE;
[8:09pm] dmitrig01|afk: beeradb: could be an ||
[8:10pm] dmitrig01|afk: beeradb: also + $permissions['administer nodes'] = 'administer nodes'; <-- you might as well have that check be earlier

also it shouldn't be under admin/ anymore

also I don't understand the access logic. if you have permissions to view content overview or w/e, but you're not allowed to create, edit, or delete any nodes, then you can't view it? what's the point?

now that i think of it i get it - but it should only be for the "create" permission - if you can delete nodes but you can't create them, nothing's going to show up.

This latest patch is based off of #57, with the following changes:

After speaking with catch in IRC I've fixed the following issues:

• On the previous DBTNG version I was using strings to build queries, which is supposed to be completely eradicated in D7. Additionally pager_query isn't for the new Database Layer. Therefor, we've decided it's best to just leave that query alone
• Made the language selection query you db_query()->fetchField() instead of DB result.
• Fixed two spacing issues in the comments

Additionally in response to dmitri's feedback:

• I agree with the comments on the access logic node_content_page_access(). I've fixed this.
• I think reorganizing pages under /admin is a separate patch. I completely agree that this should be moved out of there, but there are already existing issues for organizing those pages.
• I feel like the create, edit own, edit any, delete own, or delete any permissions on content types all denote a content editor role on various levels, which is why access is the way it is. In the event you have delete access but no create access, the "My content" page will show up with the blank state message "No posts available". Additionally the All content tab will still show up, allowing you to find/filter content you may be interested in deleting. I don't think this is an absolutely perfect system, but I think it's quite a bit better than what we have now.

Catch noticed two more nitpicks which I went ahead and incorporated..

+ $this->assertText($user_node->title, t('Users own content is displayed'));
+ $this->assertNoText($created_node->title, t('Nodes not belong to user are not displayed'));

notice the "Users" and "belong". They've been updated to "User's" and "belonging" respectively. Additionally periods have been added.

Catch noticed two more nitpicks which I went ahead and incorporated..

+ $this->assertText($user_node->title, t('Users own content is displayed'));
+ $this->assertNoText($created_node->title, t('Nodes not belong to user are not displayed'));

notice the "Users" and "belong". They've been updated to "User's" and "belonging" respectively. Additionally periods have been added.

Very last version, I almost promise :)

This updates the content admin page to use node_load_multiple, rather than the db_fetch_object it was using previously. This is more in line with the current way of doing things.

Additionally the use of global $user has been removed from node_build_filter_query in favor of $GLOBALS['user']->uid.

Discussed this with beeradb in irc and had a good look through it again and I think it's ready. The screenshots in #43 and #44 are still accurate. This makes some small changes to the built query - before anyone asks for it to be converted to dbtng, we don't have pager support yet, so that'll have to wait. It'd also double the size of the patch.

There are followups we can do to improve the filters, maybe consider moving this out of /admin altogether - but they belong in followup issues, this is already a big improvement to the complete lack of breadcrumbs we give people to find their content after they've posted it. Marking RTBC.

I tested the patch in #67 tonight, and it's missing a few things, so CNW.

First, beeradb's comment in #49, "The user may still access 'My content' without the permission to 'access administration pages'. It requires that they be able to create or edit at least one content type, and have the 'access content listings' permission." is no longer correct. I created an "Authenticated User", and then using uid1, gave the new user three permissions, 'access content overview', 'create article content', and 'edit own article content'. I then used the Authenticated User to create at least two articles, but I did not see the My Content link ever show up in the sidebar.

Second, The patch does not seem to take into account the fact that both a new installation and an update need to automatically enable the 'access content listings' permission for all authenticated users, or this patch will be worthless (no, not worthless. unnoticed).

As a followup to my #69 above, I'd like to point out that while testing the latest patch from a usability standpoint rather than a functional standpoint, I found a rather disturbing side effect. Login to your site as an administrative user, say uid1. Using devel, create several nodes of all different content types. Now, go to admin/content/node/all, as if you'd clicked it in a link.

Select several of the checkboxes at random, making sure to select all the content 'owned' by your particular user. (in my case, admin, but this could be anybody with the proper perms, remember). Now, using the Update Options dropdown menu, delete those nodes. After the confirmation screen, your selected nodes will have been deleted, but OH NOES! All of your content is GONE!

It's because once the delete confirmation form is submitted, the page reloads to admin/content/node (the default local task) and it appears that you've nuked all the content on your site since you're left staring at a 'No posts available' message. All you really wanted to do was delete the three posts you made as an admin, and leave the other three thousand posts alone. Scary.

How 'bout a AJAXY checkbox for restricting displayed nodes to only that user's content, or all content? If this is a good idea, say the word and I'll start a new issue so that this one can get committed without any dead babies. I mean kittens.

@senpai re #71 - how would that degrade for non-js situations? Does the checkbox change the filter form? I think that if the checkbox is part of the filter then we should "AJAX" all of those filter items or none of them, which would be a separate issue. I'm not sure of many examples where we have half of a form that's dynamic and half that's not - that seems like a good thing ;)

@greggles: I was envisioning that a non-js browser would display a 'warning, because javascript is disabled this box doesn't work'. ;-)

No, srsly, you're right. All the form's dropdowns would have to be ajaxed, and that's too far out of scope for this patch. I withdraw my suggestion in #71, pending a good solution to the weird user experience I describe in #70.

Here's another stab at the patch, trying to roll in senpai's feedback from #69 and #70.

Here are the high points:

• node_update_7003() is added to create the view content menu item. I was hesitant to automatically add the additional permission for authenticated users to be able to access this page since it's a completely optional permission to have, but it'd be easy to add if others feel differently.
• When deleting or mass updating nodes you now end up back on the page you came from. The implementation of this isn't as clean as I would like, but the fault there lies with the confirm_form function.. the third parameter ($path) should be setting a default #redirect if nothing is already set, since 99% (always?) you should end up on the page you came from. The $path variable is currently only used to set a destination on the cancel button.. I'm creating a new issue to (hopefully) fix that.
• I wasn't able to reproduce the issues regarding the view content links visibility, and looking at the tests that are written seem to contradict that, as the base user only has the 'access content overview', 'edit own page content', and 'create page content' permissions. If you can reproduce your claim then let me know.

Screenshots in #43 and #44 are still accurate.

Here's a quick pass for a code review. I'll test the functionality in a moment.

+function node_build_filter_query($for_user) {

node_admin_content() defaults that parameter to FALSE, so we should do the same here, no?

+  $form['#redirect'] = ($for_user ? 'admin/content/node' : 'admin/content/node/all');

We don't typically put parentheses around these, so I would remove them unless they're needed.

+  $link = array('link_path' => 'admin/content/node', 'link_title' => 'View content', 'menu_name' => 'navigation', 'plid' => 0, 'weight' => '3');

Please split one item per line, per coding standards. Same w/ default.profile.

+class ContentAdminPagesCase extends DrupalWebTestCase {

Needs a line of PHPDoc to explain what it does. See Test guidelines.

+  /**
+   * Implementation of getInfo().
+   */
+  function getInfo() {

We no longer add PHPDoc to getInfo(). See Test guidelines.

+  function getInfo() {
+     return array(
+      'name' => t('Content Admin Pages'),
+      'description' => t('Create users and content to test the content admin pages functionality.'),
+      'group' => t('Node'),
+    );
+  }
+  function setUp() {
+    parent::setUp();
+     $this->admin_user = $this->drupalCreateUser(array('access administration pages', 'administer nodes', 'bypass node access'));
+     $this->base_user = $this->drupalCreateUser(array('access content overview', 'edit own page content', 'create page content'));
+  }
+  function testContentAdminPages() {
+

Please add newlines separating those functions so they don't look allsmoooshedtogether. :)

I'm not sure about the access checking in node_content_page_access(). It feels like it might be too simplistic. http://api.drupal.org/api/function/node_access/7 is doing *crazy* things. It feels like we might need to expand the tests to ensure all the various use cases are covered. Maybe check with chx or moshe or merlinofchaos or someone with underlying knowledge of the node access system and see if they think the checks there are robust enough.

updated to respond to webchicks feedback in #75.

I also made two additional changes:
changed node_update_7003() to return an empty array (gets rid of two warnings which were outputting).
changed the two menu_link_save()'s in default.profile right above the ones I added to conform to coding standards.

I'll see if I can't hunt down someone to chime in on the access control/test questions.

oops, once more.

Because this is a usability-related issue, I'm coming into this issue fresh not having read all of the various background stories, previous issue replies, etc. So apologies if this is re-hashing things that've already been hashed.

1. The first thing I was slapped in the face with is that the table at Administer >> Content management >> Content is empty on a site with 50+ nodes. I panicked for a moment, thinking this patch had somehow deleted all of my content. I can't possibly commit this if this remains the default behaviour, as it'll result in thousands of support requests from other panicked users.

2. The "Access content overview" permission that gets auto-assigned to authenticated users does absolute squat unless I also give authenticated users "Access administration pages" permission. However, even *that* does squat because the "Administration" link ends up looking like this:

So it seems I need to give people "Access content overview," "Access administration pages," *and* "Administer nodes" permissions to make any use of this page. I'm not sure how this is helpful. I thought the intent of this patch was to give each user their own "content dashboard" kind of thing. But if this ends up being just another view of the same stuff at admin/content/node for content administrators, then a filter by username would be much more helpful there than this patch.

What I would recommend is making a totally separate page in the menu just for this. "My content" or something, to piggy-back "My account." But at that point, I'm not sure what's much different between this and Tracker module with some exposed filters.

So, so far... sorry to say, I'm not convinced of this patch and can't really justify committing it. :( I'm going to go back and read the issue now to see what I've missed.

Ah-ha. I learned in #2 (and should've remembered from reading the patch) that my fatal flaw was my "test" user didn't have any "Create $foo content" permissions. Once I added that, I was able to access this page without "Access administration pages" permissions, although there wasn't a link (I'm using the 'minimal' profile, and the new menu system no longer "bubbles up" nested menu items you have access to, so this is to be expected).

Some more bugs that this fleshed out:
1. My test user also has access to admin/content/node/all and definitely should not.
2. Even though I have "Create, Edit own, and Delete own" perms, I see nothing in the Operations column, and there are no checkboxes here for me to do anything. I would've expected to be able to perform operations that I have permissions to do on my own content.

And now I learned in #9 that the reason this is under an "admin/*" path rather than "my-content" or similar is so that an admin theme is picked up. But this seems like kind of a hack. I wonder if we should instead create a dashboard/* path (or similar) that is for "user-facing administration pages" that the administration theme picks up on and put this page under there. That way:

a) We wouldn't need to be doing these silly tricks with allowing access to one item under admin/* and not others (which, from my testing, isn't actually working in any case). Then we wouldn't need this funky "Add a manual link entry" crap and could just do a regular MENU_NORMAL_ITEM. Less special-casing in code == more maintainable code.
b) admin/content/node would retain the functionality it's had since the beginning of time, where it's a listing of the entire site content, with mass operations, visible only to content administrators. That's frankly what you expect to do at Administer >> Content management >> Content. Furthermore, I don't see any chance of someone stumbling upon this page themselves without "access administration pages" unless their install profile was smart enough to add it in which, as the minimal profile demonstrates, not all will be.
c) We could potentially put other things under this "dashboard" path as well if it makes sense. "My blog," "My private messages," "My buddies," etc. rather than these all cluttering up the "root" of the navigation menu as they often times currently do.

I'm not sure the ultimate answer here, but there are two things I'm pretty opposed to:
a) We have changed the behaviour of admin/content/node. This is going to cause a torrent of support requests.
b) This doesn't solve the "Where did my content go?" problem without lots of trickery in the access callbacks for administration pages, which is again, going to lead to massive support requests (my initial reaction: "WTF?!? WHY CAN A REGULAR USER GET INTO MY ADMIN PAGES?!?")

Sorry to say, but both of these are commit-blockers for me, unless someone tries very hard to convince me otherwise.

Another summary from IRC:

I'm really sorry to be a pain in the ass about this, because I know this is in the Usability team's top, like, 3 list of issues. But within only the scope of this issue, at least three clueful Drupal developers got confused by this change, so this means that approximately 100% of our users will be, too.

In our fervor to solve what I agree is a very serious usability problem, we should not forget that Administer >> Content management >> Content is itself a very useful page for *administrators* of the site to manage their content. We should be careful not to shoot ourselves in the foot trying to shoe-horn two vastly different users' requirements into a single section, resulting in a poor interface for both (wonder how many more foot/shoe metaphors I can fit into a single sentence? ;)).

Edit: Another one:

I realize the intent of this is to progressively reveal information to users, which is normally a worthy goal. But remember that right up there with "Where did my content go?" was also "How can I see what my users will see?" If the contents of admin/content/node change depending on who's logged in, this seems like it's working against us on that front. Something like dashboard/content would be the same for *everyone*, including admin users.

Also, while that trait solves very nicely the "I don't want to have to create a separate admin/content/node clone for my Editors, my Legal review team, etc." the big difference is that editors/legal review team are *admins* of the site, not *users* of the site. I think that this user/admin separation makes sense to people (it's the whole argument for an admin theme in the first place) and they would feel panicked if this line got blurred, especially in a section of the site they consider to be "secure" and "away from the public eye."

Subscribing

Still one of the worst problems for new users. Should we rethink our approach here?

just met someone in IRC bumping into this again.

Maybe something like "My content" on user/%user/content?

I agree with yoroy #33, table should be sortable for node_admin_content.

This is now a d7ux blocker - 'find content' is designed to be given to people like forum moderators etc. who may not have full administer nodes permissions, tagging.

I think we need to focus this patch on just the permissions changes, and have a 'my content' list in a different issue.

The permissions issue is not limited to this content page, so just moving this out to a different path would only be a stop-gap. In D5, menu items used to bubble up to the next parent you had access to, when you did not have access to the direct parent. I remember this was removed while the D6 menu rewrite was happening and it did make for a confusing menu structure indeed. But we cannot exactly tell what kind of slices would a site want to expose to people from the admin interface, it totally depends on the role:

- node moderators,
- spammer user extinguishers
- aggregator feed maintainers
- shop product maintainers (think ubercart)

etc. While these are under /admin, sites might give them out to people who otherwise do not have access to other parts of the admin area.

Re-roll, does the following:

Introduces an 'access content overview' permission.

Allows any user with this permission or administer nodes to get to admin/content.

Hides all operations on admin/content to users who don't have administer nodes and bypass node access permissions.

Only shows edit links on the table if users can edit that content.

Allows users to view their own unpublished content if they have that permissions, otherwise defaults to status = 1 if you're not a content admin.

Screenshot is for a user with 'edit own article content' and 'view own unpublished nodes' - there are two more unpublshed nodes on the site, but not by this user, so they can't see those, and they can only edit their own stuff.

Minus the typo.

If the user didn't actually have any unpublished content, you'd get IN() - so moved logic around. This also saves a pointless join if it's not needed.

grrr.

Rewrote the query to get $own_unpublished, I don't know if it is a must, but I figured it would be better.

I also added a check for administer nodes permission before fetching the unpublished nodes and also before limiting the list to published nodes, I am not sure that users with administer nodes do not get automatically "bypass node access" but looking at the code above, I guess not.

Added a "delete" operation that will display only when available to the user. This was missing and it meant that giving users the "delete content" would leave them without any easy way to delete this type of content nodes (this is also due to #196758: Having delete as a button on the node edit form means certain users don't have access to it when they should, but this one seems to be in a dead-end), although it was possible to delete them with the direct URL. After discussing this with catch and Bojhan we concluded that a delete link was useful here, and since the access check is new and this would be bad to add this link without access check we also concluded that it should be done in this patch and not in another issue. Operations are themed as an item_list, when there is more than one, and otherwise it is a simple link. I know it is not the way it is done in other tables having an "Operations" column like Revisions or Content Types, but it is the semantic way to present a list of things in my opinon, anyone wanting to use colspan or something else is welcome to post a patch. By the way, if displaying the <li> as blocks (one on each line is not "pretty"), you're also welcome to update the CSS ;)
I added the tests for the new delete link too. And I left the following test, although I am not sure to understand how this test is relevant :
$this->assertText($created_node->title, t('Edit links disabled without edit permissions.'));

I tested the functionality listed in #90 and they all work as expected, but I had troubles testing them by only using the administration interface. Because of #492186: Authoring information is never updated., I had to set the author (uid) manually in {node} and {node_revision}.

Patch Review of #96

While manually testing this patch, I found that if I create a new role called 'editor', visit admin/settings/permissions/4 and turn on both the 'create article content' & 'access content overview' permissions, I get a user who can see the node they've just created. Good.

If an admin comes by and un-publishes their node, however, they see an admin/content page that lists their article, shows that it's unpublished, and provides a link to that node in the form of a clickable title. Follow that title link, and you'll get a 403. Bad UI, bad! :)

Review Summary

This patch is close, but it needs some more logic train(tm).

Ok, possible solutions?
1) Make the admin/content node title non-clickable if the node is unpublished and the user does not have perms to view unpub content.
2) Show the user that this node was unpublished by an admin, and thus the 403 error.
3) Turn on the 'view unpublished content' permission for logged-in users by default. Sure, the site admin can disable it if necessary, but at least it won't be confusing out-of-the-box (DOOB).

Pick one.

No need to make the $own_unpublished query dynamic there.

Added class="links inline" to the theme('item_list') call. To me it looks fine with that.

I tested Senpai's bug but couldn't reproduce unpublished content appearing in the node listing when it shouldn't. However, it seemed like node->uid got set to my user/1 uid when I updated the test node to unpublished, so it didn't come under 'own' any more - could you double check that?

I think "access content overview" is good. However if given the administration privilges for admin/content/node we should probably inherited this permission?

@catch: Bug confirmed in patch #98. Ignore the bug report in #97, it's a byproduct of this bug.

Steps To Reproduce
Create a user. Create a role. Give new role to user. Grant 'create article content', 'edit own article content', and 'access content overview' to this new user.

Login as new user, and create an article node. You should see this new node on the front page, and at this user's admin/content screen.

Now use uid1 to edit that node. Do anything to the node, change it's title, unpublish it, whatever.

Expected Result
Upon node save, the changes should be made and the ownership of the node should remain with the original creator.

Actual Result (bug)
The administrative user who edited the node becomes the owner of the node, no matter what was changed within the node. In addition, if the admin user re-edits the node and changes ownership back to the original creator, upon save that change will not stick.

@Senpai: You should check what I wrote when I submitted my version of catch's patch :

I tested the functionality listed in #90 and they all work as expected, but I had troubles testing them by only using the administration interface. Because of #492186: Authoring information is never updated., I had to set the author (uid) manually in {node} and {node_revision}.

This is the bug causing your troubles, but as soon as the values are coherent in {node} and {node_revision}, catch's patch works as expected. And this bug comes only when you edit the content and change the Author because of this other issue... if you want to try it make your content type "Unpublished" by default and create a node with the created user, so you won't have to use uid1 to edit and unpublish it.
I set this "needs review" again, when that other issue will be fixed this will be solved, and we cannot do anything about it here.

Yes, TheRec, I saw your comments. I'm just submitting a bug report about this patch, in this issue, and what I experienced with it. Once the #492186: Authoring information is never updated. is committed, this one can move on.

The reason I set it to CNW is because I *don't* want other people testing this patch. It's untestable because it's untenable. With the patch applied, things do not work as intended. Without the patch, they do. Simple as that, and so we wait.

Please, if someone is reading this issue looking for something to test, go test the 492186 patch. It's holding up the works here.

There's no need to postpone this on that issue - that can be fixed any time, this (with a new permission) needs to go in before freeze IMO.

I agree with catch, no need to postpone this. I did not change the functionalities which work as expected. After discussing with catch, I added more tests and improved current tests; hopefully this will allow us to get this in before code freeze :)

I choosed to use substr_count to ensure the users with administer nodes and/or bypass node access really have the links for every node created during the tests.

Tests additions look great to me and now getting quite comprehensive for that page, whereas we previously had none. I can't RTBC this though since I've been too involved with the patch.

I fully support the reduction in scope on this patch and still think it solves the most crucial issue it originally set out to do, which is to open this content listing up to people in content administration roles who do not have the administer content permission.

I looked over the patch a bit and found a few minor issues:

Having ->execute()->fetchCol(); on the same line is a derivation from the rest of the patch, which contains one method per line. Consistency here would seem to make sense.

+    ->fields('n',array('nid'))
     ->limit(50)
-    ->execute();
+    ->execute()->fetchCol();

Coding standards.

+    $content_plain = filter_xss($this->drupalGetContent(),array());
+    $this->assertTrue(substr_count($content_plain,'edit') == $nodes_count, t('Admin user has edit link.'));
+    $this->assertTrue(substr_count($content_plain,'delete') == $nodes_count, t('Admin user has delete link.'));

More coding standards:

+    $this->assertTrue(substr_count($content_plain,'edit') == $nodes_count, t('User who can bypass node access has edit link.'));
+    $this->assertTrue(substr_count($content_plain,'delete') == $nodes_count, t('User who can bypass node access has delete link.'));

And some more coding standards:

+function node_admin_page_access() {
+return user_access('administer nodes') || user_access('access content overview');
+}

Other than these small issues, I like the new direction this patch has taken.

Corrected.

This is still good to go, but can't be switched into RTBC or it'll cause a broken UX. Still waiting on #492186: Authoring information is never updated. to be fixed before this patch can be committed.

Senpai, there's really no reason for one critical bug to hold up another unless there's a direct dependency, since we have tests on here to verify it works despite that bug, they can be worked on and committed in parallel. Giving the 'access toolbar' permission to non all-powerful content editors, then finding out they can't even access the content and comment administration screens, is also broken UX.

Catch, I'm not saying that this issue is not fixing a broken UX, cause it is. But that other issue keeps this patch from being properly tested, and if we are to remain sane and calm and fix things, they must be done in a logical order. I wouldn't want this issue to make it into core without the other one, eh? They're both really close to perfect, so I'm trying to get people to focus on that other issue first so it can be committed, and then this one can be RTBC within 30 seconds after that.

I want them both in. I just can't put my reputation on the line and approve a patch such as this one that introduces a UX bug where there was none before, even if this patch is trying to fix a problem that already exists. Patience, gang, we'll get them both. There is no way in hell that any sane person in charge would ship a Drupal instance that *didn't* have these two bugs fixored. They've both been identified, patched, and tested. They'll get into D7.

Re-rolling.. nothing changed. :)

Re-worked the patch with the help of chx. The condition to add unpublished nodes to the query is simplified and we changed a minor code standard issue regarding the theme_item_list() call.

Removed an useless db_and() call. Nothing else changed.

Ran out of things to criticize.

Err... nope...

+++ modules/node/node.admin.inc	25 Aug 2009 09:04:23 -0000
@@ -393,7 +393,9 @@
-  $header[] = theme('table_select_header_cell');
+  if (user_access('administer nodes') && user_access('bypass node access')) {
+    $header[] = theme('table_select_header_cell');
+  }
@@ -425,6 +439,7 @@
     '#title' => t('Update options'),
     '#prefix' => '<div class="container-inline">',
     '#suffix' => '</div>',
+    '#access' => user_access('administer nodes') && user_access('bypass node access'),
@@ -455,11 +470,29 @@
   $form['nodes'] = array(
     '#type' => 'checkboxes',
-    '#options' => $nodes,
+    '#options' => $nids,
+    '#access' => user_access('administer nodes') && user_access('bypass node access'),
   );
@@ -525,7 +558,9 @@
     foreach (element_children($form['title']) as $key) {
       $row = array();
-      $row[] = drupal_render($form['nodes'][$key]);
+      if (user_access('administer nodes') && user_access('bypass node access')) {
+        $row[] = drupal_render($form['nodes'][$key]);
+      }
+++ modules/node/node.module	25 Aug 2009 09:04:23 -0000
@@ -3262,3 +3266,10 @@
+function node_admin_page_access() {
+  return user_access('administer nodes') || user_access('access content overview');
+}
+++ modules/node/node.test	25 Aug 2009 09:04:44 -0000
@@ -389,6 +389,88 @@
+    $this->admin_user = $this->drupalCreateUser(array('access administration pages', 'administer nodes', 'bypass node access'));
+    $this->base_user_1 = $this->drupalCreateUser(array('access content overview'));
+    $this->base_user_2 = $this->drupalCreateUser(array('access content overview', 'view own unpublished content'));
+    $this->base_user_3 = $this->drupalCreateUser(array('access content overview', 'bypass node access'));

I like this patch a lot, but I do not understand why a user needs BOTH 'administer nodes' && 'bypass node access' to do any of the usual administrative tasks. Especially, since a new 'access content overview' permission is added...?

It looks like the tests also only ensure the functionality for 'administer nodes' && 'bypass node access', but not for 'administer nodes' only.

Note that I didn't read the issue. If all of this happens on purpose (which would mean that 'administer nodes' alone does not do anymore what it used to do), then I really think that the existing access permission descriptions need to be updated accordingly.

I'm on crack. Are you, too?

Thanks skilip, catch, beeradb, therec et al for getting this to this stage. This issue was started during drupalcon Szeged, would be great to toast on it in Paris!

@tha_sun:

http://api.drupal.org/api/function/node_access/7

This now only allows you to do all things to all nodes if you have the 'bypass node access' permission, not 'administer nodes'. I have to say I'm not really clear if that's a bug or by design

Maybe the 'adminster nodes' should be changed to something like 'author and publish nodes' ('administer' sounds really too wide for what this permission has become now that we have a specific permission to access 'admin/content'), it somewhat falls under the scope of this issue since we are adding this permission... so if what I just said is not wrong, we should dicuss the new name for 'adminster nodes'.

sun> We use the && condition there because it concernes mass-operations which consist of a dropdown list corresponding to a mix of operations which can be done by one role or the other, so we display the complete list only for users with both roles, to ensure users with only one of them do not see operations they will not be allowed to do. We could expand the scope of the issue to upgrade the list of operations and provide only the available operations for the user, but this seems a little more complicated (and would require more tests). I think we should start with this first step (the current patch) which allows more people to access their content easily... and then provide cool features like mass-operations with a list limited to their available operations, in another issue.
The access to the page is limited with ||

+function node_admin_page_access() {
+  return user_access('administer nodes') || user_access('access content overview');
+}

You're right on this though, maybe we should only allow users with 'access content overview'... I'll update the patch accordingly, once we settled for the 'administer nodes' permission.

Let's see, 'administer nodes' in a publisher's mentality used to be everything that 'create new story' wasn't, correct? And now it's becoming less powerful, in an 'edit & unpublish all nodes' sort of way, right?

Seems to me that if we name this one correctly, there will be room for a follow-up patch that allows for one more new permission of 'publish & unpublish nodes' as well as whatever this one will become, which is 'edit all nodes'.

And thus, for nodes themselves, we'll have the 'bypass node access' permission (which really isn't a permission, it's an anti-permission. woops. :) and the 'publish & unpublish all nodes' permission (for content moderators and the like) and then the 'edit & delete all nodes' permission. Of course, the newly instantiated 'access content overview' will be there to allow people who don't administer the entire site to still see the page which lists all content on the site. This, my friends, is utterly cool!

Shouldn't 'access content overview' really be named 'access content administration page' ? Sorry, I digressed for a moment there. :)

Ok... tracker.module wants to get his dirty nose in our nice content overview, let's give him the required rights...

In this new patch, it is possible to access this page only when you have the 'access content overview' permission... if this is not wanted, I can always reroll, but since no that many people answered previously I chosed to see what it would give... it allows the remove the "node_admin_page_access()" function (I used the 'access arguments' of the menu item directly). But now it is mandatory to have this access right to access the content overview... which makes some sense to me at least, it's still up for debate of course ;)

Lookie look, we can merge those test failling "Node administration"...they also test the Content overview :) Let's do everything on a single class as it tests the same interface. I'll work on a patch right now.

Ok, this should be ok. Merged the two test cases... this will spare a bit the test bot I guess :)

If we remove 'administer nodes' from the access callback, we should probably add an upgrade path for existing sites to add 'access content overview' to all roles with 'administer nodes' currently.

Right, so here's the updated patch with the upgrade path :) Oh and this is my first hook_update_N() implementation ever.

Tested the upgrade path and looked over the test changes, all looks fine and works great. Back to RTBC.

I reviewed the patch. (before lunch, but net didn't work)

I added "Access content overview" (and create,edit, delete story) permission to authenticated user, but poor user doesn't access admin/content. Is it problem of this issue, or not problem of this issue, or not problem? I forgot menu rebuilt . It works fine. (I thought, I am the problem:)

pp

user_role_set_permissions() had the time to get renamed to user_role_change_permissions() and also of course completely change it's functionality, so we must now use user_role_grant_permissions() (all this happened in #300993: User roles and permissions API)... and there also has been a new schema update meanwhile so function names were colliding. These changes does not change any functionality, it still deserves RTBC in my opinion.

Oh... I didn't see this coming, not at all.... another schema update had the time to be commited before this.

+++ modules/node/node.admin.inc	30 Sep 2009 19:38:33 -0000
@@ -455,11 +470,29 @@
+        '#markup' => theme('item_list', $operations, NULL, 'ul', array('class' => "links inline")),

'class' is an array now - everywhere.

+++ modules/system/system.install	30 Sep 2009 19:39:31 -0000
@@ -2572,6 +2572,18 @@
+  $roles = user_roles(FALSE,'administer nodes');

Missing space between arguments.

I'm on crack. Are you, too?

Ok, those are corrected. Thank you.

I scoured this issue for some clear set of instructions for testing this patch and didn't find any, so here are my results from blundering around.

* Created a role called 'editor' and gave it the 'access content overview' permission.
* Created two users: 'peon' (just auth user) and 'editor'
* Logged in as editor.

EXPECTED: Toolbar to show up and give me content link.
ACTUAL: I get no toolbar.

No problem. There's a permission for accessing the toolbar.

* As my admin user, also give 'editor' the 'Access administration toolbar' permission

EXPECTED: Toolbar to show up for editor and give me the content link.
ACTUAL: I get "Find content" in the shortcut bar, but nothing in the toolbar:

* As my admin user, also give 'editor' the "Access administration pages" permission.

EXPECTED: I finally get my cotent link.
ACTUAL: I get it, all right... and a whole crap-ton of other useless links that do nothing for me, like this:

Granted, this is not the fault of this patch. It's caused by the fact that we rolled back #296693: Restrict access to empty top level administration pages.

I can tell by all the various test coverage and reviews back and forth that the various nuances for how the page itself ought to work have been very thoroughly discussed, and that's great to see. However, the tests "cheat" because they're accessing the URL directly. The UX for the feature in general seems so absolutely and utterly broken, I'm not sure how we're actually improving anything here. :\

Can someone lend me a cluestick?

Meant this one.

I followed webchick's path with the 'administer users' permission.

* Created a role called 'editor' and gave it the 'access content overview' permission.
* Created two users: 'peon' (just auth user) and 'editor'
* Logged in as editor.

EXPECTED: Toolbar to show up and give me content link.
ACTUAL: I get no toolbar.

No problem. There's a permission for accessing the toolbar.

Ditto.

* As my admin user, also give 'editor' the 'Access administration toolbar' permission

EXPECTED: Toolbar to show up for editor and give me the content link.
ACTUAL: I get "Find content" in the shortcut bar, but nothing in the toolbar:

This time I get a completely empty toolbar, because there's no shortcuts link to admin/people - so it's actually worse.

* As my admin user, also give 'editor' the "Access administration pages" permission.

EXPECTED: I finally get my cotent link.
ACTUAL: I get it, all right... and a whole crap-ton of other useless links that do nothing for me, like this:

Ditto.

Can someone lend me a cluestick?

The toolbar is broken for users who only have one administrative permission - this counts for the current administer nodes permission, administer users etc. It's also the case in D6 that a user with only 'administer nodes' permission doesn't get a link in the navigation menu pointing to that item - it's up to the administrator to add a top level link there - when that additional step is taken, you get all the benefits of this patch without having to assign security critical permissions to use it. As you said, none of this is the fault of this patch. I opened #596006: Toolbar is broken without 'access administration pages' permission., and I'm marking this back to RTBC.

Regarding the last three or so follow-ups:

No, no. And no.

These are separate permissions on purpose. We will not merge them or whatever, because that would be a total presumption of how Drupal is or needs to be used. There are many use-cases, even _within_ a single site. We wanted (UX) features, we added them, and now you can enable them for certain users.

#596006-2: Toolbar is broken without 'access administration pages' permission. contains a suggestion for how to approach this properly.

#296693: Restrict access to empty top level administration pages is about to be fixed, but is not at all related to this issue.

So what happened here?

A perfectly fine RTBC patch was derailed with off-topic issues. If you accept that there is a major usability problem with "dependent" permissions, then let's work on that - separate - issue.

This really must be totally frustrating for TheRec after working so hard on this patch. :(

Hopefully, this patch still applies.

Thanks for re-RTBCing again, but we still need a re-roll :(

I'll allocate some time to re-roll this today (if it is possible in the same way, I think there was a funny change that make the display or hidding of the checkboxes a bit more tricky). If no one is faster than me (*winks* at catch).

sun: Thanks for reviving this :) By the way, isn't frustration part of the DX in Drupal dev community ? If it's not we should submit an issue because I've been experimenting it many times ;)

Nothing changed in terms of use.

-      'title' => l($node->title, 'node/' . $node->nid, $l_options) . ' ' . theme('mark', array('type' => node_mark($node->nid, $node->changed))),
+      'title' => l($node->title[FIELD_LANGUAGE_NONE][0]['value'], 'node/' . $node->nid, $l_options) . ' ' . theme('mark', array('type' => node_mark($node->nid, $node->changed))),

I'm not a Fields expert, I figured this was changed in the former tests of "Content overview" and that now that we use node_load_multiple() we cannot use $node->title anymore as "title" is a field and is internationalizable... so if my reasoning is wrong please feel free to explain me how it work and what should be used instead ;)

The removal of checkboxes for the users without access to the mass-update dropdown is a bit more tricky. It works fine so it's just a matter of knowing if I did it "clean" enough when you will review it.

@sun : Talk about frustrating...

This one was working for more than 4 days... not a feedback, even after trying to get some attention on it on IRC. When someone will be interested in improving usability in Drupal, with one of the major usability problem found with the usability tests "where is my content now?" (with this patch, any use should see it with the appropriate rights now... just by clicking "Content"), just say it in here or contact me on IRC, I might find time to re-roll this... but I won't re-roll this for nothing anymore.

TheRec: it's down to the core committers now to work their way through the issues that were RTBC at the time of code freeze. We only have two core committers.

The issue is critical, we all know we want to get it in. I know it's frustrating, but only RTBC stuff is on the rader of the core committers, so a re-roll would still be very much appreciated. And thanks for keeping up so far!

In case I would not be the one that re-roll this... you will surely be interested in #602522: Links in renderable arrays and forms (e.g. "Operations") are not alterable which introduces a yet undocumented #type => 'link' for renderable arrays. It the context of the current issue, this is used for the operation links. The current patch has the following behavior to provide a semantically correct "list" of operations (yes... the column is named "Operations" so you'd expect it would allow you to display more than one operations as a semantic list if needed :

1. Only one operation : A simple link inside the table cell.
2. More than one operation : A list of operation generated with theme_item_list() with the "inline" class (see #98).

I guess that if we have to keep this as renderable array (for Overlay it seems...), we would need another new #type to render an unorderlist (using drupal_render() at this point would not help, as there would be no point in using a renderable array). This is pretty much out of the scope of this issue so it would require another issue.

Another solution would be to drop the semantically correct approach, and have links one after the other inside the table cell... I don't like it, but I would like even less to see this delayed....... again...

- Re-rolled against HEAD.

- Cleaned up code.

I'm afraid this won't work. You are not using the new "#type => 'link'" for the "edit" and "delete" ... these changes have been introduced on purpose in #602522: Links in renderable arrays and forms (e.g. "Operations") are not alterable for implementing Overlay later, I doubt we will be able to regress this unfortunately (yes it degrades the DX... don't tell me).

We talked about it with catch on IRC this afternoon and ended up thinking that passing the link to theme_links or even theme_item_list is not possible anymore as we have to use "#type => 'link'" and cannot use drupal_render to pass the rendered links to the theme function ... but maybe you have a solution for this, I'd be happy to hear it... I've looked for doc about renderable arrays but unfortunately all I find is either incomplete or not up-to-date. If you don't have a solution, I suggest you take a look at #365554: Fix colspan in tableselect, this will allow us to render the $operations array as table cells and span the "Operations" header over them. That is how it is done in other parts of drupal right now, but it is not semantic as we are listing operations in a non-tabular way, we should not list them in table cells but in an unordered list (<ul>).

And I thought usability issues were always "bug" (at least we are fixing a bug here... not creating a whole system or something)... no ?

After talking with sun on IRC, he explained that is not "mandatory" to use #type => 'link' for these links and the solution he presented in his last patch is satisfactory as it allows to "alter" the list of links because they are rendered with #theme => 'links'. So you can just forget my last comment. Sorry.

This should work a lot better.

Introducing assertLinkByHref() and assertNoLinkByHref(), which I could have used in oh so many tests already and always wished that was available...

Hence, also entirely revamped the tests. Now properly testing for:

- "access content overview" + "administer nodes" + "bypass node access": Node update options, content filter, tableselect, all operation links.

- "access content overview" + "view own unpublished nodes": No update options, content filter, no tableselect, own unpublished nodes, no unpublished nodes of other users, no operation links.

- "access content overview": No update options, content filter, no tableselect, no unpublished nodes, no operation links.

- "access content overview" + "bypass node access": No update options, content filter, no tableselect, all operation links.

Everything works as expected. And the revamped tests + the new generic tests function seems pretty useful too. I have only one remark/question :

+++ modules/node/node.admin.inc	6 Nov 2009 02:25:24 -0000
@@ -452,52 +427,149 @@ function node_admin_nodes() {
+          '#title' => $node->title[FIELD_LANGUAGE_NONE][0]['safe'],
+++ modules/node/node.test	6 Nov 2009 02:12:53 -0000
@@ -948,57 +948,119 @@ class NodeAccessRebuildTestCase extends 
+    $this->assertText($nodes['unpublished_page_1']->title[FIELD_LANGUAGE_NONE][0]['value'], t('Admin user can see unpublished nodes.'));

Content overview displays "safe" value of the title field (by the way, in your pevious patch you mentioned that it would be good to have plach's opinion, is that done ?) and in the test you are using "value" of the title. It should not cause so much trouble as we are not using unsafe string as title during tests (because it is not what we want to test). Maybe it would be safer to use the same principle you used later in the patch, check the presence of link to the node view ?

This review is powered by Dreditor.

For now, let's go with what is in the patch.

Yes, there was this @todo, and I removed it. I realized that I simply cannot code against a feature that lives in data storage only, and which does not allow me to see the actual data (field translations) and behavior (language negotiation) as well as data flow by playing and debugging around.

I'm very certain that FIELD_LANGUAGE_NONE will break at some point, but I have no clue how to deal with hypothetically more field languages/translations in here. Yes, that is even despite me closely tracking and signing off the translatable fields patch.

Therefore, my conclusion and recommendation is that we badly need TF4 in core. Until that happens, this patch will work fine.

You did not answer the second part of the question :

Maybe it would be safer to use the same principle you used later in the patch, check the presence of link to the node view ?

Also, about using that node field in the content overview I was not trying to say it should be done otherwise, I know there is no other way at the moment, I also end up using it (well not the "safe", but the "value") in the last patch I posted ;)

meh. This just unveiled another flaw: 'safe' is only available in case a field uses no text format, or when it uses one, and the text format is cacheable. If it's not cacheable, then sanitized values in 'safe' are only generated for rendering.

However, we are only loading here, not rendering. -- which, btw, looks like another build mode ("list") candidate. This function shouldn't have to deal with actual field system innards. It just wants to display a list of nodes and the system should provide an output that is suitable to be used in lists. This function doesn't care whether it really gets a title, or which of all fields in the node is the title field, and, which of all possible title field value translations it ought to use.

Furthermore, I'm not quite sure how nodes without titles are handled in this content overview.... I know that we recently talked about that deeply hidden feature of the node system, and AFAICS, we even managed to take it over to the field system...

So. That's the braindump for today. As you can see, 0% of that has anything to do with this issue and patch, and 100% of that are really horribly critical issues we need to resolve.

sorry, forgot the corrected patch for 'safe' vs. 'value'.

Replaced :

    // Verify tableselect.
    $this->assertFieldByName('nodes[' . $nodes['unpublished_page_2']->nid . ']', '', t('Tableselect found.'));
    // Verify display of unpublished nodes by other users.
    $this->assertText($nodes['unpublished_page_1']->title[FIELD_LANGUAGE_NONE][0]['value'], t('Admin user can see unpublished nodes.'));

By :

    // Verify tableselect.
    $this->assertFieldByName('nodes[' . $nodes['published_page']->nid . ']', '', t('Tableselect found.'));
    // Verify display of unpublished nodes by other users.
    $this->assertLinkByHref('node/' . $nodes['unpublished_page_1']->nid);

First test changed to a published page because otherwise you'd see it fail if the unpublished nodes does not appear for some reason (a published node has less chances to be filtered by access rights... and the second test is already testing for unpublished nodes, so if the second test fails, the first one which is not related will always fail...)

The second test has changed to show you what I meant in #158 and #160, you are doing it this way for all the other tests..so why do you test the title of the node in this case ? I don't see a reason, but if you have one please explain me :)

ok, due to this back and forth I realized that we can simply test view links + tableselect fields for all nodes, because those lines are testing for the "super-admin".

Anyway, this I'd call "test-bikeshedding" ;) This was RTBC a few times before, now comes with even nicer tests, so what are we waiting for?

Call that test-bikeshedding, but the first test in #163 would have failed if there was a problem causing unpublished nodes to be filtered when they are not supposed to be, when the intial intent of the test is not to check for the visibility of unpublished nodes, but for the visibility of checkboxes of tableselect. I don't think it is useless to reduce the chances of a test to fail for unrelated reasons.

Anyways, I also feel this is RTBC and that the test coverage is pretty extensive so hopefully this will get in before it gets out of sync again. :)

What are we waiting for?

For a committer, to commit this patch

@sun: I'm always tempted to say "Christmas" when someone asks this... (in the memory of Duke Nukem) ;)

Jokes aside, I talked with webchick on tuesday. Unfortunately she does not think this can make it in, because it reveals too many "bugs". Even if they are not bugs caused by the patch itself, it is not acceptable as it creates other "UX WTF's" by committing this one first. I had to leave after this but I think it was pretty much in a dead end. She asked beeradb his opinon (in terms of usability) and as I remember it, he did not made her change is mind, so I guess that the situation right now is stuck and time is running out :(

@webchick: The summary you wanted was already done by sun in #145 (as you might already have noticed when we talked the other day).

because it reveals too many "bugs". Even if they are not bugs caused by the patch itself

Wonky reasoning.

This patch is perfectly fine, and a badly needed improvement to make any sense of the new top-level items in the toolbar for "not-so-super-admin-ish" users.

Again, the "dependent permissions" issue exists since... like Drupal 4? There's no doubt that we absolutely should fix it, but that has nothing to do with this fine patch.

Also see #143, where exactly the same bugs are quite obvious in HEAD via administer users.

If the toolbar is supposed to help 'content creators and editors', what's the point of it if you have to be a super admin to use it?

Just so there's no confusion on these 'new' bugs which are exposed, there are absolutely zero new wtfs introduced by this patch whatsoever - since every, single, issue from webchick's walk through in #143 is reproducible with 'administer nodes' too.

* Created a role called 'editor' and gave it the 'access content overview' permission.
* Logged in as editor.

EXPECTED: Toolbar to show up and give me content link.
ACTUAL: I get no toolbar.

Exactly the same with 'administer nodes'

* As my admin user, also give 'editor' the 'Access administration toolbar' permission

EXPECTED: Toolbar to show up for editor and give me the content link.
ACTUAL: I get "Find content" in the shortcut bar, but nothing in the toolbar:

Exactly the same as administer nodes - except I additionally get a 'create content' shortcut, but that's not relevant here.

* As my admin user, also give 'editor' the "Access administration pages" permission.

EXPECTED: I finally get my content link.
ACTUAL: I get it, all right... and a whole crap-ton of other useless links that do nothing for me, like this:

Exactly the same as administer nodes

Additionally, in HEAD, there's no way to give people access to 'administer comments' and have this links accessible via the toolbar, without also giving them the site-pwning 'administer nodes' permission - because comments is a tab on that page. However, with the patch, you can give them 'access content overview' (even if they don't have edit or delete permissions for content), and 'administer comments', and they'll get a proper links with proper tabs.

See screenshots:

Administer comments, no administer nodes:

Administer comments + 'access content overview'

Same permissions, this time looking at the admin comments page:

Just 'administer comments', looking at the admin comments page:

So, please explain what new wtf's this patch creates, because I am unable to find any, whlle it fixes several.

Um. Hold on now. I didn't say it couldn't make it in because it revealed bugs. I asked for a nice summary in this issue of what exactly the usability improvements that we gained from this patch are, since all I found was massive piles of #fail everywhere I went when I tested it. The fact that these bugs exist with or without this patch is immaterial... what I'm searching for here is a summary of why exactly this is so important given all of the problems it reveals when you try and actually use it for its intended purpose. What are we gaining here?

And if #145 contained such a summary, I must've missed it. It merely contained pointers off to other issues.

Well your review attributed bugs to this patch, which are neither caused by it, nor exposed by it, nor its responsibility to fix - since they are already there with existing permissions and behaviour, and more or less ignored the actual functionality this patch introduces which is to give more granular permissions to the content overview page itself - something you mentioned yourself is covered with copious test coverage and documentation. So I'd expect the point by point refutations of that review given in this issue by more than one participant to merit it another look, given you made no complaints about the documented behaviour of the patch itself or the test coverage. The toolbar, d7ux IA, and system module's mechanisms for admin page categorisation may well be "massive piles of #fail", but its not fair to blame those things on this poor patch, which either pre-dates, or post-dates them by a long way. This is an incremental improvement which doesn't, and shouldn't attempt to kill the big, mutant kittens with fangs which are system_admin_menu_block_access() and toolbar.module.

Either way, here's a summary:

Access to the top level 'content' item, which is the only place in core to get a listing of recently posted content and comments, is currently restricted to users with the 'administer nodes' permission, which on most sites is not a viable permission to give out to users who might otherwise need to edit and delete either their own content or that of others, or comments, without making a list of all the nids they ever posted so they can find them again later.

This patch does two things:

1. Introduces a new permission 'access content overview' which is just for the content administration page, as opposed to opening up random stuff like authoring information, publishing options etc. which are attached to administer nodes.

2. Makes it safe to give that permission to non-super-admin-like roles, by progressively adding or disabling features on admin/content based on the permissions the user actually has.

That way, if you have a bunch of content authors who you /only/ want to be able to post content, then find and edit it again from a central location, you can assign them the appropriate permissions and they get to actually see this page, as opposed to having to set up views and views bulk operations to recreate it, or over-assigning permissions. Therefore, it solves the 'can't find my own content which I posted 10 seconds ago' problem, which the prominent 'content' link in the toolbar was supposed to solve for 'content authors and editors', but currently doesn't for anyone who can't pwn your site by bulk deleting all of its content 50 at a time.

As a side effect, it also allows people to administer comments, without the broken theming caused by the fact that local tasks render completely barmy when you don't have access to the default local task. If a user only has access to 'administer comments' and 'access content overview' - the content overview page will still show them a nice filterable list, without allow them to do horrible things to your content, and they'll have a proper tab for administering comments. This is also a limitation in the menu system, which is documented elsewhere (although I can't place the nid), however it at least makes it possible to have comment admins without shoving that bug in their face too.

What it doesn't do, is attempt to solve the two issues linked from #145, which means that currently, if you only want your users to see a big 'administer content' link, and nothing else, you'd need to create a custom menu with that link in it, or something similar - however that's also the case in Drupal 6 with just the navigation module and administer nodes, and in Drupal 7 using the navigation menu or toolbar module and administer nodes - so it's not a regression introduced by the new permission at all. And creating a custom menu / menu item is once again a better option than over-assigning permissions or getting views bulk operations up and running just to have basic functionality which core currently attempts to provide.

Up to the point:

A: This patch allows you to access the "Content" link in the toolbar.

Q: That's accessible already?

A: No, just for you. Because you are uid 1 or a similar superadmin-alike user.

Q: So anyone who wants to access "Content" needs to be a superadmin-alike user?

A: Yes. Unless you completely replace that menu router item and link with custom output.

Q: Hm, but that's not really a 80/20 principle or UX issue? I mean, the permissions...

A: It was RTBC a long time ago. Even before 10/15. The permission dependency issue exists regardless of this patch, and we cannot solve the permission dependency issue for D7.

Q: So what does this patch exactly do?

A: It introduces a new permission "Access content overview", which grants access to admin/content. The "view" on this page is altered, so that it properly respects the user's permissions, which, due to node access restrictions, have an impact on the amount of of nodes displayed (view access), and the operation links (update/delete access) for each node. All of that is verified via tests.

I'm sorry that I did not understand your demand webchick. I think catch and sun's messages answer it... Thank you both to have written these summaries :)

I haven't talked to webchick about this yet, but I support this patch going in. I don't want to override webchick by committing this patch, so we'll make sure to discuss it shortly. Stay tuned!

Thanks; the non-snarky parts of #175 was what I was after.

My computer's in the shop atm so if you want to commit this Dries, go ahead. Else I can do it tomorrow evening.

Alright, committed to CVS HEAD. Thanks!

Awe. Some. Double plus yay and a big thank you to all who kept helping move this thing forward.

Great, thanks to all the participants.
I think Converting 6.x modules to 7.x requires some changes as we are splitting up "administer nodes" again :)

As it isn't possible for me to edit this page, here's a first draft of the API changes introduced by the committed patch :

"administer nodes" permission split into "administer nodes" and "access content overview"

(issue) The "administer nodes" permission has been split into two separate permissions, "administer nodes" and "access content overview".

The "administer nodes" works as described in the previous API change : "administer nodes" permission split into "administer nodes" and "bypass node access" but does not grant access to the content overview anymore.

The "access content overview" permission grants access the content overview page which lists the nodes present on the site. The content and actions presented on this page will now depend on the node access rules and other permissions such as "administer nodes", "bypass node access" and "view own unpublished content".

Giving "access content overview" to users with "administer comments" permission allows them to see the "Comments" tab on the content overview page, allowing them to have an entry point to the comment administration interface they are given access to.

When upgrading a site from 6.x to 7.x, roles with the "administer nodes" permission are automatically given the "access content overview" permission as well, since having the "administer nodes" permission in 6.x is equivalent to having both in 7.x.

I do not write documentation really often, so please feel free to correct it :)

I added this into the administer nodes / bypass node access split since it's highly related, marking as fixed.

Thank you catch, I didn't check again if I had the rights to edit it and now it seems that it is possible for me to modify... I wonder why this always changes.

I know this is the wrong place to ask... but what are Drupal 6 users able to do in order to resolve the issues highlighted in this topic?

Exhausted search...