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

Issue894: Marks spreadsheet: sort by User Name, Last Name, First Name, Section. #928

Merged
merged 11 commits into from Jan 30, 2013

Conversation

Projects
None yet
5 participants
@kiramccoan
Contributor

kiramccoan commented Nov 15, 2012

Ready for review.

@@ -80,6 +77,9 @@ def get_filtered_items(hash, filter, sort_by, object_hash)
if !sort_by.blank?
items = items.sort{|a,b| hash[:sorts][sort_by].call(a,b)}
end
if !desc.blank?

This comment has been minimized.

@jerboaa

jerboaa Nov 27, 2012

Member

It would be better to retrieve items in the right order in the first place. This retrieves items and later reverses the list if desc is not blank. I.e. does things twice where once is more than enough.

@jerboaa

jerboaa Nov 27, 2012

Member

It would be better to retrieve items in the right order in the first place. This retrieves items and later reverses the list if desc is not blank. I.e. does things twice where once is more than enough.

@mike-stewart

This comment has been minimized.

Show comment
Hide comment
@mike-stewart

mike-stewart Jan 30, 2013

Contributor

I merged this pull request on my machine, and confirmed that the new functionality works as expected. The included functional test passes as well. The code review didn't raise any red flags for me, so I'm ready to move forward with merging this pull request.

@oussamaBA have you had a chance to take a look at this code yet?

Contributor

mike-stewart commented Jan 30, 2013

I merged this pull request on my machine, and confirmed that the new functionality works as expected. The included functional test passes as well. The code review didn't raise any red flags for me, so I'm ready to move forward with merging this pull request.

@oussamaBA have you had a chance to take a look at this code yet?

@oussamaBA

This comment has been minimized.

Show comment
Hide comment
@oussamaBA

oussamaBA Jan 30, 2013

Contributor

I fetched this branch from Kira's repo into my machine. The functionality works as expected.

Contributor

oussamaBA commented Jan 30, 2013

I fetched this branch from Kira's repo into my machine. The functionality works as expected.

@mike-stewart

This comment has been minimized.

Show comment
Hide comment
@mike-stewart

mike-stewart Jan 30, 2013

Contributor

@reidka Can you merge?

Contributor

mike-stewart commented Jan 30, 2013

@reidka Can you merge?

reidka added a commit that referenced this pull request Jan 30, 2013

Merge pull request #928 from kiramccoan/issue894
Issue894: Marks spreadsheet: sort by User Name, Last Name, First Name, Section.

@reidka reidka merged commit 5c6534a into MarkUsProject:master Jan 30, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment