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

fix local SMTP email sending #2751

Merged
merged 9 commits into from
Aug 29, 2017
Merged

Conversation

jshimko
Copy link
Contributor

@jshimko jshimko commented Aug 28, 2017

Fixes the ability to use an SMTP provider on localhost (both UI and methods would throw errors without a username/password provided). PR can be tested with Maildev.

npm i -g maildev

# Start mail server and inbox UI 
maildev

# SMTP server running at smtp://localhost:1025
# UI running at http://localhost:1080

# grab this branch
reaction init -b jshimko-fix-local-smtp-sending mail-fix
cd mail-fix
reaction
  • Go to the shop's email settings
  • pick Maildev from the dropdown
  • click save
  • trigger an email send from somewhere in the Reaction UI
  • note the received email in Maildev's inbox UI (http://localhost:1080)

This also adds the ability to choose "custom" from the dropdown and enter a mail server host without user/password. You obviously shouldn't ever do that in production, but at least now you can do things like run a custom SMTP server on localhost for development (although I highly recommend just using Maildev as the server and use the Maildev preset to connect to it).

@jshimko jshimko changed the title fix local SMTP email sending [WIP] fix local SMTP email sending Aug 28, 2017
@jshimko jshimko changed the title [WIP] fix local SMTP email sending fix local SMTP email sending Aug 28, 2017
@aaronjudd
Copy link
Contributor

Does this replace 29196ae or should that get manually added to this?

@aaronjudd aaronjudd requested review from aaronjudd and removed request for brent-hoover August 28, 2017 15:13
@jshimko
Copy link
Contributor Author

jshimko commented Aug 28, 2017

Well, there's at least a small issue with that update. It doesn't account for settings.service to be equal to custom (which would return nothing from getServiceConfig). So the goal was sound, but the implementation needs some additional logic handling.

That's also data getting passed into a UI component, so that should be happening in the container (where the providers object already exists anyway)

Give me a couple mins and I'll update my PR.

@jshimko
Copy link
Contributor Author

jshimko commented Aug 28, 2017

Actually, nevermind. I had already done it (with the required logic for the custom setting that I mentioned).

if (settings.service !== "custom") {
newSettings = Object.assign({}, settings, providers[settings.service]);
}

@aaronjudd
Copy link
Contributor

Could you also test for #1442 and #2630?

@jshimko
Copy link
Contributor Author

jshimko commented Aug 28, 2017

#1442 was definitely out of scope of this issue, so I'm positive that hasn't changed. I could fix it, but it's definitely a bit more work and I was just putting out the immediate fire of "can't use a mail server on localhost". Fixing that issue would require creating pub/sub to push down the parsed values of MAIL_URL and/or Meteor.settings.MAIL_URL so the UI could display them. Not anything too crazy, but not 5 mins of work for sure.

#2630 can't possibly still be broken or this PR wouldn't work at all (since it's a branch off of marketplace). The console errors shown in that issue appear to be something with the components API that must've been fixed by someone. Wasn't that closed at some point anyway?

@brent-hoover
Copy link
Collaborator

When I switch to maildev I get this:
reaction

and this error in the console:

  Error: Invalid login: 500 Error: command not recognized
      at Object.Future.wait (/Users/brenthoover/.meteor/packages/meteor-tool/.1.5.1.krqt03++os.osx.x86_64+web.browser+web.cordova/mt-os.osx.x86_64/dev_bundle/server-lib/node_modules/fibers/future.js:449:15)
      at packages/meteor.js:213:24
      at [object Object].emailVerifySettings (server/methods/email.js:49:14)
      at packages/check.js:129:16

Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested. Verified working now. 👍

@brent-hoover brent-hoover merged commit 972c631 into marketplace Aug 29, 2017
@brent-hoover brent-hoover deleted the jshimko-fix-local-smtp-sending branch August 29, 2017 05:34
@spencern spencern mentioned this pull request Oct 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants