Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make job.touch() promise-based #667

Merged
merged 1 commit into from Jul 25, 2018
Merged

Make job.touch() promise-based #667

merged 1 commit into from Jul 25, 2018

Conversation

@princjef
Copy link
Contributor

princjef commented Jul 25, 2018

The job.touch() method didn't get migrated over to promises along with everything else and still expects a callback. However, when it passes that callback down to job.save(), an error is thrown because a callback is not allowed.

This managed to slip through because the test for it changed at some point to mock out the implementation of job.save() with a callback version that didn't throw when it was called with a callback.

agenda/test/job.js

Lines 352 to 364 in bd8a8e0

it('extends the lock lifetime', done => {
const lockedAt = new Date();
const job = new Job({agenda, name: 'some job', lockedAt});
job.save = function(cb) {
cb();
};
setTimeout(() => {
job.touch(() => {
expect(job.attrs.lockedAt).to.be.greaterThan(lockedAt);
done();
});
}, 2);
});

This PR makes the behavior of touch() align with the rest of the library and updates the associated test to remove the mock on save().

Side note: this seems safe to treat as a bugfix rather than a breaking change because passing a callback to touch() doesn't work with the current code.

@OmgImAlexis OmgImAlexis requested review from simison and wingsbob Jul 25, 2018
@OmgImAlexis OmgImAlexis added the bug label Jul 25, 2018
@simison

This comment has been minimized.

Copy link
Member

simison commented Jul 25, 2018

Excellent, thanks for the fix! I added this to changelog.

@simison

This comment has been minimized.

Copy link
Member

simison commented Jul 25, 2018

@princjef would you mind updating the docs before we merge? If you don't have time for that, we can also loop back to it in a separate PR.

https://github.com/agenda/agenda/blob/ff94c8a4c9bc564a0bed9eaa79de1c4fdbed0fde/README.md#touchcallback

@princjef princjef force-pushed the princjef:touch-fix branch from fc35cea to adf20ec Jul 25, 2018
@princjef

This comment has been minimized.

Copy link
Contributor Author

princjef commented Jul 25, 2018

Docs updated

Copy link
Member

simison left a comment

Great, thanks!

@simison simison merged commit 0840588 into agenda:master Jul 25, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (agenda) No manifest changes detected
@nanw1103

This comment has been minimized.

Copy link

nanw1103 commented Aug 30, 2018

Please publish an updated NPM with this fix. Otherwise the job.touch is totally broken. Thanks.

@simison

This comment has been minimized.

Copy link
Member

simison commented Aug 30, 2018

@nanw1103 Done with v2.0.1 🎉

Tom5om pushed a commit to Tom5om/agenda that referenced this pull request Sep 12, 2018
* origin/database-atlas: (47 commits)
  Changes to mongo atlas
  Fix: make `touch` promise-based (agenda#667)
  Update History.md with next version
  Update dependency sinon to v6.1.4 (agenda#663)
  Update dependency sinon to v6 (agenda#642)
  add deps:update to renovate auto labels (agenda#660)
  Update dependency mocha to v5 (agenda#640)
  Add a feature comparison table to readme (agenda#657)
  Feature/update deps (agenda#656)
  Update dependency delay to v3 (agenda#638)
  Mention promises based API in readme intro
  Update known issues in readme
  Update dependency coveralls to v3.0.2 (agenda#643)
  2.0.0
  Update changelog for v2.0.0
  Update yarn lockfile (agenda#654)
  fix(database): use db() syntax rather than pulling dbName from client
  feat(database): upgrade mongodb -> 3.1
  Update dependency sinon to v4.5.0 (agenda#636)
  Update dependency mocha to v4.1.0 (agenda#634)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.