Regression: Add new forum topic not visible within a forum

I'm afraid I haven't time to review this right now, but want to say in passing that this is rather similar to #784842: shouldn't the "post new blog entry" link have 'action-links' class?.

I've applied the patch and it fixes the bug.

RTBC :)

Wow, nice sleuthing! :D

- substr() always kinda weirds me out a bit. I can see that you wouldn't be able to use something like arg(0) here because this alter hook is only checked during "compile time", and I don't particularly have any better ideas, but could you check if there is anything in either &$data or $router_item that we could potentially re-use directly instead?

- I hemmed and hawed a bit about whether or not this should have a test, since you found not one but *two* bugs. Please check forum.test and see if it's easy to add a quick two-ish line test that clicks the link on forum/1 and verifies a node form exists. "In theory" this should be pretty simple, and would ensure we don't ever break this functionality again in the future.

There's actually a bug #3 too, but it's to do with FieldAPI so I've filed it separately: #787654: deleting a bundle doesn't remove it from the $field data.

And #4: anon users are invited to log in even if once logged in they still won't be able to post content. But that's separate to what we're fixing here: #788320: anon user is invited to log in to post in the forum, even if that won't allow them to.

- substr: it looks like we can check $root_path is either 'forum' or 'forum/%'. But #788310: hook_menu_local_tasks_alter() doc is missing 2 parameters.

- tests: not sure I've put them in the best place in forum.test, as there are quite a few things in there I don't quite follow. eg viewForumTopics -- does drupalGet automatically do an assertion? It does say DIE DIE on it though ;)
At any rate, I've added two tests and they pass with the patch.

#4: 786620.drupal.forum-action-links-subpages.4.patch queued for re-testing.

Test is wrong, suppose there should be test for /forum/X page and checking is there links.
Also when user visits container page - link to point to a first forum inside, not to /node/add/forum/{container term}

PS I'm working on #12089: Better method of showing forum containers in form so propose do not point user to a page where he could get a confusing error message about "choose forum, nnot container"

Also this is a major priority because lacking of link "Add new forum topic" is very bad regression in usability of our ugly forum

Current screens

Looks good, fix the tests and this is committable. For those reviewing, remember blog post is just an example not like that by default

Also this links should not be visible for container pages (http://api.drupal.org/api/function/forum_page/7)

Also http://api.drupal.org/api/function/forum_form_alter/7 should be fixed

Current patch
- Hides context links from /forum
- Displays them at /forum/%

I think we should hide this links when we visiting a forum container but this require fixes in tests.

While we are using interaction of Context Links so no links should be displayed on pages where no ability to add node
Also #12089: Better method of showing forum containers in form this should help to predict errors

Is this a good idea to hide links for pages which lead to error submissions?

Let's search link by it's title

Everything works, but we should decide on breadcrumbs (what should they show) for nodes that is not 'forum topic'

related
#599016: Only allow nodes to be posted to forums
#787652: Forum vocabulary alterations possibly obsolete -- possible to delete forum vocab

add-new-topic-on-forum.patch queued for re-testing.

Patch code was reviewed, the change should be fine, also it passed the test, so it's ok for community review and test.

(disclaimer: sleep deprived!)

Patch applies and works as described in #12.

This looks good.

#15: 786620-forum-add-d7.patch queued for re-testing.

+++ modules/forum/forum.module	14 Aug 2010 18:37:24 -0000
@@ -167,11 +167,10 @@ function forum_menu() {
-  if ($root_path == 'forum') {
-    $tid = (isset($router_item['page_arguments'][0]) ? $router_item['page_arguments'][0] : 0);
-    $forum_term = forum_forum_load($tid);
-    if ($forum_term) {
...
+    if (isset($router_item['page_arguments'][0]) && !in_array($router_item['page_arguments'][0]->tid, variable_get('forum_containers', array()))) {
+      $forum_term = $router_item['page_arguments'][0];

That really needs to be broken out. The original code is easy to follow, I have *absolutely* no idea what this new stuff is doing.

Or at the very least, we need a comment here to explain wtf is going on. :P

Powered by Dreditor.

Changed comment and code to exclude containers.

Summary:
1) Currently local tasks (links to "add content to forum") are displayed only on /forum and not at internal pages
Proposed change
2) Links should be displayed only within forums pages "forum/%"
3) Forum containers could not contain topics so on their pages links are hidden - this behavior could change after #12089: Better method of showing forum containers in form

Related #895774: Forum field should not rely on vocabulary machine name

I have a question about the patch. Should I expect to only see the "Add new Forum topic" link while viewing a forum? Not on the forum list, /forum?

http://skitch.com/jameselliott/diyhr/forums-d7-test

If that is expected, then I would say this should be RTBC

@james.elliott Links ("Add new Forum topic") should be visible ONLY on internals pages - forum/%, because user can't add topics into containers and /forum forum list

Makes sense from a UX perspective to me. RTBC it is.

This is definitely a bug that needs to get fixed, but not at the expense of removing "Post new forum topic" from the main forum listing. That's a UI change 9 months after UI freeze that has nothing to do with fixing a bug. Please remove that portion of the patch.

I just wanted to make a quick comment re #32. It always seemed logical to me to not put this link anywhere but on the actual forum listings. After all, anywhere else forced a user to choose a forum from the dropdown which seemed like a horrible UI. I even took it out of there in AF.

But I've found on my site that the number of topics added by other people is higher now that I've put the link back on the top level. It still doesn't make sense to me but people do use that link even on the main forum listing. So taking it out is something that needs some serious discussion and, as webchick said, not this late in the game.

Michelle

This now works as advertised. "Add new Forum topic" is now visible on both /forum and /forum/*

Great! I just made one small adjustment -- removing the leading slash on the various drupalGet() calls -- and committed to HEAD.

Thanks for cleaning up this long-standing bug!