Email 2.2 #11554
Email 2.2 #11554
Conversation
|
@xet7 what do you think? |
|
Isn't it a good a time to remove the fibers stuff, convert to async and do a major bump? |
|
@harryadel Let's leave that for the next version. I think these changes are enough for now. |
|
This feature is awesome. I was wondering if it also consider to add more than one transport in order to contemplate this feature request. |
I'll try some way way to test this. I tried:
Maybe I could first build current Wekan bundle, and then replace email.js etc in that bundle, and run that bundle. I'll try that next. |
|
@xet7 I would recommend just copying it into the local packages directory and then you can adjust the version as needed to make it work. |
I could not get it working in Then I did some changes, to port it Node 12.x and Meteor 2.2 that Wekan currently used. Here is changed email.js in .zip file: email.zip Here are changes I made: But now that I try to invite new user to Wekan board with user's email address, that would send email for invite, I get this error: So I think some of the changes have not been ported correctly think. Because I could not get this email package working yet with , I think next I'll try: a) Figure out minimal changes to current Meteor 2.2 email package that would still work with Node 12.x, Meteor 2.2 and current Wekan. b) Creating new example project with newest Meteor and this email package, to test does sending email work with newest Meteor. |
|
Or this might be related to the test failures that we see in CI. |
Fix new tests for email by making sure that they clean after themselves and allow other tests to run. Fix included search for known services. Return hook from hook registration, so that it can be stopped.
|
Alright! I have fixed the tests. The issue there was that they weren't properly cleaning after themselves. @xet7 I think you will need to make changes in |
|
Thanks! I'll try newest changes. |
|
I don't understand why this does not work: I get error: But in beginning of start-wekan.sh I have: And it shows that MAIL_URL I did set. Do I need to make some other code changes? To And then built wekan with I did also verify that new |
|
@xet7 You will need the |
I did use all of new email package, including How would known hosts work? In this case, it's detected from protocol: But usually at O356, Google Workplace, etc companies have custom domain: If it were just smtp, it's hard to detect, because Or should service be option? |
|
Custom domains would not work with know-services from what I can tell as it only links to the main domain. So in that case it would be useless. Known services takes only login and password and then builds the rest of the url from hardcoded settings. |
|
With what kind of code I could send email with |
|
If we look at, for example Gmail, this is what the well-know services knows: "Gmail": {
"aliases": ["Google Mail"],
"domains": ["gmail.com", "googlemail.com"],
"host": "smtp.gmail.com",
"port": 465,
"secure": true
},So upon detecting Gmail it then sets the host, port and secure connection with the transport and we'll add in the login and password. I'll try to improve the detection of the service a bit more today. |
|
With your latest changes, with what kind of URL syntax does custom |
|
When I tested yesterday, it still did not detect that |
|
The package settings takes precedent. Here is the logic: const getTransport = function() {
const packageSettings = Meteor.settings.packages?.email || {};
// We delay this check until the first call to Email.send, in case someone
// set process.env.MAIL_URL in startup code. Then we store in a cache until
// process.env.MAIL_URL changes.
const url = process.env.MAIL_URL;
if (this.cacheKey === undefined || (this.cacheKey !== url || this.cacheKey !== packageSettings?.service || 'settings')) {
if ((packageSettings?.service && wellKnow(packageSettings.service)) || (url && wellKnow(new URL(url).hostname) || wellKnow(url?.split(':')[0] || ''))) {
this.cacheKey = packageSettings.service || 'settings';
this.cache = knownHostsTransport(packageSettings, url);
} else {
this.cacheKey = url;
this.cache = url ? makeTransport(url, packageSettings) : null;
}
}
return this.cache;
}; |
|
What kind of code is required to send email? |
|
It seems to me that |
|
Anyway, I'll test with todays changes, just a moment. |
|
Did a complete test locally and fixed the bug you were referring to @xet7 |
|
Thanks a lot! I'll test :) |
|
Hmm, interesting error, I'll try to figure it out. |
|
It seems that this line: Has this this kind of split code: I try to figure out why it says: |
|
What kind of URL were you using? |
|
|
And also: All have same split error. |
|
Does this change fix this issue? Basically the "name" value needs to be set to the users domain in order for Google Workspace to accept the SMTP EHLO. |
|
I'm not saying that any other web framework or programming language is any better than Meteor, everything else I have tried is even more broken in many ways. |
|
@xet7 I will add your examples to the test suite and fix them, but I'm afraid that since URL can't decipher these objects we will constantly be fixing infinite sets of possible url strings. This though is more of a stop gap measure until people can migrate to the settings option which is going to be 100% accurate. |
|
@xet7 even with addition of your test cases the tests are still passing for me. I'll do a more complex test later. |
|
Thanks! I'll test. |
|
With these examples: I still get this error That file wekan/.build/bundle/programs/server/npm/node_modules/meteor/email/node_modules/nodemailer/lib/well-known/index.js has this code: This nodemailer code still works OK: |
|
Code I tried with Meteor 2.2 and Node 12.22.4 is at: |
|
@xet7 honestly I don't know what to tell you. I will try a few more things, but so far I was unable to replicate the issue. |
|
No problem. Please don't use too much time on this. I will fix this in Wekan code anyway. |
Implements new features as per: #11547