Contact form opt-out line should be excluded from admin-sent and sender-copy e-mails

Also affects 5.1.

Patch for 5.1 uploaded

No longer applies and should use two spaces instead of tabs for indentation. If this is still valid in D6 it'd be a nice one to fix.

Here's an updated patch. Should we be checking for uid==1, or uid > 1?

actual patch this time.

I would think > 1, or even <> 1 to allow for anonymous contact form submissions.

The value has be uid == 1 as this fix is to supress the message being displayed for the administrator.

The logic so far is incorrect. It's perfectly valid for me as user/1 to be able to turn off my contact form, so why should that line be hidden? We should *not* show this line if the user that *sent* the e-mail has the 'administer users' permission, since they can always send contact e-mails. We also shouldn't send that line if the e-mail is the copy sent to the sender.

With the attached patch, the only time the contact form opt-out line will show up is in the original e-mails to the recipient, and only if the e-mail was sent by a user without the 'administer users' permission'.

This makes sense, but a one-line comment above it would be great. It's not immediately obvious why you don't want to show the opt-out message in these cases, without mentioning that a user-administror can override the opt-out, and that $key == 'user_mail' filters out sender copies.

Revised patch for review. Arancaytar does this look good?

Re-rolled for latest commits.

Fixes the testbot exceptions.

Adding tags

patch -p0 < 124969-contact-opt-out-line-D7_2.patch
patching file modules/contact/contact.module
Hunk #1 succeeded at 199 (offset 21 lines).

This patch is simple so I see no reason in Tests. But +1 for port to D6

Need re-roll

No need to rush on any bug reports like this, we still have plenty of time to fix them. :) Re-rolled for latest changes.

It makes a lot of sense only to have the opt-out in the e-mail when there is an actual chance of opting out. I wrote a test for it.

Looks good. Test runs flawless, and the patch makes a lot of sense.

This looks really good. Some minor comments:

+++ modules/contact/contact.test
@@ -398,6 +398,29 @@ class ContactPersonalTestCase extends DrupalWebTestCase {
+    $this->assertTrue(FALSE != strpos($mails[1]['body'], $opt_out), t('Opt-out message included in email.'));

It would be great if we could split this up in 2 lines or so. The inline check is a bit ugly.

For consistency, it might be sense to treat the check above identical. That is, explicitly test the return value of strpos, probably using '!==' and '==' instead of '!=' and '=='.

All the comparison using strpos became really confusing, so I am using strstr instead because it has return values that are easier and prettier to work with. It will be marginally slower..

strstr() will not return TRUE so I feel that check is misleading. Let's continue to work with strpos().

Here is some rearranging of boolean comparisons. This is hopefully less confusing :)

Here is a reroll for D8.

Patch looks clean and there's a test included, nice work! Removed tags since the patch has a test, and there won't be a backport to D6. RTBC from me unless the testbot complains.

#25: 124969-25.patch queued for re-testing.

Reroll of #25.

Looking at the code, the text will still be included on emails sent from the admin users.

Needs a reroll

Updated patch with reroll diff.

+++ b/core/modules/contact/contact.module
@@ -161,7 +161,13 @@ function contact_mail($key, &$message, $params) {
+      if ($key == 'user_mail' && !user_access('administer users', $params['sender'])) {

user_access has been removed - See the change record

Rerolled #46. Attached diff for reference.

The tests are failing, I'll see if I can fix it.

It took me a while to figure out all the little errors here and there, but I finally got it.
Fixed all errors (mainly typos, wrong function names, missing calls), removed t() call from the test, moved the comment at ContactPersonalTest.php to before the if statement, as it seems to make more sense there, and rephrased some comments at contact.module to make them more concise.
Sending also a tests-only patch (expected to fail).

Should still be a good novice task to review.

1. +++ b/core/modules/contact/tests/src/Functional/ContactPersonalTest.php
   @@ -347,4 +347,28 @@ protected function submitPersonalContact(AccountInterface $account, array $messa
   +    $opt_out_excluded = strpos($mails[0]['body'], $opt_out_message) === FALSE;
   +    $this->assertTrue($opt_out_excluded, 'Opt-out message excluded in email.');
   +
   +    $opt_out_included = strpos($mails[1]['body'], $opt_out_message) !== FALSE;
   +    $this->assertTrue($opt_out_included, 'Opt-out message included in email.');
   

   The strpos and assertTrue is a bit awkward, can we use assertStringContainsString and assertStringNotContainsString instead?

2. +++ b/core/modules/contact/contact.module
   @@ -161,7 +161,14 @@ function contact_mail($key, &$message, $params) {
   +      $user_access = User::load($params['sender']->id())->hasPermission('administer users');
   +      // Only include the opt-out line in the original e-mail and not in the
   +      // copy to the sender. Also exclude this if the e-mail was sent from a
   +      // user administrator because they can always send e-mails even if the
   +      // contacted user has their contact form disabled.
   +      if ($key == 'user_mail' && !$user_access) {
   +        $message['body'][] = t("If you don't want to receive such emails, you can change your settings at @recipient-edit-url.", $variables, $options);
   +      }
   

   Nit: we should be using === here.

@mstrelan, thanks for the feedback!

#57.1 Sure! I'll change that.
#57.2 Okay.

Updated patch as per #57.

Going for the review

I applied the patch and reviewed the code, looks good to me, just as demanded on the issue.
Adding screenshots.

you can see that the second paragraph it's absent.

I noticed this because it is on 9.3.x and then re-testing a fail patch every two days.

This needs to be on 10.0.x, changing version. And I took a brief look at the patch and suggest two changes to comments.

This work is suitable for a first time contributor, adding tag.

1. +++ b/core/modules/contact/tests/src/Functional/ContactPersonalTest.php
   @@ -347,4 +347,26 @@ protected function submitPersonalContact(AccountInterface $account, array $messa
   +   * Tests if the email contains or not an opt-out message.
   

   That doesn't read well to me. How about 'Tests if the opt-out message is included correctly in contact emails.'

2. +++ b/core/modules/contact/tests/src/Functional/ContactPersonalTest.php
   @@ -347,4 +347,26 @@ protected function submitPersonalContact(AccountInterface $account, array $messa
   +    // Send an email from a normal user (should contain the opt-out message).
   

   What is a 'normal user'? Lets makes this specific,
   '// Send an email from a non admin user (should contain the opt-out message).'

Made changes as per comment #68.

Looks good now! Indeed the message suggested by @quietone is cleaner.

Moving to RTBC.

Unrelated random fail. Restoring status.

Random fail again. Restoring status.

1. +++ b/core/modules/contact/contact.module
   @@ -161,7 +161,14 @@ function contact_mail($key, &$message, $params) {
   +      $user_access = User::load($params['sender']->id())->hasPermission('administer users');
   

   I think this is redundant?

   $params['sender'] is the user,

   Its like $user = User::load($user->id());

   So I think it can just be $params['sender']->hasPermissions(...)

2. +++ b/core/modules/contact/contact.module
   @@ -161,7 +161,14 @@ function contact_mail($key, &$message, $params) {
   +      if ($key === 'user_mail' && !$user_access) {
   

   Is there any need to put $user_access in a variable if we only use it once?

3. +++ b/core/modules/contact/contact.module
   @@ -161,7 +161,14 @@ function contact_mail($key, &$message, $params) {
   +        $message['body'][] = t("If you don't want to receive such emails, you can change your settings at @recipient-edit-url.", $variables, $options);
   

   We have a mix of emails (in the code) and e-mails in the comment. We should standardize on one. Given 'email' exists in the original code, should we use that in the comments too?

4. +++ b/core/modules/contact/tests/src/Functional/ContactPersonalTest.php
   @@ -347,4 +347,26 @@ protected function submitPersonalContact(AccountInterface $account, array $messa
   +  public function testPersonalContactForm() {
   

   Can we add :void typehint here.

5. +++ b/core/modules/contact/tests/src/Functional/ContactPersonalTest.php
   @@ -347,4 +347,26 @@ protected function submitPersonalContact(AccountInterface $account, array $messa
   +    $mails = $this->getMails();
   ...
   +    $this->assertStringNotContainsString($opt_out_message, $mails[0]['body'], 'Opt-out message excluded in email.');
   +
   +    $this->assertStringContainsString($opt_out_message, $mails[1]['body'], 'Opt-out message included in email.');
   

   We can use \Drupal\Core\Test\AssertMailTrait::assertMailString here

Addressed points 1, 2, and 4 of comment #75, but still needs work for the remaining points of comment #75.

Sorry, forgot to add interdiff in #76 so added here.

I don't know if i understood #75.3 well, but i changed the text from "e-mails" to "messages" (sorry if i misunderstood that one).

I also changed assertStringContainsString to assertMailString as told at #75.5

Hope it helps. Kindly review it :)

I didn't pay attention to the assertion changes, sorry for #78.

I've corrected everything now. Still:

I don't know if i understood #75.3 well, but i changed the text from "e-mails" to "messages" (sorry if i misunderstood that one).

I also changed assertStringContainsString to assertMailString as told at #75.5

Hope it helps. Kindly review it :)

I have tested the patch in #79. It is working as expected.
Before

After

+++ b/core/modules/contact/contact.module
@@ -161,7 +161,13 @@ function contact_mail($key, &$message, $params) {
+      if ($key === 'user_mail' && !$params['sender']->hasPermission('administer users')) {

The user-copy not having this makes sense. Nice one. But it's not tested. Let's add a test for that as well.

I think we can adjust \Drupal\Tests\contact\Functional\ContactPersonalTest::submitPersonalContact() to have accept a bool for the send your self a copy and then use this in the new test.

Tried writing the new test testing against user-copy emails as per @alexpott request, and some changes to the patch in #79:

+++ b/core/modules/contact/tests/src/Functional/ContactPersonalTest.php
+    $this->drupalLogout($this->adminUser);
+
+    $this->assertMailString('body', !$opt_out_message, 1, 'Opt-out message excluded in email.');

This will always evaluate to true, assertMailString doesn't check if the string isn't there so I replaced it with assertStringNotContainsString.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Won't tag for it but this could use an issue summary update to match the standard template. Not the most clear.

Can verify this problem on D10

Think this needs some work though

Created userA and userB
userB is content editor with permissions to administer user (so they can see users) and permission to contact user
UserA opted out of contact form
Login as userB I would expect to not see the contact tab for userA but I do and can send an email
userA received an email with the

If you don't want to receive such emails, you can change your settings at

Discussed in Slack, I think that the tab access issue mentioned in #85 is out of scope, this issue is to address the email text only. And if the email text is corrected, it's not unexpected that admins can view your contact tab, because as the OP notes, admins can still contact you even if you have opted out.

So in the interest of seeing this resolved, I think it should only be concerned with the email content.

Edit: For clarity, if the access issue were resolved, then the email text wouldn't need to be updated. The email text change is necessary because access to the form is not restricted for user admins.

Updated issue summary.

MR created.

Hiding patches for clarity.

Ran test-only feature

1) Drupal\Tests\contact\Functional\ContactPersonalTest::testPersonalContactForm
Opt-out message included in email.
Failed asserting that false is true.
/builds/issue/drupal-124969/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/builds/issue/drupal-124969/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/builds/issue/drupal-124969/core/lib/Drupal/Core/Test/AssertMailTrait.php:100
/builds/issue/drupal-124969/core/modules/contact/tests/src/Functional/ContactPersonalTest.php:376
/builds/issue/drupal-124969/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!

As mentioned in slack #85 is out of scope for this issue and can be ignored.

Issue summary is clear about the problem and steps to show the issue.

When applying the MR and following the steps I get the expected behavior.

Believe this long standing bug is good to go.

Committed and pushed 1dbf2cc2ea to 11.x and e1a29d0851 to 10.3.x and 5fa98acae9 to 10.2.x. Thanks!