Add ability to set Sender for mail #375

johnwc · 2024-02-27T19:08:07Z

Consolidated target frameworks for libraries and referenced local projects over public packages.

Added a Sender(...) method to be able to set the Sender property of the MailMessage. #374
Have From() override default global From. #321

Reference projects over packages.

johnwc · 2024-02-27T19:16:37Z

@jamesmh can you please review

johnwc · 2024-03-04T18:32:39Z

@jamesmh don't mean to be a bother, but we are needing this for one of our up coming releases.

jamesmh

Thanks for the changes. I've left some feedback on some code changes that we'll have to revert so that the package doesn't break, some extra tests, and then some thinking on how this breaks a public API.

Feel free to make the code changes and throw some thoughts down around the API versioning concerns. Not sure what to do ATM but open to suggestions.

Thanks 🙂

jamesmh · 2024-03-11T17:19:46Z

Src/Coravel.Cache.Database/Coravel.Cache.Database.Core/Coravel.Cache.Database.Core.csproj

+  <ItemGroup>
+    <ProjectReference Include="..\..\Coravel\Coravel.csproj" />
+  </ItemGroup>
+


Because Coravel.Cache.Database.Core is a nuget package, we can't reference another project this way. It's unfortunate, but nuget doesn't support bringing in project references into the final published binary.

So this would actually break at either publish or runtime since the Coravel package would be missing.

We'd have to revert this back to the PackageReference.

We reference our packages like this, and all of our packages show that they have dependencies to the library that is Project Referenced. I don't think I have ever not seen that.

Looking at my local nupkg of this library, so does yours. Am I not understanding what you are referring to?

<?xml version="1.0" encoding="utf-8"?> <package xmlns="http://schemas.microsoft.com/packaging/2013/05/nuspec.xsd"> <metadata> <id>Coravel.Cache.Database.Core</id> <version>2.0.2</version> <title>Coravel Database Cache Driver Shared Library</title> <authors>James Hickey</authors> <license type="expression">MIT</license> <licenseUrl>https://licenses.nuget.org/MIT</licenseUrl> <projectUrl>https://github.com/jamesmh/coravel</projectUrl> <description>Core tools for Coravel database cache drivers</description> <tags>netcore coravel caching database</tags> <repository type="git" url="https://github.com/jamesmh/coravel" commit="0ec0e2d0ed24f99c5ab4b24a8fd575114b4f3b7b" /> <dependencies> <group targetFramework=".NETStandard2.1"> <dependency id="Coravel" version="5.0.2" exclude="Build,Analyzers" /> <dependency id="Dapper" version="2.1.28" exclude="Build,Analyzers" /> <dependency id="Newtonsoft.Json" version="12.0.2" exclude="Build,Analyzers" /> </group> </dependencies> </metadata> </package>

jamesmh · 2024-03-11T17:20:11Z

Src/Coravel.Cache.Database/Coravel.Cache.Database.Core/Coravel.Cache.Database.Core.csproj

@@ -19,9 +19,12 @@
  </PropertyGroup>

  <ItemGroup>
-    <PackageReference Include="Coravel" Version="3.0.0" />
-    <PackageReference Include="Dapper" Version="1.60.6" />
+    <PackageReference Include="Dapper" Version="2.1.28" />


What's the reasoning behind changing the version here?

Why not, it is a local build depending on the and not a runtime dependency. It is not going to have dependency issue for the consumer. Would we not want to use the latest and greatest updates from a package like Dapper, so to utilize performance and security updates?

jamesmh · 2024-03-11T17:22:13Z

Src/Coravel.Cache.Database/Coravel.Cache.PostgreSQL/Coravel.Cache.PostgreSQL.csproj

+  </ItemGroup>
+
+  <ItemGroup>
+    <ProjectReference Include="..\Coravel.Cache.Database.Core\Coravel.Cache.Database.Core.csproj" />


Again, you can't use project references in nuget like this. It will break. We'll have to revert this change.

Again, need clarification on what you are referring to, as my understanding is that it works OoB as correct dependency in built package.

jamesmh · 2024-03-11T17:23:23Z

Src/Coravel.Cache.Database/Coravel.Cache.SqlServer/Coravel.Cache.SqlServer.csproj

@@ -22,4 +18,12 @@
    <NoWarn>$(NoWarn);1591</NoWarn>
  </PropertyGroup>

+  <ItemGroup>
+    <PackageReference Include="System.Data.SqlClient" Version="4.8.6" />


I don't think we need this new package? I don't see a change to warrant this.

You need a reference to this specific to use the version that does not have security vulnerabilities. It is already a dependency of the package, just need to push it to latest to avoid security issues.

jamesmh · 2024-03-11T17:24:01Z

Src/Coravel.Cache.Database/Coravel.Cache.SqlServer/Coravel.Cache.SqlServer.csproj

+  </ItemGroup>
+
+  <ItemGroup>
+    <ProjectReference Include="..\Coravel.Cache.Database.Core\Coravel.Cache.Database.Core.csproj" />


We can't use project references, we have to stick to package references.

Same response as above.

jamesmh · 2024-03-11T17:31:49Z

Src/Coravel/Coravel.csproj

@@ -1,6 +1,6 @@
 <Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
-    <TargetFramework>.net6.0</TargetFramework>
+    <TargetFramework>netstandard2.1</TargetFramework>


I haven't run this, but pretty sure that this would break when running tests or at runtime. We need to target .net6 due to features that exist in .net6 and not in .netstandard2.1.

The 2 needed code style changes below were made to be able to target the netstandard.

jamesmh · 2024-03-11T17:33:31Z

Src/Coravel/Scheduling/Schedule/EnsureContinuousSecondTicks.cs

-        previousTick = nextTick;
-    }
-}
+namespace Coravel.Scheduling.Schedule


Could we undo this file's styling changes? Don't think it's necessary and I think would be best in a dedicated "styling fix" PR vs. combining logic changes, styling changes, bug fix, etc. in one PR 👍

This was causing it to need to point to .net6, with this style revert, it now can target .netstandard

jamesmh · 2024-03-11T17:34:08Z

Src/Coravel/Scheduling/Schedule/Event/ScheduledEvent.cs

@@ -108,7 +108,7 @@ public async Task InvokeScheduledEvent(CancellationToken cancellationToken)
            }
            else
            {
-                await using AsyncServiceScope scope = new(this._scopeFactory.CreateAsyncScope());
+                await using AsyncServiceScope scope = new AsyncServiceScope(this._scopeFactory.CreateAsyncScope());


Not sure why this was formatted this way? Could we revert this one please 🙂.

This was causing it to need to point to .net6, with this style revert, it now can target .netstandard

jamesmh · 2024-03-11T17:36:16Z

Src/UnitTests/MailerUnitTests/Mail/CustomMailerTests.cs

@@ -92,7 +92,7 @@ async Task SendMailCustom(string message, string subject, IEnumerable<MailRecipi
        [Fact]
        public async Task CustomMailerHasAttachments()
        {
-            async Task SendMailCustom(string message, string subject, IEnumerable<MailRecipient> to, MailRecipient from, MailRecipient replyTo, IEnumerable<MailRecipient> cc, IEnumerable<MailRecipient> bcc, IEnumerable<Attachment> attachments)
+            async Task SendMailCustom(string message, string subject, IEnumerable<MailRecipient> to, MailRecipient from, MailRecipient replyTo, MailRecipient sender, IEnumerable<MailRecipient> cc, IEnumerable<MailRecipient> bcc, IEnumerable<Attachment> attachments)


Just like this test, could we add a new dedicated test to ensure that the sender was dealt with successfully?

I will see what I can do to add a test for sender based on this current test.

jamesmh · 2024-03-11T17:38:51Z

Src/UnitTests/MailerUnitTests/Mail/CustomMailerTests.cs

@@ -15,7 +15,7 @@ public class CustomMailerTests
        [Fact]
        public async Task CustomMailerSucessful()
        {
-            async Task SendMailCustom(string message, string subject, IEnumerable<MailRecipient> to, MailRecipient from, MailRecipient replyTo, IEnumerable<MailRecipient> cc, IEnumerable<MailRecipient> bcc, IEnumerable<Attachment> attachments)
+            async Task SendMailCustom(string message, string subject, IEnumerable<MailRecipient> to, MailRecipient from, MailRecipient replyTo, MailRecipient sender, IEnumerable<MailRecipient> cc, IEnumerable<MailRecipient> bcc, IEnumerable<Attachment> attachments)


Could we also add some tests against the subject in GeneralMailTests and SmtpMailerTests?

I didn't think I changed the subject, is this just an ad hoc request to add this while I am doing the other test? 🙂

johnwc added 2 commits February 27, 2024 13:04

Consolidate target frameworks for libraries.

4884742

Reference projects over packages.

Add Sender setting

f6df4f4

Have From() override default global From

2f91329

jamesmh requested changes Mar 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to set Sender for mail #375

Add ability to set Sender for mail #375

johnwc commented Feb 27, 2024 •

edited

johnwc commented Feb 27, 2024

johnwc commented Mar 4, 2024

jamesmh left a comment

jamesmh Mar 11, 2024

johnwc Mar 11, 2024

jamesmh Mar 11, 2024

johnwc Mar 11, 2024

jamesmh Mar 11, 2024

johnwc Mar 11, 2024

jamesmh Mar 11, 2024

johnwc Mar 11, 2024

jamesmh Mar 11, 2024

johnwc Mar 11, 2024

jamesmh Mar 11, 2024

johnwc Mar 11, 2024

jamesmh Mar 11, 2024

johnwc Mar 11, 2024

jamesmh Mar 11, 2024

johnwc Mar 11, 2024

jamesmh Mar 11, 2024

johnwc Mar 11, 2024

jamesmh Mar 11, 2024

johnwc Mar 11, 2024 •

edited

Add ability to set Sender for mail #375

Are you sure you want to change the base?

Add ability to set Sender for mail #375

Conversation

johnwc commented Feb 27, 2024 • edited

johnwc commented Feb 27, 2024

johnwc commented Mar 4, 2024

jamesmh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnwc Mar 11, 2024 • edited

Choose a reason for hiding this comment

johnwc commented Feb 27, 2024 •

edited

johnwc Mar 11, 2024 •

edited