-
Notifications
You must be signed in to change notification settings - Fork 0
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
Autopost to new newsletter's mailchimp which has id 243735091b #5
base: master
Are you sure you want to change the base?
Conversation
Missing Mailchimp API key most likely (don't have access to it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just have some minor suggestions + questions
|
||
const mailchimp = new Mailchimp(process.env.MAILCHIMP_API_KEY); | ||
|
||
module.exports.postToMailchimp = function(userInformation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
entirely style related comment, feel free to ignore, but i think it's generally better to do something like:
const postToMailchimp = function(...) {
...
}
module.exports = {
postToMailchimp,
};
}) | ||
.catch(err => { | ||
console.log('Error adding user to mailchimp') | ||
console.log(params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feel like we should probably use console.error here, and also is params
defined?
@@ -26,6 +27,9 @@ function signup(req, res, next) { | |||
console.log(params); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like this was here already, but do we want to leave it? is it necessary i mean? typically logs like this should be removed before merging
I'll get the api key and add it to the heroku setup before we merge this. |
Hey @kidmillions -- are there any extra steps here for the mailchimp API key stuff? Do you need anything from me? |
Missing Mailchimp API key most likely (don't have access to it)