-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feat/mailgun entry point #94
base: milestone/mailgun-support
Are you sure you want to change the base?
Changes from 4 commits
867d238
04ad96a
91a98fa
7c7c76b
8a5a048
0d7eb5a
370da9d
d5f1d38
7429aa7
641a771
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
namespace Sinch.Mailgun | ||
{ | ||
/// <summary> | ||
/// Mailgun allows the ability to send and receive email in both US and EU regions. | ||
/// Be sure to use the appropriate region on which you have created your domain. | ||
/// </summary> | ||
public enum MailgunRegion | ||
{ | ||
/// <summary> | ||
/// United States region | ||
/// </summary> | ||
Us, | ||
/// <summary> | ||
/// Europe region | ||
/// </summary> | ||
Eu | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
using System; | ||
using Sinch.Core; | ||
using Sinch.Logger; | ||
|
||
namespace Sinch.Mailgun | ||
{ | ||
/// <summary> | ||
/// The Mailgun API is part of the Sinch family and enables you to send, track, and receive email effortlessly. | ||
/// </summary> | ||
public interface ISinchMailgunClient | ||
{ | ||
// TBD: add domains | ||
} | ||
|
||
/// <inheritdoc /> | ||
internal class SinchMailgunClient : ISinchMailgunClient | ||
{ | ||
public SinchMailgunClient(Uri baseUrl, Http http, LoggerFactory? loggerFactory = null) | ||
{ | ||
// TBD: implement domains | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
using System; | ||
using FluentAssertions; | ||
using Sinch.Mailgun; | ||
using Xunit; | ||
|
||
namespace Sinch.Tests.Mailgun | ||
{ | ||
public class MailgunClientTests | ||
{ | ||
[Fact] | ||
public void InitMailgunClient() | ||
{ | ||
var mailgun = new SinchClient(default, default, default).Mailgun("apikey", MailgunRegion.Eu); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know it's following the same design as Verification and Voice which are not using the same kind of credentials, but it still seems very weird to have to initialize a SinchClient with a triplet of undefined parameters to be able to access to the Mailgun constructor with its own parameters; knowing that on top of that, the overrides for the URL should be part of the optional fourth parameter of the SinchClient. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree, this seems somewhat weird, I was thinking about adding an empty constructor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure to understand what you want to do, but let's go ahead and give it a try. |
||
|
||
mailgun.Should().NotBeNull(); | ||
} | ||
|
||
[Theory] | ||
[InlineData("")] | ||
[InlineData(null)] | ||
public void FailEmptyApiKey(string apiKey) | ||
{ | ||
var mailgunCreation = () => new SinchClient(default, default, default).Mailgun(apiKey, MailgunRegion.Eu); | ||
|
||
mailgunCreation.Should().Throw<ArgumentNullException>(); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There shouldn't be any logic in this test function: here you are recoding the logic from the UrlResolver. It would be better to provide the expected value as part of the test data. And also adding a test name to improve the readability, such as:
The test would be more straightforward:
And in order to test all the branches from the UrlResolver method, you can consider adding another test for an unexpected Region value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, refactored the test
for a note,
MailgunRegion
is a enum, it's impossible to add pass unexpected region valueThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even with
ResolveMailgunUrl((MailgunRegion)(-1))
?I thought this was why you added a
default
clause in the switch statement in the ResolveMailgunUrl methodThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible with enum casting, yes, even if it's very unlikely, but totally possible, someone going to do that, I'll add that to the test case.
default
is for any unexpectancy, yes