Adding Gift Certificate in admin sets Recipient to Purchaser when Send email is checked.

I noticed the same problem, so I looked at the code. Turns out there's a bug in uc_gift_certificate.admin.inc at line 204. It should be loading the recipient, but loads the purchaser instead.

And then when the module calls its function to email the recipient, the function has a bad side effect of updating the record so that the user_id is updated with the wrong info that was loaded earlier.

To fix the immediate problem, change line 204 in uc_gift_certificate.admin.inc like so:

OLD:

  $user = user_load(array('uid' => $gc->purchaser_id));

NEW:

  $user = user_load(array('uid' => $gc->user_id));

As for the mail function that updates the record, I'm not entirely sure why that's necessary. Can one of the module maintainers weigh in? It seems dangerous to have an unintended update like that in what is assumed to be a simple mail function.

Thanks, fix is checked into dev. aasarava, could you elaborate more on the it updating the user record in the mail function? From what I can tell it will only ever call user_save() when it is creating a new user record.

jrust, when I say the mail function has a side effect of updating the certificate record, I am referring to line 185 in uc_gift_certificate.module:

     if (db_query("UPDATE {uc_gift_certificates} SET user_id = %d WHERE certificate_id = %d", $cert_user->uid, $certificate_id)) {

This update happens in the function uc_gift_certificate_mail_cert_notice(). The reason I think this is strange is because the function is supposed to be responsible for mailing out a certificate, and not anything else. So the update is a "side effect". As a developer, I was surprised and frustrated to find this update taking place inside the mail function when I was debugging the earlier issue mentioned in this thread. And I'm still not sure why this update is taking place.

If the update is necessary, it would be good to remove it from the mail function and put it in its own function, and then to document the reason for doing it.

You're right, the two functionalities should be split. This is kind of a holdover from when the module was first written, way back. I'll add it to my task list.

EDIT: Nevermind, it is required because if you're creating a new user, you need the certificate to be assigned to the newly-created user, correct? I could change that long PHP line into just:

update_certificate_user($userid, $code);

because that does the same thing and will be easier to update. But I think it's impossible to avoid needing to update the certificate with the new userid. Unless I'm missing something?

EDIT again: Indeed, I am confusing two separate issues. The issue is really the dual functionality. User creation should move into its own function. I'll submit a patch that will refactor this code soon.

Here is a patch for the first part of the process. (Changing it into a function, and the new function added to the user.inc file.)

// Only send an email if we were able to assign the certificate to the user.
  if (update_certificate_user_cid($cert_user->uid, $certificate_id)) {

/**
 * Update the user a certificate belongs to from a certificate_id
 */
function update_certificate_user_cid($userid, $certificate_id) {
  db_query("UPDATE {uc_gift_certificates} SET user_id = %d WHERE certificate_id = %d", $userid, $certificate_id);
}

Fixed in CVS: Split out the issuing of a certificate from sending it