From aef8be3bb17a7b294b8820d5344076a8508de7ea Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 10 May 2016 09:22:41 +0200 Subject: [PATCH 1/2] Use explicit scope to exclude projects pending deletion --- CHANGELOG | 1 + app/controllers/admin/dashboard_controller.rb | 2 +- app/controllers/admin/projects_controller.rb | 2 +- app/controllers/admin/runners_controller.rb | 4 ++-- app/controllers/dashboard/application_controller.rb | 2 +- app/controllers/dashboard/projects_controller.rb | 4 ++-- app/finders/personal_projects_finder.rb | 4 ++-- app/finders/projects_finder.rb | 2 +- app/models/project.rb | 4 +--- app/services/system_hooks_service.rb | 2 +- app/views/admin/dashboard/index.html.haml | 2 +- app/workers/project_destroy_worker.rb | 2 +- lib/api/projects.rb | 2 +- lib/gitlab/search_results.rb | 2 +- spec/models/project_spec.rb | 10 +++++----- 15 files changed, 22 insertions(+), 23 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 187011c60167..7ec9580ff94a 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -46,6 +46,7 @@ v 8.8.0 (unreleased) v 8.7.5 - Fix relative links in wiki pages. !4050 + - Excluding projects pending deletion is now down using explicit scoping v 8.7.4 - Links for Redmine issue references are generated correctly again !4048 (Benedikt Huss) diff --git a/app/controllers/admin/dashboard_controller.rb b/app/controllers/admin/dashboard_controller.rb index c491e5c75501..c44ed2c6c084 100644 --- a/app/controllers/admin/dashboard_controller.rb +++ b/app/controllers/admin/dashboard_controller.rb @@ -1,6 +1,6 @@ class Admin::DashboardController < Admin::ApplicationController def index - @projects = Project.limit(10) + @projects = Project.without_pending_delete.limit(10) @users = User.limit(10) @groups = Group.limit(10) end diff --git a/app/controllers/admin/projects_controller.rb b/app/controllers/admin/projects_controller.rb index 87986fdf8b13..d1e620142ba3 100644 --- a/app/controllers/admin/projects_controller.rb +++ b/app/controllers/admin/projects_controller.rb @@ -3,7 +3,7 @@ class Admin::ProjectsController < Admin::ApplicationController before_action :group, only: [:show, :transfer] def index - @projects = Project.all + @projects = Project.without_pending_delete @projects = @projects.in_namespace(params[:namespace_id]) if params[:namespace_id].present? @projects = @projects.where("projects.visibility_level IN (?)", params[:visibility_levels]) if params[:visibility_levels].present? @projects = @projects.with_push if params[:with_push].present? diff --git a/app/controllers/admin/runners_controller.rb b/app/controllers/admin/runners_controller.rb index a701d49b844b..2eec555c36e3 100644 --- a/app/controllers/admin/runners_controller.rb +++ b/app/controllers/admin/runners_controller.rb @@ -12,9 +12,9 @@ def show @builds = @runner.builds.order('id DESC').first(30) @projects = if params[:search].present? - ::Project.search(params[:search]) + ::Project.without_pending_delete.search(params[:search]) else - Project.all + Project.without_pending_delete end @projects = @projects.where.not(id: @runner.projects.select(:id)) if @runner.projects.any? @projects = @projects.page(params[:page]).per(30) diff --git a/app/controllers/dashboard/application_controller.rb b/app/controllers/dashboard/application_controller.rb index 9d3d1c23c28c..bf82fa069956 100644 --- a/app/controllers/dashboard/application_controller.rb +++ b/app/controllers/dashboard/application_controller.rb @@ -4,6 +4,6 @@ class Dashboard::ApplicationController < ApplicationController private def projects - @projects ||= current_user.authorized_projects.sorted_by_activity.non_archived + @projects ||= ProjectsFinder.new.execute(current_user).sorted_by_activity.non_archived end end diff --git a/app/controllers/dashboard/projects_controller.rb b/app/controllers/dashboard/projects_controller.rb index c08eb8115322..f606793defbd 100644 --- a/app/controllers/dashboard/projects_controller.rb +++ b/app/controllers/dashboard/projects_controller.rb @@ -4,7 +4,7 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController before_action :event_filter def index - @projects = current_user.authorized_projects.sorted_by_activity + @projects = current_user.authorized_projects.without_pending_delete.sorted_by_activity @projects = filter_projects(@projects) @projects = @projects.includes(:namespace) @projects = @projects.sort(@sort = params[:sort]) @@ -28,7 +28,7 @@ def index end def starred - @projects = current_user.viewable_starred_projects.sorted_by_activity + @projects = current_user.viewable_starred_projects.without_pending_delete.sorted_by_activity @projects = filter_projects(@projects) @projects = @projects.includes(:namespace, :forked_from_project, :tags) @projects = @projects.sort(@sort = params[:sort]) diff --git a/app/finders/personal_projects_finder.rb b/app/finders/personal_projects_finder.rb index 3ad4bd5f066f..2973f2be8e58 100644 --- a/app/finders/personal_projects_finder.rb +++ b/app/finders/personal_projects_finder.rb @@ -21,8 +21,8 @@ def execute(current_user = nil) def all_projects(current_user) projects = [] - projects << @user.personal_projects.visible_to_user(current_user) if current_user - projects << @user.personal_projects.public_to_user(current_user) + projects << @user.personal_projects.without_pending_delete.visible_to_user(current_user) if current_user + projects << @user.personal_projects.without_pending_delete.public_to_user(current_user) projects end diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb index 2f0a9659d15b..be30adce6654 100644 --- a/app/finders/projects_finder.rb +++ b/app/finders/projects_finder.rb @@ -11,7 +11,7 @@ def all_projects(current_user) projects = [] projects << current_user.authorized_projects if current_user - projects << Project.unscoped.public_to_user(current_user) + projects << Project.without_pending_delete.public_to_user(current_user) projects end diff --git a/app/models/project.rb b/app/models/project.rb index 418b85e028a0..ee2fd3734e52 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -163,9 +163,6 @@ def update_forks_visibility_level mount_uploader :avatar, AvatarUploader - # Scopes - default_scope { where(pending_delete: false) } - scope :sorted_by_activity, -> { reorder(last_activity_at: :desc) } scope :sorted_by_stars, -> { reorder('projects.star_count DESC') } scope :sorted_by_names, -> { joins(:namespace).reorder('namespaces.name ASC, projects.name ASC') } @@ -179,6 +176,7 @@ def update_forks_visibility_level scope :joined, ->(user) { where('namespace_id != ?', user.namespace_id) } scope :non_archived, -> { where(archived: false) } scope :for_milestones, ->(ids) { joins(:milestones).where('milestones.id' => ids).distinct } + scope :without_pending_delete, -> { where(pending_delete: false) } state_machine :import_status, initial: :none do event :import_start do diff --git a/app/services/system_hooks_service.rb b/app/services/system_hooks_service.rb index e43b5b51e5b8..b5969110cce5 100644 --- a/app/services/system_hooks_service.rb +++ b/app/services/system_hooks_service.rb @@ -91,7 +91,7 @@ def project_data(model) end def project_member_data(model) - project = model.project || Project.unscoped.find(model.source_id) + project = model.project || Project.find(model.source_id) { project_name: project.name, diff --git a/app/views/admin/dashboard/index.html.haml b/app/views/admin/dashboard/index.html.haml index 6dd2fef395df..19ae12848b82 100644 --- a/app/views/admin/dashboard/index.html.haml +++ b/app/views/admin/dashboard/index.html.haml @@ -104,7 +104,7 @@ %h4 Projects .data = link_to admin_namespaces_projects_path do - %h1= number_with_delimiter(Project.count) + %h1= number_with_delimiter(Project.without_pending_delete.count) %hr = link_to('New Project', new_project_path, class: "btn btn-new") .col-sm-4 diff --git a/app/workers/project_destroy_worker.rb b/app/workers/project_destroy_worker.rb index b51c6a266c9a..d06e44802928 100644 --- a/app/workers/project_destroy_worker.rb +++ b/app/workers/project_destroy_worker.rb @@ -5,7 +5,7 @@ class ProjectDestroyWorker def perform(project_id, user_id, params) begin - project = Project.unscoped.find(project_id) + project = Project.find(project_id) rescue ActiveRecord::RecordNotFound return end diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 9b5957726757..5e9e14ad00a4 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -56,7 +56,7 @@ def map_public_to_visibility_level(attrs) # GET /projects/all get '/all' do authenticated_as_admin! - @projects = Project.all + @projects = Project.without_pending_delete @projects = filter_projects(@projects) @projects = paginate @projects present @projects, with: Entities::ProjectWithAccess, user: current_user diff --git a/lib/gitlab/search_results.rb b/lib/gitlab/search_results.rb index f8ab2b1f09ec..31a7536b431c 100644 --- a/lib/gitlab/search_results.rb +++ b/lib/gitlab/search_results.rb @@ -8,7 +8,7 @@ class SearchResults def initialize(current_user, limit_projects, query) @current_user = current_user - @limit_projects = limit_projects || Project.all + @limit_projects = limit_projects || Project.without_pending_delete @query = Shellwords.shellescape(query) if query.present? end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index f6e5b132643b..2ba4d420400d 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -64,12 +64,12 @@ end end - describe 'default_scope' do - it 'excludes projects pending deletion from the results' do - project = create(:empty_project) - create(:empty_project, pending_delete: true) + describe 'without_pending_delete scope' do + it 'excludes projects pending deletion' do + create(:project) + create(:project, pending_delete: true) - expect(Project.all).to eq [project] + expect(Project.without_pending_delete.count).to be 1 end end -- GitLab From afcbe06d4943d1240934bc733dc037871a447c2b Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Fri, 13 May 2016 11:29:26 +0200 Subject: [PATCH 2/2] Add ProjectsFinder where applicable --- CHANGELOG | 2 +- app/controllers/admin/dashboard_controller.rb | 2 +- app/controllers/admin/projects_controller.rb | 2 +- app/controllers/admin/runners_controller.rb | 4 +-- .../dashboard/application_controller.rb | 3 +- .../dashboard/projects_controller.rb | 4 +-- app/finders/projects_finder.rb | 31 ++++++++++--------- app/views/admin/dashboard/index.html.haml | 2 +- lib/api/projects.rb | 11 ++++--- lib/gitlab/search_results.rb | 2 +- spec/finders/projects_finder_spec.rb | 8 +++++ 11 files changed, 42 insertions(+), 29 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 7ec9580ff94a..fb932c687229 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.8.0 (unreleased) + - Excluding projects pending deletion is now down using explicit scoping - Snippets tab under user profile. !4001 (Long Nguyen) - Fix error when using link to uploads in global snippets - Assign labels and milestone to target project when moving issue. !3934 (Long Nguyen) @@ -46,7 +47,6 @@ v 8.8.0 (unreleased) v 8.7.5 - Fix relative links in wiki pages. !4050 - - Excluding projects pending deletion is now down using explicit scoping v 8.7.4 - Links for Redmine issue references are generated correctly again !4048 (Benedikt Huss) diff --git a/app/controllers/admin/dashboard_controller.rb b/app/controllers/admin/dashboard_controller.rb index c44ed2c6c084..a302de55fcb2 100644 --- a/app/controllers/admin/dashboard_controller.rb +++ b/app/controllers/admin/dashboard_controller.rb @@ -1,6 +1,6 @@ class Admin::DashboardController < Admin::ApplicationController def index - @projects = Project.without_pending_delete.limit(10) + @projects = ProjectsFinder.execute(current_user, scope: all).limit(10) @users = User.limit(10) @groups = Group.limit(10) end diff --git a/app/controllers/admin/projects_controller.rb b/app/controllers/admin/projects_controller.rb index d1e620142ba3..4cf4ffc27912 100644 --- a/app/controllers/admin/projects_controller.rb +++ b/app/controllers/admin/projects_controller.rb @@ -3,7 +3,7 @@ class Admin::ProjectsController < Admin::ApplicationController before_action :group, only: [:show, :transfer] def index - @projects = Project.without_pending_delete + @projects = ProjectsFinder.execute(current_user, scope: :all) @projects = @projects.in_namespace(params[:namespace_id]) if params[:namespace_id].present? @projects = @projects.where("projects.visibility_level IN (?)", params[:visibility_levels]) if params[:visibility_levels].present? @projects = @projects.with_push if params[:with_push].present? diff --git a/app/controllers/admin/runners_controller.rb b/app/controllers/admin/runners_controller.rb index 2eec555c36e3..67be2b28636d 100644 --- a/app/controllers/admin/runners_controller.rb +++ b/app/controllers/admin/runners_controller.rb @@ -12,9 +12,9 @@ def show @builds = @runner.builds.order('id DESC').first(30) @projects = if params[:search].present? - ::Project.without_pending_delete.search(params[:search]) + ProjectsFinder.execute(current_user, scope: :all).search(params[:search]) else - Project.without_pending_delete + ProjectsFinder.execute(current_user, scope: :all) end @projects = @projects.where.not(id: @runner.projects.select(:id)) if @runner.projects.any? @projects = @projects.page(params[:page]).per(30) diff --git a/app/controllers/dashboard/application_controller.rb b/app/controllers/dashboard/application_controller.rb index bf82fa069956..5c9e0610781e 100644 --- a/app/controllers/dashboard/application_controller.rb +++ b/app/controllers/dashboard/application_controller.rb @@ -4,6 +4,7 @@ class Dashboard::ApplicationController < ApplicationController private def projects - @projects ||= ProjectsFinder.new.execute(current_user).sorted_by_activity.non_archived + @projects ||= ProjectsFinder.execute(current_user, scope: :authorized). + sorted_by_activity.non_archived end end diff --git a/app/controllers/dashboard/projects_controller.rb b/app/controllers/dashboard/projects_controller.rb index f606793defbd..6e98b330bf5a 100644 --- a/app/controllers/dashboard/projects_controller.rb +++ b/app/controllers/dashboard/projects_controller.rb @@ -4,7 +4,7 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController before_action :event_filter def index - @projects = current_user.authorized_projects.without_pending_delete.sorted_by_activity + @projects = ProjectsFinder.execute(current_user, scope: :authorized).sorted_by_activity @projects = filter_projects(@projects) @projects = @projects.includes(:namespace) @projects = @projects.sort(@sort = params[:sort]) @@ -28,7 +28,7 @@ def index end def starred - @projects = current_user.viewable_starred_projects.without_pending_delete.sorted_by_activity + @projects = ProjectsFinder.execute(current_user, scope: :viewable_starred_projects).sorted_by_activity @projects = filter_projects(@projects) @projects = @projects.includes(:namespace, :forked_from_project, :tags) @projects = @projects.sort(@sort = params[:sort]) diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb index be30adce6654..55133d0ad008 100644 --- a/app/finders/projects_finder.rb +++ b/app/finders/projects_finder.rb @@ -1,18 +1,21 @@ class ProjectsFinder < UnionFinder - def execute(current_user = nil, options = {}) - segments = all_projects(current_user) + # Used for: + # - all projects (Admin) => :all + # - projects I can access => :authorized + # - visible to me or public => :public_to_user + def self.execute(current_user = nil, scope: :public_to_user) + case scope + when :all + Project.without_pending_delete + when :authorized + return self.execute unless current_user - find_union(segments, Project) - end - - private - - def all_projects(current_user) - projects = [] - - projects << current_user.authorized_projects if current_user - projects << Project.without_pending_delete.public_to_user(current_user) - - projects + current_user.authorized_projects.without_pending_delete + when :viewable_starred_projects + current_user.viewable_starred_projects.without_pending_delete + when :public_to_user + # #public_to_user(nil) will yield public projects only + Project.without_pending_delete.public_to_user(current_user) + end end end diff --git a/app/views/admin/dashboard/index.html.haml b/app/views/admin/dashboard/index.html.haml index 19ae12848b82..fa84a52f954e 100644 --- a/app/views/admin/dashboard/index.html.haml +++ b/app/views/admin/dashboard/index.html.haml @@ -104,7 +104,7 @@ %h4 Projects .data = link_to admin_namespaces_projects_path do - %h1= number_with_delimiter(Project.without_pending_delete.count) + %h1= number_with_delimiter(ProjectsFinder.execute(current_user, scope: all).count) %hr = link_to('New Project', new_project_path, class: "btn btn-new") .col-sm-4 diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 5e9e14ad00a4..13f81e9506f9 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -22,10 +22,10 @@ def map_public_to_visibility_level(attrs) # Example Request: # GET /projects get do - @projects = current_user.authorized_projects - @projects = filter_projects(@projects) - @projects = paginate @projects - present @projects, with: Entities::ProjectWithAccess, user: current_user + projects = ProjectsFinder.execute(current_user, scope: :authorized) + projects = filter_projects(projects) + projects = paginate(projects) + present projects, with: Entities::ProjectWithAccess, user: current_user end # Get an owned projects list for authenticated user @@ -43,6 +43,7 @@ def map_public_to_visibility_level(attrs) # # Example Request: # GET /projects/starred + # TODO ZJ -- Use the ProjectsFinder here get '/starred' do @projects = current_user.viewable_starred_projects @projects = filter_projects(@projects) @@ -56,7 +57,7 @@ def map_public_to_visibility_level(attrs) # GET /projects/all get '/all' do authenticated_as_admin! - @projects = Project.without_pending_delete + @projects = ProjectsFinder.execute(current_user, scope: :all) @projects = filter_projects(@projects) @projects = paginate @projects present @projects, with: Entities::ProjectWithAccess, user: current_user diff --git a/lib/gitlab/search_results.rb b/lib/gitlab/search_results.rb index 31a7536b431c..78ac2e916962 100644 --- a/lib/gitlab/search_results.rb +++ b/lib/gitlab/search_results.rb @@ -8,7 +8,7 @@ class SearchResults def initialize(current_user, limit_projects, query) @current_user = current_user - @limit_projects = limit_projects || Project.without_pending_delete + @limit_projects = limit_projects || ProjectFinder.execute(current_user, scope: :authorized) @query = Shellwords.shellescape(query) if query.present? end diff --git a/spec/finders/projects_finder_spec.rb b/spec/finders/projects_finder_spec.rb index 0a1cc3b3df7d..d199e8787ff7 100644 --- a/spec/finders/projects_finder_spec.rb +++ b/spec/finders/projects_finder_spec.rb @@ -91,5 +91,13 @@ end end end + + describe 'as admin' do + it 'returns all projects' do + admin = create(:user, :admin) + + expect(ProjectsFinder.new.execute(admin)).to eq([shared_project, public_project, internal_project, private_project]) + end + end end end -- GitLab