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.

Sign up for GitHub

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

Jump to bottom

Updated notification support #131

Closed

solocommand wants to merge 13 commits into cygnusb2b:master from solocommand:notifications

Contributor

solocommand commented Jul 10, 2018

Resolves #49

When creating or modifying a campaign, all configured notifications are created or updated to be sent in the future.
If an email has already been sent, the notification is updated but the status is left unchanged.
If an email errored on send, it is recreated to retry sending.
If an email has not yet been sent, the dates and other fields are updated to match the latest change.
A node cron process checks every 10 minutes to see if there are any unsent notifications
- If any are found, they are findAndModified within a loop to start the async send handler
Updates to use account service data and virtual fields for URIs
Remove link-to handler

solocommand added 9 commits

July 10, 2018 09:55


          Add cron package

e587e1e


          Allow setting sendgrid in .env

5b92b3b


          Update contact notifier service to support cron and scheduled notific…

0b250b4

…ations


          Add cron job to root

db970a1


          Ensure notifications are updated when campaigns are changed

28782a7


          Make uris virtuals

cbc9d09


          Per hbs project recommendation, await before render rather than await…

fce3329

… in template


          Update templates, remove link-to helper

0b3b65f


          fix lint issues

1182c44

solocommand requested review from brandonbk and zarathustra323

July 10, 2018 16:00

zarathustra323 requested changes

View reviewed changes

src/graph/resolvers/campaign.js

@@ @@ -20,6 +20,12 @@ const getNotifyDefaults = async (advertiserId, user) => { @@
                 return notify;
               };
+              const updateNotifications = async (campaignId) => {
+                contactNotifier.scheduleCampaignCreated({ campaignId });

Member

zarathustra323 Jul 11, 2018

The updateNotifications isn't awaiting the scheduled email calls, is that intentional?

Contributor Author

solocommand Jul 16, 2018

Yes

src/graph/resolvers/campaign.js

@@ @@ -20,6 +20,12 @@ const getNotifyDefaults = async (advertiserId, user) => { @@
                 return notify;
               };
+              const updateNotifications = async (campaignId) => {
+                contactNotifier.scheduleCampaignCreated({ campaignId });
+                contactNotifier.scheduleCampaignStarted({ campaignId });

Member

zarathustra323 Jul 11, 2018

Also, have a look at the each function within the async package. These are probably a good candidates for running in parallel.
https://caolan.github.io/async/docs.html#each

src/graph/resolvers/campaign.js

@@ @@ -148,16 +159,20 @@ module.exports = { @@
                     const campaign = await Campaign.findById(campaignId);
                     if (!campaign) throw new Error(`Unable to set campaign URL: no campaign found for '${campaignId}'`);
                     campaign.url = url;
-                    return campaign.save();
+                    await campaign.save();
+                    await updateNotifications(campaignId);

Member

zarathustra323 Jul 11, 2018

Does updateNotifications need to be called on every mutation?

Contributor Author

solocommand Jul 16, 2018

IMO yes, as any campaign field could be used within the email message itself, and changes to criteria would necessitate changes to delivery

src/schema/campaign-notification.js Outdated

+                  required: true,
+                  validate: {
+                    async validator(v) {
+                      const doc = await connection.model('campaign').findOne({ _id: v }, { _id: 1 });

Member

zarathustra323 Jul 11, 2018

Change to .findById(v, { _id: 1 })
My bad for not doing that before you Cmd C+V

src/schema/campaign-notification.js Outdated

+                cc: [String],
+                bcc: [String],
+                subject: String,

Member

zarathustra323 Jul 11, 2018

Should subject, text, and html all be required? Well, maybe not text

src/services/contact-notifier.js

+                  // Run every 10 minutes
+                  const job = new CronJob({
+                    cronTime: '* */10 * * * *',
+                    onTick: this.check,

Member

zarathustra323 Jul 11, 2018

Does this need a bind?

Contributor Author

solocommand Jul 16, 2018

No, the context: this parameter handles the binding

src/services/contact-notifier.js Outdated

    
                async sendInternalCampaignCreated({ campaign }) {

                  const html = await emailTemplates.render('internal/campaign.created', { campaign });

                async scheduleCampaignCreated({ campaignId }) {

                  const campaign = await Campaign.findOne({ _id: campaignId });

Member

zarathustra323 Jul 11, 2018

findById

src/services/contact-notifier.js Outdated

+                },
+                async scheduleCampaignEnded({ campaignId }) {
+                  const campaign = await Campaign.findOne({ _id: campaignId });

Member

zarathustra323 Jul 11, 2018

findById

src/services/contact-notifier.js Outdated

-                  const html = await emailTemplates.render('external/campaign.created', { campaign });
-                  const subject = 'A new campaign was created!';
+                async scheduleCampaignStarted({ campaignId }) {
+                  const campaign = await Campaign.findOne({ _id: campaignId });

Member

zarathustra323 Jul 11, 2018

findById

src/services/contact-notifier.js

-                  const to = await this.resolveAddresses(campaign.get('notify.internal'));
-                  return this.send({ to, subject, html });
+                  const to = await this.resolveAddresses(campaign.get('notify.external'));
+                  const cc = await this.resolveAddresses(campaign.get('notify.internal'));

Member

zarathustra323 Jul 11, 2018

All these functions seem pretty repetitive... any way we can clean them up / consolidate?

zarathustra323 requested changes

View reviewed changes

src/services/contact-notifier.js Outdated

+                    notification.set('status', 'Sent');
+                    output.write('✉️  ✉️  ✉️   Successfully sent a notification!');
+                  } catch (e) {
+                    notification.set('error', e);

Member

zarathustra323 Jul 11, 2018

Just to make it more clear... e.message please :)

zarathustra323 reviewed

View reviewed changes

src/services/contact-notifier.js Outdated

+                  } catch (e) {
+                    notification.set('error', e);
+                    notification.set('status', 'Errored');
+                    notification.save();

Member

zarathustra323 Jul 11, 2018

Remove me

solocommand added 4 commits

July 16, 2018 08:30


          Make subject and html required

001e796


          Use findById

44f8728


          More findBys

b847404


          cleanup error handling

9f96f5b

solocommand closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet