Bugfix: useSystemNetMailSettings=false still uses .config settings + feature: PickupDirectoryLocation from nlog.config #939
Conversation
Change require parameter check.
Override "pickupdirectorylocation settings and expose it as a parameter.
|
@dnlgmzddr looks good. Can we unit test this? The documentation is just editing the wiki. |
|
@304NotModified, I'm not sure how to unit test, this feature. However the test cases, are quite simple (all involving useSystemNetMailSettings=false):
Never the less, I don't know how to test the first scenario, and the other one is already being partially tested in the existing tests. |
|
The unit test should test "useSystemNetMailSettings=false", so if "useSystemNetMailSettings=false", then I should not use the values of the web.config. Just check the values of the settings.
Those are not needed, as they test somethings else.
Unit test should be simple :) |
304NotModified
changed the title
|
Need some help @dnlgmzddr ? |
|
@304NotModified Finally, I do have to adjust the mockups to test the the new scenarios. Let me now if you have any more comments. |
| { | ||
| throw new NLogRuntimeException(string.Format(RequiredPropertyIsEmptyFormat, "SmtpServer")); | ||
|
|
There was a problem hiding this comment.
note: it's allowed to have one of the two as null, smtp uses Host, pickupdir doesn't use Host
304NotModified
changed the title
304NotModified
changed the title
|
Looks very good! Nice work! Just two code comments, please check them, I think they are important. Can you also document |
* Fix validation logic. * Add new Property to ISmtpClient, DeliveryMethod
|
@304NotModified I just upload some changes, and documented Have a nice Sunday. |
| client.Host = renderedSmtpServer; | ||
|
|
||
| // The network delivery method take precedence if both Host and PickupDirectoryLocation are present. | ||
| client.DeliveryMethod = SmtpDeliveryMethod.Network; |
There was a problem hiding this comment.
@304NotModified should we also expose the DeliveryMethod as part of the MailTarget properties, or can we assume Network when there a Host and SpecifiedPickupDirectory where there not a Host but PickUpDirectoryValue? (should we also consider PickupDirectoryFromIis?)
regarding the other comments, will these kind of check be more appropriate ?
There was a problem hiding this comment.
I think we prefer exposing DeliveryMethod, because it's unclear when both are filled. So good idea :)
* Improve validation. * Add support for DelivereyMethod.PickupDirectoryFromIis
Current coverage is
|
Bugfix: useSystemNetMailSettings=false still uses .config settings + feature: PickupDirectoryLocation from nlog.config
|
Nice work! Thanks! :) |
|
@dnlgmzddr can you update the wiki? Just add the two new properties, and add "introduced in NLog 4.2" https://github.com/NLog/NLog/wiki/Mail-target
|
|
@304NotModified The wiki have been updated. |
|
Thanks! |
Address bug/690, the PickUpDirectory location is exposed in the Mail Target.
However given that the test are using a mock-up for the SMTP class it does not make a lot of sense to create a test for this bug.
@304NotModified the documentation of this property is still pending. I will check the documentation project.