Posted by bradlis7 on March 5, 2007 at 6:42pm
11 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | contact.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Dave Reid |
| Status: | needs review |
| Issue tags: | needs backport to D6, Needs tests |
Issue Summary
I just noticed that when an admin sends an email to a user, it has this message:
Brad Landis (site/user/1) has sent you a message via your
contact form (site/user/12/contact) at siteName.If you don't want to receive such e-mails, you can change your settings at
site/user/12.
Ironically, even if you change the settings, the admin can still contact you, which was the case for this email. If the admin user sends the email, this message should not be included.
Comments
#1
Also affects 5.1.
Patch for 5.1 uploaded
#2
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.
#3
Here's an updated patch. Should we be checking for uid==1, or uid > 1?
#4
actual patch this time.
#5
I would think > 1, or even <> 1 to allow for anonymous contact form submissions.
#6
The value has be uid == 1 as this fix is to supress the message being displayed for the administrator.
#7
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'.
#8
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.#9
#10
Revised patch for review. Arancaytar does this look good?
#11
Re-rolled for latest commits.
#12
The last submitted patch failed testing.
#13
Fixes the testbot exceptions.
#14
Adding tags
#15
patch -p0 < 124969-contact-opt-out-line-D7_2.patchpatching 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
#16
The last submitted patch failed testing.
#17
Need re-roll
#18
No need to rush on any bug reports like this, we still have plenty of time to fix them. :) Re-rolled for latest changes.
#19
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.
#20
Looks good. Test runs flawless, and the patch makes a lot of sense.
#21
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 '=='.
#22
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..
#23
strstr() will not return TRUE so I feel that check is misleading. Let's continue to work with strpos().
#24
Here is some rearranging of boolean comparisons. This is hopefully less confusing :)