Audit approach weekly review #94
Conversation
jrobcodes
left a comment
jrobcodes
left a comment
There was a problem hiding this comment.
@phoniks looks like you're on the right path! Some comments about formatting and a lot of console.logs and comments. Please ensure you are not arbitrarily adding changes to files that do not relate to your feature (there was a file or two that had new whitespace, or whitespace removed, where it should not have been)
| table_name: { | ||
| type: Sequelize.STRING | ||
| }, | ||
| field_id: { |
There was a problem hiding this comment.
I think we were going to rename field_id and field_name?
| new_value: { | ||
| type: Sequelize.STRING | ||
| }, | ||
| field_type: { |
models/audit.js
Outdated
| module.exports = function(sequelize, DataTypes) { | ||
| var Audit = sequelize.define('Audit', { | ||
| table_name: DataTypes.STRING, | ||
| field_id: DataTypes.STRING, |
There was a problem hiding this comment.
Same comments as for migration, probably need to regenerate the model to correct the names of properties.
models/item.js
Outdated
| // user.username = 'Toni' | ||
| // }, | ||
| afterUpdate: function(item, options) { | ||
| const Audit = sequelize.import('../models/audit'); |
There was a problem hiding this comment.
We should import this at the top of this module, so that it need not be re-imported for every execution of this hook.
| parent_id: DataTypes.INTEGER, | ||
| user_id: DataTypes.INTEGER | ||
| }, | ||
|
|
There was a problem hiding this comment.
Omit unnecessary whitespace please.
|
|
||
| const AUTH_OPTIONS = { | ||
| successRedirect: '/items', | ||
| successRedirect: '/items/weekly', |
There was a problem hiding this comment.
This is a very surprising change - why does authentication change to a weekly route?
routes/items.js
Outdated
| .catch( error => response.json({ success: false, id, message: error.message })) | ||
| }) | ||
|
|
||
| // if ( request.body.hasOwnProperty('completed') ) { |
routes/items.js
Outdated
|
|
||
| Item.filterParameters( request.body ) | ||
| .then(result => { | ||
| Item.update( result, { where, individualHooks: true }) //request.body = {completed: true} |
routes/items.js
Outdated
| Item.filterParameters( request.body ) | ||
| .then(result => { | ||
| Item.update( result, { where, individualHooks: true }) //request.body = {completed: true} | ||
| .then( jsonResponse => { |
There was a problem hiding this comment.
indentation is off for the .then calls here
routes/items.js
Outdated
| // }) | ||
| // } | ||
|
|
||
| if ( request.body.hasOwnProperty('title') ){ |
There was a problem hiding this comment.
Not sure what these are for, please remove.
2 participants
Looking for feedback on the hooks approach.