diff --git a/app/controllers/packages_controller.rb b/app/controllers/packages_controller.rb index 8589acf..0a8942b 100644 --- a/app/controllers/packages_controller.rb +++ b/app/controllers/packages_controller.rb @@ -288,9 +288,8 @@ class PackagesController < ApplicationController # Return packages matching the criteria given by parameters def query_packages - # Eager load the screenshots to avoid N+1 queries to check Screenshot.accessible_by later - packages = Package.includes(:screenshots).order(visits: :desc) - + packages = Package.order(visits: :desc) + # text search if params[:search].present? logger.debug "Searching for #{params[:search]}" @@ -300,15 +299,11 @@ class PackagesController < ApplicationController case params[:show] when 'with' # Enrich the result with the screenshots readable by the current user (CanCanCan) - packages = packages.where(screenshots: Screenshot.accessible_by(current_ability, :view)) + packages = packages.with_screenshots logger.debug 'Limiting packages to those with screenshots' when 'without' packages = packages.without_screenshots logger.debug 'Limiting packages to those without screenshots' - else - packages = packages.where(screenshots: Screenshot.accessible_by(current_ability, :view)).or( - packages.without_screenshots) - end return packages diff --git a/app/controllers/welcome_controller.rb b/app/controllers/welcome_controller.rb index e53c3df..72536eb 100644 --- a/app/controllers/welcome_controller.rb +++ b/app/controllers/welcome_controller.rb @@ -1,7 +1,6 @@ class WelcomeController < ApplicationController def home - # TODO: Only approved screenshots! - @newest_upload = Screenshot.newest.accessible_by(current_ability, :view).first + @newest_upload = Screenshot.where(approved: true).first @most_popular_package = Package.with_screenshots.order(visits: :desc).first diff --git a/app/models/ability.rb b/app/models/ability.rb index 73b514a..bd6ec04 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -33,6 +33,12 @@ class Ability # https://github.com/CanCanCommunity/cancancan/wiki/Defining-Abilities if user.present? # Logged-in users + # Allow to view all public/approved screenshots + can :view, Screenshot, approved: true + # Allow to view any own uploads (even not-yet-approved) + can :view, Screenshot, user_id: user.id + can :destroy, Screenshot, user_id: user.id + if user.admin_role? can :approve, Screenshot can :destroy, Screenshot @@ -45,12 +51,6 @@ class Ability can :view, Screenshot can :destroy, Screenshot end - if user.pseudo? - # Allow to view all public/approved screenshots - can :view, Screenshot, approved: true - # Allow to view any own uploads (even not-yet-approved) - can :view, Screenshot, user_id: user.id - end else # Nobody logged in can :view, Screenshot, approved: true end diff --git a/app/models/package.rb b/app/models/package.rb index 3f8e546..d494d51 100644 --- a/app/models/package.rb +++ b/app/models/package.rb @@ -57,7 +57,7 @@ class Package < ApplicationRecord end - # Return a query of all packages that have screenshots + # Return a query of all packages that do not have screenshots def self.without_screenshots # Query for all packages who's ID does not appear in a screenshot's "package_id" field subselect = Screenshot.select(:package_id) diff --git a/app/views/packages/browse.slim b/app/views/packages/browse.slim index ebb39a4..84d8599 100644 --- a/app/views/packages/browse.slim +++ b/app/views/packages/browse.slim @@ -18,7 +18,8 @@ .cell.pkgcard data-equalizer-watch=true a.black href=package_path(name: pkg.name) .image - = small_img(pkg.screenshots.first, cls: '') + / This leads to an N+1 SQL query for each image. Ideas for optimization welcome. + = small_img(pkg.screenshots.accessible_by(current_ability, :view).first, cls: '') .text.pkgname = pkg.name .text @@ -29,7 +30,8 @@ .small-12.medium-4.cell.pkgcard a.black href=package_path(name: pkg.name) .image - = small_img(pkg.screenshots.first, cls: '') + / This leads to an N+1 SQL query for each image. Ideas for optimization welcome. + = small_img(pkg.screenshots.accessible_by(current_ability, :view).first, cls: '') .small-12.medium-8.cell h2 a href=package_path(name: pkg.name)