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

Added category "delayed" #29

Merged
merged 2 commits into from
Apr 18, 2016
Merged

Added category "delayed" #29

merged 2 commits into from
Apr 18, 2016

Conversation

andris9
Copy link
Contributor

@andris9 andris9 commented Apr 18, 2016

This update adds new job category "Delayed" to correctly display jobs that previously were considered as stuck. Addresses #26

module.exports = function (app) {
var getDelayedModel = function(req, res){
var dfd = q.defer();
redisModel.getStatus("delayed").done(function(delayed){
Copy link

@wearhere wearhere Apr 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you flatten this pyramid by using then?

redisModel.getStatus("delayed").then(function(delayed) {
  return redisModel.getJobsInList(delayed);
}).then(function(keys) {
  return redisModel.formatKeys(keys);
}); // etc.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also need to call catch at the end no?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah or maybe we shouldn't catch here but let the routes catch and return an error response/render an error page.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it looks as it looks but this seems to be the standard way of doing things in this project. For example, compare with this file

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good impulse to maintain code style, but when it comes at the cost of correctness… I'm concerned that without catching errors, this UI will not be reliable. (This is true of other views in the project, of course.)

At the least I think we should file an issue about surfacing errors in the UI. And then I think it would be nice to start by surfacing errors in this view while you're in the code—less to remember later. What do you think?

@wearhere
Copy link

Kudos for figuring out ko etc., well done!

@ShaneK
Copy link
Owner

ShaneK commented Apr 18, 2016

Damn, I really need to rewrite this stuff... This is the definition of callback hell. I'm sorry!
Also thinking about changing the UI to use angular instead of knockout (knockout was part of the stack we used when I worked for the company I made this for), thoughts anyone? I'll open an issue about it for comments.
Also thanks for the comment on that date divisor. Interesting information.

Thanks a lot for the pull request!

@ShaneK ShaneK merged commit c061fc3 into ShaneK:master Apr 18, 2016
@bradvogel
Copy link

Thanks for merging! Mind doing a npm publish for us?

@ShaneK
Copy link
Owner

ShaneK commented Apr 18, 2016

Just did it! Sorry, had to increment the version and such first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants