-
-
Notifications
You must be signed in to change notification settings - Fork 243
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
Add ability to set Sender for mail #375
base: master
Are you sure you want to change the base?
Conversation
Reference projects over packages.
@jamesmh can you please review |
@jamesmh don't mean to be a bother, but we are needing this for one of our up coming releases. |
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.
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 🙂
<ItemGroup> | ||
<ProjectReference Include="..\..\Coravel\Coravel.csproj" /> | ||
</ItemGroup> | ||
|
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.
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
.
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.
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>
@@ -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" /> |
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.
What's the reasoning behind changing the version here?
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.
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?
</ItemGroup> | ||
|
||
<ItemGroup> | ||
<ProjectReference Include="..\Coravel.Cache.Database.Core\Coravel.Cache.Database.Core.csproj" /> |
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.
Again, you can't use project references in nuget like this. It will break. We'll have to revert this change.
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.
Again, need clarification on what you are referring to, as my understanding is that it works OoB as correct dependency in built package.
@@ -22,4 +18,12 @@ | |||
<NoWarn>$(NoWarn);1591</NoWarn> | |||
</PropertyGroup> | |||
|
|||
<ItemGroup> | |||
<PackageReference Include="System.Data.SqlClient" Version="4.8.6" /> |
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.
I don't think we need this new package? I don't see a change to warrant this.
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.
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.
</ItemGroup> | ||
|
||
<ItemGroup> | ||
<ProjectReference Include="..\Coravel.Cache.Database.Core\Coravel.Cache.Database.Core.csproj" /> |
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.
We can't use project references, we have to stick to package references.
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.
Same response as above.
@@ -1,6 +1,6 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> | |||
<PropertyGroup> | |||
<TargetFramework>.net6.0</TargetFramework> | |||
<TargetFramework>netstandard2.1</TargetFramework> |
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.
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
.
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.
The 2 needed code style changes below were made to be able to target the netstandard.
previousTick = nextTick; | ||
} | ||
} | ||
namespace Coravel.Scheduling.Schedule |
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.
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 👍
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.
This was causing it to need to point to .net6, with this style revert, it now can target .netstandard
@@ -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()); |
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.
Not sure why this was formatted this way? Could we revert this one please 🙂.
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.
This was causing it to need to point to .net6, with this style revert, it now can target .netstandard
@@ -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) |
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.
Just like this test, could we add a new dedicated test to ensure that the sender
was dealt with successfully?
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.
I will see what I can do to add a test for sender based on this current test.
@@ -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) |
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.
Could we also add some tests against the subject
in GeneralMailTests
and SmtpMailerTests
?
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.
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? 🙂
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. #374Have From() override default global From. #321