From 916e8010c3d5b7245926af1648ed7633b6beaf04 Mon Sep 17 00:00:00 2001 From: hajush Date: Tue, 21 Mar 2017 12:43:04 -0600 Subject: [PATCH 1/2] Code Review - DO NOT MERGE! - Review comments and make changes separately. --- helpers/scrape.js | 4 ++-- helpers/scrapeAndSyncRunner.js | 1 + helpers/sync.js | 2 ++ helpers/syncRunner.js | 1 + models/pet.js | 1 + routes/petRoutes.js | 1 + src/components/App.js | 1 + test/helpers/scrape.test.js | 2 ++ tools/srcServer.js | 3 +++ 9 files changed, 14 insertions(+), 2 deletions(-) diff --git a/helpers/scrape.js b/helpers/scrape.js index 2d84cab..c5a41ec 100644 --- a/helpers/scrape.js +++ b/helpers/scrape.js @@ -1,4 +1,4 @@ - +// since you are using babel-node on the server, you can use import - Harold let request = require('request'); let cheerio = require('cheerio'); let scraper = {}; @@ -25,7 +25,7 @@ scraper.scrapePetango = function(url, callback) { }; //This function takes a response which is an HTML string -//and returns and array of url strings. +//and returns an array of url strings. scraper.parseAnimalListResponse = function(html) { let urlStrings = []; let $ = cheerio.load(html); diff --git a/helpers/scrapeAndSyncRunner.js b/helpers/scrapeAndSyncRunner.js index cd7bf13..da1312a 100644 --- a/helpers/scrapeAndSyncRunner.js +++ b/helpers/scrapeAndSyncRunner.js @@ -1,3 +1,4 @@ +// since you are using babel-node on the server, you can use import - Harold let mongoose = require("mongoose"); let uriUtil = require('mongodb-uri'); mongoose.Promise = global.Promise; diff --git a/helpers/sync.js b/helpers/sync.js index 79a15bf..2d9e001 100644 --- a/helpers/sync.js +++ b/helpers/sync.js @@ -1,5 +1,7 @@ //You scrape petango and you get one pet object back. //You want to look that pet up in your database by animalId. + +// since you are using babel-node on the server, you can use import - Harold let Pet = require('../models/pet'); let sync = {}; diff --git a/helpers/syncRunner.js b/helpers/syncRunner.js index 1c00cbc..2a2f580 100644 --- a/helpers/syncRunner.js +++ b/helpers/syncRunner.js @@ -4,6 +4,7 @@ //Loki is not in the scrape and needs to be removed from the database. //Baltazaar is in both the scrape and the DB so he should stay put. +// since you are using babel-node on the server, you can use import - Harold let mongoose = require("mongoose"); let uriUtil = require('mongodb-uri'); mongoose.Promise = global.Promise; diff --git a/models/pet.js b/models/pet.js index 1876b6d..af169de 100644 --- a/models/pet.js +++ b/models/pet.js @@ -1,3 +1,4 @@ +// since you are using babel-node on the server, you can use import - Harold let mongoose = require("mongoose"); let PetSchema = new mongoose.Schema({ diff --git a/routes/petRoutes.js b/routes/petRoutes.js index 33247bc..9b8277e 100644 --- a/routes/petRoutes.js +++ b/routes/petRoutes.js @@ -1,5 +1,6 @@ import React from 'react'; import webpack from 'webpack'; +// Best to be consistent and use import instead of require below - Harold const Pet = require ('../models/pet'); let express = require('express'); let router = express.Router(); diff --git a/src/components/App.js b/src/components/App.js index 6f38bad..280c16c 100644 --- a/src/components/App.js +++ b/src/components/App.js @@ -13,6 +13,7 @@ class App extends React.Component { render() { return (
+ {/* Best to put this link once in your index.html - Harold */}
diff --git a/test/helpers/scrape.test.js b/test/helpers/scrape.test.js index edbc5c8..84a6f7c 100644 --- a/test/helpers/scrape.test.js +++ b/test/helpers/scrape.test.js @@ -9,6 +9,8 @@ describe('Scrape', () => { expect(scrape.scrapePetango("http://ws.petango.com/Webservices/adoptablesearch/wsAdoptableAnimals.aspx?species=Dog&sex=All&agegroup=All&colnum=1&authkey=1t4v495156y98t2wd78317102f933h83or1340ptjm31spd04d")).to.eql([]); }); }); + // rather than put this html string right in your code - would be better to + // save the html in a file and read it (using NodeJS file reading) - Harold let html = ` diff --git a/tools/srcServer.js b/tools/srcServer.js index 7cdbeae..d54f656 100644 --- a/tools/srcServer.js +++ b/tools/srcServer.js @@ -32,6 +32,9 @@ const Pet = require('../models/pet'); const petRoutes = require('../routes/petRoutes'); //End +// Best not to expose all your React components here, only the files you want +// to serve up without transpilation. You can move the index.html and style.css +// to a folder (maybe 'public'), and express.static only those two files. - Harold app.use(express.static('src')); app.use(require('webpack-dev-middleware')(compiler, { From b4a3e39ae0e95befb466fd67deedeb55d222216b Mon Sep 17 00:00:00 2001 From: hajush Date: Thu, 30 Mar 2017 13:24:05 -0600 Subject: [PATCH 2/2] Code review 2 --- README.md | 3 +++ helpers/scrape.js | 21 ++++++++------------- helpers/scrape.test.js | 5 ++++- helpers/sync.js | 2 +- routes/petRoutes.js | 2 +- routes/userRoutes.js | 2 +- src/components/DisplayPets.js | 1 + tools/configAuth.js | 1 + tools/srcServer.js | 9 +++++---- 9 files changed, 25 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index 4913ea5..f6bc306 100644 --- a/README.md +++ b/README.md @@ -1,5 +1,8 @@ # Montana Code School: Pet Project +# This is an important page for your demo day! Add your contributor names and +# a little about the project technologies being used - Harold + ## Run ~~~ $ npm install diff --git a/helpers/scrape.js b/helpers/scrape.js index e1862ee..acf2ef4 100644 --- a/helpers/scrape.js +++ b/helpers/scrape.js @@ -1,10 +1,11 @@ -// since you are using babel-node on the server, you can use import - Harold -let request = require('request'); -// import request from 'request'; -let cheerio = require('cheerio'); -// import cheerio from 'cheerio'; -let scraper = {}; +// seems to work using import. good to resolve any remaining issues. +// Please only include the needed helper code now - not sure what is still being used +// -Harold + +import request from 'request'; +import cheerio from 'cheerio'; +let scraper = {}; scraper.scrapePetango = function(url, callback) { //Make a GET request @@ -71,10 +72,4 @@ scraper.parseIndividualAnimalResponse = function(html) { return petObject; }; - - - - - - -module.exports = scraper; +export default scraper; diff --git a/helpers/scrape.test.js b/helpers/scrape.test.js index 1937e6b..d9db65b 100644 --- a/helpers/scrape.test.js +++ b/helpers/scrape.test.js @@ -1,7 +1,10 @@ import {expect} from 'chai'; -let scrape = require('./scrape'); +import scrape from './scrape'; describe('Scrape', () => { + // Still not reading from a file. + // You can do some expects around the number of animals in the data etc, + // No need to do a full .to.eql comparison. - Harold //We need to figure out how to run callbacks and mock the data so that we can //run this test. xdescribe('scrapePetango', () => { diff --git a/helpers/sync.js b/helpers/sync.js index ed8818c..764f759 100644 --- a/helpers/sync.js +++ b/helpers/sync.js @@ -1,7 +1,7 @@ //You scrape petango and you get one pet object back. //You want to look that pet up in your database by animalId. -// since you are using babel-node on the server, you can use import - Harold +// Let's get import working (and ES6 export at bottom)- Harold let Pet = require('../models/pet'); let sync = {}; diff --git a/routes/petRoutes.js b/routes/petRoutes.js index dcc0233..0ebc368 100644 --- a/routes/petRoutes.js +++ b/routes/petRoutes.js @@ -1,6 +1,6 @@ import React from 'react'; import webpack from 'webpack'; -// Best to be consistent and use import instead of require below - Harold +// Use import instead of require below - Harold const Pet = require ('../models/pet'); let express = require('express'); let router = express.Router(); diff --git a/routes/userRoutes.js b/routes/userRoutes.js index c6449d1..4359a1d 100644 --- a/routes/userRoutes.js +++ b/routes/userRoutes.js @@ -4,7 +4,7 @@ import hash from 'password-hash'; import User from '../models/user'; import jwt from 'jsonwebtoken'; import configAuth from '../tools/configAuth'; - +// Use import below - Harold let express = require('express'); let userRoutes = express.Router(); let app = express(); diff --git a/src/components/DisplayPets.js b/src/components/DisplayPets.js index b0630b0..a81a46f 100644 --- a/src/components/DisplayPets.js +++ b/src/components/DisplayPets.js @@ -12,6 +12,7 @@ export default class DisplayPets extends React.Component { super(); this.state = { petPics: [], + // Not clear why the window.location below? - Harold species: window.location.hash.split("species=")[1].split("&")[0] }; this.loadPetsFromDb = this.loadPetsFromDb.bind(this); diff --git a/tools/configAuth.js b/tools/configAuth.js index 9c6e60f..da50771 100644 --- a/tools/configAuth.js +++ b/tools/configAuth.js @@ -1,3 +1,4 @@ +// This isn't safe - let's move this to an environment variable export default { secret: 'ilovedogs' }; diff --git a/tools/srcServer.js b/tools/srcServer.js index c50e164..49eeb76 100644 --- a/tools/srcServer.js +++ b/tools/srcServer.js @@ -12,6 +12,8 @@ import User from '../models/user'; import Pet from'../models/pet'; import petRoutes from '../routes/petRoutes'; import userRoutes from '../routes/userRoutes'; +import scrapeRunner from '../helpers/scrape'; +import syncRunner from '../helpers/sync'; /* eslint-disable no-console */ const port = process.env.PORT || 3000; @@ -41,14 +43,13 @@ mongoose.connect(mongooseUri, options); const PROD = process.env.NODE_ENV === 'production'; //Timed scrape and sync -let scrapeRunner = require('../helpers/scrape'); -let syncRunner = require('../helpers/sync'); -// + +// Not too worried - but may look better to hide in an environment variable let url = "http://ws.petango.com/Webservices/adoptablesearch/" + "wsAdoptableAnimals.aspx?sex=All&agegroup=All&colnum=" + "1&authkey=1t4v495156y98t2wd78317102f933h83or1340ptjm31spd04d"; //Call it when you npm start -// scrapeAndSync(); +scrapeAndSync(); //Call again every hour setInterval(scrapeAndSync, 3600000);