Walk you through what happened:
1. Clean HEAD install, enabled Forum module.
2. Went to create a Forum topic, it let me enter all the node details and I pressed save but since I didn't have any 'forums' or forum containers, it wouldn't let me save the Forum topic.
WTF... Can we maybe add a default Forum called 'General discussion' much like we created a default site-wide contact category? Or at least not allow forum topics to be created until a forum or forum container is created?
3. Once I created my forum and a new forum topic node, I noticed comments were disabled.
WTF... Can Forum module possibly enable comments for forum topic nodes by default? I have a feeling lots of people will have to go back just as I did and enable comments on each node until they figure out they have to change the content type's settings.
Comment | File | Size | Author |
---|---|---|---|
#40 | forum-default-taxonomy-tests-613272-40.patch | 3.74 KB | Kevin Hankens |
#37 | forum-default-taxo-tests-613272-37.patch | 3.84 KB | Kevin Hankens |
#36 | forum-default-taxo-tests-613272-36.patch | 3.83 KB | Kevin Hankens |
#34 | forum-default-taxo-613272-34.patch | 1.84 KB | Kevin Hankens |
#33 | forum-test-613272-33.patch | 1.99 KB | pcarman |
Comments
Comment #1
heather CreditAttribution: heather commentedI agree, Configuring the forum module could be made easier by having a default 'General Discussion' forum created to start.
We should make them easier to use; especially by configuring common settings by default.
I also think it's confusing you can add a forum topic, when no forum exists. See related issue: #221527: Don't show Create Forum topic (node/add/forum) when there isn't a forum
However, I didn't see your number 3: that comments were disabled by default?
Do you mean that permissions don't allow forum creation by authenticated users by default? That probably won't be changed.
Not sure I understand 3.
Comment #2
heather CreditAttribution: heather commentedAdd usability tag so other Usability team people can find this.
Comment #3
JuliaKM CreditAttribution: JuliaKM commentedI just tested this out on a fresh install and was able to replicate #2. After enabling forum and attempting to add a new forum post, I received the validation error message that "Forums field is required."
Next, I added a term to my Forums vocabulary and was able to post without an error message.
After this, I tried to reproduce the error message "Forums field is required" by deleting the term I had added to my Forums vocabulary. However, when I clicked "edit" under Operations (screenshot attached), I am sent to the page listing all content tagged with that term. There doesn't appear to be any way to delete the forum taxonomy terms after you add them.
Finally, in reference to #3, my first forum topic had comments automatically enabled.
Comment #4
Bojhan CreditAttribution: Bojhan commentedSure, patch please.
Comment #5
DamienMcKennaA related issue, but it'd be really great if there was a system-wide setting to control whether modules were expected to create sample data when installed so that beginners had less of an "ok, now what" reaction upon installing something? This could be used to decide whether a single "General Chat" forum was added, maybe even an initial topic & comment?
Comment #6
Dave ReidComment #7
joachim CreditAttribution: joachim commentedA dsm() on install with a link to the forum creation admin would be a partway solution. Or if we create a single term to start the user off, tell them about that, eg 'An initial forum board has been created for you. Customize the boards on the [link]admin page'.
Comment #8
Dave ReidInitial work to provide a 'General discussion' forum on first install/enable. Haven't touched tests yet so we'll so how it does.
Comment #9
marcingy CreditAttribution: marcingy commentedFrom a functional point of view this seems to work just fine and it makes complete sense from a usability point of view.
Comment #10
marcingy CreditAttribution: marcingy commentedAs the bot likes it mark RTBC
Comment #11
Bojhan CreditAttribution: Bojhan commentedSensible defaults, this is a good improvement.
Comment #12
Dries CreditAttribution: Dries commentedWhy was this added? Also, it seems to be the only error message starting with 'You'.
Comment #13
Dave ReidWe needed to add that validation because the forum module makes it's 'Forum selector' taxonomy term option required, but fieldapi doesn't validate when the user selects '- None -', so we have to do it manually. I'll re-roll without the 'You'.
Comment #14
sun.core CreditAttribution: sun.core commentedCause for the re-roll, probably.
This belongs into hook_install(), not _enable().
6 days to code freeze. Better review yourself.
Comment #15
sun.core CreditAttribution: sun.core commentedBut, yeah, this would be nice, but not strictly critical.
Comment #16
Dave ReidForum does all its taxonomy term setup in enable so I don't think that the Taxonomy term setup belongs in install. Re-rolled for HEAD.
Comment #17
Dave ReidNote test bot is having problems reporting back on patch status, but #16 passed: http://qa.drupal.org/pifr/test/26664
Comment #18
yoroy CreditAttribution: yoroy commented'You must' is a copy writing no-go, so changing
"+ form_set_error('taxonomy_forums', t('You must select a forum.'));
to
+ form_set_error('taxonomy_forums', t('Select a forum.'));
(don't tell me what I should do, just tell me what needs doing)
Comment #19
yoroy CreditAttribution: yoroy commented#18 passes too.
Comment #20
Dave ReidYep, looks good.
Comment #21
webchickSorry, but I'm going to have to -1 creating sample content in hook_enable(). That's what install profiles are for, since it's safe to make assumptions about said content there, but not at the module level. It's too bad our core forum module is not up to snuff enough to enable it in default.profile, but maybe we can try and organize some effort around this in Drupal 8.
I also don't see anything in the patch that enables comments by default on forums, even though that's in the original bug report. Additionally, it smells like we could use some test coverage here.
Comment #22
Dave ReidYes that's a valid reason for modules required by the default install profile, but for other modules we've already started doing this: #597540: Create a default contact category on install. The module is just not usable and causes weird errors for users without a default container.
Comment #23
Dave ReidAnd ++1000 to having standard_modules_installed() implemented in the default profile for standardizing adding test data in D8.
Comment #24
joachim CreditAttribution: joachim commentedforum_enable() already creates a vocabulary for the forum, so I don't see that also creating a term makes it worse.
Comment #25
webchickUgh. I didn't see that other patch go in. I guess there's precedence for this then, sadly... :(
I do fundamentally believe that modules are *functional* pieces and should not make assumptions about how they will be used in the field. The creation of the vocabulary is something that is functionally required for this module to actually work, the creation of terms is not. It also makes assumptions that they will not want to group their forums under categories, etc. etc. Ick.
But sigh. I guess it's no worse than what Contact module does now. Still needs tests, though.
Comment #26
joachim CreditAttribution: joachim commented> Ugh. I didn't see that other patch go in. I guess there's precedence for this then, sadly... :(
LOL... I'm pretty sure that's been in since 4.7 or earlier -- image_galleries module cribs it for its own vocab!
> the creation of terms is not.
Hmm... I think it would be enough to have a drupal_set_message on installation that says 'Now go and create some forum topics!'.
Comment #27
rgristroph CreditAttribution: rgristroph commentedHere is a patch that includes that from #18 above and also adds a test to the forum.test file which tests for the creation of the default forum. It works for me.
Comment #28
naxoc CreditAttribution: naxoc commentedSetting status for the testbot to have a go at the patch
Comment #30
Kevin Hankens CreditAttribution: Kevin Hankens commentedThe .install file patch above causes an exception to get thrown because of a duplicate key error.
Steps to replicate:
1. apply the patch
2. enable the module
3. disable/uninstall the module
4. re-enable the module.
This patch catches that exception with a message that the vocabulary already exists.
NOTE: the simpletest patch in #27 is not included here.
Comment #31
dwightaspinwall CreditAttribution: dwightaspinwall commentedTest fails (test #27)
Comment #32
Damien Tournoud CreditAttribution: Damien Tournoud commentedWhat?
Comment #33
pcarman CreditAttribution: pcarman commentedThis patch only updates the forum.test. The patch includes the additional tests written in comment #27 without the changes to other forum files.
Since the addition of the default forum vocabulary, the orphan test was failing. The test has been updated to validate the testing for orphan items.
This patch works with the patch from comment #30.
Comment #34
Kevin Hankens CreditAttribution: Kevin Hankens commentedOk, here's a simpler version of #30. The variable "forum_nav_vocabulary" was being deleted as part os #27. #30 handled an exception that was thrown as a result. After talking with Dave Reid briefly, it made more sense to just /not/ delete the variable :)
So, the current status of this issue is:
Patch from comment #33 adds tests
Patch from this comment fixes handling of default vocabulary and avoids errors in hook_uninstall.
Comment #35
rgristroph CreditAttribution: rgristroph commentedIt seems to me that the patch in #30 is on the right track, but instead of just catching the exception and putting out an error, maybe it should at least check the exception for that one case and only handle it there if it is the exception we expect ?
Probably better would be to check for the vocabulary existing, and not create it if it is there.
The patch in #34 I am not sure of, on one hand it seems better to delete the variable behind us to clean up, on the other hand we are not deleting the Forum vocabulary itself, it would seem like a bad idea to do delete the vocabulary, and leaving behind the variable as a descriptor of what it is seems OK. I think on the balance it is the smartest thing to do.
Comment #36
Kevin Hankens CreditAttribution: Kevin Hankens commentedConferring with pcarman we decided to roll the test patch in with the default vocabulary patch as the automated testing was failing.
Here it is in one bundle
Comment #37
Kevin Hankens CreditAttribution: Kevin Hankens commentedHere's a slightly revised version with proper comment formatting.
Comment #38
fending CreditAttribution: fending commentedReviewed in context of #30+, RTBC. Looks great, works well; thanks for fixing up those comments in *.test.
Comment #39
Dave ReidSee below.
I don't think this is necessary for the tests to pass. The module list should be refreshed between every test case.
Are we re-using this function anywhere else? If not, let's just put this into testForum().
Powered by Dreditor.
Comment #40
Kevin Hankens CreditAttribution: Kevin Hankens commentedGreat, here's the revised patch that addresses those concerns.
Comment #41
rgristroph CreditAttribution: rgristroph commentedI can confirm that the patch in #40 passes all the forum tests and works, I also red through it and all makes sense.
Comment #42
heather CreditAttribution: heather commentedHeya @rgristroph do you mean RTBC?
By your comments it seems so.
Comment #43
rgristroph CreditAttribution: rgristroph commentedYes, I think this is RTBC.
Comment #44
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.
Comment #46
kenorb CreditAttribution: kenorb commentedAny solution/backport for 6.x?