Change GET events endpoints

[mattlub]

UPDATE 14/8/17: READY FOR REVIEW

This has turned into a bigger PR than intended, but not too much deep stuff going on.

Includes

• [x] populate placeId field with place information for getAll and getById
• [x] sort events by date in getAll
• [x] convert controllers to use new rejectIfNull function, remove old custom schema methods
• [x] update docs

doesn't include (can come in another PR):

• [ ] change placeId field to place (?) #96
• [ ] date-related query params for events

---

after discussing, I think these need to be changed, as Mohamed is working on the nazareth-events and it is already causing problems.

relates #81 #89

notes (outdated):

• brings up need for dropCollectionsAndEnd
• cannot implement population easily for getById because of the helper functions in db Utils
• docs will need to be updated

[mattlub]

@des-des @m4v15

I updated the getAll function as so and it works nicely.

However, I cannot add .populate to the custom getByIdOrError function (defined here). I kind of understand why it shouldn't, it's no longer a mongoose query so you can't do .populate, but I'm not certain about lots of promises stuff.

Is there any solution you can see, the only ideas I have are:

• pass an options object to the getByIdOrError function, including populate options (quite messy)
• somehow construct and return a mongoose query instead of a resolved/rejected promise (difficult)

[m4v15]

@mattlub When you say you can't add it to the getByIdOrError function, do you mean you can't chain .populate after it, or you can't literally put it inside where we define the function, at the end of this line?

[mattlub]

@m4v15 can't chain it on getByIdOrError. Don't want to put it inside because the function is generic and used for all the models.

[m4v15]

Ah I see - I think passing a populate options then wouldn't be super messy eg:

const findByIdOrError = function (id, fieldsToPopulate) {
  return this.findById(id)
    .populate(fieldsToPopulate||'')
    .then(res => {
      if (res === null) {
        return Promise.reject(createCustomDbError(errMessages.GET_ID_NOT_FOUND))
      }
      return Promise.resolve(res)
    })
}

If you pass populate an empty string it won't do anything (if you pass it null it will crash)

I mean I guess this option isn't ideal, as we're running populate when we don't need to, but I think it will work

[mattlub]

@m4v15 @des-des

const findByIdOrError = function (id, options) {
  const query = this.findById(id)
  if (options && options.populate) {
    query.populate(options.populate)
  }
  return query 
    .then(res => {
      if (res === null) {
        return Promise.reject(createCustomDbError(errMessages.GET_ID_NOT_FOUND))
      }
      return Promise.resolve(res)
    })
}

not overly messy, but in the controller it's slightly inconsistent because you would have:

Event.find()
  .populate('place')
  .then(etc.)

and

Event.findByIdOrError(id, {populate: 'place'})
  .then(etc.)

thoughts?

[m4v15]

@mattlub Yeah or that, which is nicer (you could put the if on one line?) - or are you thinking this is too messy

[m4v15]

@mattlub, sorry I never saw your edited comment before. Yeah I mean I can't see another way of doing it really - the whole point of not doing a mongoose query was so we could get an error - the inconsistency is slightly annoying but not the end of the world?

Also I just added a dropCollectionsAndEnd function to tidy up your test - there might be a better way of doing it but recursion and promises was wigging me out a little and this way makes sense to me (somewhat).

[des-des]

I think better just to have a wrapper

rejectIfEmpty = dbRes => dbRes.length === 0
  ? Promise.reject(notFoundError())
  : dbRes
}

(req, res, next) => {
  rejectIfEmpty(Events.findOne({}))
    .then(event => res.json(res))
    .catch(next)
 }

Then you can handle the error in next

[des-des]

But whatever works for you

[mattlub]

@des-des @m4v15 Have done it more like @des-des 's way, I think it's better as it removes the need for the custom mongoose model methods (getByIdOrError etc.). Have a look, if you approve I will implement it for all the models.

(look at the files changed instead of the individual commits because I changed things a lot in the last commit)

[mattlub]

@des-des @m4v15 updated and ready for review