-
Notifications
You must be signed in to change notification settings - Fork 52
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
Transmission.Options.UseSink #133
base: master
Are you sure you want to change the base?
Conversation
Always remove UseSink from Options mapping
I think this will work, but I'd like the client-level change to be made. The story I'm thinking about in my head is: If I'm in a certain environment, like one set up for development or for testing, I may want to disable these emails entirely... but I still want them to be fired. Without getting into details, I know of... someone.... who has to work in such a setup, and a blanket rule of "do not send emails from this environment, but still go through SparkPost and log and all of that stuff" might be nice. This is just a programming style, but I'd never want that to set that flag on the transmission itself. I wouldn't want the concept creeping so low into the details. 😄 |
@asherber This is the first time I've used Github's tool for resolving the merge conflict. 😎 |
Are you saying you want it on the client instead of the transmission, or in addition to the transmission? I do disagree with putting this on the client. For one thing, since this is an option which alters the addresses on individual transmissions, I think the transmission is where it belongs. I'm also thinking of the use case where a client is instantiated for general use, but certain emails (say, for customers who are not yet active) should be sent to the sink. And while it's not directly relevant, other ESPs all seem to have this at the message level. If someone wanted to sink all emails from an environment, it would be easy enough to create a facade class: public class SinkableTransmission: Transmission
{
public SinkableTransmission()
{
Options.UseSink = valueOfSomeEnvironmentVariable;
}
}
var myTransmission = new SinkableTransmission(); |
But the facade class would then have to be used, which is still getting the detail. I think we could do it in both places... have it both at the transmission level, but also allow it at the client level.
In those cases, I think the client-level one would still be very helpful. Those emails that should be sent thru sink could be configured to use a different client via IoC. This is what I mean about it being a level of style... let's say that I have one particular email that I do not want to be fired from the development system. But since the development system is meant to be a fully-functioning system, I still want the jobs to run. So in my IoC, I'd set it up something like: var standardClient = new Client("MYAPIKEY");
var sunkClient = new Client("MYAPIKEY", new { sunk = true} );
For<IClient>().Use(standardClient);
For<FancyEmailService>().UseDependency<IClient>(sunkClient); I don't want the "sunk" concept dipping into my |
@asherber I think some programmers would prefer to have the "sunk" detail at a low level, and some programmers (like me) would prefer to keep the modules clean of that detail and configure it with IoC. That's all I'm saying. 😄 |
Okay, so how should the two flags interact?
|
Any news on this issue? I would really like an easy way of enabling/disabling "sink"-mode. |
Fixes #132
Had some time today, so I knocked this out. I'm happy to refactor or tweak as needed if you prefer a different approach.