Resolved issues #56 and #57: added sourceContentType and pulling FB photos #80

pulpopurpura · 2017-08-14T15:50:07Z

Resolved git issue #56 and #57 (adding sourceContentType and addressing side effects in item.js, et al.) and grabbing FB photos --

…ls; removed contentType references from Source model; made changes to item controller and Source model to integrate new model

markmhendrickson

@pulpopurpura Please globally gitignore all .DS_Store files as well and remove them from the commit

markmhendrickson · 2017-08-14T19:04:54Z

.idea/inspectionProfiles/Project_Default.xml

@@ -0,0 +1,6 @@
+<component name="InspectionProjectProfileManager">


@pulpopurpura I'm not sure why we have this .idea directory and the corresponding files but we should remove them from the commit and I'd recommend adding them to your global gitignore so they don't get added back in automatically: https://help.github.com/articles/ignoring-files/#create-a-global-gitignore

markmhendrickson · 2017-08-14T19:05:31Z

Gruntfile.js

@@ -43,7 +43,8 @@ module.exports = function(grunt) {
    },
    nodemon: {
      dev: {
-        script: 'app/server.js'
+        script: 'app/server.js',
+


@pulpopurpura This new line should be removed since it seems extraneous

markmhendrickson

It's a bit hard to review the changes since the spaces used for tabbing have been changed in many places and therefore create block-level diffs that don't reveal the real changes easily. Please convert those back so the diff shows only substantive changes.

markmhendrickson · 2017-08-14T19:07:57Z

app/controllers/item.js

@@ -31,134 +32,141 @@ var UserSourceAuth = require('app/models/userSourceAuth');
 var UserStorageAuth = require('app/models/userStorageAuth');
 var validateParams = require('app/lib/validateParams');

+var util= require('util');


It seems we can remove this module since it's unused.

markmhendrickson · 2017-08-14T19:11:27Z

app/controllers/item.js

 var queue = kue.createQueue();

-queue.process('storeItemData', function(queueJob, done) {
-  debug('process queueJob %s', queueJob.id);
+var tools = require('app/lib/utils/debuggingTools');


requires should all take place at the start of the module in alphabetical order unless there's a particular need to place them elsewhere in the module.

markmhendrickson · 2017-08-14T19:12:08Z

app/controllers/item.js

-      }
-    });
-  };
+const STORE_ITEM_DATA = 'storeItemData';


Is there are a reason for having this be a constant?

Because it should never change as it's a string constant -- in general it's better to have these than hard-code strings, even if in a case like this it's a bit overkill as the string was used only once or twice. Just bothered me…

markmhendrickson · 2017-08-14T19:13:07Z

app/controllers/item.js

+const STORE_ITEM_DATA = 'storeItemData';
+
+queue.process(STORE_ITEM_DATA, function(queueJob, done) {
+    console.log('process queueJob %s', queueJob.id);


Please replace four-space indentation with two-space throughout all your changes since that's the standard throughout the codebase.

markmhendrickson · 2017-08-14T19:13:55Z

app/controllers/item.js

 });

 /**
 * Callback resource found at URL.
+ * (Get resource from passed URL and pass to callback (done)


Was this comment intentional? In general they shouldn't start with a parenthesis like here.

I was my notes to myself based on our conversations, trying to make the comment clearer -- I've modified it a bit, removed the ()

…ved .idea and .DS_Store from git, removed references to unused models in item controller.

…e.contentType from tests/controllers/item/itemsGetUrl.js in attempt to correct other test errors

markmhendrickson

Please fix the noted formatting issues and add the enable the following for ESLint so they're automatically detected in the future for all contributors. All should use default options unless otherwise specified here.

https://eslint.org/docs/rules/key-spacing
https://eslint.org/docs/rules/func-call-spacing
https://eslint.org/docs/rules/block-spacing
https://eslint.org/docs/rules/array-bracket-spacing
https://eslint.org/docs/rules/keyword-spacing
https://eslint.org/docs/rules/semi-spacing
https://eslint.org/docs/rules/arrow-spacing
https://eslint.org/docs/rules/no-tabs
https://eslint.org/docs/rules/indent (2 spaces)

markmhendrickson · 2017-08-18T12:05:34Z

app/models/source.js

-    itemDataObjectsFromPagePathTemplate: { type: String, default: 'data' },
-    // this is where you specifiy what you want from source
- //   itemsGetUrlTemplate: { type: String, default: 'https://${host}/${contentTypePluralCamelName}?access_token=${accessToken}&limit=${limit}&offset=${offset}' },
+  apiVersion: String,


The latest commit looks a lot cleaner than the prior (good work!) but it seems there are still indentation issues, such as here and elsewhere, where I'll note separately. Let's get these resolved too before I review the code for its functional aspects.

markmhendrickson · 2017-08-18T12:05:47Z

app/models/source.js

-    passportStrategy: String,
-    slug: String,
-    totalItemsAvailableFromPagePathTemplate: String
+  itemStorageEnabled: {type: Boolean, default: false},


Indentation issue?

markmhendrickson · 2017-08-18T12:05:54Z

app/models/source.js

-        patch: 'admin',
-        post: 'admin'
-    }
+  jsonapi: {


Indentation issue?

markmhendrickson · 2017-08-18T12:06:01Z

app/models/sourceContentType.js

-    source: { ref: 'Source',required: true },
-    contentType: { ref: 'ContentType',required: true },
-    itemsGetUrlTemplate:{ type:String, default: "https://${sourceHost}/${contentTypePluralCamelName}?access_token=${sourceToken}&limit=${sourceItemsLimit}&offset=${offset}"}
+  source: { ref: 'Source',required: true },


Indentation issue?

Also watch your spacing. There should be a single space after semi-colons and commas.

markmhendrickson · 2017-08-18T12:07:17Z

app/models/userSourceAuth.js

@@ -15,10 +15,10 @@ var queryConditions = require('./queryConditions');
 * @property {module:models/user~User} [user] - User to authenticate at source
 */
 module.exports = modelFactory.new('UserSourceAuth', {
-  source: { ref: 'Source', required: true },
+  source: {ref: 'Source', required: true},


Spacing issues

markmhendrickson · 2017-08-18T12:07:22Z

app/models/userSourceAuth.js

  sourceToken: String,
  sourceUser: String,
-  user: { ref: 'User' }
+  user: {ref: 'User'}


Spacing issues

markmhendrickson · 2017-08-18T12:08:18Z

app/models/source.js

-            contentTypePluralLowercaseName: contentType ? contentType.pluralLowercaseName() : undefined
-        });
-    },
+  totalItemsAvailableFromPagePath: function (contentType) {


Please remove spaces after "function" declarations and the start of parentheses such as here.

Sure -- but it seems to be everywhere -- in a number of files. For instance in models/jobs.js as well -- I didn't touch that. I did just run ESLint --fix though, but I don't know that that should have changed this. What would you like me to do?

The rule is func-call-spacing -- I have added that to .eslintrc.js and it doesn't seem to be flagging anything. Weird.

I believe https://eslint.org/docs/rules/func-call-spacing just applies to how functions are executed, not declared. There might be another rule that applies to declarations.

This appears to be the rule we need to use for declarations: https://eslint.org/docs/rules/space-before-function-paren

Indeed it is. Thanks --

…her changes in source files)

…nction-paren); fix errors that ESLint detected

markmhendrickson

More ESLint clean-up =)

markmhendrickson · 2017-08-22T11:31:25Z

.eslintrc.js

]
+

These extra lines should be removed. Can you check to see if there is an ESLint to check for extraneous lines inside of objects such as here? And is it possible for ESLint to check its own configuration file?

markmhendrickson · 2017-08-22T11:32:35Z

.gitignore

+node_modules
+.DS_Store


These two lines should not be in the repo's .gitignore file but rather set as global ignores because they are system-specific, not app-specific: https://help.github.com/articles/ignoring-files/#create-a-global-gitignore

markmhendrickson · 2017-08-22T11:33:04Z

Gruntfile.js

@@ -43,7 +43,7 @@ module.exports = function(grunt) {
    },
    nodemon: {
      dev: {
-        script: 'app/server.js'
+        script: 'app/server.js',


This trailing comma should be removed. Is there an ESLint rule to check automatically for trailing commas such as here?

markmhendrickson · 2017-08-22T11:33:20Z

app/controllers/item.js

@@ -31,9 +31,13 @@ var UserSourceAuth = require('app/models/userSourceAuth');
 var UserStorageAuth = require('app/models/userStorageAuth');
 var validateParams = require('app/lib/validateParams');

+


Please remove this extra line break.

markmhendrickson · 2017-08-22T11:33:40Z

app/controllers/item.js

 var queue = kue.createQueue();

-queue.process('storeItemData', function(queueJob, done) {
+
+const STORE_ITEM_DATA = 'storeItemData';


I don't see a reason for this constant. Can you explain or remove it?

markmhendrickson · 2017-08-22T12:37:24Z

app/models/source.js

@@ -5,7 +5,9 @@

 var modelFactory = require('app/factories/model');
 var nameMethods = require('./methods/name');
+// a way to interporlate strings to get a final URL


Please remove this comment since the module should be self-documenting.

markmhendrickson · 2017-08-22T12:38:56Z

app/models/source.js

 var templateCompiler = require('es6-template-strings');
+var SourceContentType = require('app/models/sourceContentType');


Please order required modules alphabetically at the top of the file as possible and integrate this rule to enforce: https://eslint.org/docs/rules/sort-imports

markmhendrickson · 2017-08-22T12:39:43Z

app/models/source.js

+  },
+
+  /**
+   * @param done {Function} A callback that will be passed a the error (Error) and optionally the sourceContentTypes results


Please provide a description for this method

markmhendrickson · 2017-08-22T12:40:33Z

app/models/source.js

+      if (err) {
+        return done(err);
+      } else {
+


I think this rule should be able to prevent these extra line breaks as well: https://eslint.org/docs/rules/padding-line-between-statements

markmhendrickson · 2017-08-22T12:41:39Z

package.json

    "supertest": "^3.0.0",
    "underscore": "^1.8.3",
    "url": "^0.11.0",
+    "util": "^0.10.3",


Is the util module not included in Node by default and therefore doesn't need explicit installation as a package?

…tiple-empty-lines, padded-blocks, padding-line-between-statements, sort-imports, comma-dangle) -- prior to "--fix"

…tiple-empty-lines, padded-blocks, padding-line-between-statements, sort-imports, comma-dangle) -- after "--fix"

markmhendrickson · 2017-08-27T16:27:41Z

.eslintrc.js

+    "func-call-spacing": [
+      "error",
+      "never"
+    ], "block-spacing": [


Let's put all value declarations on its own line.

markmhendrickson · 2017-08-27T16:32:36Z

.eslintrc.js

@@ -32,6 +32,46 @@ module.exports = {
    "semi": [
      "warn",
      "always"
+    ],
+    "key-spacing": [
+      "warn"


Let's make this error instead of warn so issues don't slip through?

remove debugTools module and factor its code to debug module

Remove additions to debug module (from previous commit)

markmhendrickson · 2017-08-27T17:02:54Z

app/controllers/item.js

-  var storeAllItems = function(done) {
-    debug.start('storeAllItems (contentTypes: %s)', source.contentTypes.length);
-    async.eachSeries(source.contentTypes, storeAllForUserStorageSourceContentType, done);
+  let getSourceContentTypesFunction = function(done) {


Let's remove "Function"

markmhendrickson · 2017-08-27T17:04:34Z

app/models/source.js

+   * returns the sourceContentTypes associated with this source
+   * @param done {Function} A callback that will be passed a the error (Error) and optionally the sourceContentTypes results
+   */
+  getSourceContentTypesForSource: function(done) {


Let's remove "ForSource"

markmhendrickson · 2017-08-27T17:05:26Z

app/models/source.js

+   * @param done {Function} A callback that will be passed a the error (Error) and optionally the sourceContentTypes results
+   */
+  getSourceContentTypesForSource: function(done) {
+    SourceContentType.find({ source: this.id }, function(err, sourceContentTypes) {


We can replace this inline function passed as a callback to "find" with simply the "done" function since the parameters match

markmhendrickson · 2017-08-27T17:08:46Z

app/models/source.js

  itemDataObjectsFromPagePathTemplate: { type: String, default: 'data' },
-  itemsGetUrlTemplate: { type: String, default: 'https://${host}/${contentTypePluralCamelName}?access_token=${accessToken}&limit=${limit}&offset=${offset}' },
+


Remove line break

markmhendrickson · 2017-08-27T17:08:56Z

app/models/sourceContentType.js

+  source: { ref: 'Source',required: true },
+  contentType: { ref: 'ContentType',required: true },
+  itemsGetUrlTemplate: { type: String, default: 'https://${sourceHost}/${contentTypePluralCamelName}?access_token=${sourceToken}&limit=${sourceItemsLimit}&offset=${offset}' }
+


Remove line break

markmhendrickson · 2017-08-27T17:10:47Z

app/models/sourceContentType.js

+ * @property {module:models/contentType~ContentType} contentType for this sourceContentType
+ */
+module.exports = modelFactory.new('SourceContentType', {
+  source: { ref: 'Source',required: true },


Alphabetical ordering

markmhendrickson · 2017-08-27T17:11:40Z

app/models/sourceContentType.js

+}, {
+  jsonapi: {
+    get: 'public',
+    post: 'public',


Let's remove the post and del values here.

various eslint fixes, move getItemURL from source.js to controllers/item.js, add sourceContentType model to fixtures/model.js

…ferences to sourceContentType to contentType

pulpopurpura added 2 commits August 14, 2017 17:36

Added new model SourceContentType to link Source and ContentType mode…

827cf71

…ls; removed contentType references from Source model; made changes to item controller and Source model to integrate new model

Corrections to acess FB photos

957c425

pulpopurpura requested a review from markmhendrickson August 14, 2017 15:50

markmhendrickson reviewed Aug 14, 2017

View reviewed changes

markmhendrickson requested changes Aug 14, 2017

View reviewed changes

pulpopurpura added 2 commits August 16, 2017 14:41

Changes as per pull request review: set indentation to 2 spaces, remo…

b30f128

…ved .idea and .DS_Store from git, removed references to unused models in item controller.

Correct ESLint errors from 'npm run test'; remove references to sourc…

2227cd7

…e.contentType from tests/controllers/item/itemsGetUrl.js in attempt to correct other test errors

markmhendrickson reviewed Aug 18, 2017

View reviewed changes

pulpopurpura added 3 commits August 21, 2017 18:10

Edit .eslintrc.js to implement new ESLint rules (prior to making furt…

5b87b61

…her changes in source files)

Edit .eslintrc.js to implement new ESLint rules (add /space-before-fu…

8f10be7

…nction-paren); fix errors that ESLint detected

Further correct ESLint errors (controllers/item.js, lib/jsonapi.js)

5fe7233

markmhendrickson requested changes Aug 22, 2017

View reviewed changes

pulpopurpura added 2 commits August 22, 2017 18:00

Further correct ESLint errors and further configure .eslintrc (no-mul…

702dac2

…tiple-empty-lines, padded-blocks, padding-line-between-statements, sort-imports, comma-dangle) -- prior to "--fix"

Further correct ESLint errors and further configure .eslintrc (no-mul…

cace2ea

…tiple-empty-lines, padded-blocks, padding-line-between-statements, sort-imports, comma-dangle) -- after "--fix"

markmhendrickson reviewed Aug 27, 2017

View reviewed changes

pulpopurpura added 2 commits August 27, 2017 18:44

Add new ESLint rules to put space inside brackets {};

2c4d9e3

remove debugTools module and factor its code to debug module

Remove .DS_Store files;

f9ea3dd

Remove additions to debug module (from previous commit)

markmhendrickson reviewed Aug 28, 2017

View reviewed changes

pulpopurpura added 3 commits August 28, 2017 14:51

Make changes as per code review:

ce8a544

various eslint fixes, move getItemURL from source.js to controllers/item.js, add sourceContentType model to fixtures/model.js

Correct errors resulting from refactoring code to reflect changing re…

44a89c6

…ferences to sourceContentType to contentType

Fix ESLint Test Errors

0b9c6c9

		@@ -0,0 +1,6 @@
		<component name="InspectionProjectProfileManager">

		@@ -31,9 +31,13 @@ var UserSourceAuth = require('app/models/userSourceAuth');
		var UserStorageAuth = require('app/models/userStorageAuth');
		var validateParams = require('app/lib/validateParams');

		var templateCompiler = require('es6-template-strings');
		var SourceContentType = require('app/models/sourceContentType');

		itemDataObjectsFromPagePathTemplate: { type: String, default: 'data' },
		itemsGetUrlTemplate: { type: String, default: 'https://${host}/${contentTypePluralCamelName}?access_token=${accessToken}&limit=${limit}&offset=${offset}' },

		node_modules
		.DS_Store

Resolved issues #56 and #57: added sourceContentType and pulling FB photos #80

Are you sure you want to change the base?

Resolved issues #56 and #57: added sourceContentType and pulling FB photos #80

Conversation

pulpopurpura commented Aug 14, 2017

markmhendrickson left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markmhendrickson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markmhendrickson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markmhendrickson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markmhendrickson left a comment •

edited

Loading