Skip to content
This repository has been archived by the owner on Jan 24, 2019. It is now read-only.

Generate thumbnail generation tails after a bulk transaction #167

Merged
merged 4 commits into from
Aug 19, 2015

Conversation

cadecairos
Copy link
Contributor

Fixes #165

This PR is missing three lines of test coverage. We need to craft specific test payloads so that these code paths can be executed.

cc/ @ashleygwilliams & @jbuck

}
var row = result.rows[0];
server.methods.pages.min([
page.project_id
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious about this - what's the reason that you want the lowest page id in a project?

Copy link
Member

Choose a reason for hiding this comment

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

Like, is the lowest page id always guaranteed to be the center?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what we determined to be "the thumbnail page" for all projects. in most cases, it's the first page made, unless they delete it.

pendingUpdates.push(result);
return pendingUpdates;
}, [])
.map(function(result) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be a forEach since you're not returning the value here, right?

@ashleygwilliams
Copy link
Contributor

i am r+ this with several conditions:

  • this bug occurred because the bulk endpoint does not use the same handlers as the normal routes the bulk endpoint contains. we need to refactor this.
  • we have a lot of logic that is difficult to test. this logic should be put in modules and unit tested IMO.
  • helper functions should be moved into lib directories to reduce the length of file.

@jbuck
Copy link
Member

jbuck commented Aug 19, 2015

@ashleygwilliams ++

@cadecairos r+

@cadecairos
Copy link
Contributor Author

@ashleygwilliams Lets file some issues around those points

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

Successfully merging this pull request may close these issues.

3 participants