cronofy / cronofy-node

Node wrapper for the Cronofy API
https://docs.cronofy.com/developers
MIT License
49 stars 23 forks source link

fix(readevents): fix next page handling #29

Closed AdriVanHoudt closed 7 years ago

AdriVanHoudt commented 7 years ago

Closes #28

gshutler commented 7 years ago

While I can see this would work, it doesn't feel quite right.

I'm thinking if this.urls.api + path was moved to _httpGet, _httpPost, and _httpDelete then we could have:

if (details.options.next_page) {
  return this. _httpCall('GET', details.options.next_page, details.options, details.callback);
} else {
  return this._httpGet('/v1/events', details.options, details.callback);
}

So you use _httpCall direct when you have the full URL, but _httpGet and friends are helpers so you only need to specify the path.

WDYT?

AdriVanHoudt commented 7 years ago

👍 nice suggestion Would it make sense to omit the next_page from the options so it does not end up in the entity? e.g. do return this._httpCall('GET', details.options.next_page, details.options, details.callback, ['next_page']);

gshutler commented 7 years ago

Yes, I think so. I don't think it will do any harm but as there's no point in sending it we shouldn't.

This needs the this.urls.api + path moving from _httpCall to _httpGet and friends like so:

cronofy.prototype._httpGet = function(path, options, callback, optionsToOmit){
  return this._httpCall('GET', this.urls.api + path, options, callback, optionsToOmit);
}

cronofy.prototype._httpPost = function(path, options, callback, optionsToOmit){
  return this._httpCall('POST', this.urls.api + path, options, callback, optionsToOmit);
}

cronofy.prototype._httpDelete = function(path, options, callback, optionsToOmit){
  return this._httpCall('DELETE', this.urls.api + path, options, callback, optionsToOmit);
}

/* ... */

cronofy.prototype._httpCall = function(method, path, options, callback, optionsToOmit){
  var settings = {
    method: method,
    path: path,
    headers: {
      Authorization: 'Bearer ' + options.access_token,
      'Content-Type': 'application/json'
    },
    entity: _.omit(options, optionsToOmit || ['access_token'])
  };
AdriVanHoudt commented 7 years ago

Totally missed that 👌

gshutler commented 7 years ago

Now just need to work out the best way to handle the inevitable merge conflict with #30 😄

AdriVanHoudt commented 7 years ago

I'll rebase once you merge this, no sweat 💦

gshutler commented 7 years ago

Great, thanks!