From 423bc9312d26aef6f1e6d3ef69cbd790edb663b4 Mon Sep 17 00:00:00 2001
From: Henry Oswald <henry.oswald@sharelatex.com>
Date: Tue, 28 Aug 2018 12:32:20 +0100
Subject: [PATCH] only set status and merge fields if required

Not strictly nessaserry but it is a bit safer

also improve error reporting for change email
---
 .../Newsletter/NewsletterManager.coffee       | 39 ++++++++++++-------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/services/web/app/coffee/Features/Newsletter/NewsletterManager.coffee b/services/web/app/coffee/Features/Newsletter/NewsletterManager.coffee
index eee7258985..1195796ec3 100644
--- a/services/web/app/coffee/Features/Newsletter/NewsletterManager.coffee
+++ b/services/web/app/coffee/Features/Newsletter/NewsletterManager.coffee
@@ -16,10 +16,10 @@ module.exports =
 
 	subscribe: (user, callback = () ->)->
 		options = buildOptions(user, true)
-		logger.log options:options, user:user, email:user.email, "trying to subscribe user to the mailing list"
+		logger.log options:options, user:user, email:user.email, "subscribing user to the mailing list"
 		mailchimp.request options, (err)->
 			if err?
-				logger.err err:err, "error subscribing person to newsletter"
+				logger.err err:err, user:user, "error subscribing person to newsletter"
 			else
 				logger.log user:user, "finished subscribing user to the newsletter"
 			callback(err)
@@ -29,7 +29,7 @@ module.exports =
 		options = buildOptions(user, false)
 		mailchimp.request options, (err)->
 			if err?
-				logger.err err:err, "error unsubscribing person to newsletter"
+				logger.err err:err, user:user, "error unsubscribing person to newsletter"
 			else
 				logger.log user:user, "finished unsubscribing user to the newsletter"
 			callback(err)
@@ -39,12 +39,15 @@ module.exports =
 		delete options.body.status
 		options.body.email_address = newEmail
 		mailchimp.request options, (err)->
-			if err? and err?.message?.indexOf("merge fields were invalid") 
-				logger.log {oldEmail, newEmail}, "unable to change email in newsletter as user has not subscribed"
+			if err? and err?.message?.indexOf("merge fields were invalid") != -1
+				logger.log {oldEmail, newEmail}, "unable to change email in newsletter, user has never subscribed"
 				return callback()
-			# if the user has unsubscribed mailchimp will error on email address change
-			else if err? and err?.message?.indexOf("could not be validated") == -1
-				logger.err err:err, "error changing email in newsletter"
+			else if err? and err?.message?.indexOf("could not be validated") != -1
+				logger.log {oldEmail, newEmail}, 
+					"unable to change email in newsletter, user has previously unsubscribed or new email already exist on list"
+				return callback(err)
+			else if err?
+				logger.err {err, oldEmail, newEmail}, "error changing email in newsletter"
 				return callback(err)
 			else
 				logger.log "finished changing email in the newsletter"
@@ -54,18 +57,24 @@ hashEmail = (email)->
 	crypto.createHash('md5').update(email.toLowerCase()).digest("hex")
 
 buildOptions = (user, is_subscribed)->
-	status =  if is_subscribed then "subscribed" else "unsubscribed"
 	subscriber_hash = hashEmail(user.email)
 	opts =
 		method: "PUT"
 		path: "/lists/#{Settings.mailchimp?.list_id}/members/#{subscriber_hash}"
 		body:
-			status_if_new: status
-			status: status
 			email_address:user.email
-			merge_fields:
-				FNAME: user.first_name
-				LNAME: user.last_name
-				MONGO_ID:user._id
+				
+	status = if is_subscribed then "subscribed" else "unsubscribed"
+	if is_subscribed?
+		opts.body.status = status
+		opts.body.status_if_new = status
+
+	if user._id?
+		opts.body.merge_fields = 
+			FNAME: user.first_name
+			LNAME: user.last_name
+			MONGO_ID:user._id
+
+	console.log opts
 	return opts