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

Frontend list works #57

Merged
merged 24 commits into from
Aug 22, 2014
Merged

Frontend list works #57

merged 24 commits into from
Aug 22, 2014

Conversation

ebalder
Copy link
Contributor

@ebalder ebalder commented Aug 11, 2014

Ready to test:
- listing and paging works (1st, prev, next)
- filtering and unfiltering by current user id
- removing works
- changing work visibility by batch
- batch selection should be only visible if user has write permission

linkMap.next = url.format(linkUrl);

if (req.query.page > 1) {
linkUrl.query.page = req.query.page - 1;
linkUrl.query.page = current - 1;
linkMap.previous = url.format(linkUrl);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"previous" page was pointing to the current page.

Copy link
Member

Choose a reason for hiding this comment

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

I don't quite get this: why would req.query.page not work? I tested req.query.page (which is forced to a reasonable number in rest.js) and it seems to work, but I might have missed some use case.

The reason I zoomed in here is that if page isn't included in the URL at all, then current becomes undefined and the paging links break. That is avoided when using req.query.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.

I assumed that it would either return error or force page to 1 if page wasn't defined. The problem here was that req.query.page was being increased by 1 and then decreased by one, when it should've been decreased by 2. I just liked more to have a "current" var

@@ -886,6 +886,7 @@ exports.listWorks = function listWorks(context, conditions, sort, skip, limit) {
}
)
.map(function(work) {
setWorkPerms(context)(work);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

needed the permissions

petli added a commit that referenced this pull request Aug 22, 2014
@petli petli merged commit d165e6c into master Aug 22, 2014
@petli petli deleted the frontend-list-works branch September 5, 2014 14:51
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.

2 participants