simplyspoke / node-harvest

Node.js Client API for the Harvest time tracking service
http://simplyspoke.github.io/node-harvest/
MIT License
106 stars 57 forks source link

Regression with querystring params in 1.0.7 #79

Closed elskwid closed 6 years ago

elskwid commented 6 years ago

Hi @simplyspoke! Thank you for all your hard work maintaining this project.

We are using this library on a new internal project and needed to pass the of_user param when retrieving daily time tracking entries.

Our call looked like this:

harvest.timeTracking.daily({of_user: 12345}, function(err, response, entries) {
  if (err) throw new Error(err);

  console.log(entries);
});

No matter how we changed the params we were only seeing time entries for the logged in user (me). We saw the changeset for #78 and noticed the change to the arguments for assign() here.

It looks like the call to assign is merging the properties into a new object but that new object is not used in the call to request, it uses options so the new query params from task.query.qs are not making it to the new request.

node-harvest 2018-01-07 14-20-07

We reverted to 1.0.6 and our params worked fine.

jayjanssen commented 6 years ago

+1

simplyspoke commented 6 years ago

Yup, I am blind. Will update by assigning to a variable and then passing that in. Probably ship a patch up in a hours or two.

Good catch!

simplyspoke commented 6 years ago

In the process of running a proposed fix through CI tests, will then version bump and publish.

simplyspoke commented 6 years ago

Patch is up in 1.0.8.

Thanks for the help and let me know if there is anything further I can do.

jayjanssen commented 6 years ago

Hi -- testing 1.0.8 and I am getting these errors on any call (works fine with 1.0.6):

test-harvest │ Uncaught TypeError: Cannot read property 'headers' of undefined test-harvest │ at Object.getId (node_modules/harvest/lib/helpers.js:37:23) test-harvest │ at Request._callback (node_modules/harvest/lib/client.js:79:24) test-harvest │ at self.callback (node_modules/request/request.js:186:22) test-harvest │ at Request.init (node_modules/request/request.js:232:17) test-harvest │ at new Request (node_modules/request/request.js:128:8) test-harvest │ at request (node_modules/request/index.js:53:10) test-harvest │ at node_modules/harvest/lib/client.js:61:5 test-harvest │ at node_modules/harvest/node_modules/async/dist/async.js:4082:9 test-harvest │ at Object.process (node_modules/harvest/node_modules/async/dist/async.js:2330:17) test-harvest │ at node_modules/harvest/node_modules/async/dist/async.js:2238:19 test-harvest │ at Immediate._onImmediate (node_modules/harvest/node_modules/async/dist/async.js:119:16)

simplyspoke commented 6 years ago

1.0.9 should do it. Passed all tests and also cleaned up a few stale query params.

elskwid commented 6 years ago

Thank you so much @simplyspoke.