Skip to content

Ideas for updates #1

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

Open
roblouie wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Ideas for updates #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4d55939
Some ideas for changes to make things easier to understand. This coul…
roblouie Jan 4, 2018
7b2354e
Some ideas for changes to make things easier to understand. This coul…
roblouie Jan 4, 2018
3ed84e3
Update surveyRoutes.js
roblouie Jan 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions server/routes/authRoutes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,12 @@
const passport = require('passport');

module.exports = app => {
app.get(
'/auth/google',
passport.authenticate('google', {
app.get('/auth/google', passport.authenticate('google', {
scope: ['profile', 'email']
})
);

app.get(
'/auth/google/callback',
passport.authenticate('google'),
(req, res) => {
app.get('/auth/google/callback', passport.authenticate('google'), (req, res) => {

@roblouie roblouie Jan 4, 2018

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Just spacing changes here to make it consistent with everythign else, idk why it was totally different here.

res.redirect('/surveys');
}
);
Expand Down
44 changes: 9 additions & 35 deletions server/routes/surveyRoutes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
const _ = require('lodash');
const Path = require('path-parser');
const { URL } = require('url');
const mongoose = require('mongoose');

const requireLogin = require('../middlewares/requireLogin');
const requireCredits = require('../middlewares/requireCredits');
const Mailer = require('../services/Mailer');
const surveyTemplate = require('../services/emailTemplates/surveyTemplate');

const SurveyService = require('../service/survey.service.js');

@roblouie roblouie Jan 4, 2018

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

This is my new service, notice my file naming convention of survey.service.js. It's all lowercase, and has a .type naming convention, that way you know what type of object your dealing with. I kept it in the services folder for now, but as I mentioned you should group by feature, it's just not worth doing at this point on this small of a project.

const Survey = mongoose.model('surveys');

const surveyService = new SurveyService();

module.exports = app => {
app.get('/api/surveys', requireLogin, async (req, res) => {
const surveys = await Survey.find({ _user: req.user.id }).select({
Expand All @@ -23,35 +24,8 @@ module.exports = app => {
});

app.post('/api/surveys/webhooks', (req, res) => {
const p = new Path('/api/surveys/:surveyId/:choice');

_.chain(req.body)
.map(({ email, url }) => {
const match = p.test(new URL(url).pathname);
if (match) {
return { email, surveyId: match.surveyId, choice: match.choice };
}
})
.compact()
.uniqBy('email', 'surveyId')
.each(({ surveyId, email, choice }) => {
Survey.updateOne(
{
_id: surveyId,
recipients: {
$elemMatch: { email: email, responded: false }
}
},
{
$inc: { [choice]: 1 },
$set: { 'recipients.$.responded': true },
lastResponded: new Date()
}
).exec();
})
.value();

res.send({});
const result = surveyService.updateSurveys(req.body);
res.send(result);
});

@roblouie roblouie Jan 4, 2018

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Since this was a bunch of logic and wrote to the database, it needed moved into a service, so I did that.

It's all replaced by me calling my service method and sending the result along.


app.post('/api/surveys', requireLogin, requireCredits, async (req, res) => {
Expand All @@ -61,13 +35,13 @@ module.exports = app => {
title,
subject,
body,
recipients: recipients.split(',').map(email => ({ email: email.trim() })),
recipients: surveyService.getRecipientListFromEmailString(recipients),

@roblouie roblouie Jan 4, 2018

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I also added a helper method to my service to handle this confusing logic. Putting this code directly in an object definition is gross. Instead I made a nicely named function in my service and I call it here. Because it's nicely named, you know what it does just by looking, getRecipientListFromEmailString. It also keeps more business logic in the service layer. Really most of this could be moved to the service, but whatever, this is fine for now.

_user: req.user.id,
dateSent: Date.now()
});

// Great place to send an email!
const mailer = new Mailer(survey, surveyTemplate(survey));
const surveyContent = surveyTemplate(survey);
const mailer = new Mailer(survey, surveyContent);

@roblouie roblouie Jan 4, 2018

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Passing a function call into a function is kind of ugly, I'd rather have one more line of code and have it be easier to understand. So I create a new variable surveyContent and store the filled survey there. Then I pass it into the Mailer constructor.


try {
await mailer.send();
Expand Down
19 changes: 7 additions & 12 deletions server/services/Mailer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,22 @@ const helper = sendgrid.mail;
const keys = require('../config/keys');

class Mailer extends helper.Mail {
constructor({ subject, recipients }, content) {
constructor(survey, content) {
super();

@roblouie roblouie Jan 4, 2018

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

This was the confusing argument destructor I was talking about. This constructor, as we saw just above, takes a survey as it's first argument. However, you wouldnt' know that just by looking at it due to the argument deconstructions.

I think it's much easier to ready by simply saying a survey object is passed in.


this.sgApi = sendgrid(keys.sendGridKey);
this.from_email = new helper.Email('no-reply@emaily.com');
this.subject = subject;
this.subject = survey.subject;
this.body = new helper.Content('text/html', content);
this.recipients = this.formatAddresses(recipients);
this.recipients = this.formatAddresses(survey.recipients);

@roblouie roblouie Jan 4, 2018

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Since we didn't deconstruct the survey when it was passed in, we have to access the properties of the survey, so I add the survey.. This to me is just much easier to understand than before.


this.addContent(this.body);
this.addClickTracking();
this.addRecipients();
}

formatAddresses(recipients) {
return recipients.map(({ email }) => {
return new helper.Email(email);
});
return recipients.map(recipient => new helper.Email(recipient.email));

@roblouie roblouie Jan 4, 2018

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

For simple arrow function callbacks, you can make it a single line and omit the curly brace and return keyword, which I find easier to read.

}

addClickTracking() {
Expand All @@ -34,21 +32,18 @@ class Mailer extends helper.Mail {
addRecipients() {
const personalize = new helper.Personalization();

this.recipients.forEach(recipient => {
personalize.addTo(recipient);
});
this.recipients.forEach(recipient => personalize.addTo(recipient));
this.addPersonalization(personalize);
}

async send() {
send() {
const request = this.sgApi.emptyRequest({
method: 'POST',
path: '/v3/mail/send',
body: this.toJSON()
});

const response = await this.sgApi.API(request);
return response;
return this.sgApi.API(request);

@roblouie roblouie Jan 4, 2018

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

The function that calls this method also uses async/await...which makes no sense. Using it here means this simply returns the result of the API call, you don't await whatever this api call returns.

Since the function that calls it calls other async functions, it makes sense to simply remove it here.

The calling function looks like this:

try {
      await mailer.send();  // This makes no sense with the old code, since .send() will simply return a response, there's nothing to "await" because the .send() call already waited to return the object.
      await survey.save();
      req.user.credits -= 1;
      const user = await req.user.save();

      res.send(user);
    } catch (err) {
      res.status(422).send(err);
    }

}
}

Expand Down
4 changes: 1 addition & 3 deletions server/services/passport.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@ passport.serializeUser((user, done) => {
});

passport.deserializeUser((id, done) => {
User.findById(id).then(user => {
done(null, user);
});
User.findById(id).then(user => done(null, user));

@roblouie roblouie Jan 4, 2018

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Again, simple code so why not remove the curly braces and one-line it.

});

passport.use(
Expand Down
60 changes: 60 additions & 0 deletions server/services/survey.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
const Path = require('path-parser');
const { URL } = require('url');

module.exports = class SurveyService {

async updateSurveys(data) {

@roblouie roblouie Jan 4, 2018 •
edited
Loading

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

So, here's my big service method that does some work on the data sent in and then updates the database. The old code had many problems besides just being in the router.

The biggest was that even though it was an async operation (it wrote to the database), it did not use async await. This meant that if there was an error writing to the database, you would never know it. It would simply return instantly while asyncrhounously the database would be written to. I fix that by actually using async/await here, and wrapping it in a try catch block.

Notice I also do not use lodash at all, as it is obsolete and just another thing to learn that you don't need. It was also being used incorrectly, as the uniqBy function only takes one argument...so there was actually a major functional bug in the code.

try {

const uniqueServeys =
data.map(this.buildSurveyDataFromBody)
.filter(this.filterOutNullAndUndefined)
.filter(this.filterByUniqueEmailAndSurveyId);

@roblouie roblouie Jan 4, 2018

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Here I chain regular old Javascript array methods. I take the data passed in and then I run some operations on it. This is the same as what was being done with lodash, except it uses native javascript, and actually works as it's supposed to.

Also notice that I broke each part of the functionality into it's own named function. This makes the chain of events much simpler to ready. You can see that first I buildSurveyDataFromBody, then I filterOutNullAndUndefined , and finally filterByUniqueEmailAndSurveyId.

The functionality for each of those is stored in it's own nicely named function. This helps readability, and keeps your logic separted nicely. Remember you want functions to try to serve a single purpose. Break things down into their smallest logical pieces and name them nicely.


for (let survey of uniqueServeys) {
await Survey.updateOne(
{
_id: survey.surveyId,
recipients: {
$elemMatch: {email: survey.email, responded: false}
}
},
{
$inc: {[survey.choice]: 1},
$set: {'recipients.$.responded': true},
lastResponded: new Date()
}
).exec();
}

@roblouie roblouie Jan 4, 2018

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

If we weren't using async/await for this, I would just chain a .forEach onto my javascript array chain, which would look nice:

data.map(this.buildSurveyDataFromBody)
  .filter(this.filterOutNullAndUndefined)
  .filter(this.filterByUniqueEmailAndSurveyId)
  .forEach(this.writeSurveysToDatabase);

But forEach doesn't work with async/await, so I had to use a for/of loop instead. This mean the loop waits until the database write is complete before continuing, and then this function won't return until all writes are complete.


return { success: true }

@roblouie roblouie Jan 4, 2018

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Rather than returning nothing like the old method, I return this success data just for a way to verify it worked.

} catch(error) {
return error;

@roblouie roblouie Jan 4, 2018

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

And here if there was an error, I return that error. You wouldn't want to do this in a real production app, as it could expose details about your code or database that you wouldn't want the public to know, but for this example it's fine, and will actually let you know theres an error, unlike the original code which would fail silently.

}
}

getRecipientListFromEmailString(recipients) {
return recipients.split(',').map(email => {
return { email };
})
}

@roblouie roblouie Jan 4, 2018

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

This is that helper function for the recipient formatting, pretty basic, make an array from a comma separated string and then turn those strings into object with email as the key and the string as the value.



buildSurveyDataFromBody(bodyItem) {
const path = new Path('/api/surveys/:surveyId/:choice');
const match = path.test(new URL(surveyData.url).pathname);
if (match) {
return { email: surveyData.email, surveyId: match.surveyId, choice: match.choice };
}
}

@roblouie roblouie Jan 4, 2018

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Just noticed an error from when I moved code around. I call the passed in object bodyItem, but then later call it surveyData. I forgot to fix this after moving it into it's own service. They should be the same thing.

This is called by the map function (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/map). Map just runs on an array and returns a new array based on the function passed in. So here we had an array of objects with emails and urls, and we turn it into an array of objects with emails, surveyIds, and choices.


filterOutNullAndUndefined(item) {
return item !== null && item !== undefined;
}

@roblouie roblouie Jan 4, 2018

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

That previous map function passes that array of data to this filter callback, which removes any null or undefined items. This should be impossible since the .map function above only adds items that match the URL, but since it was in the original code and I didn't actually test to be sure...I left it in. This can probably be removed though.


filterByUniqueEmailAndSurveyId(survey, index, array) {
const emailSurveyIdKeyArray = array.map(item => item.email + item.surveyId);

return emailSurveyIdKeyArray.indexOf(survey.email + survey.id) === index;
}

@roblouie roblouie Jan 4, 2018

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

This part replaces lodash, it's another filter method here. The lodash method was actually broken, as he tried to make it unique by both email and survey ID, but if you check the lodash docuemtation, uniqBy only takes a single string to be unique by. So...his old code was only unique by email, not survey ID. Meaning...it was broken.

This works properly and filters down to a unique list by email and survey ID, by creating a new array with a key string of email + survey ID, and testing if the index of the current email + id is the same as the current index.

};