Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@ node_modules
npm-debug.log
src/public/dist/*
.DS_Store
./idm
./idm
.npmrc
20 changes: 20 additions & 0 deletions artifacts/abnormal-hyena.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# abnormal-hyena @hjbowers @zezemanolo
## Description
Working on Noob's Admin page(back end and front end).

## Project Specs
- [ ] [Issue #91:](https://github.com/GuildCrafts/noob/issues/91) Create CRUD for tags
- [ ] [Issue #92:](https://github.com/GuildCrafts/noob/issues/92) Add Admin page UI for 'tags' CRUD

## Quality
* Making sure all tests pass.
* Make sure all new code has tests.
* Test React components as well.

* Commit messages are concise and descriptive
* Every pull request has a description summarizing the changes made.

* Code is easily readable with descriptive variable names.
* No stray comments and console logs left in code.
* Formatting is accounted for (whitespace, etc.)
* All frontend code has basic styling
17 changes: 17 additions & 0 deletions src/database/migrations/20170306153436_tag.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@

exports.up = function(knex, Promise) {

return Promise.all([
knex.schema.createTable('tag', function(table) {
table.increments('id').primary();
table.text('names');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why plural? Each tag has a single string representing multiple names?

})
])
};

exports.down = function(knex, Promise) {

return Promise.all([
knex.schema.dropTable('tag')
])
};
21 changes: 21 additions & 0 deletions src/database/queries/tag.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import knex from '../knex'
import * as _ from './utilities'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

_ is a weird variable name. The libraries underscore.js and lowdash.js are being fun and cleaver but you shouldn't :P

If the file is called utilities I would call this variable utilities. But you could also just import the functions you need.

import { createRecord, deleteAll, findAll, findAllWhere, deleteRecord } from './utilities'


const add = attributes =>
_.createRecord( 'tag', attributes )

const deleteAll = () =>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

instead of exporting a deleteAll which you only need for testing, I would just use knex in your test code to truncate this table beforeEach test

_.deleteAll( 'tag' )

const getAll = () =>
_.findAll( 'tag' )
.orderBy('names', 'asc')

const getBy = ( column, data ) =>
_.findAllWhere( 'tag', column, data )

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shouldn't this have a .first()? I think you want this to yield a single record or null as apposed to an array of 1 record or an empty array.


const expunge = ( column, data ) =>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

expunge is a silly name. deleteTag?

export const deleteTag = (names) => 
  _.deleteRecord('tag', 'names', names)

_.deleteRecord( 'tag', column, data )


export { add, deleteAll, getAll, getBy, expunge }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think it would be easier to use these functions if they were given tag specific names addTag, deleteAllTags, getAllTags, getTagBy, deleteTag

Also you do not need this export at the end if you just put export in front of each const

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A comment on the API you've build here: If looks like your tags are just single unique strings. Why not just expose those strings and hide all other SQL stuffs.

export const addTag(tagName) => 
  _.createRecord('tags', {name: tagName})
    .then(record => record.name)

export const removeTag(tagName) =>
  _.deleteRecord('tags', 'name', tagName)

export const getAllTags() =>
  _.findAll('tags')
    .orderBy('name', 'asc')
    .map(tag => tag.name)

With this API nothing but the tag name itself is ever used or exposed. The SQL database ID is completely hidden behind this simpler api. Which I think can be used to do all the things you're doing in your tests.

33 changes: 33 additions & 0 deletions src/routes/tag.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import express from 'express'
import path from 'path'
import groupBy from 'lodash/groupBy'
import * as tag from '../database/queries/tag'
const router = express.Router()

router.get('/', function(req, res, next){
tag.getAll()
.then( results => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

.then( tags => {
  tags = groupBy(tags, tag => tag.names)
  res.json(tags)
})

const tags = groupBy(results, tag => tag.names)
res.json(tags)
})
})

router.post('/', function(req, res, next){
tag.add(req.body)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this is an async function that returns a promise. You need to handle the resolution or rejection of this promise before you respond to the HTTP request.

const newTag = {
names: req.body.names
}
tag.add(newTag)
.then(results => {
res.json(results[0])
})
})

router.delete('/:id', function(req, res, next){

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why not use the tag name in the url / route?

router.delete('/:tag', function(req, res, next){

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since Tag names are already unique we don't need the tag ids to be exposed ourside of the DB. We already have a unique id :D

const {id} = req.params
tag.expunge('id', id).then(result => {
res.json({message: 'Successfully deleted the tag.'});
})
})

export default router
41 changes: 41 additions & 0 deletions tests/queries_tests/tag_test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import chai, { expect } from 'chai'
import * as tag from '../../src/database/queries/tag'

describe('tag', () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

the mocha docs highly recommend you avoid passing describe, context, and itwith an=>` function.

use:

describe('tag', function(){


const fakeTags = [
{
names: 'Massage'
},
{
names: 'Spa'
}
]

beforeEach( () =>
Promise.all([
tag.deleteAll(),
tag.add(fakeTags)
])
)

it('should exist', () =>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would never omit the {} of an arrow function unless it was both:

  • a one-liner
  • the implicitly returned value is being used

you should put curlies here

expect(tag).to.be.a('object')
)

it('should return all tags ordered ascending by names', () =>
tag.getAll().then( tags => {
expect( fakeTags[0].names ).to.equal('Massage')
expect( fakeTags[1].names ).to.equal('Spa')
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a huge problem that happens all the time. You have accidentally made a test that doesn't test you code.

It helps to avoid this if you remeber to always make your tests fail in predictable ways. If you write a test and is passes be skeptical! Go into the code your testing and make sure intended failures cause intended tests to fail.

If you had done that you would have notice this test doesn't even look at the result of tag.getAll(). All you're doing is confirming your fakeTags array has the data you gave it.

it('should return all tags ordered ascending by names', function(){
  return tag.getAll()
    .then( tags => {
      expect( tags.length ).to.equal(fakeTags.length)
      expect( tags[0].names ).to.equal(fakeTags[0].names)
      expect( tags[1].names ).to.equal(fakeTags[0].names)
    })
})

)

it('deletes a tag by id', () =>
tag.expunge('id', 1).then( _ =>
tag.getBy('id', 1).then( deletedTag =>
expect(deletedTag).to.deep.equal([])
)
)
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

How do you know that you have a tag with ID 1? I don't think you can safely assume this.

I'd have written this like this:

context('expunge', function(){
  it('deletes a tag by id', function(){
    return tag.getAll()
      .then( tags => {
        expect(tags.length).to.equal(2)
        return tag.expunge('id', tags[0].id)
      })
      .then( _ => tag.getAll())
      .then( tags => {
        expect(tags.length).to.equal(1)
      })
  })  
})


})