_user_mail_notify() always sends emails when user is activated [#2770369]

Problem

Setting notify to FALSE on "status_activated" doesn't block _user_mail_notify() to send the email.

function _user_mail_notify($op, $account, $langcode = NULL) {
  // By default, we always notify except for canceled and blocked.
  $notify = \Drupal::config('user.settings')->get('notify.' . $op);
  if ($notify || ($op != 'status_canceled' && $op != 'status_blocked')) {

Proposed resolution

I think it lacks condition "&& $op != 'status_activated'" :

function _user_mail_notify($op, $account, $langcode = NULL) {
  // By default, we always notify except for canceled and blocked.
  $notify = \Drupal::config('user.settings')->get('notify.' . $op);
  if ($notify || ($op != 'status_canceled' && $op != 'status_blocked' && $op != 'status_activated' )) {

How to reproduce

- Set notification to false on "account activation" on Administration > Configuration > People > Account settings
- Set user.settings.notify.status_activated FALSE
- Block user
- Activate user

Comment	File	Size	Author
#2	drupal-core-user-notify-2770369-1.patch	1.36 KB	ozin
#2	8.1.x: PHP 5.5 & MySQL 5.5 18,700 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

21 July 2016 at 07:34

djidane35 created an issue. See original summary.

Comment #2

ozin

Ukrainian

🇺🇦Lviv

CreditAttribution: ozin as a volunteer and commented 26 July 2016 at 11:21

Status:

Active

» Needs review

File	Size
drupal-core-user-notify-2770369-1.patch	1.36 KB
8.1.x: PHP 5.5 & MySQL 5.5 18,700 pass

Hi @djidane35

Please check my patch if it works for you. Thanks.

Comment #3

smaz

English

England, UK

CreditAttribution: smaz at Five Mile Media commented 26 July 2016 at 20:20

I've just tested the patch in #2 by doing the following:

Visiting /admin/config/people/accounts, selecting the 'Account activation' tab in the 'Emails' field and unchecking the 'Notify user when account is activated' checkbox.
Setting user.settings.notify.status_activated to false using drush:
drush config-set user.settings notify.status_activated 0
Creating a new user, ensuring they are active.
Edited & blocked the user
Edited & activated the user.

I received the activation email.

After the patch, I re-blocked & re-activated the user, and this time didn't receive the email notification.

The patch looks fine code wise, too.

However, does this need an automated test? I can see that there are tests currently that set notification settings etc., but as there currently isn't a test for activating a user I'm not sure if one is needed or what to start with.

Cheers

Comment #4

ozin

Ukrainian

🇺🇦Lviv

CreditAttribution: ozin as a volunteer and commented 26 July 2016 at 21:47

Hi @smaz, thanks for your testing. Regarding Automated tests, I think we do not need it since it is not a new functionality or feature.

Comment #5

markdorison

he/him

CreditAttribution: markdorison at Chromatic commented 27 July 2016 at 01:24

Status:	Needs review	» Needs work
Issue tags:	-user user.module _user_mail_notify status_activated	+Needs tests

@ozin, I believe @smaz is correct; this is going to need a test.

Comment #6

ozin

Ukrainian

🇺🇦Lviv

CreditAttribution: ozin as a volunteer and commented 27 July 2016 at 06:56

Ok, then I will add tests as soon as possible.

Comment #7

djidane35 CreditAttribution: djidane35 commented 27 July 2016 at 09:26

Hi, in my opinion this kind of fix is not sustainable over time.

For example, in the future, if a notification is deactivale for another operation, it will think about going to add the operation into the array $configurable_ops.

I think we should handle this with a global configuration to be effective.