SMTPS wire and unit test for SMTPS protocol #50
Conversation
|
@amihaiemil Many thanks, I will find someone to review it before we merge |
|
@mkordas it's yours, please review |
| .putAll(new Protocol.SMTP(this.host, this.port).entries()) | ||
| .put("mail.smtps.auth", Boolean.TRUE.toString()) | ||
| .put("mail.smtps.host", this.host) | ||
| .put("mail.smtps.port", Integer.toString(this.port)) |
There was a problem hiding this comment.
@mkordas Properties from Protocol.SMTP were not ok since the smtps transport used in the SMTPS wire looks up the properties with ".smtps." in the middle, not ".smtp.". If it does not find them, the defaults are used (localhost and port 465) and so the unit test passed locally, but failed on the linux server since you need root priviledge to access ports < 1024 .
There was a problem hiding this comment.
@amihaiemil please address these comments to me now
|
@amihaiemil we meet again, I'll try to do intial review today :) |
|
@amihaiemil I'm on it |
| @@ -42,11 +42,13 @@ | |||
| * <pre> Postman postman = new Postman.Default( | |||
| * new SMTP( | |||
| * new Token("user", "password").access( | |||
| * new Protocol.SMTPS("smtp.gmail.com", 587) | |||
| * new Protocol.SMTP("bind", "port") | |||
There was a problem hiding this comment.
@amihaiemil what's the reason for this change?
There was a problem hiding this comment.
@mkordas Gmail does not work with SMTP protocol through SMTP wire, so the Javadoc was somewhat misleading. It's usually the first thing anyone tries, so you don't want your first example to not work.
There was a problem hiding this comment.
@amihaiemil but isn't it better to provide real SMTPS gmail address instead of bind?
There was a problem hiding this comment.
@mkordas I don't know what you mean - that is the only pair of bind/port google provides for sending email, and as I said above, it does not work with SMTP protocol through SMTP wire. If I leave it there, it's not a good example since it does not work. People will start looking into their code or simply assume there's a bug in our lib.
This is a javadoc for SMTP, not for SMTPS.
And I don't have other examples of bind/port pairs... but it's a pretty common thing I wouldn't worry. If someone doesn't know what bind and port are, then they for sure have never heard of SMTP and sending email libs.
There was a problem hiding this comment.
@amihaiemil thanks for explanation, my bad - now I understand
|
@amihaiemil see my review above |
|
@mkordas done |
|
@amihaiemil I'm on it :) |
|
@amihaiemil very nice, just 2 open comments I think |
|
@mkordas I added a higher timeout for the Greenmail server startup (one of the previous builds failed and it said in the logs that the timeout should be raised) |
|
@amihaiemil sure, I'll cheek it soon |
|
@amihaiemil I'm on it |
|
@amihaiemil really nice! |
|
@rultor merge |
|
@yegor256 ping |
|
@yegor256 ping again |
|
@rultor try to merge |
|
@rultor deploy pls |
PR for #46
Also added SMTPS wire, since Protocol.SMTPS does not work with
smtptransport, it needssmtpstransport.Updated javadocs too since they were not ok.
Fixed bug in Protocol.SMTPS which was setting some properties for smtp protocol, not for smtps.
More details:
1) This SMTPS Wire is needed since the Greenmail unit test does not work with the SMTP wire .
This issue was not found before since apparently Gmail server works fine if called with the SMTP wire and SMTPS protocol.
I am not sure why Gmail worked with the SMTP wire, but it works with this SMTPS wire as well, so I would leave this as it is now, since it makes sense the most: If you send an email through SMTPS protocol, a smtps transport should be used.
2) Properties from Protocol.SMTP were not ok since the smtps transport used in the SMTPS wire looks up the properties with ".smtps." in the middle, not ".smtp.". If it does not find them, the defaults are used (localhost and port 465) and so the unit test passed locally, but failed on the linux server since you need root priviledge to access ports < 1024 .