Send a Single Email to a Single Recipient #331 #350

M3LiNdRu · 2016-10-15T11:22:59Z

This pull request doesn´t solve the issue #331. But it´s an inflexion point to start following the same arquitecture and conventions. Due to the refactoring I'm applying to the source code I prefered split the issue in small Pull Request.

In this PR I included:

Added new project for Integration Test using AAA pattern
Started refactor in SendGridAPIClient to follow SOLID principles
Created Mail Service to implement operations related to Mail

Created Interface for Mail Service Refactored Client for using services

M3LiNdRu · 2016-10-15T11:35:09Z

@thinkingserious I would like to discuss a couple of architecture concepts.

Following the single responsibility principle. I think Client class should be responsible for handling all HTTP stuff and API Services (Mail Service) shouldn't deal with any HTTP concept like Request or Response. EmailService only would send his Custom Request object to Client and then Client would has a internal serializer to serialize/deserialize Custom Request/Response objects.
Also, In my opinion, the mail helper has too much responsabilities. We should be refactored.

If you are agree with the above points, I don't have any problem to create issues for covering that.

Many Thanks

thinkingserious · 2016-10-17T18:27:36Z

Hello @M3LiNdRu,

I agree with your comments on the responsibility of the Client class.

What responsibilities do you think the mail helper should cover?

M3LiNdRu · 2016-10-18T17:44:50Z

In my opinion, regarding the api v3 reference, the mail helper it would be responsible of the mail request body parameters. I think the mail helper should be refactored as an object, not as a helper. It contains all mail info.

@thinkingserious What do you think?

…ed methods. Also, created a new method in client to send post data. Refactored classes names for convention

M3LiNdRu · 2016-10-18T22:03:51Z

That PR fix the issue #331. Changes applied:

Refactored Client.cs, it's splitted in two classes (SendGridAPI, SendGridHTTPClient). SendGridAPI contains the API and SendGridHttpClient all HTTP stuff. A new method has been created for posting data (PostAsync) to send grid api.
Refactor Mail Helper. It´s splitted in two classes as well (Models.Mail, MailExtensions) Models.Mail contains the real object described in the SendGrid API Reference. MailExtensions contains all methods related to the Models.Mail
Changed some names and following a convention. Right now, extend the code with more funcionalities is really handy and it doesn't require too much overwork. Just creating the interface/service, isolate the object to Models folder and registering the service in SendGridAPI class.

thinkingserious · 2016-10-19T20:59:42Z

SendGrid/Example/Example.cs


            mail.TemplateId = "13b8f94f-bcae-4ec6-b752-70d6cb59f932";
            mail.Personalization[0].AddSubstitution("-name-", "Example User");
            mail.Personalization[0].AddSubstitution("-city-", "Denver");

-            dynamic response = await sg.client.mail.send.post(requestBody: mail.Get());
+            dynamic response = await sg.client.mail.send.post(requestBody: mail.Serialize());


I've removed this dynamic functionality, please the code I just merged.

@thinkingserious I'd say I'm not able to get this change until you accept your pull request and you merge it to the branch

Correct, that is coming soon. I apologize for the delay. I had a couple of days of PTO and I'm currently trying to catch up. Please stay tuned ...

No hassle, there is no rush ;)

Thanks for your patience!

thinkingserious · 2016-10-19T21:06:56Z

SendGrid/Example/Example.cs

@@ -35,13 +35,13 @@ private static void Main()
            String subject = "I'm replacing the subject tag";
            Email to = new Email("[email protected]");
            Content content = new Content("text/html", "I'm replacing the <strong>body tag</strong>");
-            Mail mail = new Mail(from, subject, to, content);
+            SendGrid.Models.Mail mail = new SendGrid.Models.Mail(from, subject, to, content);


I'm thinking we move this code behind the scenes.

I think something like this would replace this example:

String apiKey = Environment.GetEnvironmentVariable("SENDGRID_APIKEY", EnvironmentVariableTarget.User); email = new Email(SendGrid.SendGridAPIClient(apiKey)); // The internal Email object would be renamed to Contact and the object would be instantiated behind the scenes, same with all the other getters and setters. email.SetFrom("[email protected]"); email.AddSubject("I'm replacing the subject tag"); email.AddContent("text/html", "I'm replacing the <strong>body tag</strong>"); email.SetTemplateID("13b8f94f-bcae-4ec6-b752-70d6cb59f932"); email.AddSubstitution("-name-", "Example User"); email.AddSubstitution("-city-", "Denver"); if(email.Verify() == True){ response = email.Send(); } Console.WriteLine(response); Console.ReadLine();

I agree to inject the Api client as a dependency, but I think it should be injected in the EmailService not in the Email object. In my opinion, Email object should be just a DTO.

thinkingserious · 2016-10-19T21:14:38Z

Please see my comments in the review, and following are my thoughts on your previous comment:

Refactored Client.cs, it's splitted in two classes (SendGridAPI, SendGridHTTPClient). SendGridAPI contains the API and SendGridHttpClient all HTTP stuff. A new method has been created for posting data (PostAsync) to send grid api.

Please see my comments along with the branch I just merged into v9beta to checkout the direction I'm thinking.

Refactor Mail Helper. It´s splitted in two classes as well (Models.Mail, MailExtensions) Models.Mail contains the real object described in the SendGrid API Reference. MailExtensions contains all methods related to the Models.Mail

I like the direction you are going. I'll dig in deeper when I finish my current tasks.

Changed some names and following a convention. Right now, extend the code with more funcionalities is really handy and it doesn't require too much overwork. Just creating the interface/service, isolate the object to Models folder and registering the service in SendGridAPI class.

Thanks for all of your help thus far. I'll be able to collaborate with you closer once I finish up my current tasks. That said, please do continue the conversation.

thinkingserious · 2016-10-19T22:41:50Z

Here is my working pull request/branch: #356

jduncanator · 2016-11-16T00:03:24Z

Looking at this very briefly, it doesn't look like it will work very well (if at all) in multi-threaded applications. Why do you assign the mail request to the client and then call SendAsync? Why not pass the Mail model into the SendAsync method and send it from there. At least the current implementation can share a Sendgrid Client across threads. With the code in it's current state, I have to implement locking on the MailRequest member so as to make sure I do not overwrite a MailRequest from one thread whilst another thread is currently sending the email.

I personally think this is a step in the wrong direction.

jduncanator · 2016-11-16T00:07:31Z

SendGrid/SendGrid.IntegrationTests/MailServiceTest.cs

+
+
+            //Act
+            _client.Mail.MailRequest = mail;


How will this work in multi-threaded applications? If one thread writes to this variable whilst another thread is sending an email, you have a recipe for disaster. In it's current state it would be up to the consumer to implement locking on this variable. This should be handled internally and the library should be completely thread-safe.

chadwackerman · 2016-11-16T01:22:18Z

I'm sorry if any of this comes of as inappropriate but I cannot believe the amount of effort that has been expended in this thread and the end result as @jduncanator points out is junior engineering bugs.

Can you please just hire a single experienced C# dev to write this API for you? This thing is totally off in the weeds. It's taken a month for someone sane to step in and say "uh, this isn't how anyone else does it guys." Good lord.

thinkingserious · 2016-11-16T01:24:53Z

@chadwackerman @jduncanator,

I'll be coming back to this thread shortly, I need to finish this first: #356

M3LiNdRu added 4 commits October 15, 2016 12:36

Added a project for IntegrationTest. Example project is deprecated

6b4a6ac

Isolated HttpVerbs to a new internal class

d6af31e

Created Interface for Mail Service Refactored Client for using services

Implement MailService

a21c87c

Added first integration test

27b72c6

This was referenced Oct 15, 2016

Send a Single Email to Multiple Recipients #333

Closed

Add interfaces to support SOLID #201

Closed

M3LiNdRu added 2 commits October 18, 2016 20:57

Refactored Mail Helper. Isolated Mail model in a new class and extend…

2b39013

…ed methods. Also, created a new method in client to send post data. Refactored classes names for convention

using another mail in test method

874ad04

thinkingserious added type: community enhancement feature request not on Twilio's roadmap status: cla signed status: work in progress Twilio or the community is in the process of implementing labels Oct 18, 2016

M3LiNdRu added 2 commits October 18, 2016 23:38

Disposing objects in Mail Service

17c3152

Added Integration Test for SendMailToMultipleRecipients

2d714b0

M3LiNdRu changed the title ~~Send a Single Email to a Single Recipient #331~~ Send a Single Email to a Single Recipient 1/2 #331 Oct 18, 2016

M3LiNdRu changed the title ~~Send a Single Email to a Single Recipient 1/2 #331~~ Send a Single Email to a Single Recipient #331 Oct 18, 2016

thinkingserious requested changes Oct 19, 2016

View reviewed changes

thinkingserious mentioned this pull request Nov 15, 2016

BREAKING CHANGES #317

Closed

jduncanator reviewed Nov 16, 2016

View reviewed changes

thinkingserious closed this Feb 22, 2017

Send a Single Email to a Single Recipient #331 #350

Send a Single Email to a Single Recipient #331 #350

Uh oh!

Conversation

M3LiNdRu commented Oct 15, 2016

Uh oh!

M3LiNdRu commented Oct 15, 2016

Uh oh!

thinkingserious commented Oct 17, 2016

Uh oh!

M3LiNdRu commented Oct 18, 2016

Uh oh!

M3LiNdRu commented Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thinkingserious commented Oct 19, 2016

Uh oh!

thinkingserious commented Oct 19, 2016

Uh oh!

jduncanator commented Nov 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chadwackerman commented Nov 16, 2016

Uh oh!

thinkingserious commented Nov 16, 2016

Uh oh!

Uh oh!

M3LiNdRu commented Oct 18, 2016 •

edited

Loading

jduncanator commented Nov 16, 2016 •

edited

Loading