Error when adding duplicate category to contact form module.

Also happens on 7.x-dev.

Steps to reproduce:

1. Enable contact module
2. Add a contact form category (Administer > Site building > Contact Form and click on the "Add Category".)
3. Add another contact form category with the same category name

Patch attached adds additional validation on the category field, making sure the category does not already exist.

Possible testbot failure, requesting re-test.

Patch is good, and resolve the problem.

db_result() is deprecated, you can use db_query()->fetchField() instead. The conditions probably don't need wrapping in extra parentheses. Also we usually use double quotes for strings with single quotes in, rather than \' since it's easier for translators to deal with and more readable generally.

Other than that, looks good.

Is there a unique key on category name? We could consider adding one as well to enforce it there as well.

Assigning myself to reroll this patch with a test.

Re-rolled with DB:TNG and a simple test case.

+++ modules/contact/contact.admin.inc	31 Jan 2010 16:10:16 -0000
@@ -112,6 +112,18 @@
+    form_set_error('category', t('A contact form with category \'@category\' already exists.', array('@category' => $category)));

Let's just use %category instead of '@category'.

+++ modules/contact/contact.test	31 Jan 2010 16:10:17 -0000
@@ -93,6 +93,11 @@
+    $this->addCategory($category, implode(',', array($recipients[0], $recipients[1], $recipients[2])), '', FALSE);

Instead of adding e-mail recipients, let's just leave that parameter as an empty string.

+++ modules/contact/contact.test	31 Jan 2010 16:10:17 -0000
@@ -93,6 +93,11 @@
+    $this->assertNoRaw(t('Category %category has been saved.', array('%category' => $category)), t('Category not saved.'));
+    $this->assertRaw(t('A contact form with category %category already exists.', array('%category' => $category)), t('Duplicate category error found.'));    

Use assertText and assertNoText. We only use assertRaw when we need to. I know we're using it elsewhere in the contact test but I'd like to fix that eventually.

Powered by Dreditor.

Thanks for the review, new patch attached.

Use assertText and assertNoText. We only use assertRaw when we need to. I know we're using it elsewhere in the contact test but I'd like to fix that eventually.

I think in this case they are needed since assertText/assertNoText looks for stripped html, causing the assertion to fail on the <em>s in '%category'.

I'm 100% sure assertText ignores HTML, but the tests are good anyway. I just prefer to use assertText since its simpler. :) Patch looks good and tested locally.

I also think we should always add 'Would you like to edit the existing [foo]?' whenever we get duplicate messages like this, but its something to work on in D8, not now.

Committed to HEAD. Thanks!

I agree assertText() seems better too, but the surrounding ones use assertRaw() so this is consistent. Could file a follow-up patch about that though.

Well, I agree that assertText looks better and makes more sense, but I'm also 100%99% sure that it won't work with the because of the <em>s :)

From the assertions page:

Asserts that the text in $text appears in the content of the current page in the SimpleTest browser (html-stripped).

Let's run it through the testbot to be sure.

setting back to fixed since we can't replace the assertRaw with assertText.

The problem is you need to provide assertText() with actual non-HTML, so instead of
$this->assertText(t('A contact form with category %category already exists.', array('%category' => $category))

You'd do:
$this->assertText(t('A contact form with category @category already exists.', array('@category' => $category))

Eh, I'd say leave it for now. If we want to go through contact.test whole-sale and clean-up the assertRaw -> assertText in another issue, then cool. But this isn't the only instance in the file, at least from my quick check.

@webchick: Oh agreed. It would be a follow-up if anything, but it's not important enough for me to work on it. :)