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

Sort copy courses list to show own courses first #3277

Merged
merged 4 commits into from
Jan 10, 2022

Conversation

jorg-vr
Copy link
Contributor

@jorg-vr jorg-vr commented Jan 6, 2022

This pull request changes the order a user sees the course list when he wants to copy a course. Now their own courses are displayed first. Original order is kept as secondary ordering.

image

Closes #1661.
Closes #3049.

@jorg-vr jorg-vr requested a review from a team as a code owner January 6, 2022 12:31
@jorg-vr jorg-vr requested review from bmesuere and niknetniko and removed request for a team January 6, 2022 12:31
Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

In this implementation, it isn't really clear why some courses are sorted first. It would be clearer if the ones where the user is admin have an additional icon as is done in the general course listing:

image

I'm not sure about the way the sorting is done (do we already require will_paginate/array in other pages?). Is this done at the ruby side or using a query?

@jorg-vr
Copy link
Contributor Author

jorg-vr commented Jan 6, 2022

I have added the Icon
image

@bmesuere require 'will_paginate/array' is not used anywhere else in the code base
This was needed because sort_by returns an array instead of an Active Record Relation
sort_by preforms the sort in ruby, not in a database query

The query order/reorder does not support custom functions like current_user.course_admin?(course), which is why I chose sort_by
I could write a database statement with the same result, but that would be rewriting the functionality of current_user.course_admin?(course) which could lead to two differing implementations in the future

Other option is using two or three separate select statements as in #3049 (but merge them instead of duplicating the listing code as @chvp suggested)

@jorg-vr jorg-vr requested a review from bmesuere January 6, 2022 17:17
@jorg-vr
Copy link
Contributor Author

jorg-vr commented Jan 7, 2022

Updated code sort_by to a select and reject statement to move the filtering logic to the database and also reduce complexity

require 'will_paginate/array' is still needed

@jorg-vr
Copy link
Contributor Author

jorg-vr commented Jan 7, 2022

By using concat instead of + I removed the need for require 'will_paginate/array'

Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

Ok for me if it is for @chvp

We generally try to keep the PR start post up to date before merging so it reflects the state of what is being merged. This usually means updating the screenshots to the final version.

@jorg-vr jorg-vr merged commit 1ca05fd into develop Jan 10, 2022
@jorg-vr jorg-vr deleted the feature/1661-list-own-courses-first branch January 10, 2022 12:43
@chvp chvp added the enhancement A change that isn't substantial enough to be called a feature label Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A change that isn't substantial enough to be called a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy course page does not list own courses first
4 participants