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

refactor: mail #49

Merged
merged 7 commits into from
Mar 9, 2018
Merged

Conversation

sinchang
Copy link
Contributor

@sinchang sinchang commented Mar 8, 2018

Ref: issue #14

Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

common services

Description of change

common mail method

@sinchang sinchang force-pushed the feat/common-email branch from 41410af to 767808c Compare March 8, 2018 13:32
@codecov
Copy link

codecov bot commented Mar 8, 2018

Codecov Report

Merging #49 into master will increase coverage by 0.31%.
The diff coverage is 68.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #49      +/-   ##
==========================================
+ Coverage   59.17%   59.49%   +0.31%     
==========================================
  Files          33       34       +1     
  Lines         926      958      +32     
==========================================
+ Hits          548      570      +22     
- Misses        378      388      +10
Impacted Files Coverage Δ
config/config.default.js 100% <100%> (ø) ⬆️
app/service/mail.js 65.51% <65.51%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce9247a...81e1e29. Read the comment docs.

// debug 为 true 时,用于本地调试
config.debug = true;

// use for cookie sign key, should change to your own and keep security
config.keys = appInfo.name + '_1519887194138_3450';
config.keys = '_1519887194138_3450';
Copy link
Member

Choose a reason for hiding this comment

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

这个不能修改的。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

那我把它移动到 service 层。

package.json Outdated
@@ -4,6 +4,7 @@
"description": "",
"private": true,
"dependencies": {
"async-retry": "^1.2.1",
Copy link
Member

Choose a reason for hiding this comment

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

这个其实没有必要了。for 循环就可以的。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

package.json Outdated
@@ -18,6 +19,9 @@
"loader-koa": "^2.0.1",
"lodash": "^4.17.5",
"markdown-it": "^8.4.1",
"nodemailer": "^4.6.2",
"nodemailer-smtp-transport": "^2.7.4",
"util": "^0.10.3",
Copy link
Member

Choose a reason for hiding this comment

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

这个是啥模块?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const from = util.format('%s <%s>', config.name, config.mail_opts.auth.user); 用在这,看老 cnode 是这么写的。

const mailer = require('nodemailer');
const smtpTransport = require('nodemailer-smtp-transport');
const util = require('util');
const config = require('../../config/config.default')();
Copy link
Member

Choose a reason for hiding this comment

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

这个不能这样用的。如果不行的话 我们把 mail 的功能放进 service 里好了。

Copy link
Member

Choose a reason for hiding this comment

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

封装成 egg-plugin 会更好一点吧

Copy link
Contributor Author

Choose a reason for hiding this comment

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

移动到 service 去做,这样 config logger 都可以拿到了,不需要做 plugin 了。

@sinchang sinchang force-pushed the feat/common-email branch from acc1f39 to 63d9beb Compare March 9, 2018 02:18
@JacksonTian JacksonTian merged commit bb4284e into cnodejs:master Mar 9, 2018
@JacksonTian
Copy link
Member

Thanks.

@sinchang sinchang deleted the feat/common-email branch March 9, 2018 02:38
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