(Tests needed) No welcome mail sent to new users after site administrator approval

How about this one? Does it work?

uploading the right patch.

I can not confirm the bug as it is described. The current D7 CVS Head is working. almost. This is what i did:

- Installed D7 Core from CVS
- Created an account and received the welcome message (your account is waiting for approval) to the new user and the mail for the webmasters account to approve the new user.
- logged in as webmaster (uid 1) and headed to admin/people
- Select the new user and choose "Unblock the selected User".

I then receive the confirmation mail as the new user with a link pointing to /user. BUT: The user has no chance to set a password, because the link in the mail is malformed. I figured out, the the default mail text in user.module uses the wrong variable. Please review the patch attached (ok, it is very simple).

I cannot reproduce the issue reported in the OP.

To investigate this issue I added a watchdog() call inside _user_mail_notify(), and the call to drupal_mail() around line 3338 in user.module,v 1.1195 is indeed called.

Further, I added watchdog() calls at the end of _user_mail_text(), and verified that it returns the correct values.

Can you provide more detail about exactly what you did to create this issue?
Do you have non-default language settings?
In order to test, I used the "bulk operations" on admin/people to unblock, block, and re-block, as well as editing the user directly at user/%uid/edit.

Can you provide evidence that this is an issue at the Drupal layer and not the server or mail delivery layer?
ie. check that the smtp log shows the welcome email but not the approval email; check your mail client's spam folder; etc.

also: there is no patch to review yet. All the patches in this issue address #893276.
Switching back to active.

Proper title. It doesn't look like these patches attempt to fix the bug?

The proper way to prove that an expectation is not met is to write a test for it (or fix existing ones). I'd recommend to start doing just that.

I've been able to reproduce this problem locally, and may have a fix.

Here is the code in question again for reference, same as #18:

function user_profile_form_submit($form, &$form_state) {
  $account = $form_state['user'];
  $category = $form['#user_category'];
  // Remove unneeded values.
  form_state_values_clean($form_state);

  entity_form_submit_build_entity('user', $account, $form, $form_state);

  // Populate $edit with the properties of $account, which have been edited on
  // this form by taking over all values, which appear in the form values too.
  $edit = array_intersect_key((array) $account, $form_state['values']);

  user_save($account, $edit, $category);

For user_save() to send an email when a user is activated, $account->status must equal 0, and $edit->status must equal 1 at the time of the call to user_save(), so that user_save() can determine that the status has just been changed (Again, this is pointed out in #18)

The problem is that entity_form_submit_build_entity() in the above code overwrites the values of $account with values from $form_state (ie, destroying the previous state of $account), before user_save() is called. This makes it impossible to detect what has changed.

ADDITIONALLY, I believe the parameter order for array_intersect_key() is backwards; array_intersect_key() populates the return array using values from the FIRST parameter (which is $account here), but we want $edit to be populated using values from the newly submitted form data in $form_state. The only reason the current code works (insofar as saving the data properly) is because entity_form_submit_build_entity() copies all the $form_state data into $account prior to the array_intersect_key() call.

So, my solution is twofold:

1) Reverse the parameter order of array_intersect_key(), to populate $edit from $form_state values instead of $account values

2) Don't call entity_form_submit_build_entity() until after user_save()

The code above, with these two changes:

function user_profile_form_submit($form, &$form_state) {
  $account = $form_state['user'];
  $category = $form['#user_category'];
  // Remove unneeded values.
  form_state_values_clean($form_state);

  // Populate $edit with the properties of $account, which have been edited on
  // this form by taking over all values, which appear in the form values too.
  $edit = array_intersect_key($form_state['values'], (array) $account);

  user_save($account, $edit, $category);
  entity_form_submit_build_entity('user', $account, $form, $form_state);

With these changes, activating a user account now sends the email properly on my local install.

I'm afraid I don't really know my way around creating patches or tests, but I am attending the Copenhagen code sprint tomorrow (webchick invited me in person, so I can hardly say no ;) to hopefully learn just that, so I will make it my mission to get a patch for this change created and tested. (Unless someone else beats me to it)

Well nuts. I was worried that moving entity_form_submit_build_entity() might have unintended consequences.

Or, is it possible that the test itself causes problems? I have no idea.

1) Is it possible we try the change by itself, without the new test? Or is that strictly forbidden?

2) I will also try a different approach, preserving the value of $account in a separate value, and passing that to user_save()

OK, I'm attempting to re-submit the patch with the test removed, to see how that affects the results.

OK, I have a new approach here, preserving the original status value and then restoring it, leaving everything else unchanged. This tests OK locally (email sends properly).

Here is the code:

/**
 * Submit function for the user account and profile editing form.
 */
function user_profile_form_submit($form, &$form_state) {
  $account = $form_state['user'];
  
  // preserve original value of account status
  $original_status = $account->status;
  
  $category = $form['#user_category'];
  // Remove unneeded values.
  form_state_values_clean($form_state);

  entity_form_submit_build_entity('user', $account, $form, $form_state);

  // Populate $edit with the properties of $account, which have been edited on
  // this form by taking over all values, which appear in the form values too.
  $edit = array_intersect_key((array) $account, $form_state['values']);

  // restore original account status, so user_save() can detect status change and send email
  $account->status = $original_status;
  
  user_save($account, $edit, $category);

Can you try to make a new patch with this?

I am attempting to get setup here with CVS properly so I can make my own patches, but that might take a while.

created patch from #35. worked for me on local installation

Ha, I JUST finished downloading the whole 2.5GB Xcode, installed to get CVS, and finally rolled my very own first patch... and you beat me to it. ;)

To give an update, there's about 3-4 of us here at the CPH code sprint looking into this bug. We have another approach to try after this one too, which may be a "better" solution that simply saving and restoring the status flag.

Hi,

I have a slightly different approach. The problem in the code was that $account is just a reference to $form_state['user']. So changes to $account change it everywhere an thus overwrite the original status, I cloned $form_state['user'] into account and used $edit as a reference to that and changed it from there.

The array_interset_key() call actually does the same as entity_form_submit_build_entity, so I removed it. My patch includes the test from #37.

hm, just preserving the status seems like a hack for this special use-case to me. Attached patch tries to fix the problem in general.

Yes agreed, #35 is definitely a workaround. I tried to get more at the root problem in #28 but clearly don't know what I'm mucking with, hence my more "minimal changes possible" approach in #35.

Volx has a better approach in #42 with more general solution, I think similar or identical to fago's approach in #45.

Volx's solution has passed the entire test suite on his local, including reglogge's test in #37.

Here is fago's patch with the tests from regloggle in #37.

I revoke my comment that we can remove the array_intersect_key() line, so that makes my patch equivalent to patch #48.

#42 has inconsistent variable naming for $edit - an array of values - and the user account. $edit should always be what it is, and not sometimes something else.

Following the steps in original post the patch at #48 is successfully sending emails to the new user once the account is activated

+++ modules/user/user.test	27 Aug 2010 10:44:09 -0000
@@ -1084,6 +1084,26 @@ class UserAdminTestCase extends DrupalWe
+    // Test unblocking of a user from /admin/people page and sending of activation mail
...
+    // Test blocking and unblocking another user from /user/[uid]/edit form and sending of activation mail

Comments should wrap at 80 chars.

+++ modules/user/user.test	27 Aug 2010 10:44:09 -0000
@@ -1084,6 +1084,26 @@ class UserAdminTestCase extends DrupalWe
+    $this->assertEqual($account->status, 1, 'User C unblocked');
+    $this->assertMail("to", $account->mail, "Activation mail sent to user C");
...
+    $this->assertEqual($account1->status, 1, 'User D unblocked');
+    $this->assertMail("to", $account1->mail, "Activation mail sent to user D");

These are the only two mail assertions in here.

1) This patch does not test whether the mail that is sent is the welcome mail (NOT the activation mail).

2) The test should parse + use the link from the mail that is sent.

Powered by Dreditor.

Tagging.

Thanks!

+++ modules/user/user.test	31 Aug 2010 11:09:28 -0000
@@ -1,5 +1,5 @@
-// $Id: user.test,v 1.101 2010/08/27 11:28:45 webchick Exp $
+// $Id: user.test,v 1.100 2010/08/05 23:53:39 webchick Exp $

Watch out, your local working copy is outdated. A 'cvs update' is required.

+++ modules/user/user.test	31 Aug 2010 11:09:28 -0000
@@ -29,6 +29,7 @@ class UserRegistrationTestCase extends D
+    $this->assertMailString('body', 'user/reset/' . $new_user->uid, 1);

For all these URLs, we should be checking the entire, absolute URL, i.e.:

url('user/reset/' . $uid, array('absolute' => TRUE))

And, at the right point, we want to actually use ("click") that URL/link -- that's why we should rather use pattern/regex matching here, so we get the full URL including the mail- and user-specific token.

+++ modules/user/user.test	31 Aug 2010 11:09:28 -0000
@@ -1077,33 +1078,38 @@ class UserAdminTestCase extends DrupalWe
-    $this->assertEqual($account->status, 1, 'User C not blocked');
+    $this->assertEqual($account->status, 1, t('User C not blocked'));

Can we remove these unrelated changes, please? There's still no clear direction about whether we want t() in assertion messages or not. That's discussed in a (very long) separate issue already.

Powered by Dreditor.

Only touching tests, so demoting to normal.