Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes event#64: Send event confirmation to submitted email address #21669

Merged
merged 1 commit into from
Oct 7, 2021

Conversation

MegaphoneJon
Copy link
Contributor

Overview

An event registration that's submitted with an email that differs from the (existing) contact's primary email will go to the primary email. It should go to the submitted email.

Before

Event registration confirmations go to the existing primary email.

After

Event registration confirmations go to the email submitted.

Technical Details

My last line isn't necessary - the Manage Event screen won't allow you to enable email confirmations if we don't ask for an email, so it should always be present. But who knows what those extension writers get up to?

@civibot
Copy link

civibot bot commented Sep 29, 2021

(Standard links)

@civibot civibot bot added the master label Sep 29, 2021
@demeritcowboy
Copy link
Contributor

Thanks for the patch I can take a look. I think this is addressing the same (or similar) problem that #18984 was.

@MegaphoneJon
Copy link
Contributor Author

@demeritcowboy Yes, this seems like it's the same as #18984. I see Eileen requested a test there, I'll see what I can do about that. It's a little tricky because it's form-level but we'll work it out.

@demeritcowboy
Copy link
Contributor

Great, thanks.
I was actually just looking at this and am noticing that by removing line 1110 it's possible now that $email could retain its value from earlier on in the code and so end up being the notify address, and not defaulting to primary. I.e. initialize $email to null before the loop, or change it to use a different variable.

@MegaphoneJon
Copy link
Contributor Author

OK - I incorporated feedback from #18984. I've changed the variable name so that we don't accidentally reuse an old one, changed the thank-you page email to match the confirmation email recipient, and added a test.

@demeritcowboy
Copy link
Contributor

Thanks - I should be able to look tomorrow.

@demeritcowboy
Copy link
Contributor

demeritcowboy commented Oct 7, 2021

  • General standards
    • [r-explain] PASS
    • [r-user] PASS
    • [r-doc] PASS
    • [r-run] PASS
  • Developer standards
    • [r-tech] PASS
    • [r-code] PASS
      • I think I was mistaken on the other PR about the effect of isset vs empty (and here in this PR the use of NOT (!)), because we're talking about a situation where the person is most likely a logged-in user. Otherwise the only way anything comes up is because it's a weird dedupe rule that doesn't use email, and again there's no practical difference.
    • [r-maint] PASS
    • [r-test] PASS

There are some other unusual use-cases which I'd say ignore for now but just noting that if email is optional and you leave it blank, then there's a mismatch:

  • The thank-you says it was sent to your primary, but it doesn't actually send anything.
  • It's not clear which is the desired outcome - if I leave it blank, to me it means don't send me anything, but I suppose it's possible some people might think the other way that it should default to primary, which would also be what the form does when there is no email field on the form.

There's also a pre-existing weirdness that if there is no primary and you don't enter an email, then the Thank-you page says "A registration confirmation email has also been sent to", with no email listed.

@demeritcowboy demeritcowboy merged commit 0335975 into civicrm:master Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants