| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | node.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Favorite-of-Dries, Needs screenshot, Needs text review, Quick fix, Usability |
Issue Summary
'Post settings' was a major red herring for users during usability testing at the university of Minnesota - it makes you think something useful will be there - like about settings, for posting, but instead you get a hodgepodge of settings which would all be happier in other places.
Discussed this with yoroy in IRC, and here's a rough plan:
Number of post on main page - move this to site information (and make it number of posts in node listings, since 'main page' is a misnomer on something which applies to user blogs and taxonomy terms too as well as RSS feeds).
'Preview' goes into admin/build/types/$type as a per-content type variable - most likely in the workflow settings fieldset.
Teaser length - also move this to admin/built/types/$type as a per-content type variable. I'm less sure about this, but can't think of anywhere else to put it, and I can see valid use cases for it - making blogs have a long teaser length, but news items short for example.
Comments
#1
"Number of items in content lists" or something. Move it to the Site information page because it's a pretty basic, "initial set-up" config.
'Preview" as part of the workflow settings per-content type makes sense, no?
Teaser length is a bit trickier. I suggested that maybe it might be useful to have it configurable per content type, but I don't want to dream up features just to create a nice place for it i the ui.
Maybe connect it more to the "number of items in content lists" setting? They are kind of related:
Show [x] items in content lists and show the first [xx] characters of each item.#2
Heck yes. This could easily be incorporated into better pages and removed.
#3
Let me second this by saying “**** yeah!” (aka “subscribe”)
#4
Then I'd like to hear your thoughts on where you think the teaser length setting should go :)
#5
I like "Show [x] items in content lists and show the first [xx] characters of each item."
#6
Well, I’m kind of torn…
I see the use-case of per-content-type teaser lengths. Hmm… maybe that should be (is?) a Views.module feature; “trim this field to x chars.” Because its really per-view teaser lengths that would be the most useful, not per-content-type per se.
Well, its a good thing I really like: "Show [x] items in content lists and show the first [xx] characters of each item."
Let’s go with that one! And make this issue an official usability issue.
#7
Sounds like good changes, I think for the moment it would be more of a views option. The wording options sound good.
#8
#9
#10
THANK YOU for suggesting a change to the "Number of posts" variable. We've tripped over it more than once.
Regarding the number of posts and teaser lengths, please forgive my ignorance: Is view.module part of core for 7.x? I get the impression it is from the decision to move these options there.
Please disregard me if I'm derailing the conversation. I'm looking through the "Needs text review" tag for tasks, but knowing what's up with views.module will help me make better suggestions.
#11
Views isn't in core for Drupal 7. Lots of people would like it to be, but there's absolutely no guarantees, and a vast amount of work to do in order for that to happen. So while views in core would make this a lot more flexible, it's worth doing the best we can without it (and the amount of code required to do this will be fairly small - so it's nothing which can't easily be changed later).
#12
Yeah, I only mentioned Views because we were contemplating whether we should do extra work to have per-content-type teaser lengths or if we should just leave it as is (i.e. one teaser length to rule them all). I think we’re going to go with yoroy's suggestion in #1 above.
#13
catch and JohnAlbin: In that case, I think "Length of trimmed post" and "Preview post" should be configurable per content type. If this is too difficult to change, perhaps *all* of these "Post settings" could be moved to its own fieldset or tab in "Site information."
Possibly OT: Has anyone considered changing the wording of "Site configuration" and "Site information"? The two confuse some users.
#14
Todd: I think per-content-type teaser lengths is now out of scope of the favored approach to this issue. Goal here is to remove the 'post settings' admin screen by moving the *existing* functionality there to other places in the admin sections.
I only mentioned the idea of per-type teaser lengths as a way to find an appropriate place in the admin ui, but like I said in #1, I don't think that's the way to go about it.
Customizable teaser lengths would be great, feel free to open a feature request for it if you like, let's *not* make it part of this patch. We're really picky about scope!
Changing the wording of admin sections etc. is here: http://drupal.org/node/228236
#15
yoroy: Understood. I mentioned per-content type configuration because, as you said, it had already been suggested by you in comment #1. Perhaps my comment wasn't read in full:
Elimination of "Post settings" in virtually any form would be a welcome improvement.
#16
yoroy: And thanks for the link to the admin categories issue. I figured it had been up for debate from some time, but I was having a hard time finding it.
#17
Here's my first crack at a solution. Since the Site Information page and the Post Settings pages are handled by the system and node modules respectively, I decided to keep the implementation in the node module via a hook_form_alter. Otherwise, this is a pretty simple cut and paste job. All the functionality of the old post settings page can now be found under the Site Information page, as per #1 description.
Also, there's a function in the regular post settings page that involves rebuilding permissions in the case of a bad database. I moved the functionality to the same page. This might be something that should move to another page besides Site Information. Performance, perhaps?
BTW, please forgive any funkiness with the patch. CVS is not my forte, and I don't break out the Terminal app as often as I probably should. ;-)
#18
Not got time to do a proper review this evening, but marking as a patch in case someone else does.
#19
Brandonian: Looks like you have two different CVS versions being compared, there's lots of field API code that changed that is getting caught in with your patch.
Here's a quick revision on my own that moves only the #posts/page to site information, but then moves the teaser length and preview setting to admin/build/types/settings. I consider this site building rather than under content. The node permissions rebuilding has been moved to admin/content/node/rebuild instead of admin/content/node-settings/rebuild.
#20
Ah. That's what happens when you don't pay attention to what you're doing. D'oh.
#21
No need to reset the status. :)
#22
Some screen shots to help others discover what this patch does:
Before:
After (UPDATED):
#23
Review:
'description' => 'Control posting behavior, such as teaser length, requiring previews before posting, and the number of posts on the front page.',This is no longer accurate, since we moved "number of posts on the front page" elsewhere.
- $items['admin/content/node-settings/rebuild'] = array(+ $items['admin/content/node/rebuild'] = array(
Why not admin/building/types/settings/rebuild ?
As shown in the screen shot, it's jarring that the first setting calls for "Default front page" but the second setting says "Number of posts on main page." I know there's another issue for renaming this to "Main page" everywhere, but for now it should match what the existing thing is called, which is "Front page."
We also should make note that this setting does absolutely nothing if front page is not set to "node." Maybe in a future patch we can use jQuery or something to show/hide this field, but for now just enhance the description.
#24
Sadly that's not the case, Drupal's been lying to us for years.
$ grep -rl "default_nodes_main" *modules/node/node.module
modules/node/node.admin.inc
modules/blog/blog.pages.inc
modules/taxonomy/taxonomy.pages.inc
modules/taxonomy/taxonomy.module
So it really means "Posts per page in default content listings" - I don't know if that's any good for the actual description though, or whether we want to split that variable for blogs/taxonomy...
#25
Drupal!!! HOW COULD YOU!!! And the day before Valentine's Day, no less!!!
Personally I'd rather set this in one place than in 50 places. But then the feedback still applies. We need to change the title/description of this field so it makes sense in context.
#26
Here's a reroll, moved the node access rebuild call back under admin/build/types too.
#27
The main reason I saw to move the node access rebuild to admin/content/node/rebuild was because anyone with the permission 'access administration pages' can actually perform the rebuild. Maybe we should display the rebuild notice on admin/content/node instead of admin/build/types/settings since users that can administer nodes could rebuild the access permissions.
From the current node_menu():
<?php$items['admin/content/node-settings/rebuild'] = array(
'title' => 'Rebuild permissions',
'page arguments' => array('node_configure_rebuild_confirm'),
// Any user than can potentially trigger a node_access_needs_rebuild(TRUE)
// has to be allowed access to the 'node access rebuild' confirm form.
'access arguments' => array('access administration pages'),
'type' => MENU_CALLBACK,
);
?>
#28
I'm not sure about putting the notice on admin/content/node, sites with trusted content editors may well grant the administer nodes permission, but those users aren't necessarily administrators as such, and are unlikely to know what the message means. Don't have a strong opinion on it though.
#29
The last submitted patch failed testing.
#30
Doesn't fail locally, re-testing.
#31
Tested and applies perfectly as well as functions perfectly. However, I disagree with the placement of the "Settings" Under "Sontent types" though and put it under "Content Management > Content " instead (admin/content/node/settings). There was also some associated help pages that weren't addressed in the original patch and needed updating if we move it so i tacked them on for good measure. I have also created another issue for rolling user setting under user as a LMT too (#391412). With both of these patches applied user and content settings will be predictably found and the menu will be a little less cluttered with so many options.
#32
Furthermore, now that we are looking at it. Id really like to move this to a per content type setting to instead of a site wide default. As well as add an option to disable "Preview" which ill write myself really quickly. But ill file those as separate issues to be handled after this one.
#33
:-( Please give some reasoning as to WHY you disagree with the placement of the settings under content types before simply doing it.
Because up till now everybody was happy with putting them there and not as a tab on the node listing page (if I understand you correctly).
#34
Sure. Post Settings was previously under Content Management with content this is where old users will look for it. Because it relates to all content and has nothing to do with content types I suggested moving it to the LMT under Content. I have a patch as well listed above to move the User Settings under Users as well for consistency. Now if someone is interested in having these change on a per content type basis (which i think would be VERY useful). Then i agree they should be under content types but under each type itself. Thoughts? -mf
#35
I think it should be per-content type, and it probably wouldn't be out of scope for this issue to do that here - then we wouldn't need to worry about where the local task goes because it'd be gone completely.
#36
@ catch. Agreed. As stated above. Where should we store the settings per type? In the node_type table? I can write it today with a little direction.
#37
Id appreciate if someone would review #275368: Allow disabling comment preview + unify with node preview settings and possibly commit it if its acceptable though so i dont have to keep rerolling these related issues.
#38
Ok ive moved the "post preview" option to under each "content type" setting (under the "form submission" fieldset) and made it content type specific. I also added a third option that allows users to disable the preview button for types that don't make sense to preview (think about "add a link" in a link directory). It is BC with previous settings.
Ive also broken out the teasers into the content type settings under the "display" fieldset but the node "type" wasn't available in the node_teaser() function so i had to add it somehow. There were two options:
1. Add it to the method signature and chase down the other methods that call it. This wasnt a bad route until i ended up at the aggregator module which calls node_teaser() and doesnt work on nodes.
2. Add the "size" argument to node_teaser() in the 2 calls from the node module (submit and preview). This uses variable_get() and the $node->type is present in both.
I chose the second option because it didnt require changing a method signature for very little gain and left node_teaser() capable of making teasers for non nodes as well. (shouldnt this be a common function maybe instead if we are using it elsewhere for non node operations?)
#39
Subscribing.
#40
Added quite a bit. So in total this patch adds a new preview "disabled" option, makes teaser and previews content type specific and adds a teaser length option for the aggregator module (which was usingthe now nonexistant default).
The patch:
* Adds a third option for disabled preview button that is backwards compat with existing installs.
* Moves the preview and teaser settings under each content type in the appropriate fieldset.
* Added teaser length setting to aggregator module for description (admin/content/aggregator/settings).
* Upgrade path for aggregator module and node module that uses old settings for all content types and new aggretator setting.
* Removed old variables where applicable but left "teaser_length" so agregator could use it to pull form on its update.
* Added uninstallation of teaser and preview variables for each conent type when that content type is destroyed
* Added uninstallation of new aggregator variable.
I think that should cover everything. From where i stand this looks pretty RTBC to me upon code review of others. I'm officially passing the buck :). Please comment. -mf
#41
Needs screenshots
#42
Looks good to me.
Here's screenshots for the new per-content-type preview and teaser settings.
The only other changes is post settings no longer existing. Didn't take a screenshot of the aggregator changes - but that'll be much the same as content types.
#43
I think the patch uploaded is for the wrong issue. Or at least I see nothing in there that removes the post settings screen.
#44
Also, adding a disabled option is another issue; please don't introduce it in this patch. This patch should *only* be about removing the post settings screen, and moving its options elsewhere.
In the meantime, here's a review of the code that I did before I realized that the patch wasn't quite doing what I thought it was doing.
---
Wow. Quite some kittens crept into this patch since last I looked at it. ;)
!= 1, != 2.. let's define some constants for these.
+ '#options' => array(t('Optional'), t('Required'), t('Disabled')),Break options onto separate lines.
+function _node_characters($length) {That function name is not at all descriptive of what it actually does. I realize this wasn't your doing, but since we're now calling this in Aggregator module as well, we should take the opportunity to rename it.
+ //get original settings and all types...
+ //apply original settings to all types
...
+ //delete old variable but leave 'teaser_length' for aggregator module upgrade
Should be "// Apply original settings to all types." and so on. (leading space, starts with capital, ends with period)
+ * Add aggregator teaser lenght to settings from old global default teaser lengthlenght -> length
+ '#type' => 'select', '#title' => t('Length of trimmed description'),Missing newline before '#title'
#45
Yes it looks like the actual post setting removal got lost in the most recent patch, by looks good I meant the screenshots :)
#46
@webchick: Addressed every issue except "function _node_characters". Would like to do some additional work on the teasers system so ill save it for that issue if thats ok.
This patch adds:
* Remove post-settings page
* Relocates "node access grants" report and rebuild option to reports page (as requested by #drupal-usability)
* Adds DRUPAL_DISABLED, DRUPAL_OPTIONAL, and DRUPAL_REQUIRED as constants. Chose not to prefix with NODE so they can be repurposed (ill submit a patch to cleanup a little more of core with these if you like them).
Previously:
* Adds a third option for disabled preview button that is backwards compat with existing installs.
* Moves the preview and teaser settings under each content type in the appropriate fieldset.
* Added teaser length setting to aggregator module for description (admin/content/aggregator/settings).
* Upgrade path for aggregator module and node module that uses old settings for all content types and new aggretator setting.
* Removed old variables where applicable but left "teaser_length" so agregator could use it to pull form on its update.
* Added uninstallation of teaser and preview variables for each conent type when that content type is destroyed
* Added uninstallation of new aggregator variable.
#47
Patch above is old. Rerolled against UNSTABLE6.
#48
The last submitted patch failed testing.
#49
Chalk one up for unit testing. I forgot to change the default value of the per-content type settings from 0 to 1 when I introduced the defined constants. This one passes tests locally now.
#51
The last submitted patch failed testing.
#52
@webchick: you asked me to remove it the preview post option from the patch but then asked me to introduce constants for it too. So i left it in but i can pull it if you dont think if belongs. Rerolling for patchbot compliance with head.
#53
I really think we should move the preview option to a separate patch, as far as I can see the constants comment by webchick was from before she'd noticed what the options were for in the first place.
+ //Get original settings and all types.Missing a space.
+ drupal_function_exists('_node_characters');I think it's better to just duplicate the code here and kill the helper, either that or rename this function, make it public, and put it in system.module or common.inc. Teaser changes are being worked on in body as field but calling private functions from aggregator in the meanwhile isn't good.
+function node_requirements($phase) {No phpdoc.
+ $node->teaser = empty($node->body) ? '' : node_teaser($node->body, $node->format, variable_get('teaser_length_' . $type, 600));I don't understand why this change is here when we're just moving a form to a different page.
#54
A couple of minor comments on this text, which IMO needs a rewrite since it is full of ambiguity and inaccuracy, but could be done as a follow-up rather than delaying this issue:
- when Drupal works out where to trim the post it considers HTML markup to be part of the post's content, so you may end up with significantly fewer visible characters than you expect (see #220783: HTML markup is counted in for the length of trimmed posts.)
- the text implies that only "long" posts will be trimmed, which could be a bit confusing since a relatively short post with 201 characters will get trimmed if the teaser length is set to 200
- the term "disable teasers" is a bit confusing; what we mean is that the full version of the post will be displayed rather than an automatically trimmed version (change to something like "To disable automatic trimming of posts, set to ..."). (NB Manually trimmed posts will still appear in their trimmed form in appropriate places.)
- Final words "existing teasers" should be something like "the trimmed version of existing posts".
#55
Yes, please. Tagging.
#56
@catch: Thx for the review. First three code issues fixed.
Regarding Last: Teasers are now defined *per content type* under Admin > Build > Content Types. As a result we have to pass the length of the content types teaser specifically. The node_teaser() function doesn't have the content type information to discover the correct length. nor do I think we want it too.
You're right about the preview post option in that it would probably make a good second issue but the update routines in the install files and a lot of the changes are somewhat integrated here and play off each other. Since we are moving it over anyway I thought it might be a good time to merge it with the same disable option I wrote for comments too. If it complicates things too much I can move it over but hopefully this works for everyone.
#57
The last submitted patch failed testing.
#58
Don't know how the user.module breakage got into that last patch. Sorry about that. Remove the conflict and reuploaded same patch as last time. -mf
#59
This looks good now. Committed to CVS HEAD. We can create follow-up patches if necessary.
Interestingly enough, we didn't had to update and tests ... this suggests we might not have sufficient test coverage for those things.
#60
node_get_type isn't anymore in nodeapi. see http://drupal.org/node/224333#node_type_get_functions
#61
#62
Committed to CVS HEAD. Thanks.
#63
The node update misses a return value and issues a warning on the update results page.
Quick fix, I dare say it's RTBC.
#64
Committed, thanks. :)
#65
Fixes a test that somehow never failed until committed?!? Was a capitalization issue on "Rebuild permissions" link in status reports. Needs to be committed quickly as tests are failing. Thanks to BrianV for noticing the failing tests.
#66
That string in l() needs a t() around it.
#67
Done.
#68
The last submitted patch failed testing.
#69
I believe this got fixed in #361277: Remove the 'post settings' admin screen and relocate contents. I've switched testing bot back on.
#70
There are some lingering references to "Post settings" that still need to be removed. Since this issue is done, I've opened a new issues to clean up these residual bits at #489402: Removing references to admin/content/node-settings.
#71
Automatically closed -- issue fixed for 2 weeks with no activity.