Skip to content

SAN-2944-email-admins#1192

Merged
Nathan219 merged 29 commits intomasterfrom
SAN-2944-invite-teammembers
Dec 18, 2015
Merged

SAN-2944-email-admins#1192
Nathan219 merged 29 commits intomasterfrom
SAN-2944-invite-teammembers

Conversation

@Nathan219
Copy link
Copy Markdown
Member

This PR adds the SendGrid api and the ability for the team-invitations route to send emails to potential users

Reviewers*

Adding api wrapper for sendgrid
Adding service model to generate the emails for both admin and regular users
Finishing up the final tweaks to the model
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add jsdoc block comment

And fixing everything Casey mentioned
Comment thread configs/.env Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Move this to devops-scripts ansible variables?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Could you explain?

Comment thread lib/models/apis/sendgrid.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do we have this email address setup?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sendgrid uses this as the alias of the sender

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I mean is this real email address. Do we have it registered on our mail server? If I write to this email address will it bounced to accepted

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree with @podviaznikov - it should be a valid address

@podviaznikov
Copy link
Copy Markdown
Member

minor questions/comments from me (non-blockers). But looks good: 👍

Comment thread lib/models/apis/sendgrid.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would actually ask you do log.fatal - you're gonna be crashing the process here, so it's fatal

Comment thread unit/models/apis/sendgrid.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

did we go to aggressive? you still want to throw an error in a then before this catch, otherwise, you'll never see that it didn't break

@bkendall
Copy link
Copy Markdown
Contributor

that looks much better to me

@bkendall
Copy link
Copy Markdown
Contributor

double check that all the sendgrid environment variables are in devops scrips that you need, and then marge away!

@Nathan219
Copy link
Copy Markdown
Member Author

Comment thread lib/models/apis/sendgrid.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

SendGridModel.prototype.inviteAdmin - > inviteAdmin

@cflynn07
Copy link
Copy Markdown
Contributor

@Nathan219 for all your trace/error/warn logs after your function's initial info log, use just the prototype property name for the prefix to your msg field instead of the full prototype path.

ex:

Class.prototype.someMethod = function () {
  log.info({}, 'Class.prototype.someMethod');
  // do stuff
  log.trace({}, 'someMethod stuff happened')
}

https://github.com/CodeNow/devops-scripts/wiki/Guide-to-Using-Log-Levels

Fixing stupid bluebird promisify binding
@cflynn07
Copy link
Copy Markdown
Contributor

LGTM!

Nathan219 added a commit that referenced this pull request Dec 18, 2015
@Nathan219 Nathan219 merged commit 5f957a1 into master Dec 18, 2015
@Nathan219 Nathan219 deleted the SAN-2944-invite-teammembers branch December 18, 2015 02:23
@bkendall bkendall removed their assignment Jul 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants