Feature - Add batch email support to .NET client #158
Conversation
…ail request to reuse it in bulk email feature.
- Updated Batch Email Send Request and Response structures - Updated Send Email Response structure - Updated/Corrected validation rules for EmailRequest, SendEmailRequest and BatchEmailRequest and corresponding tests are added/modified - Added tests for BatchEmailResponse and BatchSendEmailResponse to ensure correct serialization and deserialization. - Removed obsolete EmailResponseTests and consolidated error handling in response tests.
…/mailtrap-dotnet into feature/95-batch-email
- Refactor email request validation and documentation improvements
- Added Batch Email Send Example - Updated Response Models: error list no need to be nullable
- documentation improvements - code cleanup for tests
- Few comments added in batch email sending example
WalkthroughAdds batch email support and refactors email clients to a generic interface model. Introduces ISendEmailClient and IBatchEmailClient, generic IEmailClient<TRequest,TResponse>, EmailRequest/BatchEmailRequest models, validators, builders, responses, endpoint/factory updates, DI registrations, a batch example project, and many tests. C# LangVersion bumped to 13.0. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant MailtrapClient
participant EmailClientFactory
participant EndpointProvider
participant SendEmailClient as SendClient
participant API as Mailtrap API
App->>MailtrapClient: Transactional()/Bulk()/Test(inboxId)
MailtrapClient->>EmailClientFactory: Create(...inboxId)
EmailClientFactory->>EndpointProvider: GetRequestUri(isBatch=false,isBulk,inboxId)
EndpointProvider-->>EmailClientFactory: Uri (/api/.../send)
EmailClientFactory-->>MailtrapClient: ISendEmailClient (SendClient)
App->>SendClient: Send(SendEmailRequest)
SendClient->>API: POST /send payload
API-->>SendClient: SendEmailResponse
SendClient-->>App: SendEmailResponse
sequenceDiagram
autonumber
actor App
participant MailtrapClient
participant EmailClientFactory
participant EndpointProvider
participant BatchEmailClient as BatchClient
participant API as Mailtrap API
App->>MailtrapClient: BatchTransactional()/BatchBulk()/BatchTest(inboxId)
MailtrapClient->>EmailClientFactory: CreateBatch(...inboxId)
EmailClientFactory->>EndpointProvider: GetRequestUri(isBatch=true,isBulk,inboxId)
EndpointProvider-->>EmailClientFactory: Uri (/api/.../batch)
EmailClientFactory-->>MailtrapClient: IBatchEmailClient (BatchClient)
App->>BatchClient: Send(BatchEmailRequest)
BatchClient->>API: POST /batch payload
API-->>BatchClient: BatchEmailResponse (responses[], errors[])
BatchClient-->>App: BatchEmailResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Mailtrap.Abstractions/TestingMessages/Requests/ForwardTestingMessageRequestValidator.cs (1)
4-13: Add XML documentation to align with established validator patterns.This validator is missing XML documentation. According to the established pattern in this repository, all validators should have comprehensive XML documentation covering the class purpose and the static Instance property.
Based on learnings.
Apply this diff to add XML documentation:
+/// <summary> +/// Validator for <see cref="ForwardTestingMessageRequest"/>. +/// Ensures that the email address is valid. +/// </summary> internal sealed class ForwardTestingMessageRequestValidator : AbstractValidator<ForwardTestingMessageRequest> { + /// <summary> + /// Gets the singleton instance of the validator. + /// </summary> public static ForwardTestingMessageRequestValidator Instance { get; } = new();tests/Mailtrap.UnitTests/Emails/Requests/SendEmailRequestTests.cs (1)
93-101: Fix the incorrect test method name.The method is named
Validate_Should_Fail_WhenRequestIsValidbut the test body asserts that validation should pass (result.IsValid.Should().BeTrue()). This is a critical naming inconsistency.Apply this diff to correct the method name:
- public void Validate_Should_Fail_WhenRequestIsValid() + public void Validate_Should_Pass_WhenRequestIsValid() { var request = CreateValidRequest(); var result = request.Validate(); result.IsValid.Should().BeTrue(); result.Errors.Should().BeEmpty(); }src/Mailtrap.Abstractions/IMailtrapClient.cs (1)
49-82: Document breaking return type change and update examples
- Changing
IMailtrapClient.Email(),Transactional(),Bulk(), andTest()return types fromIEmailClient<…>toISendEmailClientis a binary-breaking change. Add a release-note entry and bump the major version.- Update
src/Mailtrap.Abstractions/README.mdanddocs/cookbook/send-email.mdto replaceIEmailClientwithISendEmailClient(andIBatchEmailClientwhere applicable), and include migration snippets.
🧹 Nitpick comments (43)
tests/Mailtrap.UnitTests/Emails/Requests/SendEmailRequestTests.cs (3)
30-60: Address the ignored test or remove it.The test is ignored due to "Flaky JSON comparison" and contains a TODO comment about finding a more stable assertion approach. An ignored test provides no value and creates maintenance burden.
Consider one of these approaches:
- Remove the test entirely if the round-trip test at lines 18-27 provides sufficient coverage.
- Fix the flakiness by using
JsonDocumentto parse and compare JSON structures rather than string comparison:- [Test] - [Ignore("Flaky JSON comparison")] - public void Should_SerializeAndDeserializeCorrectly_2() + [Test] + public void Should_SerializeTemplateVariablesCorrectly() { var request = SendEmailRequest .Create() .From("[email protected]", "John Doe") .To("[email protected]") .Template("ID") .TemplateVariables(new { Var1 = "First Name", Var2 = "Last Name" }); var serialized = JsonSerializer.Serialize(request, MailtrapJsonSerializerOptions.NotIndented); - - // TODO: Find more stable way to assert JSON serialization. - serialized.Should().Be( - "{" + - "\"from\":{\"email\":\"[email protected]\",\"name\":\"John Doe\"}," + - "\"to\":[{\"email\":\"[email protected]\"}]," + - "\"cc\":[]," + - "\"bcc\":[]," + - "\"attachments\":[]," + - "\"headers\":{}," + - "\"custom_variables\":{}," + - "\"template_uuid\":\"ID\"," + - "\"template_variables\":{\"var1\":\"First Name\",\"var2\":\"Last Name\"}" + - "}"); - - - // Below would not work, considering weakly-typed nature of the template variables property. - //var deserialized = JsonSerializer.Deserialize<TemplatedEmailRequest>(serialized, MailtrapJsonSerializerOptions.NotIndented); - //deserialized.Should().BeEquivalentTo(request); + + using var doc = JsonDocument.Parse(serialized); + var root = doc.RootElement; + + root.GetProperty("from").GetProperty("email").GetString().Should().Be("[email protected]"); + root.GetProperty("template_uuid").GetString().Should().Be("ID"); + root.GetProperty("template_variables").GetProperty("var1").GetString().Should().Be("First Name"); + root.GetProperty("template_variables").GetProperty("var2").GetString().Should().Be("Last Name"); }
79-90: Strengthen the error message assertion.The assertion
.Contain(e => e.Contains("recipient"))is too weak and could pass with unrelated error messages containing the word "recipient".Apply this diff to use a more specific assertion:
var result = req.Validate(); result.IsValid.Should().BeFalse(); - result.Errors.Should().Contain(e => e.Contains("recipient")); + result.Errors.Should().Contain(e => e.Contains("At least one recipient"));Alternatively, if you know the exact error message from the validator, assert on that specific message.
117-135: Strengthen the error message assertion.Similar to the earlier test, the assertion
.Contain(e => e.Contains(invalidRecipientType))is too weak. It could pass if any error message contains "To", "Cc", or "Bcc" in any context.Apply this diff to use a more specific assertion pattern:
var result = request.Validate(); result.IsValid.Should().BeFalse(); - result.Errors.Should().Contain(e => e.Contains(invalidRecipientType)); + result.Errors.Should().Contain(e => + e.Contains(invalidRecipientType) && + (e.Contains("must not exceed") || e.Contains("1000")));This ensures the error is specifically about the recipient count limit, not just any error mentioning the recipient type.
src/Mailtrap/Emails/Requests/BatchEmailRequestBuilder.cs (1)
37-44: Initialize Requests with ??= to avoid throws; avoid depending on caller to pre-initCurrent guards will throw if Requests is null. Safer to coalesce to a concrete list before add.
Apply:
@@ - Ensure.NotNull(batchRequest, nameof(batchRequest)); - Ensure.NotNull(batchRequest.Requests, nameof(batchRequest.Requests)); - Ensure.NotNull(requests, nameof(requests)); + Ensure.NotNull(batchRequest, nameof(batchRequest)); + Ensure.NotNull(requests, nameof(requests)); + batchRequest.Requests ??= new List<SendEmailRequest>(); @@ - batchRequest.Requests.AddRange(requests); + batchRequest.Requests.AddRange(requests); @@ - Ensure.NotNull(batchRequest, nameof(batchRequest)); - Ensure.NotNull(batchRequest.Requests, nameof(batchRequest.Requests)); - Ensure.NotNull(requests, nameof(requests)); + Ensure.NotNull(batchRequest, nameof(batchRequest)); + Ensure.NotNull(requests, nameof(requests)); + batchRequest.Requests ??= new List<SendEmailRequest>(); @@ - batchRequest.Requests.AddRange(requests); + batchRequest.Requests.AddRange(requests); @@ - Ensure.NotNull(batchRequest, nameof(batchRequest)); - Ensure.NotNull(batchRequest.Requests, nameof(batchRequest.Requests)); - Ensure.NotNull(request, nameof(request)); + Ensure.NotNull(batchRequest, nameof(batchRequest)); + Ensure.NotNull(request, nameof(request)); + batchRequest.Requests ??= new List<SendEmailRequest>();Note: if List requires a using, fully qualify or ensure System.Collections.Generic is available.
Also applies to: 72-79, 107-114
src/Mailtrap/Emails/Requests/EmailRequestBuilder.cs (4)
195-204: Make Attach resilient: coalesce Attachments before AddRangeAvoid throwing if Attachments is null by initializing it.
public static T Attach<T>(this T request, params Attachment[] attachments) where T : EmailRequest { Ensure.NotNull(request, nameof(request)); - Ensure.NotNull(request.Attachments, nameof(request.Attachments)); Ensure.NotNull(attachments, nameof(attachments)); + request.Attachments ??= new List<Attachment>(); request.Attachments.AddRange(attachments); return request; }
333-342: Initialize Headers to avoid null dereference; key guard is goodCoalesce the dictionary before use.
public static T Header<T>(this T request, string key, string value) where T : EmailRequest { Ensure.NotNull(request, nameof(request)); - Ensure.NotNull(request.Headers, nameof(request.Headers)); Ensure.NotNullOrEmpty(key, nameof(key)); + request.Headers ??= new Dictionary<string, string>(); request.Headers[key] = value; return request; }
416-425: Initialize CustomVariables to avoid null dereference; key guard is goodCoalesce the dictionary before use.
public static T CustomVariable<T>(this T request, string key, string value) where T : EmailRequest { Ensure.NotNull(request, nameof(request)); - Ensure.NotNull(request.CustomVariables, nameof(request.CustomVariables)); Ensure.NotNullOrEmpty(key, nameof(key)); + request.CustomVariables ??= new Dictionary<string, string>(); request.CustomVariables[key] = value; return request; }
633-641: Optional: clear conflicting fields when TemplateId is set (per remarks)Docs state Subject, TextBody, HtmlBody, and Category are forbidden when TemplateId is used. Builder can enforce this for users.
public static T Template<T>(this T request, string templateId) where T : EmailRequest { Ensure.NotNull(request, nameof(request)); Ensure.NotNullOrEmpty(templateId, nameof(templateId)); request.TemplateId = templateId; + // Clear fields that must be null when using templates + request.Subject = null; + request.TextBody = null; + request.HtmlBody = null; + request.Category = null; return request; }src/Mailtrap.Abstractions/Emails/Models/AttachmentValidator.cs (1)
4-22: Add XML documentation to match validator conventions.The validator implementation looks good, but we lost the XML summary that accompanies every validator in this codebase. Please add class-level (and
Instanceproperty) XML docs so the generated API documentation stays consistent. Based on learningssrc/Mailtrap/Emails/EmailClientEndpointProvider.cs (1)
13-22: Refresh the XML docs to match the method behavior.This comment block still describes a constructor and misrepresents the parameter semantics. Please rewrite it so the summary and
<param>tags explain the URI-building logic (batch vs send, bulk host selection, inbox scoping).- /// <summary> - /// Initializes a new instance of the <see cref="EmailClientEndpointProvider"/> class. - /// </summary> - /// <param name="isBatch">if <c>true</c> the batch segment will be used; otherwise, the regular send segment will be used.</param> - /// <param name="isBulk">if <c>true</c> the bulk email endpoint will be used; - /// if <c>true</c> and <paramref name="inboxId"/> is provided, the test email endpoint will be used; - /// otherwise, the single email endpoint will be used.</param> - /// <param name="inboxId">If inbox identifier provided, the request will be scoped to that inbox.</param> - /// <returns></returns> + /// <summary> + /// Builds the request <see cref="Uri"/> for send or batch operations. + /// </summary> + /// <param name="isBatch">When <c>true</c>, append the batch segment; otherwise append the send segment.</param> + /// <param name="isBulk">When <c>true</c>, use the bulk host; otherwise use the standard send host.</param> + /// <param name="inboxId">When supplied, scope the request to the inbox and switch to the test host.</param> + /// <returns>The fully qualified endpoint URI.</returns>examples/Mailtrap.Example.Email.BatchSend/Program.cs (2)
96-99: Remove FailFast in example; prefer clean exit/logging
Environment.FailFastterminates the process abruptly and is unsuitable for sample apps. Log and rethrow or set exit code.- Environment.FailFast(ex.Message); - throw; + // Consider returning non-zero exit code instead of fail-fast in examples + Environment.ExitCode = -1; + throw;
291-304: Guard file I/O to avoid crashes when example files are missing
File.ReadAllByteson hardcoded path can throw. Mirror the existence check used earlier and skip when missing.- var filePath = @"C:\files\preview.pdf"; - var fileName = "preview.pdf"; - var bytes = File.ReadAllBytes(filePath); - var fileContent = Convert.ToBase64String(bytes); - var attachment = new Attachment( - content: fileContent, - fileName: fileName, - disposition: DispositionType.Attachment, - mimeType: MediaTypeNames.Application.Pdf, - contentId: "attachment_1"); - - request.Attachments.Add(attachment); + var filePath = @"C:\files\preview.pdf"; + if (File.Exists(filePath)) + { + var fileName = "preview.pdf"; + var bytes = File.ReadAllBytes(filePath); + var fileContent = Convert.ToBase64String(bytes); + var attachment = new Attachment( + content: fileContent, + fileName: fileName, + disposition: DispositionType.Attachment, + mimeType: MediaTypeNames.Application.Pdf, + contentId: "attachment_1"); + + request.Attachments.Add(attachment); + }src/Mailtrap.Abstractions/Emails/Extensions/BatchEmailRequestExtensions.cs (2)
15-18: Return empty sequence instead of null to simplify consumersAvoid returning null; return an empty enumerable when
Requestsis null. Optionally materialize to avoid deferred enumeration surprises.- internal static IEnumerable<SendEmailRequest>? GetMergedRequests(this BatchEmailRequest batchRequest) + internal static IEnumerable<SendEmailRequest> GetMergedRequests(this BatchEmailRequest batchRequest) { - return batchRequest.Requests?.Select(request => MergeWithBase(request, batchRequest.Base)); + return (batchRequest.Requests ?? Enumerable.Empty<SendEmailRequest>()) + .Select(request => MergeWithBase(request, batchRequest.Base)); }
31-48: Clarify “empty string means inherit” and shallow-copy of collections
- Using
string.IsNullOrEmptytreats empty as “unset,” inheriting from base. If empty should override base with empty, switch torequest.Subject is null ? baseRequest.Subject : request.Subject(same for Text/Html/Category/TemplateId) orIsNullOrWhiteSpaceif whitespace should inherit. Please confirm intended behavior.Attachments/Headers/CustomVariables/TemplateVariablesare shallow-copied references. Merged request may alias base collections, causing accidental cross-mutation. Consider cloning when inheriting.Example adjustment:
- Subject = string.IsNullOrEmpty(request.Subject) ? baseRequest.Subject : request.Subject, + Subject = request.Subject is null ? baseRequest.Subject : request.Subject, ... - Attachments = request.Attachments ?? baseRequest.Attachments, + Attachments = request.Attachments ?? (baseRequest.Attachments is null ? null : new List<Attachment>(baseRequest.Attachments)), - Headers = request.Headers ?? baseRequest.Headers, + Headers = request.Headers ?? (baseRequest.Headers is null ? null : new Dictionary<string,string>(baseRequest.Headers)), - CustomVariables = request.CustomVariables ?? baseRequest.CustomVariables, + CustomVariables = request.CustomVariables ?? (baseRequest.CustomVariables is null ? null : new Dictionary<string,string>(baseRequest.CustomVariables)), - TemplateVariables = request.TemplateVariables ?? baseRequest.TemplateVariables + TemplateVariables = request.TemplateVariables ?? (baseRequest.TemplateVariables is null ? null : new Dictionary<string,string>(baseRequest.TemplateVariables))tests/Mailtrap.UnitTests/Emails/Requests/BatchEmailRequestTests.cs (1)
56-59: Confirm validator messaging vs spec (“at least one of Text/Html”)The test expects both "'Text Body' must not be empty." and "'Html Body' must not be empty." when both are empty. If the rule is “at least one must be specified,” consider a single aggregate message or conditional messages to avoid implying both are required.
tests/Mailtrap.UnitTests/Emails/Requests/EmailRequestBuilderTests.HtmlBody.cs (1)
46-46: Use value equality for strings in assertions
BeSameAschecks reference identity and can be brittle with string interning. PreferBe(_html)/Be(otherHtml).- request.HtmlBody.Should().BeSameAs(_html); + request.HtmlBody.Should().Be(_html); ... - request.HtmlBody.Should().BeSameAs(otherHtml); + request.HtmlBody.Should().Be(otherHtml);Also applies to: 59-59
tests/Mailtrap.UnitTests/Emails/Requests/EmailRequestBuilderTests.From.cs (1)
63-66: Remove unused local variableThe local request is unused in this test. Drop it to avoid warnings.
- var request = EmailRequest.Create(); - var act = () => EmailRequestBuilder.From<EmailRequest>(null!, SenderEmail);src/Mailtrap.Abstractions/Emails/Validators/EmailRequestValidator.cs (3)
65-78: Use default messages for simple Null rules to match validator styleCustom messages on Null rules are unnecessary per repo convention. Keep custom text only for complex rules.
When(r => !string.IsNullOrEmpty(r.TemplateId), () => { RuleFor(r => r.Subject) - .Null() - .WithMessage($"'{nameof(EmailRequest.Subject)}' should be null, when '{nameof(EmailRequest.TemplateId)}' is specified."); + .Null(); RuleFor(r => r.TextBody) - .Null() - .WithMessage($"'{nameof(EmailRequest.TextBody)}' should be null, when '{nameof(EmailRequest.TemplateId)}' is specified."); + .Null(); RuleFor(r => r.HtmlBody) - .Null() - .WithMessage($"'{nameof(EmailRequest.HtmlBody)}' should be null, when '{nameof(EmailRequest.TemplateId)}' is specified."); + .Null(); });Based on learnings
45-63: Avoid duplicate errors for “either TextBody or HtmlBody” by using a single ruleCurrent dual rules can emit two errors when both are empty/null. Consolidate into one Must with a single message; rely on default messages for Subject.
- When(r => string.IsNullOrEmpty(r.TemplateId), () => - { - RuleFor(r => r.Subject) - .NotNull() - .MinimumLength(1) - .When(r => !isBase); - - RuleFor(r => r.TextBody) - .NotNull() - .MinimumLength(1) - .When(r => string.IsNullOrEmpty(r.HtmlBody) && !isBase) - .WithMessage($"Either '{nameof(EmailRequest.TextBody)}' or '{nameof(EmailRequest.HtmlBody)}' or both should be set to non-empty string, when template is not specified."); - - RuleFor(r => r.HtmlBody) - .NotNull() - .MinimumLength(1) - .When(r => string.IsNullOrEmpty(r.TextBody) && !isBase) - .WithMessage($"Either '{nameof(EmailRequest.TextBody)}' or '{nameof(EmailRequest.HtmlBody)}' or both should be set to non-empty string, when template is not specified."); - }); + When(r => string.IsNullOrEmpty(r.TemplateId) && !isBase, () => + { + RuleFor(r => r.Subject).NotEmpty(); + + RuleFor(r => r) + .Must(r => !string.IsNullOrEmpty(r.TextBody) || !string.IsNullOrEmpty(r.HtmlBody)) + .WithMessage($"Either '{nameof(EmailRequest.TextBody)}' or '{nameof(EmailRequest.HtmlBody)}' or both should be set to non-empty string, when template is not specified."); + });Based on learnings
19-23: Fix XML doc wording for claritySmall grammar tweak.
- /// If <paramref name="isBase"/> is true, the validator is configured for Base request validation. - /// This means that the no properties are required and perform basic validation. + /// If <paramref name="isBase"/> is true, the validator is configured for Base request validation. + /// This means no properties are required; only basic validations are performed.src/Mailtrap.Abstractions/Emails/Requests/BatchEmailRequest.cs (5)
10-12: XML doc typo“Gets or sets and object” → “Gets or sets an object.” Minor fix.
- /// Gets or sets and object with general properties of all emails in the batch.<br /> + /// Gets or sets an object with general properties of all emails in the batch.<br />
17-20: Make Base optional to avoid serializing empty objects and unnecessary validationDefaulting Base to new() causes “base”: {} to be serialized and always validated. Prefer nullable Base and omit when null.
- [JsonPropertyName("base")] - [JsonPropertyOrder(1)] - public EmailRequest Base { get; set; } = new(); + [JsonPropertyName("base")] + [JsonPropertyOrder(1)] + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + public EmailRequest? Base { get; set; }Note: Merge/validator code already accepts EmailRequest? in MergeWithBase; builders continue to set Base explicitly.
35-37: Preserve list instance on deserializationAlign with Errors in responses and avoid replacement on deserialize by populating the existing list.
[JsonPropertyName("requests")] [JsonPropertyOrder(2)] - public IList<SendEmailRequest> Requests { get; set; } = []; + [JsonObjectCreationHandling(JsonObjectCreationHandling.Populate)] + public IList<SendEmailRequest> Requests { get; set; } = [];
31-34: 50 MB remark not enforced in validatorDocs mention 50 MB total payload; current validator enforces only count<=500. Decide if the client should pre-validate size or leave to server. I can add a conservative preflight check (sum of base+items attachments), if desired.
49-54: Validator message style consistencyBatchEmailRequestValidator uses custom WithMessage for simple rules; project style favors default FluentValidation messages for such cases.
Based on learnings
src/Mailtrap.Abstractions/Emails/Responses/BatchSendEmailResponse.cs (2)
9-12: XML doc grammar“Gets errors, associated with the response.” → “Gets the errors associated with the response.”
- /// Gets errors, associated with the response. + /// Gets the errors associated with the response.
22-31: Prefer direct assignment for MessageIds (consistent with SendEmailResponse)Simplify CreateSuccess/Failure overload and match base pattern.
internal static new BatchSendEmailResponse CreateSuccess(params string[] messageIds) { - var response = new BatchSendEmailResponse - { - Success = true, - }; - messageIds.ToList().ForEach(response.MessageIds.Add); - - return response; + return new BatchSendEmailResponse + { + Success = true, + MessageIds = messageIds + }; } @@ internal static BatchSendEmailResponse CreateFailure(string[] messageIds, string[] errors) { - var response = new BatchSendEmailResponse - { - Success = false, - Errors = errors - }; - messageIds.ToList().ForEach(response.MessageIds.Add); - return response; + return new BatchSendEmailResponse + { + Success = false, + Errors = errors, + MessageIds = messageIds + }; }Also applies to: 42-51
tests/Mailtrap.UnitTests/Emails/EmailClientFactoryTests.cs (1)
76-76: Assert interfaces, not concrete implementationsReduce coupling to implementation details by asserting assignability to ISendEmailClient/IBatchEmailClient.
- result.Should().BeOfType<SendEmailClient>(); + result.Should().BeAssignableTo<ISendEmailClient>(); @@ - result.Should().BeOfType<SendEmailClient>(); + result.Should().BeAssignableTo<ISendEmailClient>(); @@ - result.Should().BeOfType<SendEmailClient>(); + result.Should().BeAssignableTo<ISendEmailClient>(); @@ - result.Should().BeOfType<SendEmailClient>(); + result.Should().BeAssignableTo<ISendEmailClient>(); @@ - result.Should().BeOfType<SendEmailClient>(); + result.Should().BeAssignableTo<ISendEmailClient>(); @@ - result.Should().BeOfType<BatchEmailClient>(); + result.Should().BeAssignableTo<IBatchEmailClient>(); @@ - result.Should().BeOfType<BatchEmailClient>(); + result.Should().BeAssignableTo<IBatchEmailClient>(); @@ - result.Should().BeOfType<BatchEmailClient>(); + result.Should().BeAssignableTo<IBatchEmailClient>(); @@ - result.Should().BeOfType<BatchEmailClient>(); + result.Should().BeAssignableTo<IBatchEmailClient>(); @@ - result.Should().BeOfType<BatchEmailClient>(); + result.Should().BeAssignableTo<IBatchEmailClient>();Optional: add Verify on endpoint provider mocks to ensure correct URI selection per case.
Also applies to: 102-102, 128-128, 154-154, 180-181, 207-207, 233-233, 259-259, 285-285, 311-311
tests/Mailtrap.UnitTests/Emails/Models/AttachmentValidatorTests.cs (1)
272-275: Use valid emails in length-only tests to avoid unrelated failuresGenerate syntactically valid addresses when testing only collection size.
- for (var i = 1; i <= 1001; i++) + for (var i = 1; i <= 1001; i++) { - internalRequest.Cc($"recipient{i}.domain.com"); + internalRequest.Cc($"recipient{i}@domain.com"); } @@ - for (var i = 1; i <= 1000; i++) + for (var i = 1; i <= 1000; i++) { - internalRequest.Cc($"recipient{i}.domain.com"); + internalRequest.Cc($"recipient{i}@domain.com"); } @@ - for (var i = 1; i <= 1000; i++) + for (var i = 1; i <= 1000; i++) { - internalRequest.Bcc($"recipient{i}.domain.com"); + internalRequest.Bcc($"recipient{i}@domain.com"); }Also applies to: 289-292, 350-353
tests/Mailtrap.UnitTests/Emails/Requests/BatchEmailRequestTests.Validator.Requests.cs (1)
90-99: Add a robustness test for null items in RequestsEnsure a null element in Requests doesn’t throw and yields a validation error at the correct path.
+ [Test] + public void Validation_Requests_Should_Fail_WhenItemIsNull() + { + var request = BatchEmailRequest.Create(); + request.Requests.Add(null!); + + var result = BatchEmailRequestValidator.Instance.TestValidate(request); + + result.ShouldHaveValidationErrorFor($"{nameof(BatchEmailRequest.Requests)}[0]"); + }tests/Mailtrap.UnitTests/Emails/Requests/EmailRequestBuilderTests.Attachment.cs (1)
78-89: Consider making the helper more flexible.The
Attach_CreateAndValidatehelper method acceptsparams Attachment[] attachmentsbut hardcodes an expectation of exactly 2 attachments at line 85. While this works for the current usage, it reduces reusability.Consider updating the assertion to be more flexible:
-private static EmailRequest Attach_CreateAndValidate(params Attachment[] attachments) +private EmailRequest Attach_CreateAndValidate(params Attachment[] attachments) { var request = EmailRequest .Create() .Attach(attachments); request.Attachments.Should() - .HaveCount(2).And + .HaveCount(attachments.Length).And .ContainInOrder(attachments); return request; }tests/Mailtrap.UnitTests/Emails/Requests/EmailRequestTests.Validator.cs (1)
7-9: Consider using readonly fields for test data.The test data fields are never reassigned, so they could be declared as readonly fields instead of properties for clarity and slight performance improvement.
-private string _validEmail { get; } = "[email protected]"; -private string _invalidEmail { get; } = "someone"; -private string _templateId { get; } = "ID"; +private readonly string _validEmail = "[email protected]"; +private readonly string _invalidEmail = "someone"; +private readonly string _templateId = "ID";src/Mailtrap.Abstractions/Emails/Requests/EmailRequest.cs (1)
96-99: Ensure Attachments is a mutable collection
= []may materialize as an array depending on language version/target and can be fixed-size at runtime. Use a List to guarantee mutability for builder extensions and validators.- public IList<Attachment> Attachments { get; set; } = []; + public IList<Attachment> Attachments { get; set; } = new List<Attachment>();tests/Mailtrap.UnitTests/Emails/Requests/EmailRequestBuilderTests.Category.cs (1)
45-46: Prefer value equality for string assertionsUse
Beinstead ofBeSameAsto avoid relying on string reference identity.- request.Category.Should().BeSameAs(_category); + request.Category.Should().Be(_category); ... - request.Category.Should().BeSameAs(otherCategory); + request.Category.Should().Be(otherCategory);Also applies to: 58-59
tests/Mailtrap.UnitTests/Emails/Requests/EmailRequestBuilderTests.Subject.cs (1)
45-46: Prefer value equality for string assertionsUse
Beto assert string content rather than reference identity.- request.Subject.Should().BeSameAs(_subject); + request.Subject.Should().Be(_subject); ... - request.Subject.Should().BeSameAs(otherSubject); + request.Subject.Should().Be(otherSubject);Also applies to: 58-59
src/Mailtrap.Abstractions/Emails/Responses/SendEmailResponse.cs (2)
29-33: Guarantee a mutable MessageIds collection
= []can yield a fixed-size collection. Prefer an explicitList<string>to ensure consistent mutability and serializer population semantics.- [JsonObjectCreationHandling(JsonObjectCreationHandling.Populate)] - public IList<string> MessageIds { get; private set; } = []; + [JsonObjectCreationHandling(JsonObjectCreationHandling.Populate)] + public IList<string> MessageIds { get; private set; } = new List<string>();
35-42: Avoid assigning arrays to MessageIds; populate the list insteadAssigning
string[]toIList<string>can produce a fixed-size list; subsequent additions would throw. Populate the existing list (mirrors BatchSendEmailResponse.CreateSuccess).-internal static SendEmailResponse CreateSuccess(params string[] messageIds) -{ - return new SendEmailResponse - { - Success = true, - MessageIds = messageIds - }; -} +internal static SendEmailResponse CreateSuccess(params string[] messageIds) +{ + var response = new SendEmailResponse + { + Success = true, + }; + if (messageIds is { Length: > 0 }) + { + foreach (var id in messageIds) + { + response.MessageIds.Add(id); + } + } + return response; +}src/Mailtrap/MailtrapClient.cs (1)
12-13: Eager init of default clients; consider lazy for startup perfCreating defaults in ctor is fine. Optionally defer creation until first use to reduce startup cost.
Also applies to: 29-31
tests/Mailtrap.UnitTests/Emails/Requests/EmailRequestBuilderTests.ReplyTo.cs (1)
61-66: Remove unused local variableThe local variable ‘request’ is unused in this test. Safe to remove.
tests/Mailtrap.UnitTests/Emails/Requests/EmailRequestBuilderTests.CustomVariable.cs (1)
64-67: Avoid relying on dictionary enumeration order in assertionsUsing ContainInOrder on a dictionary can be brittle. Prefer key/value presence and counts (ContainKeys, BeEquivalentTo) without ordering.
Also applies to: 78-81, 90-93
src/Mailtrap.Abstractions/Emails/Validators/SendEmailRequestValidator.cs (1)
4-8: Clarify XML doc to mention composed validations (optional).
Add that this validator also applies EmailRequestValidator rules (From/ReplyTo/attachments/body/template), not only recipients.tests/Mailtrap.UnitTests/Emails/Requests/BatchEmailRequestTests.Validator.Base.cs (1)
28-35: Tests are comprehensive and aligned with base-mode validation.
Optional: use EmailRequest.Create() consistently when passing to Base(...) for readability (SendEmailRequest works via inheritance).src/Mailtrap.Abstractions/Emails/Validators/BatchEmailRequestValidator.cs (1)
20-26: Prefer default FluentValidation messages for simple rules (consistency).
Remove custom WithMessage for NotEmpty and LessThanOrEqualTo to align with validator conventions.Based on learnings
- RuleFor(r => r.Requests) - .NotEmpty() - .WithMessage("'Requests' must not be empty."); + RuleFor(r => r.Requests) + .NotEmpty(); - RuleFor(r => r.Requests.Count) - .LessThanOrEqualTo(500) - .WithMessage("'Requests' shouldn't exceed 500 records.") - .When(r => r.Requests is not null); + RuleFor(r => r.Requests.Count) + .LessThanOrEqualTo(500) + .When(r => r.Requests is not null);
tests/Mailtrap.UnitTests/Emails/Requests/SendEmailRequestTests.Validator.cs
Show resolved
Hide resolved
tests/Mailtrap.UnitTests/Emails/Requests/SendEmailRequestTests.Validator.cs
Show resolved
Hide resolved
dr-3lo
changed the title
- documentation and enhance fluent builder methods
- Update error handling in email sending to set exit code instead of failing fast. - Improve attachment MIME type detection based on file extension. - Add null check for SendEmailRequest in MergeWithBase method. - Update documentation for BatchEmailRequest and EmailRequest to clarify properties. - Refactor tests to ensure proper validation and serialization of email requests.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/cookbook/send-email.md (1)
300-303: Typo: use OperationCanceledException (single L).Update the catch clause.
Apply this diff:
-catch (OperationCancelledException ocex) +catch (OperationCanceledException ocex) { // handle cancellation }
♻️ Duplicate comments (2)
src/Mailtrap.Abstractions/Emails/Requests/EmailRequest.cs (1)
161-166: Doc/validation mismatch: Category with TemplateIdDocs say Category must be null when TemplateId is set, but the validator allows Category (only enforces length). Update docs to reflect behavior.
- /// If provided, then <see cref="Subject"/>, <see cref="Category"/>, <see cref="TextBody"/> and <see cref="HtmlBody"/> - /// properties are forbidden and must be <see langword="null"/>.<br /> - /// Email subject, text and html will be generated from template using optional <see cref="TemplateVariables"/>. + /// If provided, then <see cref="Subject"/>, <see cref="TextBody"/> and <see cref="HtmlBody"/> + /// are forbidden and must be <see langword="null"/>.<br /> + /// Email subject, text and html will be generated from the template using optional <see cref="TemplateVariables"/>.tests/Mailtrap.UnitTests/Emails/Requests/SendEmailRequestTests.Validator.cs (1)
204-207: Thank you for fixing the test email formats in CC/BCC loopsThe updated addresses now include '@', so positive-path tests won’t fail on format. LGTM.
Also applies to: 219-222, 268-271, 283-286
🧹 Nitpick comments (15)
tests/Mailtrap.UnitTests/Emails/Requests/EmailRequestBuilderTests.ReplyTo.cs (1)
133-133: Consider a more descriptive test name.The
_2suffix differentiates this test from the one at line 43, but the distinction could be clearer. Consider renaming to something likeReplyTo_Should_OverrideReplyTo_WhenCalledSeveralTimesWithStringOverloadto explicitly indicate it tests the string parameter overload.tests/Mailtrap.UnitTests/Emails/Requests/BatchEmailRequestTests.Validator.Requests.cs (2)
39-39: Simplify test parameter.The
[Values]attribute with a single value is unnecessary here. Consider using[Test]without parameters or inline the value directly in the test body for clarity.Apply this diff:
- public void Validation_Should_Fail_WhenRequestsCountIsGreaterThan500([Values(501)] int count) + public void Validation_Should_Fail_WhenRequestsCountIsGreaterThan500() { + const int count = 501; var request = BatchEmailRequest.Create()
221-221: Simplify test parameter.The
[Values]attribute with a single value is unnecessary. Consider removing the parameter and using a const value directly.Apply this diff:
- public void Validation_Requests_Should_Fail_WhenToLengthExceedsLimit([Values(1001)] int count) + public void Validation_Requests_Should_Fail_WhenToLengthExceedsLimit() { + const int count = 1001; var request = BatchEmailRequest.Create()src/Mailtrap.Abstractions/README.md (1)
11-12: Call out the breaking change and add a 1–2 line migration note.Since IEmailClient was replaced by ISendEmailClient and IBatchEmailClient, add a short “Breaking changes” note or a parenthetical in Main Types to guide users (e.g., “IEmailClient → ISendEmailClient/IBatchEmailClient; see docs for migration”). This reduces upgrade friction.
docs/cookbook/batch-send-email.md (2)
294-309: Make the link text descriptive (MD059).Apply this diff:
-## See also -More examples available [here](https://github.com/railsware/mailtrap-dotnet/tree/main/examples). +## See also +More examples: [Mailtrap .NET examples on GitHub](https://github.com/railsware/mailtrap-dotnet/tree/main/examples).
80-98: Optional: add missing import in snippets using MediaTypeNames/DispositionType.Readers may need using System.Net.Mime; to compile examples as-is.
Consider adding:
using System.Net.Mime;Also applies to: 160-174
docs/cookbook/send-email.md (1)
339-340: Make the link text descriptive (MD059).Apply this diff:
-## See also -More examples available [here](https://github.com/railsware/mailtrap-dotnet/tree/main/examples). +## See also +More examples: [Mailtrap .NET examples on GitHub](https://github.com/railsware/mailtrap-dotnet/tree/main/examples).src/Mailtrap/Emails/EmailClientEndpointProvider.cs (1)
20-20: Avoid boolean-flag API; consider enum or dedicated methods.Two booleans are easy to misorder/misuse. Prefer:
- GetSendRequestUri(isBulk, inboxId) and GetBatchRequestUri(isBulk, inboxId), or
- GetRequestUri(EmailOperation.Send|Batch, isBulk, inboxId).
Improves readability and call-site safety.
tests/Mailtrap.UnitTests/Emails/EmailClientFactoryTests.cs (2)
186-209: Solid coverage for batch factory paths. Minor nits: add Verify, rename var.
- Add Moq Verify to assert correct args were used.
- Rename sendUri → batchUri in batch tests for clarity.
Example for one test:
- var sendUri = new Uri("https://localhost/api/batch"); + var batchUri = new Uri("https://localhost/api/batch"); ... - .Returns(sendUri); + .Returns(batchUri); ... - result.ResourceUri.Should().Be(sendUri); + result.ResourceUri.Should().Be(batchUri); + emailClientEndpointProviderMock.Verify( + x => x.GetRequestUri(true, isBulk, inboxId), + Times.Once);Also applies to: 212-235, 238-261, 264-287, 290-313
55-55: Use positive inboxId values in test data.[Random(2)] for long can yield 0/negative, which isn’t a valid inbox id. Prefer explicit positives:
-[Test] -public void Create_ShouldReturnEmailClient([Values] bool isBulk, [Random(2)] long inboxId) +[Test] +public void Create_ShouldReturnEmailClient([Values] bool isBulk, [Values(1L, 42L)] long inboxId)Apply similarly to other tests.
src/Mailtrap/Emails/Requests/BatchEmailRequestBuilder.cs (2)
41-41: EnsureAddRangeextension is in scope and accessibleThese calls rely on
CollectionExtensions.AddRange<T>:
- Import its namespace explicitly, or the extension won’t resolve without a global using.
- If
AddRangeisinternalin another assembly, ensureInternalsVisibleToor make itpublic; otherwise this won’t compile.Recommend adding an explicit import to avoid relying on globals:
+using Mailtrap.Core.Extensions; namespace Mailtrap.Emails.Requests;Also applies to: 76-76
28-35: Fix XML docs: remove non-standardidattribute on<exception>C# XML docs expect
cref(notid). Dropid="ArgumentNullException"to avoid analyzer/doc tooling warnings.tests/Mailtrap.UnitTests/Emails/Requests/EmailRequestBuilderTests.Attachment.cs (1)
100-105: Remove unused local to reduce noise
requestis declared but unused in this test. Safe to remove.- var request = EmailRequest.Create(); - var act = () => EmailRequestBuilder.Attach<EmailRequest>(null!, Content, FileName);tests/Mailtrap.UnitTests/Emails/Requests/SendEmailRequestTests.cs (1)
41-66: Strengthen JSON shape assertions (add from.name, keep fixture fidelity)Consider also asserting the sender display name to fully cover the "from" shape:
- Check from.name == "John Doe".
This keeps the test aligned with the server’s expected payload shape. Based on learnings.
- root.GetProperty("from").GetProperty("email").GetString().Should().Be("[email protected]"); + var from = root.GetProperty("from"); + from.GetProperty("email").GetString().Should().Be("[email protected]"); + from.GetProperty("name").GetString().Should().Be("John Doe");tests/Mailtrap.UnitTests/Emails/Requests/SendEmailRequestTests.Validator.cs (1)
26-35: Target the ‘Recipients’ rule explicitly for clarityThese tests intend to validate the recipients rule only. Asserting on the root object (r => r) can pass even when other property-level errors exist (From/Subject/Body). Prefer asserting the specific path to make intent explicit and resilient.
- result.ShouldNotHaveValidationErrorFor(r => r); + result.ShouldNotHaveValidationErrorFor("Recipients");Apply similarly to “OnlyCcRecipientsPresent” and “OnlyBccRecipientsPresent”.
Also applies to: 38-47, 50-59
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
docs/cookbook/batch-send-email.md(1 hunks)docs/cookbook/send-email.md(1 hunks)examples/Mailtrap.Example.Email.BatchSend/Program.cs(1 hunks)src/Mailtrap.Abstractions/Emails/Extensions/BatchEmailRequestExtensions.cs(1 hunks)src/Mailtrap.Abstractions/Emails/Requests/BatchEmailRequest.cs(1 hunks)src/Mailtrap.Abstractions/Emails/Requests/EmailRequest.cs(1 hunks)src/Mailtrap.Abstractions/Emails/Responses/BatchSendEmailResponse.cs(1 hunks)src/Mailtrap.Abstractions/Emails/Validators/SendEmailRequestValidator.cs(1 hunks)src/Mailtrap.Abstractions/README.md(1 hunks)src/Mailtrap/Emails/EmailClientEndpointProvider.cs(1 hunks)src/Mailtrap/Emails/Requests/BatchEmailRequestBuilder.cs(1 hunks)tests/Mailtrap.UnitTests/Emails/EmailClientFactoryTests.cs(10 hunks)tests/Mailtrap.UnitTests/Emails/Requests/BatchEmailRequestTests.Validator.Base.cs(1 hunks)tests/Mailtrap.UnitTests/Emails/Requests/BatchEmailRequestTests.Validator.Requests.cs(1 hunks)tests/Mailtrap.UnitTests/Emails/Requests/EmailRequestBuilderTests.Attachment.cs(6 hunks)tests/Mailtrap.UnitTests/Emails/Requests/EmailRequestBuilderTests.From.cs(3 hunks)tests/Mailtrap.UnitTests/Emails/Requests/EmailRequestBuilderTests.ReplyTo.cs(3 hunks)tests/Mailtrap.UnitTests/Emails/Requests/SendEmailRequestTests.Validator.cs(35 hunks)tests/Mailtrap.UnitTests/Emails/Requests/SendEmailRequestTests.cs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Mailtrap.Abstractions/Emails/Requests/BatchEmailRequest.cs
- tests/Mailtrap.UnitTests/Emails/Requests/BatchEmailRequestTests.Validator.Base.cs
- src/Mailtrap.Abstractions/Emails/Responses/BatchSendEmailResponse.cs
- src/Mailtrap.Abstractions/Emails/Extensions/BatchEmailRequestExtensions.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-04T12:23:59.276Z
Learnt from: dr-3lo
PR: railsware/mailtrap-dotnet#139
File: tests/Mailtrap.IntegrationTests/Contacts/Update_Success.json:16-17
Timestamp: 2025-09-04T12:23:59.276Z
Learning: Test fixtures in the Mailtrap .NET client should accurately represent the actual server response format and should not be modified to match client-side converters or serialization preferences.
Applied to files:
tests/Mailtrap.UnitTests/Emails/Requests/SendEmailRequestTests.cs
🧬 Code graph analysis (12)
tests/Mailtrap.UnitTests/Emails/Requests/BatchEmailRequestTests.Validator.Requests.cs (6)
src/Mailtrap.Abstractions/Emails/Requests/BatchEmailRequest.cs (1)
BatchEmailRequest(47-47)src/Mailtrap/Emails/Requests/BatchEmailRequestBuilder.cs (6)
BatchEmailRequest(35-44)BatchEmailRequest(70-79)BatchEmailRequest(105-114)BatchEmailRequest(141-151)BatchEmailRequest(177-186)BatchEmailRequest(208-216)src/Mailtrap.Abstractions/Emails/Validators/BatchEmailRequestValidator.cs (2)
BatchEmailRequestValidator(9-34)BatchEmailRequestValidator(13-33)src/Mailtrap.Abstractions/Emails/Extensions/BatchEmailRequestExtensions.cs (1)
SendEmailRequest(31-54)src/Mailtrap.Abstractions/Emails/Requests/EmailRequest.cs (1)
EmailRequest(194-194)src/Mailtrap.Abstractions/Emails/Models/EmailAddress.cs (1)
EmailAddress(62-68)
src/Mailtrap/Emails/EmailClientEndpointProvider.cs (4)
src/Mailtrap/Emails/IEmailClientEndpointProvider.cs (1)
Uri(9-9)src/Mailtrap/Core/Extensions/UriExtensions.cs (7)
Uri(12-19)Uri(24-31)Uri(36-43)Uri(48-60)Uri(66-72)Uri(79-92)Uri(101-109)src/Mailtrap/Core/Constants/Endpoints.cs (1)
Endpoints(7-32)src/Mailtrap/Core/Constants/UrlSegments.cs (1)
UrlSegments(4-11)
src/Mailtrap.Abstractions/Emails/Validators/SendEmailRequestValidator.cs (2)
src/Mailtrap.Abstractions/Emails/Validators/EmailRequestValidator.cs (2)
EmailRequestValidator(12-80)EmailRequestValidator(23-79)src/Mailtrap.Abstractions/Emails/Validators/SendEmailRecipientsValidator.cs (2)
SendEmailRecipientsValidator(10-42)SendEmailRecipientsValidator(14-41)
tests/Mailtrap.UnitTests/Emails/EmailClientFactoryTests.cs (3)
src/Mailtrap/Emails/EmailClientFactory.cs (12)
ISendEmailClient(29-34)ISendEmailClient(36-36)ISendEmailClient(38-38)ISendEmailClient(40-40)ISendEmailClient(42-42)EmailClientFactory(7-59)EmailClientFactory(14-26)IBatchEmailClient(45-50)IBatchEmailClient(52-52)IBatchEmailClient(54-54)IBatchEmailClient(56-56)IBatchEmailClient(58-58)src/Mailtrap/Emails/EmailClientEndpointProvider.cs (1)
Uri(20-33)src/Mailtrap/Emails/IEmailClientEndpointProvider.cs (1)
Uri(9-9)
tests/Mailtrap.UnitTests/Emails/Requests/EmailRequestBuilderTests.From.cs (3)
src/Mailtrap/Emails/Requests/EmailRequestBuilder.cs (1)
EmailRequestBuilder(7-679)src/Mailtrap.Abstractions/Emails/Requests/EmailRequest.cs (1)
EmailRequest(194-194)src/Mailtrap.Abstractions/Emails/Models/EmailAddress.cs (1)
EmailAddress(62-68)
src/Mailtrap/Emails/Requests/BatchEmailRequestBuilder.cs (4)
examples/Mailtrap.Example.Email.BatchSend/Program.cs (8)
BatchEmailRequest(106-136)BatchEmailRequest(142-166)SendEmailRequest(171-191)SendEmailRequest(197-206)SendEmailRequest(212-224)SendEmailRequest(229-267)SendEmailRequest(272-328)SendEmailRequest(333-407)src/Mailtrap.Abstractions/Emails/Requests/BatchEmailRequest.cs (1)
BatchEmailRequest(47-47)src/Mailtrap.Abstractions/Core/Extensions/Ensure.cs (2)
Ensure(9-106)NotNull(18-33)src/Mailtrap.Abstractions/Emails/Requests/EmailRequest.cs (1)
EmailRequest(194-194)
examples/Mailtrap.Example.Email.BatchSend/Program.cs (8)
src/Mailtrap.Abstractions/Emails/IEmailClient.cs (1)
Task(67-67)src/Mailtrap.Abstractions/Emails/Requests/BatchEmailRequest.cs (2)
BatchEmailRequest(47-47)ValidationResult(50-55)src/Mailtrap/Emails/Requests/BatchEmailRequestBuilder.cs (6)
BatchEmailRequest(35-44)BatchEmailRequest(70-79)BatchEmailRequest(105-114)BatchEmailRequest(141-151)BatchEmailRequest(177-186)BatchEmailRequest(208-216)src/Mailtrap.Abstractions/Emails/Responses/BatchEmailResponse.cs (2)
BatchEmailResponse(51-58)BatchEmailResponse(60-67)src/Mailtrap.Abstractions/Emails/Responses/BatchSendEmailResponse.cs (3)
BatchSendEmailResponse(22-31)BatchSendEmailResponse(33-40)BatchSendEmailResponse(42-51)src/Mailtrap.Abstractions/Emails/Models/EmailAddress.cs (1)
EmailAddress(62-68)src/Mailtrap.Abstractions/Emails/Extensions/BatchEmailRequestExtensions.cs (1)
SendEmailRequest(31-54)src/Mailtrap.Abstractions/Emails/Models/Attachment.cs (1)
Attachment(159-174)
tests/Mailtrap.UnitTests/Emails/Requests/EmailRequestBuilderTests.ReplyTo.cs (3)
src/Mailtrap/Emails/Requests/EmailRequestBuilder.cs (1)
EmailRequestBuilder(7-679)src/Mailtrap.Abstractions/Emails/Requests/EmailRequest.cs (1)
EmailRequest(194-194)src/Mailtrap.Abstractions/Emails/Models/EmailAddress.cs (1)
EmailAddress(62-68)
src/Mailtrap.Abstractions/Emails/Requests/EmailRequest.cs (3)
src/Mailtrap.Abstractions/Emails/Models/EmailAddress.cs (1)
EmailAddress(62-68)src/Mailtrap.Abstractions/Emails/Models/Attachment.cs (1)
Attachment(159-174)src/Mailtrap.Abstractions/Emails/Validators/EmailRequestValidator.cs (2)
EmailRequestValidator(12-80)EmailRequestValidator(23-79)
tests/Mailtrap.UnitTests/Emails/Requests/SendEmailRequestTests.Validator.cs (3)
tests/Mailtrap.UnitTests/Emails/Requests/SendEmailRequestTests.cs (1)
SendEmailRequest(144-152)src/Mailtrap.Abstractions/Emails/Models/EmailAddress.cs (1)
EmailAddress(62-68)src/Mailtrap.Abstractions/Emails/Validators/SendEmailRequestValidator.cs (2)
SendEmailRequestValidator(10-22)SendEmailRequestValidator(14-21)
tests/Mailtrap.UnitTests/Emails/Requests/SendEmailRequestTests.cs (4)
src/Mailtrap/Configuration/MailtrapJsonSerializerOptions.cs (1)
MailtrapJsonSerializerOptions(4-19)src/Mailtrap.Abstractions/Emails/Requests/SendEmailRequest.cs (1)
SendEmailRequest(71-71)src/Mailtrap/Emails/Requests/SendEmailRequestBuilder.cs (2)
SendEmailRequest(35-44)SendEmailRequest(80-85)src/Mailtrap.Abstractions/Emails/Models/EmailAddress.cs (1)
EmailAddress(62-68)
tests/Mailtrap.UnitTests/Emails/Requests/EmailRequestBuilderTests.Attachment.cs (3)
src/Mailtrap/Emails/Requests/EmailRequestBuilder.cs (1)
EmailRequestBuilder(7-679)src/Mailtrap.Abstractions/Emails/Requests/EmailRequest.cs (1)
EmailRequest(194-194)src/Mailtrap.Abstractions/Emails/Models/Attachment.cs (1)
Attachment(159-174)
🪛 markdownlint-cli2 (0.18.1)
docs/cookbook/batch-send-email.md
309-309: Link text should be descriptive
(MD059, descriptive-link-text)
🔇 Additional comments (19)
tests/Mailtrap.UnitTests/Emails/Requests/EmailRequestBuilderTests.ReplyTo.cs (1)
1-149: LGTM! Well-structured test refactoring with comprehensive coverage.The test file has been correctly refactored from
SendEmailRequestBuildertoEmailRequestBuilder, with consistent updates to:
- Test fixture and class naming
- Test method naming (using the
Should_XXX_Whenpattern with underscores)- API usage (
EmailRequest.Create()andEmailRequestBuilder.ReplyTo<EmailRequest>())The test coverage is thorough, validating null checks, empty strings, initialization, and override behavior for both
ReplyTo(EmailAddress)andReplyTo(string, string?)overloads.docs/cookbook/batch-send-email.md (2)
176-179: Minor grammar polish for clarity.
[ suggest_optional_refactor ]
Apply this diff:-After creating a batch request, validate it on a client side to ensure it meets API requirements and sending won't throw validation exceptions it also will minimize unnecessary HTTP round-trips. +After creating a batch request, validate it on the client side to ensure it meets API requirements. This helps prevent validation exceptions and minimizes unnecessary HTTP round-trips.
69-72: Docs limit aligns with code BatchEmailRequestValidator enforces a maximum of 500 requests, matching the documentation.docs/cookbook/send-email.md (1)
317-319: LGTM: interface rename to ISendEmailClient.The examples align with the updated public API.
Also applies to: 326-331
src/Mailtrap/Emails/EmailClientEndpointProvider.cs (2)
10-10: Batch segment and URI assembly look correct.Choosing segment via isBatch and appending api/ is clear and consistent with existing Append().
Also applies to: 28-31
22-26: Confirm host selection when inboxId is set.Current logic routes any inbox-scoped request to TestDefaultUrl, ignoring isBulk. If intentional, add/keep a unit test asserting this to prevent regressions.
Also applies to: 32-33
tests/Mailtrap.UnitTests/Emails/EmailClientFactoryTests.cs (2)
62-63: Mocks updated to new endpoint signature — looks good.Setups correctly pass isBatch=false for send paths.
Also applies to: 88-89, 114-115, 140-141, 166-167
76-76: Assert against ISendEmailClient — good.Interface-based assertions keep tests resilient to concrete type changes.
Also applies to: 102-102, 128-128, 154-154, 180-181
tests/Mailtrap.UnitTests/Emails/Requests/EmailRequestBuilderTests.From.cs (1)
14-40: From builder tests look solidGood coverage for nulls, initialization, and override semantics. No issues.
src/Mailtrap.Abstractions/Emails/Validators/SendEmailRequestValidator.cs (1)
16-21: Validator composition LGTMApplying
EmailRequestValidatorthenSendEmailRecipientsValidatoris clean and correct.tests/Mailtrap.UnitTests/Emails/Requests/EmailRequestBuilderTests.Attachment.cs (1)
171-207: Attachment builder tests are thoroughCovers nulls, optional params, and property initialization. Looks good.
tests/Mailtrap.UnitTests/Emails/Requests/SendEmailRequestTests.cs (3)
18-27: Serialization round‑trip test looks goodDeserialization-based equivalence is more robust than string compare. LGTM.
100-108: Valid request happy‑path test looks correctCovers From/To/Subject/Text. LGTM.
110-122: Recipient presence validation (positive path) is soundMinimal valid recipients + required fields. LGTM.
tests/Mailtrap.UnitTests/Emails/Requests/SendEmailRequestTests.Validator.cs (5)
141-149: Boundary tests for To length are appropriate1001 fails; 1/500/1000 pass. Clear coverage. LGTM.
Also applies to: 152-160
163-176: Per-item email format checks are preciseIndex-based assertions on collection items are accurate and readable. LGTM.
Also applies to: 179-191
328-338: Attachment validation coverage is solidCovers invalid MimeType and all-valid cases with precise paths. LGTM.
Also applies to: 341-353
362-372: TemplateId mutual exclusivity rules are well specifiedSubject/Text/Html forbidden with TemplateId; happy path passes. LGTM.
Also applies to: 375-385, 388-398, 401-412
478-486: Body rules (null/empty/one/both) comprehensively coveredThorough and unambiguous assertions. LGTM.
Also applies to: 489-500, 503-513, 516-526, 529-540
tests/Mailtrap.UnitTests/Emails/Requests/BatchEmailRequestTests.Validator.Requests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/Mailtrap.UnitTests/Emails/Requests/BatchEmailRequestTests.Validator.Requests.cs (1)
283-286: Past issue resolved.The missing '@' symbol in Cc test email addresses has been correctly fixed. Both tests now use the valid format
recipient{i}@domain.com.Also applies to: 300-302
🧹 Nitpick comments (2)
tests/Mailtrap.UnitTests/Emails/Requests/BatchEmailRequestTests.Validator.Requests.cs (2)
816-846: Consider renamingCreateValidBaseRequestfor clarity.The helper method returns an empty
EmailRequestbut the name suggests it would contain valid base properties. Consider renaming toCreateEmptyBaseRequestor populating it with base properties if that better reflects test intent.Alternatively, if the empty base is intentional for testing minimal scenarios, the current implementation is acceptable.
500-500: Consider using consistent assertion style.Line 500 uses
result.IsValid.Should().BeTrue()while other tests (e.g., line 71) useresult.ShouldNotHaveAnyValidationErrors(). Consider standardizing on one approach for consistency.Both styles are correct; this is purely for code consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/cookbook/batch-send-email.md(1 hunks)docs/cookbook/send-email.md(2 hunks)src/Mailtrap.Abstractions/Emails/Requests/EmailRequest.cs(1 hunks)src/Mailtrap/Emails/Requests/BatchEmailRequestBuilder.cs(1 hunks)tests/Mailtrap.UnitTests/Emails/Requests/BatchEmailRequestTests.Validator.Requests.cs(1 hunks)tests/Mailtrap.UnitTests/Emails/Requests/EmailRequestBuilderTests.Attachment.cs(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-13T14:27:42.453Z
Learnt from: dr-3lo
PR: railsware/mailtrap-dotnet#158
File: tests/Mailtrap.UnitTests/Emails/Requests/SendEmailRequestTests.cs:86-97
Timestamp: 2025-10-13T14:27:42.453Z
Learning: In the Mailtrap .NET client's SendEmailRequest validation, when no recipients are provided (To, Cc, and Bcc are all empty), the validator returns a specific error message: "There should be at least one email recipient added to either To, Cc or Bcc." This message uses lowercase "recipient" rather than a capitalized property name.
Applied to files:
tests/Mailtrap.UnitTests/Emails/Requests/BatchEmailRequestTests.Validator.Requests.cs
🧬 Code graph analysis (4)
tests/Mailtrap.UnitTests/Emails/Requests/BatchEmailRequestTests.Validator.Requests.cs (4)
src/Mailtrap/Emails/Requests/BatchEmailRequestBuilder.cs (6)
BatchEmailRequest(35-44)BatchEmailRequest(70-79)BatchEmailRequest(105-114)BatchEmailRequest(141-151)BatchEmailRequest(177-186)BatchEmailRequest(208-216)src/Mailtrap.Abstractions/Emails/Requests/BatchEmailRequest.cs (1)
BatchEmailRequest(47-47)src/Mailtrap.Abstractions/Emails/Validators/BatchEmailRequestValidator.cs (2)
BatchEmailRequestValidator(9-34)BatchEmailRequestValidator(13-33)src/Mailtrap.Abstractions/Emails/Extensions/BatchEmailRequestExtensions.cs (1)
SendEmailRequest(31-54)
src/Mailtrap.Abstractions/Emails/Requests/EmailRequest.cs (3)
src/Mailtrap.Abstractions/Emails/Models/EmailAddress.cs (1)
EmailAddress(62-68)src/Mailtrap.Abstractions/Emails/Models/Attachment.cs (1)
Attachment(159-174)src/Mailtrap.Abstractions/Emails/Validators/EmailRequestValidator.cs (2)
EmailRequestValidator(12-80)EmailRequestValidator(23-79)
src/Mailtrap/Emails/Requests/BatchEmailRequestBuilder.cs (4)
src/Mailtrap.Abstractions/Emails/Requests/BatchEmailRequest.cs (1)
BatchEmailRequest(47-47)examples/Mailtrap.Example.Email.BatchSend/Program.cs (2)
BatchEmailRequest(106-136)BatchEmailRequest(142-166)src/Mailtrap.Abstractions/Core/Extensions/Ensure.cs (2)
Ensure(9-106)NotNull(18-33)src/Mailtrap.Abstractions/Emails/Requests/EmailRequest.cs (1)
EmailRequest(194-194)
tests/Mailtrap.UnitTests/Emails/Requests/EmailRequestBuilderTests.Attachment.cs (3)
src/Mailtrap/Emails/Requests/EmailRequestBuilder.cs (1)
EmailRequestBuilder(7-679)src/Mailtrap.Abstractions/Emails/Requests/EmailRequest.cs (1)
EmailRequest(194-194)src/Mailtrap.Abstractions/Emails/Models/Attachment.cs (1)
Attachment(159-174)
🔇 Additional comments (21)
docs/cookbook/send-email.md (2)
300-303: Correct exception type spellingRenaming to
OperationCanceledExceptionmatches the actual .NET type; looks good.
317-331: Updated interface references read wellSwitching to
ISendEmailClientkeeps the samples aligned with the new client API.tests/Mailtrap.UnitTests/Emails/Requests/EmailRequestBuilderTests.Attachment.cs (1)
1-208: LGTM! Well-executed refactoring with comprehensive test coverage.The transition from
SendEmailRequesttoEmailRequestand the corresponding builder update is done correctly throughout this test file. The tests comprehensively cover:
- Null validation for request and parameters (lines 17-22, 25-32, 98-143)
- Empty parameter handling (lines 35-42, 66-75)
- Single and multiple attachment addition (lines 45-48, 51-63)
- Both
Attachoverloads with all parameter combinations (lines 146-205)- Correct use of explicit generic type parameters when calling static methods with null arguments (lines 19, 100)
The consistent test naming convention (
Attach_Should_...) improves readability, and the helper methodAttach_CreateAndValidatecorrectly returnsEmailRequestinstead of the old type.tests/Mailtrap.UnitTests/Emails/Requests/BatchEmailRequestTests.Validator.Requests.cs (10)
1-10: LGTM!Test setup with clear, reusable constants for valid/invalid test data.
11-85: LGTM!Comprehensive coverage of Requests collection validation including null, empty, and count limit scenarios. Proper use of parameterized tests for boundary conditions.
88-145: LGTM!Thorough validation of individual request items including null checks and recipient requirements. Tests correctly verify that at least one of To, Cc, or Bcc must be present.
149-214: LGTM!Proper validation of From and ReplyTo fields with correct email format checking and property path assertions.
218-404: LGTM!Comprehensive validation of recipient collections (To, Cc, Bcc) with proper count limits (1000), format validation, and boundary testing. All email formats are correct.
408-440: LGTM!Proper validation of attachment collections with correct property path assertions for indexed items.
444-523: LGTM!Critical validation logic ensuring templates and content fields (Subject, Text, Html) are mutually exclusive. Tests thoroughly cover both per-item and base template scenarios.
527-601: LGTM!Thorough Subject validation covering required field checks, base request merging, and template conflicts. Good use of parameterized tests for null/empty scenarios.
605-665: LGTM!Proper Category validation with length limits (255) and base request merging. Good use of random string generation for boundary testing.
669-814: LGTM!Exceptional coverage of Body validation with all combinations of Text/Html presence, base request merging, and template conflicts. Tests ensure at least one body format is always required.
src/Mailtrap.Abstractions/Emails/Requests/EmailRequest.cs (1)
1-203: LGTM overall with documentation clarification needed.The EmailRequest record is well-structured with:
- Proper JSON serialization attributes and property ordering
- Comprehensive XML documentation for all properties
- Nullable reference types used appropriately
- Collection properties initialized with default empty instances
- Factory method and validation implementation
The only concern is the Category property documentation inconsistency noted above.
src/Mailtrap/Emails/Requests/BatchEmailRequestBuilder.cs (7)
35-44: LGTM! Proper null checks and fluent chaining.The params overload correctly validates all arguments and uses AddRange for efficient bulk addition.
70-79: LGTM! IEnumerable overload complements params version.Good use of
inheritdocto avoid documentation duplication while maintaining the same validation and chaining pattern.
105-114: LGTM! Single request overload for convenience.Clean implementation for adding individual requests with consistent null checking.
141-151: LGTM! Inline configuration is a nice fluent API touch.The Action overload enables clean inline request configuration, creating a more expressive builder syntax as shown in the example code.
177-186: LGTM! Base configuration follows the same fluent pattern.Consistent with the Requests overloads, allowing inline Base request configuration.
208-216: LGTM! Direct Base assignment completes the API.Provides flexibility to set a pre-configured EmailRequest as the Base.
1-219: Excellent fluent builder implementation!This builder class demonstrates solid design principles:
- Comprehensive method overloads for different usage patterns
- Consistent null checking using Ensure.NotNull
- Proper fluent chaining with return values
- Well-organized with regions
- Thorough XML documentation with effective use of inheritdoc
- Aligns perfectly with the example usage shown in Program.cs
The multiple overloads (params, IEnumerable, single item, Action configuration) provide excellent flexibility for consumers.
| /// <remarks> | ||
| /// Should be less or equal to 255 characters. | ||
| /// </remarks> | ||
| [JsonPropertyName("category")] | ||
| [JsonPropertyOrder(80)] | ||
| public string? Category { get; set; } |
There was a problem hiding this comment.
🧩 Analysis chain
Follow up: Category documentation still incomplete.
The past review comment flagged that Category should be null when TemplateId is set (per API docs), but the validator doesn't enforce this. You responded that you would "update XML doc," but the documentation at lines 128-130 still only mentions the length constraint and doesn't clarify the relationship with TemplateId.
Please update the XML documentation to explicitly state whether Category must be null when TemplateId is set, or confirm that the API allows Category with templates and update the original concern.
Consider adding to the remarks section:
/// <remarks>
-/// Should be less or equal to 255 characters.
+/// Must be null when <see cref="TemplateId"/> is set.<br />
+/// Otherwise, should be less than or equal to 255 characters.
/// </remarks>If Category is indeed forbidden with templates, the validator should also enforce this rule (see past review comment for suggested implementation).
🏁 Script executed:
#!/bin/bash
# Locate Validate method in EmailRequest.cs
rg -n "public\s\+void\s\+Validate" -n src/Mailtrap.Abstractions/Emails/Requests/EmailRequest.cs
# Show surrounding lines in Validate to check for Category and TemplateId logic
rg -nP "Validate\(\)\s*\{" -C5 src/Mailtrap.Abstractions/Emails/Requests/EmailRequest.cs | sed -n '1,200p'
# Specifically search for Category and TemplateId references in Validate
rg -nP "Category|TemplateId" -C5 src/Mailtrap.Abstractions/Emails/Requests/EmailRequest.csLength of output: 2718
🏁 Script executed:
#!/bin/bash
# Locate Validate method signature and body
rg -nP "public\s+void\s+Validate\s*\(\)" -n src/Mailtrap.Abstractions/Emails/Requests/EmailRequest.cs
rg -nP "public\s+void\s+Validate\s*\(\)" -C200 src/Mailtrap.Abstractions/Emails/Requests/EmailRequest.csLength of output: 0
🏁 Script executed:
#!/bin/bash
# Find the exact path of EmailRequest.cs
fd EmailRequest.cs
# Search for any "validate" occurrences in the located file
rg -i "validate" -n src/Mailtrap.Abstractions/Emails/Requests/EmailRequest.cs -C10Length of output: 707
🏁 Script executed:
#!/bin/bash
# Find the validator source file
fd EmailRequestValidator.cs
# Display its content around the class definition
rg -n "class EmailRequestValidator" -C5 $(fd EmailRequestValidator.cs)
# Search for rules regarding Category
rg -nP "Category" -C3 $(fd EmailRequestValidator.cs)
# Search for rules regarding TemplateId
rg -nP "TemplateId" -C3 $(fd EmailRequestValidator.cs)Length of output: 6191
Enforce Category-with-TemplateId constraint
- EmailRequest.cs (lines 128–130): update
<remarks>to/// <remarks> /// Must be null when <see cref="TemplateId"/> is set;<br/> /// otherwise ≤255 characters. /// </remarks>
- EmailRequestValidator.cs (after line 65): inside
When(r => !string.IsNullOrEmpty(r.TemplateId), () => { … })
addRuleFor(r => r.Category) .Null() .WithMessage($"'{nameof(EmailRequest.Category)}' should be null when '{nameof(EmailRequest.TemplateId)}' is specified.");
🤖 Prompt for AI Agents
In src/Mailtrap.Abstractions/Emails/Requests/EmailRequest.cs around lines
128–133, update the XML remarks for Category to state it "Must be null when
TemplateId is set; otherwise ≤255 characters." so the docs reflect the
TemplateId constraint; and in
src/Mailtrap.Abstractions/Emails/Requests/EmailRequestValidator.cs after line
65, inside the When(r => !string.IsNullOrEmpty(r.TemplateId), () => { … }) block
add a RuleFor for Category that enforces Null and supplies the message
"'EmailRequest.Category' should be null when 'EmailRequest.TemplateId' is
specified." to validate the mutual exclusivity.
2 participants
Motivation
Batch Send API support into .NET client #95
Changes
How to test
Summary by CodeRabbit
New Features
Breaking Changes
Improvements
Chores