Improve 'tagged_courses' view and add Wiki Expert column #5959
Conversation
- Forked the 'tagged_courses' view from the campaign programs view for customization. - Added a column to display assigned Wiki Expert(s) for each course. - Optimized query to avoid N+1 issues when retrieving Wiki Experts.
Abishekcs
changed the title
app/presenters/courses_presenter.rb
Outdated
| @@ -164,4 +165,26 @@ def wikidata_stats | |||
| def creation_date | |||
| I18n.l campaign.created_at.to_date | |||
| end | |||
|
|
|||
| # Returns the wiki expert username for the given course, if available | |||
| def assigned_wiki_expert_for_course(course_id) | |||
There was a problem hiding this comment.
I'd prefer for the new code for this not to live in CoursesPresenter. Since this is only being used for tagged courses, it could be done within the controller.
|
Overall, this looks good. Happy to merge once the new setup code is moved out of the presenter. |
|
I have made the necessary changes and moved the logic to load the wiki expert to the controller. 2024-09-21.20-36-08.mp4 |
.rubocop.yml
Outdated
| @@ -36,6 +36,7 @@ Metrics/ClassLength: | |||
| - 'app/controllers/courses_controller.rb' | |||
| - 'app/controllers/users_controller.rb' | |||
| - 'app/controllers/assignments_controller.rb' | |||
| - 'app/presenters/courses_presenter.rb' | |||
There was a problem hiding this comment.
This is no longer necessary, correct?
There was a problem hiding this comment.
This is no longer necessary, correct?
Yes, I removed it just now.
| @@ -25,7 +25,8 @@ def alerts | |||
| def programs | |||
| set_page | |||
| set_courses_and_presenter | |||
| render 'campaigns/programs' | |||
| load_wiki_experts | |||
| render 'tagged_courses/programs' | |||
There was a problem hiding this comment.
Since this template is now the one with the same naming pattern as the controller, I think an explicit 'render' might not be necessary for either articles or programs?
|
|
||
| # Loads CoursesUsers records with role 4 and filters by wiki experts, avoiding N+1 queries | ||
| def load_wiki_experts | ||
| return @wiki_experts if @wiki_experts # Avoid re-loading if already loaded |
There was a problem hiding this comment.
In what situation will @wiki_experts already be loaded?
There was a problem hiding this comment.
Actually, this wasn't required. I was using it to test something and forgot to remove it.😅
| @wiki_experts = CoursesUsers | ||
| .where(course_id: course_ids, role: 4) | ||
| .includes(:user) | ||
| .select { |course_user| wiki_experts_set.include?(course_user.user.username) } |
There was a problem hiding this comment.
I think this could be done a little bit more efficiently, by using SpecialUsers.wikipedia_experts to the user records.
There's also no need to pluck IDs for the courses; you can instead do CoursesUsers.where(course: @courses, user: SpecialUsers.wikipedia_experts, role: ROLE)
| wiki_experts_set = SpecialUsers.special_users[:wikipedia_experts]&.to_set | ||
|
|
||
| @wiki_experts = CoursesUsers | ||
| .where(course_id: course_ids, role: 4) |
There was a problem hiding this comment.
Use the CoursesUsers::ROLES constant rather than the magic number 4 here.
- Removed unnecessary view rendering for articles and programs. - Replaced inefficient load_wiki_experts logic with a more concise and performant query.
|
@ragesoss, I have made the required changes. Could you please take a look? Thank you again for your review!" 2024-09-24.12-22-45.mp4 |
#5952
What this PR does
This PR forks the
tagged_courses viewfrom the existing campaign programs view to allow for further customizations specific to tagged courses. The main feature added in this PR is a new column that displays theWiki Expert(s)assigned to each course and to avoid N+1 queries on how Wiki Experts are retrieved for each course.Addresses the
first pointof the issue regarding the addition of a Wiki Expert column in the tagged_courses view. This change is part of a larger enhancement of the tagged_courses view used by Wiki Education admin to manage courses based on tags.Screenshots
Before:
Before.mp4
After:
After.mp4