fix: email testing and settings change from ACP

- changing email SMTP settings wouldn't apply the first time
- "Send Test Email" now will report emailer errors in most cases

public/src/admin/settings/email.js

@@ -22,6 +22,7 @@ define('admin/settings/email', ['ace/ace', 'admin/settings'], function (ace) {
 		$('button[data-action="email.test"]').off('click').on('click', function () {
 			socket.emit('admin.email.test', { template: $('#test-email').val() }, function (err) {
 				if (err) {
+					console.error(err.message);
 					return app.alertError(err.message);
 				}
 				app.alertSuccess('Test Email Sent');

src/emailer.js

@@ -37,12 +37,9 @@ Emailer.listServices = () => Object.keys(wellKnownServices);
 Emailer._defaultPayload = {};
 
 const smtpSettingsChanged = (config) => {
-	if (!prevConfig) {
-		prevConfig = meta.config;
-	}
-
 	const settings = [
 		'email:smtpTransport:enabled',
+		'email:smtpTransport:pool',
 		'email:smtpTransport:user',
 		'email:smtpTransport:pass',
 		'email:smtpTransport:service',
@@ -113,8 +110,8 @@ Emailer.getTemplates = async (config) => {
 };
 
 Emailer.setupFallbackTransport = (config) => {
-	winston.verbose('[emailer] Setting up SMTP fallback transport');
+	winston.verbose('[emailer] Setting up fallback transport');
-	// Enable Gmail transport if enabled in ACP
+	// Enable SMTP transport if enabled in ACP
 	if (parseInt(config['email:smtpTransport:enabled'], 10) === 1) {
 		const smtpOptions = {
 			pool: config['email:smtpTransport:pool'],
@@ -177,6 +174,11 @@ Emailer.registerApp = (expressApp) => {
 	Emailer.setupFallbackTransport(meta.config);
 	buildCustomTemplates(meta.config);
 
+	// need to shallow clone the config object
+	// otherwise prevConfig holds reference to meta.config object,
+	// which is updated before the pubsub handler is called
+	prevConfig = { ...meta.config };
+
 	// Update default payload if new logo is uploaded
 	pubsub.on('config:update', (config) => {
 		if (config) {
@@ -204,8 +206,7 @@ Emailer.registerApp = (expressApp) => {
 
 Emailer.send = async (template, uid, params) => {
 	if (!app) {
-		winston.warn('[emailer] App not ready!');
+		throw Error('[emailer] App not ready!');
-		return;
 	}
 
 	const [userData, userSettings] = await Promise.all([
@@ -214,13 +215,13 @@ Emailer.send = async (template, uid, params) => {
 	]);
 
 	if (!userData || !userData.email) {
-		winston.warn('uid : ' + uid + ' has no email, not sending.');
+		winston.warn(`uid : ${uid} has no email, not sending "${template}" email.`);
 		return;
 	}
 
 	const allowedTpls = ['verify_email', 'welcome', 'registration_accepted'];
 	if (meta.config.requireEmailConfirmation && !userData['email:confirmed'] && !allowedTpls.includes(template)) {
-		winston.warn('uid : ' + uid + ' has not confirmed email, not sending "' + template + '" email.');
+		winston.warn(`uid : ${uid} (${userData.email}) has not confirmed email, not sending "${template}" email.`);
 		return;
 	}
 
@@ -229,11 +230,7 @@ Emailer.send = async (template, uid, params) => {
 	params.uid = uid;
 	params.username = userData.username;
 	params.rtl = await translator.translate('[[language:dir]]', userSettings.userLang) === 'rtl';
-	try {
 	await Emailer.sendToEmail(template, userData.email, userSettings.userLang, params);
-	} catch (err) {
-		winston.error(err.stack);
-	}
 };
 
 Emailer.sendToEmail = async (template, email, language, params) => {
@@ -313,7 +310,7 @@ Emailer.sendToEmail = async (template, email, language, params) => {
 	}
 };
 
-Emailer.sendViaFallback = (data, callback) => {
+Emailer.sendViaFallback = async (data) => {
 	// Some minor alterations to the data to conform to nodemailer standard
 	data.text = data.plaintext;
 	delete data.plaintext;
@@ -323,12 +320,7 @@ Emailer.sendViaFallback = (data, callback) => {
 	delete data.from_name;
 
 	winston.verbose('[emailer] Sending email to uid ' + data.uid + ' (' + data.to + ')');
-	Emailer.fallbackTransport.sendMail(data, (err) => {
+	await Emailer.fallbackTransport.sendMail(data);
-		if (err) {
-			winston.error(err.stack);
-		}
-		callback();
-	});
 };
 
 Emailer.renderAndTranslate = async (template, params, lang) => {

src/notifications.js

@@ -181,8 +181,8 @@ async function pushToUids(uids, notification) {
 		}
 		body = posts.relativeToAbsolute(body, posts.urlRegex);
 		body = posts.relativeToAbsolute(body, posts.imgRegex);
-		await async.eachLimit(uids, 3, function (uid, next) {
+		await async.eachLimit(uids, 3, async (uid) => {
-			emailer.send('notification', uid, {
+			await emailer.send('notification', uid, {
 				path: notification.path,
 				notification_url: notification.path.startsWith('http') ? notification.path : nconf.get('url') + notification.path,
 				subject: utils.stripHTMLTags(notification.subject || '[[notifications:new_notification]]'),
@@ -190,7 +190,7 @@ async function pushToUids(uids, notification) {
 				body: body,
 				notification: notification,
 				showUnsubscribe: true,
-			}, next);
+			}).catch(err => winston.error('[emailer.send] ' + err.stack));
 		});
 	}
 

src/socket.io/admin/email.js

@@ -1,25 +1,24 @@
 'use strict';
 
-const async = require('async');
 const userDigest = require('../../user/digest');
 const userEmail = require('../../user/email');
 const notifications = require('../../notifications');
 const emailer = require('../../emailer');
-const utils = require('../../../public/src/utils');
+const utils = require('../../utils');
 
 const Email = module.exports;
 
-Email.test = function (socket, data, callback) {
+Email.test = async function (socket, data) {
 	const payload = {
 		subject: '[[email:test-email.subject]]',
 	};
 
 	switch (data.template) {
 		case 'digest':
-			userDigest.execute({
+			await userDigest.execute({
 				interval: 'alltime',
 				subscribers: [socket.uid],
-			}, callback);
+			});
 			break;
 
 		case 'banned':
@@ -28,42 +27,36 @@ Email.test = function (socket, data, callback) {
 				until: utils.toISOString(Date.now()),
 				reason: 'Test Reason',
 			});
-			emailer.send(data.template, socket.uid, payload, callback);
+			await emailer.send(data.template, socket.uid, payload);
 			break;
 
 		case 'welcome':
-			userEmail.sendValidationEmail(socket.uid, {
+			await userEmail.sendValidationEmail(socket.uid, {
 				force: 1,
-			}, callback);
+			});
 			break;
 
-		case 'notification':
+		case 'notification': {
-			async.waterfall([
+			const notification = await notifications.create({
-				function (next) {
-					notifications.create({
 				type: 'test',
 				bodyShort: '[[email:notif.test.short]]',
 				bodyLong: '[[email:notif.test.long]]',
 				nid: 'uid:' + socket.uid + ':test',
 				path: '/',
 				from: socket.uid,
-					}, next);
+			});
-				},
+			await emailer.send('notification', socket.uid, {
-				function (notifObj, next) {
+				path: notification.path,
-					emailer.send('notification', socket.uid, {
+				subject: utils.stripHTMLTags(notification.subject || '[[notifications:new_notification]]'),
-						path: notifObj.path,
+				intro: utils.stripHTMLTags(notification.bodyShort),
-						subject: utils.stripHTMLTags(notifObj.subject || '[[notifications:new_notification]]'),
+				body: notification.bodyLong || '',
-						intro: utils.stripHTMLTags(notifObj.bodyShort),
+				notification,
-						body: notifObj.bodyLong || '',
-						notification: notifObj,
 				showUnsubscribe: true,
-					}, next);
+			});
-				},
+		} break;
-			], callback);
-			break;
 
 		default:
-			emailer.send(data.template, socket.uid, payload, callback);
+			await emailer.send(data.template, socket.uid, payload);
 			break;
 	}
 };

src/socket.io/admin/user.js

@@ -87,9 +87,18 @@ User.sendValidationEmail = async function (socket, uids) {
 		throw new Error('[[error:email-confirmations-are-disabled]]');
 	}
 
+	const failed = [];
+
 	await async.eachLimit(uids, 50, async function (uid) {
-		await user.email.sendValidationEmail(uid, { force: true });
+		await user.email.sendValidationEmail(uid, { force: true }).catch((err) => {
+			winston.error('[user.create] Validation email failed to send\n[emailer.send] ' + err.stack);
+			failed.push(uid);
+		});
 	});
+
+	if (failed.length) {
+		throw Error(`Email sending failed for the following uids, check server logs for more info: ${failed.join(',')}`);
+	}
 };
 
 User.sendPasswordResetEmail = async function (socket, uids) {

src/socket.io/user.js

@@ -1,6 +1,7 @@
 'use strict';
 
 const util = require('util');
+const winston = require('winston');
 const sleep = util.promisify(setTimeout);
 
 const api = require('../api');
@@ -117,7 +118,7 @@ SocketUser.reset.commit = async function (socket, data) {
 		username: username,
 		date: parsedDate,
 		subject: '[[email:reset.notify.subject]]',
-	});
+	}).catch(err => winston.error('[emailer.send] ' + err.stack));
 };
 
 SocketUser.isFollowing = async function (socket, data) {

src/user/approval.js

@@ -1,6 +1,7 @@
 'use strict';
 
 const validator = require('validator');
+const winston = require('winston');
 const cronJob = require('cron').CronJob;
 
 const db = require('../database');
@@ -78,7 +79,7 @@ module.exports = function (User) {
 			subject: '[[email:welcome-to, ' + (meta.config.title || meta.config.browserTitle || 'NodeBB') + ']]',
 			template: 'registration_accepted',
 			uid: uid,
-		});
+		}).catch(err => winston.error('[emailer.send] ' + err.stack));
 		const total = await db.incrObjectField('registration:queue:approval:times', 'totalTime', Math.floor((Date.now() - creation_time) / 60000));
 		const counter = await db.incrObjectField('registration:queue:approval:times', 'counter', 1);
 		await db.setObjectField('registration:queue:approval:times', 'average', total / counter);

src/user/bans.js

@@ -53,11 +53,7 @@ module.exports = function (User) {
 			until: until ? (new Date(until)).toUTCString().replace(/,/g, '\\,') : false,
 			reason: reason,
 		};
-		try {
+		await emailer.send('banned', uid, data).catch(err => winston.error('[emailer.send] ' + err.stack));
-			await emailer.send('banned', uid, data);
-		} catch (err) {
-			winston.error('[emailer.send] ' + err.message);
-		}
 
 		return banData;
 	};

src/user/create.js

@@ -1,6 +1,8 @@
 'use strict';
 
 const zxcvbn = require('zxcvbn');
+const winston = require('winston');
+
 const db = require('../database');
 const utils = require('../utils');
 const slugify = require('../slugify');
@@ -116,7 +118,7 @@ module.exports = function (User) {
 		if (userData.email && userData.uid > 1 && meta.config.requireEmailConfirmation) {
 			User.email.sendValidationEmail(userData.uid, {
 				email: userData.email,
-			});
+			}).catch(err => winston.error('[user.create] Validation email failed to send\n[emailer.send] ' + err.stack));
 		}
 		if (userNameChanged) {
 			await User.notifications.sendNameChangeNotification(userData.uid, userData.username);

src/user/digest.js

@@ -125,7 +125,6 @@ Digest.send = async function (data) {
 
 		emailsSent += 1;
 		const now = new Date();
-		try {
 		await emailer.send('digest', userObj.uid, {
 			subject: '[[email:digest.subject, ' + (now.getFullYear() + '/' + (now.getMonth() + 1) + '/' + now.getDate()) + ']]',
 			username: userObj.username,
@@ -136,10 +135,7 @@ Digest.send = async function (data) {
 			popularTopics: popularTopics,
 			interval: data.interval,
 			showUnsubscribe: true,
-			});
+		}).catch(err => winston.error('[user/jobs] Could not send digest email\n[emailer.send] ' + err.stack));
-		} catch (err) {
-			winston.error('[user/jobs] Could not send digest email\n' + err.stack);
-		}
 
 		if (data.interval !== 'alltime') {
 			await db.sortedSetAdd('digest:delivery', now.getTime(), userObj.uid);

src/user/invite.js

@@ -49,7 +49,6 @@ module.exports = function (User) {
 		}
 
 		const data = await prepareInvitation(uid, email, groupsToJoin);
-
 		await emailer.sendToEmail('invitation', email, meta.config.defaultLang, data);
 	};
 

src/user/profile.js

@@ -3,6 +3,7 @@
 
 const async = require('async');
 const validator = require('validator');
+const winston = require('winston');
 
 const utils = require('../utils');
 const slugify = require('../slugify');
@@ -246,7 +247,7 @@ module.exports = function (User) {
 				email: newEmail,
 				subject: '[[email:email.verify-your-email.subject]]',
 				template: 'verify_email',
-			});
+			}).catch(err => winston.error('[user.create] Validation email failed to send\n[emailer.send] ' + err.stack));
 		}
 	}
 

src/user/reset.js

@@ -55,7 +55,7 @@ UserReset.send = async function (email) {
 		subject: '[[email:password-reset-requested]]',
 		template: 'reset',
 		uid: uid,
-	});
+	}).catch(err => winston.error('[emailer.send] ' + err.stack));
 };
 
 UserReset.commit = async function (code, password) {

test/api.js

@@ -68,9 +68,13 @@ describe('API', async () => {
 	async function dummySearchHook(data) {
 		return [1];
 	}
+	async function dummyEmailerHook(data) {
+		// pretend to handle sending emails
+	}
 
 	after(async function () {
 		plugins.unregisterHook('core', 'filter:search.query', dummySearchHook);
+		plugins.unregisterHook('emailer-test', 'filter:email.send');
 	});
 
 	async function setupData() {
@@ -145,6 +149,11 @@ describe('API', async () => {
 			hook: 'filter:search.query',
 			method: dummySearchHook,
 		});
+		// Attach an emailer hook so related requests do not error
+		plugins.registerHook('emailer-test', {
+			hook: 'filter:email.send',
+			method: dummyEmailerHook,
+		});
 
 		jar = await helpers.loginUser('admin', '123456');
 

test/socket.io.js

@@ -240,6 +240,21 @@ describe('socket.io', function () {
 
 	describe('validation emails', function () {
 		var meta = require('../src/meta');
+		var plugins = require('../src/plugins');
+
+		async function dummyEmailerHook(data) {
+			// pretend to handle sending emails
+		}
+		before(function () {
+			// Attach an emailer hook so related requests do not error
+			plugins.registerHook('emailer-test', {
+				hook: 'filter:email.send',
+				method: dummyEmailerHook,
+			});
+		});
+		after(function () {
+			plugins.unregisterHook('emailer-test', 'filter:email.send');
+		});
 
 		it('should validate emails', function (done) {
 			socketAdmin.user.validateEmail({ uid: adminUid }, [regularUid], function (err) {

test/user.js

@@ -25,7 +25,18 @@ describe('User', function () {
 	var testUid;
 	var testCid;
 
+	var plugins = require('../src/plugins');
+
+	async function dummyEmailerHook(data) {
+		// pretend to handle sending emails
+	}
 	before(function (done) {
+		// Attach an emailer hook so related requests do not error
+		plugins.registerHook('emailer-test', {
+			hook: 'filter:email.send',
+			method: dummyEmailerHook,
+		});
+
 		Categories.create({
 			name: 'Test Category',
 			description: 'A test',
@@ -39,6 +50,9 @@ describe('User', function () {
 			done();
 		});
 	});
+	after(function () {
+		plugins.unregisterHook('emailer-test', 'filter:email.send');
+	});
 
 	beforeEach(function () {
 		userData = {