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

Transmission.Options.UseSink #133

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

asherber
Copy link
Contributor

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.

@asherber asherber changed the title Transmission.UseSink Transmission.Options.UseSink Jan 19, 2017
@darrencauthon
Copy link
Owner

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. 😄

@darrencauthon
Copy link
Owner

@asherber This is the first time I've used Github's tool for resolving the merge conflict. 😎

@asherber
Copy link
Contributor Author

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();

@darrencauthon
Copy link
Owner

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.

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.

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 FancyEmailService. So I can use dependency injection to control which sparkpost clients are used where on the application level.

@darrencauthon
Copy link
Owner

@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. 😄

@asherber
Copy link
Contributor Author

Okay, so how should the two flags interact?

Client Transmission Result
True True True
True False True?
False True True
False False False

@peterwind
Copy link

peterwind commented Mar 19, 2018

Any news on this issue? I would really like an easy way of enabling/disabling "sink"-mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants