jamesmh / coravel

Near-zero config .NET library that makes advanced application features like Task Scheduling, Caching, Queuing, Event Broadcasting, and more a breeze!
https://docs.coravel.net/Installation/
MIT License
3.92k stars 257 forks source link

Calling From() in Build() to override globalFrom in Mailing #321

Closed lwestfall closed 2 months ago

lwestfall commented 1 year ago

Describe the solution you'd like Calling .From("someone@somewhere.com") in Build() doesn't seem to override the globalFrom address. As discussed in #320 it's better if calling .From() would override the globalFrom mailbox. Essentially this would mean that globalFrom acts more as a default from address that can easily be overridden conditionally or on a Mailable implementation basis.

I can work on this one and #320 together. Might be a couple of days before I get to it.

lwestfall commented 1 year ago

Found some free time today to take a stab - I think it was pretty easy (basically just swapping the null coalescing precedence on what was this._globalFrom ?? @from).

Issue I'm having is writing tests:

I'm assuming we want tests for at least SmtpMailer or FileLogMailer (if not both). Here are a few options as I see them, not mutually exclusive:

  1. Provide a public getter for Mailable._from (or could mark field or getter internal and make internal members visible to the test assembly if we want to keep _from invisible to client assemblies)
  2. Write a test class for FileLogMailer and use approach detailed in 3rd bullet above
  3. Give a globalFrom to AssertMailer (but is sort of moot since it doesn't test my changes to SmtpMailer and FileMailer)
  4. Am I missing anything?

Hope this all makes sense, let me know if you have any suggestions.

jamesmh commented 1 year ago

Thanks for looking into this.

I'm okay with adding some public getters, as long as we keep the setters private. Notably, for the SMTP Mailer this is needed for the tests to work.

For the FileLogMailer, if you want to check the headers that would work 👍

I'm indifferent about the AssertMailer since it's more-or-less a testing tool vs. production thing.

Let me know if there's anything that I missed, etc. Thanks!

jamesmh commented 2 months ago

This is fixed in Mailer 6.0.0. https://docs.coravel.net/Mailing/#sender