Skip to content
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

Email.send silent fail #11640

Open
harryadel opened this issue Sep 23, 2021 · 4 comments · May be fixed by #11678
Open

Email.send silent fail #11640

harryadel opened this issue Sep 23, 2021 · 4 comments · May be fixed by #11678

Comments

@harryadel
Copy link
Contributor

@harryadel harryadel commented Sep 23, 2021

Email.send was failing silently because I failed to provide from property

// process.env does contain MAIL_URL
const options = {
			to: user.emails[0].address,
			subject: 'test',
			html: `
      <!doctype html>
        <html lang="en">
          ...
        </html>
      `,
		};
		Meteor.defer(() => {
			Email.send(options);
		});

And in the docs from is considered to be a required field, so I was thinking maybe if we add a little check statement inside Email.send method to check for important properties and ensure they're of the correct type.
https://github.com/meteor/meteor/blob/devel/packages/email/email.js#L213

Just like how it's done in validated-methods.

I guess the question is which properties should we check for? Given my use case, I'd say we can start with from and see if we need to add more later on.

Side note: Also, shouldn't validated-methods be moved into meteor repo similar to other packages currently being moved?

@StorytellerCZ
Copy link
Contributor

@StorytellerCZ StorytellerCZ commented Sep 23, 2021

@harryadel Good catch, I'll see about adding it in for 2.5. As for moving packages that is done in steps and right now focuses on npm packages.

@djroxx2000
Copy link

@djroxx2000 djroxx2000 commented Oct 5, 2021

Can I take this up? As far as I understand, it only requires an import and a line to check 'from' to be a valid String with regex mail validation. Should I also add a todo comment for checking more fields? Also, I see that the check function Match patterns don't accept regex as a pattern. Should that be added or keeping regex separate was intentional?

@StorytellerCZ
Copy link
Contributor

@StorytellerCZ StorytellerCZ commented Oct 5, 2021

@djroxx2000 go ahead. Though I think the check should be super simple, check that the field is filled and has @ symbol, to make this more permissive.

djroxx2000 added a commit to djroxx2000/meteor that referenced this issue Oct 5, 2021
Added null check using a simple regex for 'from' address in email options.
@djroxx2000
Copy link

@djroxx2000 djroxx2000 commented Oct 5, 2021

@StorytellerCZ I have created the pull request here #11678. I didn't use the check function since the regex check for null and undefined as well. Please let me know if any other changes required here.

@StorytellerCZ StorytellerCZ linked a pull request that will close this issue Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants