Tag backend

[zezevazquez]

Fixes # .

Overview

Add tag migration, query, route, and test"

Data Model / DB Schema Changes

Create tag migration schema and query

Environment / Configuration Changes

N/A

Notes

All tests pass!

[jaredatron]

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

[jaredatron]

_ 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'

[jaredatron]

expunge is a silly name. deleteTag?

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

[jaredatron]

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

[jaredatron]

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.

[jaredatron]

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

[jaredatron]

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

use:

describe('tag', function(){

[jaredatron]

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

[jaredatron]

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)
    })
})

[jaredatron]

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.

[jaredatron]

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)
      })
  })  
})