From e1bdb41bcd6622a2c0dfab89eb0ea3ca0bdb717d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Mon, 20 Jul 2015 08:32:25 +0200 Subject: [PATCH] More complete specs wrt. coverage, cleanup --- app/models/repository.rb | 17 +-- app/models/repository/git.rb | 6 +- app/models/repository/subversion.rb | 6 +- ...b => delete_managed_repository_service.rb} | 10 +- .../scm/repository_factory_service.rb | 10 +- app/views/repositories/destroy_info.html.erb | 52 +++---- lib/open_project/scm/adapters/base.rb | 23 +-- lib/open_project/scm/exceptions.rb | 4 + .../create_managed_repository_service_spec.rb | 111 +++++++++++++++ .../delete_managed_repository_service_spec.rb | 121 ++++++++++++++++ .../scm/repository_factory_service_spec.rb | 133 ++++++++++++++++++ .../repositories_controller_spec.rb | 18 ++- .../repositories/create_repository_spec.rb | 66 +++++---- ...ry_spec.rb => repository_settings_spec.rb} | 58 ++++---- .../scm/adapters/git_adapter_spec.rb | 27 ++-- .../scm/adapters/subversion_adapter_spec.rb | 74 +++++++++- spec/models/changeset_spec.rb | 2 +- spec/models/repository/git_spec.rb | 2 +- spec/models/repository/subversion_spec.rb | 2 +- spec/models/user_deletion_spec.rb | 4 +- spec/support/repository_helpers.rb | 24 +++- spec/support/tempdir.rb | 37 +++++ 22 files changed, 657 insertions(+), 150 deletions(-) rename app/services/scm/{delete_repository_service.rb => delete_managed_repository_service.rb} (92%) create mode 100644 spec/app/services/scm/create_managed_repository_service_spec.rb create mode 100644 spec/app/services/scm/delete_managed_repository_service_spec.rb create mode 100644 spec/app/services/scm/repository_factory_service_spec.rb rename spec/features/repositories/{manage_repository_spec.rb => repository_settings_spec.rb} (77%) create mode 100644 spec/support/tempdir.rb diff --git a/app/models/repository.rb b/app/models/repository.rb index eea75ea9583..ef4d9ba7190 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -31,9 +31,6 @@ class Repository < ActiveRecord::Base include Redmine::Ciphering include OpenProject::Scm::ManageableRepository - class BuildFailed < StandardError - end - belongs_to :project has_many :changesets, order: "#{Changeset.table_name}.committed_on DESC, #{Changeset.table_name}.id DESC" @@ -83,7 +80,7 @@ class Repository < ActiveRecord::Base def scm @scm ||= scm_adapter.new(url, root_url, login, password, path_encoding) - self.root_url = @scm.root_url if root_url.blank? + @scm.root_url = @scm.root_url.presence || root_url @scm end @@ -271,7 +268,8 @@ class Repository < ActiveRecord::Base # # @param [Symbol] type SCM tag to determine the type this repository should be built as # - # @raise [Repository::BuildFailed] Raised when the instance could not be built + # @raise [OpenProject::Scm::RepositoryBuildError] + # Raised when the instance could not be built # given the parameters. # @raise [::NameError] Raised when the given +vendor+ could not be resolved to a class. def self.build(project, vendor, params, type) @@ -298,8 +296,9 @@ class Repository < ActiveRecord::Base klass = OpenProject::Scm::Manager.registered[vendor] if klass.nil? - raise BuildFailed.new I18n.t('repositories.errors.disabled_or_unknown_vendor', - vendor: vendor) + raise OpenProject::Scm::Exceptions::RepositoryBuildError.new( + I18n.t('repositories.errors.disabled_or_unknown_vendor', vendor: vendor) + ) else klass end @@ -337,7 +336,9 @@ class Repository < ActiveRecord::Base if service.call true else - raise BuildFailed.new service.localized_rejected_reason + raise OpenProject::Scm::Exceptions::RepositoryBuildError.new( + service.localized_rejected_reason + ) end end end diff --git a/app/models/repository/git.rb b/app/models/repository/git.rb index f0659cc8c5e..55044ca4cb8 100644 --- a/app/models/repository/git.rb +++ b/app/models/repository/git.rb @@ -37,10 +37,12 @@ class Repository::Git < Repository OpenProject::Scm::Adapters::Git end - def configure(scm_type, args) + def configure(scm_type, _args) if scm_type == MANAGED_TYPE unless manageable? - raise BuildError.new I18n.t('repositories.managed.error_not_manageable') + raise OpenProject::Scm::Exceptions::RepositoryBuildError.new( + I18n.t('repositories.managed.error_not_manageable') + ) end self.root_url = managed_repository_path diff --git a/app/models/repository/subversion.rb b/app/models/repository/subversion.rb index 20b8394293a..aa0dfe59405 100644 --- a/app/models/repository/subversion.rb +++ b/app/models/repository/subversion.rb @@ -38,10 +38,12 @@ class Repository::Subversion < Repository OpenProject::Scm::Adapters::Subversion end - def configure(scm_type, args) + def configure(scm_type, _args) if scm_type == MANAGED_TYPE unless manageable? - raise BuildError.new I18n.t('repositories.managed.error_not_manageable') + raise OpenProject::Scm::Exceptions::RepositoryBuildError.new( + I18n.t('repositories.managed.error_not_manageable') + ) end self.root_url = managed_repository_path diff --git a/app/services/scm/delete_repository_service.rb b/app/services/scm/delete_managed_repository_service.rb similarity index 92% rename from app/services/scm/delete_repository_service.rb rename to app/services/scm/delete_managed_repository_service.rb index df39c2b0a04..3b7c45a9f21 100644 --- a/app/services/scm/delete_repository_service.rb +++ b/app/services/scm/delete_managed_repository_service.rb @@ -29,7 +29,7 @@ ## # Implements the asynchronous deletion of a local repository. -Scm::DeleteRepositoryService = Struct.new :repository do +Scm::DeleteManagedRepositoryService = Struct.new :repository do ## # Checks if a given repository may be deleted # Registers an asynchronous job to delete the repository on disk. @@ -38,15 +38,17 @@ Scm::DeleteRepositoryService = Struct.new :repository do if repository.manageable? && repository.managed? # Create necessary changes to repository to mark - # it as managed by OP, but create asynchronously. + # it as managed by OP, but delete asynchronously. managed_path = repository.managed_repository_path managed_root = repository.managed_root if File.directory?(managed_path) Delayed::Job.enqueue Scm::DeleteRepositoryJob.new(managed_root, managed_path) end + + true + else + false end - repository.destroy - true end end diff --git a/app/services/scm/repository_factory_service.rb b/app/services/scm/repository_factory_service.rb index efe9b2b9242..03435e67ae0 100644 --- a/app/services/scm/repository_factory_service.rb +++ b/app/services/scm/repository_factory_service.rb @@ -43,7 +43,9 @@ Scm::RepositoryFactoryService = Struct.new :project, :params do if repository.save repository else - raise Repository::BuildFailed.new repository.errors.full_messages.join("\n") + raise OpenProject::Scm::Exceptions::RepositoryBuildError.new( + repository.errors.full_messages.join("\n") + ) end end end @@ -55,7 +57,7 @@ Scm::RepositoryFactoryService = Struct.new :project, :params do # @return [Boolean] true iff the repository was built def build_temporary build_guarded do - build_with_type(nil) + build_with_type(nil) end end @@ -67,7 +69,7 @@ Scm::RepositoryFactoryService = Struct.new :project, :params do ## # Helper to actually build the repository and return it. - # May raise +Repository::BuildFailed+ internally. + # May raise +OpenProject::Scm::Exceptions::RepositoryBuildError+ internally. # # @param [Symbol] scm_type Type to build the repository with. May be nil # during temporary build @@ -83,7 +85,7 @@ Scm::RepositoryFactoryService = Struct.new :project, :params do def build_guarded @repository = yield @repository.present? - rescue Repository::BuildFailed => e + rescue OpenProject::Scm::Exceptions::RepositoryBuildError => e @build_failed_msg = e.message nil end diff --git a/app/views/repositories/destroy_info.html.erb b/app/views/repositories/destroy_info.html.erb index d0e99fa8500..e3ddf709985 100644 --- a/app/views/repositories/destroy_info.html.erb +++ b/app/views/repositories/destroy_info.html.erb @@ -28,30 +28,32 @@ See doc/COPYRIGHT.rdoc for more details. ++#%> <%= toolbar title: l(:label_confirmation) %>
-

<%= l('repositories.destroy.title') %>

-

- <% if @repository.managed? %> - <%= l('repositories.destroy.managed_text', project_name: @project.identifier) %> -
- <%= l('repositories.destroy.managed_path_note', path: @repository.root_url) %> - <% else %> - <%= simple_format l('repositories.destroy.linked_text', - project_name: @project.identifier, url: @repository.url) %> - <% end %> -

-

- <%= link_to project_repository_path(@project), - :confirm => l(:text_are_you_sure), - :method => :delete, - :class => 'button -highlight' do %> - - <%= l(:button_delete) %> - <% end %> - <%= link_to @back_link, - :class => 'button' do %> - - <%= l(:button_cancel) %> - <% end %> -

+
+

<%= l('repositories.destroy.title') %>

+

+ <% if @repository.managed? %> + <%= l('repositories.destroy.managed_text', project_name: @project.identifier) %> +
+ <%= l('repositories.destroy.managed_path_note', path: @repository.root_url) %> + <% else %> + <%= simple_format l('repositories.destroy.linked_text', + project_name: @project.identifier, url: @repository.url) %> + <% end %> +

+

+ <%= link_to project_repository_path(@project), + :confirm => l(:text_are_you_sure), + :method => :delete, + :class => 'button -highlight' do %> + + <%= l(:button_delete) %> + <% end %> + <%= link_to @back_link, + :class => 'button' do %> + + <%= l(:button_cancel) %> + <% end %> +

+
diff --git a/lib/open_project/scm/adapters/base.rb b/lib/open_project/scm/adapters/base.rb index bb8fd5c0179..92322d6b0f1 100644 --- a/lib/open_project/scm/adapters/base.rb +++ b/lib/open_project/scm/adapters/base.rb @@ -33,19 +33,17 @@ module OpenProject module Scm module Adapters class Base + attr_accessor :url, :root_url + def initialize(url, root_url = nil) - @url = url - @root_url = root_url + self.url = url + self.root_url = root_url end def local? false end - def remote? - !local? - end - def available? check_availability! true @@ -62,14 +60,6 @@ module OpenProject self.class.name.demodulize end - def root_url - @root_url - end - - def url - @url - end - def info nil end @@ -148,11 +138,6 @@ module OpenProject path ||= '' (path[-1, 1] == '/') ? path[0..-2] : path end - - def retrieve_root_url - info = self.info - info ? info.root_url : nil - end end class Info diff --git a/lib/open_project/scm/exceptions.rb b/lib/open_project/scm/exceptions.rb index b613858c924..7ec59f738bf 100644 --- a/lib/open_project/scm/exceptions.rb +++ b/lib/open_project/scm/exceptions.rb @@ -33,6 +33,10 @@ module OpenProject class ScmError < StandardError end + # Exception marking an error in the repository build process + class RepositoryBuildError < ScmError + end + # Exception marking an error in the execution of a local command. class CommandFailed < ScmError attr_reader :program diff --git a/spec/app/services/scm/create_managed_repository_service_spec.rb b/spec/app/services/scm/create_managed_repository_service_spec.rb new file mode 100644 index 00000000000..c9508538360 --- /dev/null +++ b/spec/app/services/scm/create_managed_repository_service_spec.rb @@ -0,0 +1,111 @@ +#-- encoding: UTF-8 +#-- copyright +# OpenProject is a project management system. +# Copyright (C) 2012-2015 the OpenProject Foundation (OPF) +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See doc/COPYRIGHT.rdoc for more details. + +require 'spec_helper' + +describe Scm::CreateManagedRepositoryService do + let(:user) { FactoryGirl.build(:user) } + let(:project) { FactoryGirl.build(:project) } + + let(:repository) { FactoryGirl.build(:repository_subversion) } + subject(:service) { Scm::CreateManagedRepositoryService.new(repository) } + + let(:config) { {} } + + before do + allow(OpenProject::Configuration).to receive(:[]).and_call_original + allow(OpenProject::Configuration).to receive(:[]).with('scm').and_return(config) + end + + shared_examples 'does not create a filesystem repository' do + it 'does not create a filesystem repository' do + expect(repository.managed?).to be false + expect(service.call).to be false + end + end + + context 'with no managed configuration' do + it_behaves_like 'does not create a filesystem repository' + end + + context 'with managed repository' do + + # Must not .create a managed repository, or it will call this service itself! + let(:repository) { + repo = Repository::Subversion.new(scm_type: :managed) + repo.project = project + repo + } + + context 'but no managed config' do + it 'does not create a filesystem repository' do + expect(repository.managed?).to be true + expect(service.call).to be false + end + end + end + + context 'with managed config' do + include_context 'with tmpdir' + let(:config) { + { + Subversion: { manages: File.join(tmpdir, 'svn') }, + Git: { manages: File.join(tmpdir, 'git') } + } + } + + let(:repository) { + repo = Repository::Subversion.new(scm_type: :managed) + repo.project = project + repo.configure(:managed, nil) + repo + } + + before do + allow_any_instance_of(Scm::CreateRepositoryJob) + .to receive(:repository).and_return(repository) + end + + it 'creates the repository' do + expect(service.call).to be true + expect(File.directory?(repository.managed_repository_path)).to be true + end + + context 'with pre-existing path on filesystem' do + before do + allow(File).to receive(:directory?).and_return(true) + end + + it 'does not create the repository' do + expect(service.call).to be false + expect(service.localized_rejected_reason) + .to eq(I18n.t('repositories.errors.exists_on_filesystem')) + end + end + end +end diff --git a/spec/app/services/scm/delete_managed_repository_service_spec.rb b/spec/app/services/scm/delete_managed_repository_service_spec.rb new file mode 100644 index 00000000000..55a2b671c71 --- /dev/null +++ b/spec/app/services/scm/delete_managed_repository_service_spec.rb @@ -0,0 +1,121 @@ +#-- encoding: UTF-8 +#-- copyright +# OpenProject is a project management system. +# Copyright (C) 2012-2015 the OpenProject Foundation (OPF) +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See doc/COPYRIGHT.rdoc for more details. + +require 'spec_helper' + +describe Scm::DeleteManagedRepositoryService do + let(:user) { FactoryGirl.build(:user) } + let(:project) { FactoryGirl.build(:project) } + + let(:repository) { FactoryGirl.build(:repository_subversion) } + subject(:service) { Scm::DeleteManagedRepositoryService.new(repository) } + + let(:config) { {} } + + before do + allow(OpenProject::Configuration).to receive(:[]).and_call_original + allow(OpenProject::Configuration).to receive(:[]).with('scm').and_return(config) + end + + shared_examples 'does not delete the repository' do + it 'does not delete the repository' do + expect(repository.managed?).to be false + expect(service.call).to be false + end + end + + context 'with no managed configuration' do + it_behaves_like 'does not delete the repository' + end + + context 'with managed repository, but no config' do + let(:repository) { FactoryGirl.build(:repository_subversion, scm_type: :managed) } + + it 'does not delete the repository' do + expect(repository.managed?).to be true + expect(service.call).to be false + end + end + + context 'with managed repository and managed config' do + include_context 'with tmpdir' + let(:config) { + { + Subversion: { manages: File.join(tmpdir, 'svn') }, + Git: { manages: File.join(tmpdir, 'git') } + } + } + + # Must not .create a managed repository, or it will call this service itself! + let(:repository) { + repo = Repository::Subversion.new(scm_type: :managed) + repo.project = project + repo.configure(:managed, nil) + + repo.save! + repo + } + + before do + allow_any_instance_of(Scm::CreateRepositoryJob) + .to receive(:repository).and_return(repository) + end + + it 'deletes the repository' do + expect(File.directory?(repository.managed_repository_path)).to be true + expect(service.call).to be true + expect(File.directory?(repository.managed_repository_path)).to be false + end + + context 'and parent project' do + let(:parent) { FactoryGirl.create(:project) } + let(:project) { FactoryGirl.create(:project, parent: parent) } + let(:repo_path) { + Pathname.new(File.join(tmpdir, 'svn', parent.identifier, "#{project.identifier}.svn")) + } + + it 'deletes the parent path when empty' do + expect(service.call).to be true + path = Pathname.new(repository.managed_repository_path) + expect(path).to eq(repo_path) + + expect(path.exist?).to be false + expect(path.parent.exist?).to be false + end + + it 'keeps the parent path when not empty' do + other_repo = repo_path.parent + 'foobar' + other_repo.mkdir + + expect(service.call).to be true + expect(repo_path.exist?).to be false + expect(repo_path.parent.exist?).to be true + end + end + end +end diff --git a/spec/app/services/scm/repository_factory_service_spec.rb b/spec/app/services/scm/repository_factory_service_spec.rb new file mode 100644 index 00000000000..05b9587f5b2 --- /dev/null +++ b/spec/app/services/scm/repository_factory_service_spec.rb @@ -0,0 +1,133 @@ +#-- encoding: UTF-8 +#-- copyright +# OpenProject is a project management system. +# Copyright (C) 2012-2015 the OpenProject Foundation (OPF) +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See doc/COPYRIGHT.rdoc for more details. + +require 'spec_helper' + +describe Scm::RepositoryFactoryService do + let(:user) { FactoryGirl.build(:user) } + let(:project) { FactoryGirl.build(:project) } + + let(:enabled_scms) { ['Subversion', 'Git'] } + + let(:params_hash) { {} } + let(:params) { ActionController::Parameters.new params_hash } + + subject(:service) { Scm::RepositoryFactoryService.new(project, params) } + + before do + allow(Setting).to receive(:enabled_scm).and_return(enabled_scms) + end + + context 'with empty hash' do + it 'should not build a repository' do + expect(service.build_temporary).not_to be true + expect(service.repository).to be_nil + end + end + + context 'with valid vendor' do + let(:params_hash) { + { scm_vendor: 'Subversion' } + } + + it 'should allow temporary build repository' do + expect(service.build_temporary).to be true + expect(service.repository).not_to be_nil + end + + it 'should not allow to persist a repository' do + expect { service.build_and_save } + .to raise_error(ActionController::ParameterMissing) + + expect(service.repository).to be_nil + end + end + + context 'with invalid vendor' do + let(:params_hash) { + { scm_vendor: 'NotSubversion', scm_type: 'foo' } + } + + it 'should not allow to temporary build repository' do + expect { service.build_temporary }.not_to raise_error + + expect(service.repository).to be_nil + expect(service.build_error).to include('The SCM vendor NotSubversion is disabled') + end + + it 'should not allow to persist a repository' do + expect { service.build_temporary }.not_to raise_error + + expect(service.repository).to be_nil + expect(service.build_error).to include('The SCM vendor NotSubversion is disabled') + end + end + + context 'with vendor and type' do + let(:params_hash) { + { scm_vendor: 'Subversion', scm_type: 'existing' } + } + + it 'should not allow to persist a repository without URL' do + expect(service.build_and_save).not_to be true + + expect(service.repository).to be_nil + expect(service.build_error).to include("URL can't be blank") + end + end + + context 'with invalid hash' do + let(:params_hash) { + { + scm_vendor: 'Subversion', scm_type: 'existing', + repository: { url: '/tmp/foo.svn' } + } + } + + it 'should not allow to persist a repository URL' do + expect(service.build_and_save).not_to be true + + expect(service.repository).to be_nil + expect(service.build_error).to include("URL is invalid") + end + end + + context 'with valid hash' do + let(:params_hash) { + { + scm_vendor: 'Subversion', scm_type: 'existing', + repository: { url: 'file:///tmp/foo.svn' } + } + } + + it 'should allow to persist a repository without URL' do + expect(service.build_and_save).to be true + expect(service.repository).to be_kind_of(Repository::Subversion) + end + end +end diff --git a/spec/controllers/repositories_controller_spec.rb b/spec/controllers/repositories_controller_spec.rb index 967dd906194..9aa4504439e 100644 --- a/spec/controllers/repositories_controller_spec.rb +++ b/spec/controllers/repositories_controller_spec.rb @@ -84,6 +84,17 @@ describe RepositoriesController, type: :controller do it_behaves_like 'successful settings response' end + context 'with #destroy' do + before do + allow(repository).to receive(:destroy).and_return(true) + xhr :delete, :destroy + end + + it 'redirects to settings' do + expect(response).to redirect_to(settings_project_path(id: project.id, tab: 'repository')) + end + end + context 'with #update' do before do xhr :put, :update @@ -94,12 +105,13 @@ describe RepositoriesController, type: :controller do context 'with #create' do before do - xhr :post, :create, scm_vendor: 'Subversion', scm_type: 'local', url: 'file:///tmp/repo.svn/' + xhr :post, :create, scm_vendor: 'Subversion', + scm_type: 'local', url: 'file:///tmp/repo.svn/' end it 'renders a JS redirect' do - expect(response.body) - .to match(/window\.location = '\/projects\/(#{project.identifier}|#{project.id})\/settings\/repository'/) + path = "\/projects\/(#{project.identifier}|#{project.id})\/settings\/repository" + expect(response.body).to match(/window\.location = '#{path}'/) end end end diff --git a/spec/features/repositories/create_repository_spec.rb b/spec/features/repositories/create_repository_spec.rb index bb279164030..912ed72aee2 100644 --- a/spec/features/repositories/create_repository_spec.rb +++ b/spec/features/repositories/create_repository_spec.rb @@ -39,7 +39,8 @@ describe 'Create repository', type: :feature, js: true do let(:enabled_scms) { %w[Subversion Git] } let(:config) { nil } - let(:scm_vendor_input) { find('select[name="scm_vendor"]') } + let(:scm_vendor_input_css) { 'select[name="scm_vendor"]' } + let(:scm_vendor_input) { find(scm_vendor_input_css) } before do allow(User).to receive(:current).and_return current_user @@ -76,19 +77,26 @@ describe 'Create repository', type: :feature, js: true do end describe 'with submitted vendor form' do - let(:scm_types) { page.all('input[name="scm_type"]') } before do settings_page.visit_repository_settings - scm_vendor_input.find('option', text: vendor).select_option + find("option[value='#{vendor}']").select_option end - shared_examples "displays only the type" do |type| + shared_examples 'displays only the type' do |type| it 'should display one type, but expanded' do - expect(scm_vendor_input.value).to eq(vendor) - expect(scm_types.length).to eq(1) - expect(scm_types[0].value).to eq(type) - expect(scm_types[0][:selected]).to be_truthy - expect(scm_types[0][:disabled]).to be_falsey + # There seems to be an issue with how the + # select is accessed after the async form loading + # Thus we explitly find it here to allow some wait + # even though it is available in let + scm_vendor = find(scm_vendor_input_css) + expect(scm_vendor.value).to eq(vendor) + + page.assert_selector('input[name="scm_type"]', :count => 1) + scm_type = find('input[name="scm_type"]') + + expect(scm_type.value).to eq(type) + expect(scm_type[:selected]).to be_truthy + expect(scm_type[:disabled]).to be_falsey content = find("#toggleable-attribute-group--content-#{type}") expect(content).not_to be_nil @@ -121,7 +129,7 @@ describe 'Create repository', type: :feature, js: true do expect(content[:hidden]).to be_falsey find('input[type="radio"][value="managed"]').set(true) - content = find("#toggleable-attribute-group--content-managed") + content = find('#toggleable-attribute-group--content-managed') expect(content).not_to be_nil expect(content[:hidden]).to be_falsey end @@ -155,16 +163,15 @@ describe 'Create repository', type: :feature, js: true do it_behaves_like 'displays only the type', 'existing' context 'and managed repositories' do - Dir.mktmpdir do |dir| - let(:config) { - { Subversion: { manages: dir } } - } - it_behaves_like 'has managed and other type', 'existing' - it_behaves_like 'it can create the managed repository' - it_behaves_like 'it can create the repository of type with url', - 'existing', - 'file:///tmp/svn/foo.svn' - end + include_context 'with tmpdir' + let(:config) { + { Subversion: { manages: tmpdir } } + } + it_behaves_like 'has managed and other type', 'existing' + it_behaves_like 'it can create the managed repository' + it_behaves_like 'it can create the repository of type with url', + 'existing', + 'file:///tmp/svn/foo.svn' end end @@ -180,17 +187,16 @@ describe 'Create repository', type: :feature, js: true do end context 'and managed repositories' do - Dir.mktmpdir do |dir| - let(:config) { - { Git: { manages: dir } } - } + include_context 'with tmpdir' + let(:config) { + { Git: { manages: tmpdir } } + } - it_behaves_like 'has managed and other type', 'local' - it_behaves_like 'it can create the managed repository' - it_behaves_like 'it can create the repository of type with url', - 'local', - '/tmp/git/foo.git' - end + it_behaves_like 'has managed and other type', 'local' + it_behaves_like 'it can create the managed repository' + it_behaves_like 'it can create the repository of type with url', + 'local', + '/tmp/git/foo.git' end end end diff --git a/spec/features/repositories/manage_repository_spec.rb b/spec/features/repositories/repository_settings_spec.rb similarity index 77% rename from spec/features/repositories/manage_repository_spec.rb rename to spec/features/repositories/repository_settings_spec.rb index 2c0ce2290b6..4b105173696 100644 --- a/spec/features/repositories/manage_repository_spec.rb +++ b/spec/features/repositories/repository_settings_spec.rb @@ -29,7 +29,7 @@ require 'spec_helper' require 'features/repositories/repository_settings_page' -describe 'Manage repository', type: :feature, js: true do +describe 'Repository Settings', type: :feature, js: true do let(:current_user) { FactoryGirl.create (:admin) } let(:project) { FactoryGirl.create(:project) } let(:settings_page) { RepositorySettingsPage.new(project) } @@ -62,10 +62,11 @@ describe 'Manage repository', type: :feature, js: true do end it 'deletes the repository' do + expect(Repository.exists?(repository)).to be true find('a.icon-delete', text: I18n.t(:button_delete)).click # Confirm the notification warning - warning = type == 'managed' ? '-error' : '-warning' + warning = (type == 'managed') ? '-error' : '-warning' expect(page).to have_selector(".notification-box.#{warning}") find('a', text: I18n.t(:button_delete)).click @@ -79,6 +80,9 @@ describe 'Manage repository', type: :feature, js: true do expect(selected.text).to eq('--- Please select ---') expect(selected[:disabled]).to be_truthy expect(selected[:selected]).to be_truthy + + # Project should have no repository + expect(Repository.exists?(repository)).to be false end end @@ -95,37 +99,35 @@ describe 'Manage repository', type: :feature, js: true do it_behaves_like 'manages the repository with', 'Git', 'local' context 'managed repositories' do - Dir.mktmpdir do |dir| - let(:config) { - { - Subversion: { manages: File.join(dir, 'svn') }, - Git: { manages: File.join(dir, 'git') } - } + include_context 'with tmpdir' + let(:config) { + { + Subversion: { manages: File.join(tmpdir, 'svn') }, + Git: { manages: File.join(tmpdir, 'git') } } + } - let(:repository) { - repo = Repository.build( - project, - managed_vendor, - # Need to pass AC params here manually to simulate a regular repository build - ActionController::Parameters.new({}), - :managed - ) + let(:repository) { + repo = Repository.build( + project, + managed_vendor, + # Need to pass AC params here manually to simulate a regular repository build + ActionController::Parameters.new({}), + :managed + ) - repo.save! - repo - } + repo.save! + repo + } - context 'Subversion' do - let(:managed_vendor) { 'Subversion' } - it_behaves_like 'manages the repository', 'Subversion', 'managed' - end + context 'Subversion' do + let(:managed_vendor) { 'Subversion' } + it_behaves_like 'manages the repository', 'Subversion', 'managed' + end - context 'Git' do - let(:managed_vendor) { 'Git' } - it_behaves_like 'manages the repository', 'Git', 'managed' - end + context 'Git' do + let(:managed_vendor) { 'Git' } + it_behaves_like 'manages the repository', 'Git', 'managed' end end - end diff --git a/spec/lib/open_project/scm/adapters/git_adapter_spec.rb b/spec/lib/open_project/scm/adapters/git_adapter_spec.rb index ecede7198d3..06f0b390891 100644 --- a/spec/lib/open_project/scm/adapters/git_adapter_spec.rb +++ b/spec/lib/open_project/scm/adapters/git_adapter_spec.rb @@ -66,6 +66,8 @@ describe OpenProject::Scm::Adapters::Git do .and_return(git_string) expect(adapter.client_version).to eq(expected_version) + expect(adapter.client_available).to be true + expect(adapter.client_version_string).to eq(expected_version.join('.')) end end @@ -94,24 +96,23 @@ describe OpenProject::Scm::Adapters::Git do end describe 'empty repository' do - Dir.mktmpdir do |dir| - let(:url) { dir } + include_context 'with tmpdir' + let(:url) { tmpdir } - before do - adapter.initialize_bare_git - end + before do + adapter.initialize_bare_git + end - describe '.check_availability!' do - it 'should be marked empty' do - expect { adapter.check_availability! } - .to raise_error(OpenProject::Scm::Exceptions::ScmEmpty) - end + describe '.check_availability!' do + it 'should be marked empty' do + expect { adapter.check_availability! } + .to raise_error(OpenProject::Scm::Exceptions::ScmEmpty) end end end describe 'local repository' do - with_filesystem_repository('git', 'git') do |repo_dir| + with_git_repository do |repo_dir| let(:url) { repo_dir } it 'reads the git version' do @@ -290,10 +291,6 @@ describe OpenProject::Scm::Adapters::Git do expect(rev.author).to eq('Felix Schäfer ') end - it 'retrieves associated revisions' do - entries = adapter.entries('', '83ca5fd') - end - it 'can be retrieved by tag' do entries = adapter.entries(nil, 'tag01.annotated') expect(entries.length).to eq(3) diff --git a/spec/lib/open_project/scm/adapters/subversion_adapter_spec.rb b/spec/lib/open_project/scm/adapters/subversion_adapter_spec.rb index 1f81cf4b825..eed0e57e84e 100644 --- a/spec/lib/open_project/scm/adapters/subversion_adapter_spec.rb +++ b/spec/lib/open_project/scm/adapters/subversion_adapter_spec.rb @@ -30,9 +30,10 @@ require 'spec_helper' describe OpenProject::Scm::Adapters::Subversion do - let(:url) { '/tmp/bar.svn' } + let(:root_url) { '/tmp/bar.svn' } + let(:url) { "file://#{root_url}" } let(:config) { {} } - let(:adapter) { OpenProject::Scm::Adapters::Subversion.new url } + let(:adapter) { OpenProject::Scm::Adapters::Subversion.new url, root_url } before do allow(adapter).to receive(:config).and_return(config) @@ -57,6 +58,8 @@ describe OpenProject::Scm::Adapters::Subversion do .and_return(svn_string) expect(adapter.client_version).to eq(expected_version) + expect(adapter.client_available).to be true + expect(adapter.client_version_string).to eq(expected_version.join('.')) end end @@ -66,9 +69,71 @@ describe OpenProject::Scm::Adapters::Subversion do it_behaves_like 'correct client version', "1.6.2\r\n1.8.1\r\n1.9.1", [1, 6, 2] end + describe 'invalid repository' do + describe '.check_availability!' do + it 'should not be available' do + expect(Dir.exists?(url)).to be false + expect(adapter).not_to be_available + expect { adapter.check_availability! } + .to raise_error(OpenProject::Scm::Exceptions::ScmUnavailable) + end + + it 'should raise a meaningful error if shell output fails' do + error_string = <<-ERR.strip_heredoc + svn: E215004: Authentication failed and interactive prompting is disabled; see the --force-interactive option + svn: E215004: Unable to connect to a repository at URL 'file:///tmp/bar.svn' + svn: E215004: No more credentials or we tried too many times. + Authentication failed + ERR + + allow(adapter).to receive(:popen3) + .and_yield(StringIO.new(''), StringIO.new(error_string)) + + expect { adapter.check_availability! } + .to raise_error(OpenProject::Scm::Exceptions::ScmUnauthorized) + end + end + end + + describe 'empty repository' do + include_context 'with tmpdir' + let(:root_url) { tmpdir } + + describe '.create_empty_svn' do + context 'with valid root_url' do + it 'should create the repository' do + expect(Dir.exists?(root_url)).to be true + expect(Dir.entries(root_url).length).to eq 2 + expect { adapter.create_empty_svn }.not_to raise_error + + expect(Dir.exists?(root_url)).to be true + expect(Dir.entries(root_url).length).to be >= 5 + end + end + context 'with non-existing root_url' do + let(:root_url) { File.join(tmpdir, 'foo', 'bar') } + + it 'should fail' do + expect { adapter.create_empty_svn } + .to raise_error(OpenProject::Scm::Exceptions::CommandFailed) + + expect(Dir.exists?(root_url)).to be false + end + end + end + + describe '.check_availability!' do + it 'should be marked empty' do + adapter.create_empty_svn + expect { adapter.check_availability! } + .to raise_error(OpenProject::Scm::Exceptions::ScmEmpty) + end + end + end + describe 'local repository' do - with_filesystem_repository('subversion', 'svn') do |repo_dir| - let(:url) { "file://#{repo_dir}" } + with_subversion_repository do |repo_dir| + let(:root_url) { repo_dir } it 'reads the Subversion version' do expect(adapter.client_version.length).to be >= 3 @@ -202,6 +267,7 @@ describe OpenProject::Scm::Adapters::Subversion do expect(revisions.length).to eq(1) expect(revisions[0].identifier).to eq('11') + expect(revisions[0].format_identifier).to eq('11') paths = revisions[0].paths expect(paths.length).to eq(2) diff --git a/spec/models/changeset_spec.rb b/spec/models/changeset_spec.rb index ac30a4d1e54..bad5fc75b02 100644 --- a/spec/models/changeset_spec.rb +++ b/spec/models/changeset_spec.rb @@ -31,7 +31,7 @@ require 'spec_helper' describe Changeset, type: :model do let(:email) { 'bob@bobbit.org' } - with_created_subversion_repository do + with_virtual_subversion_repository do let(:changeset) { FactoryGirl.build(:changeset, repository: repository, diff --git a/spec/models/repository/git_spec.rb b/spec/models/repository/git_spec.rb index 53e589c0f25..9a83033102a 100644 --- a/spec/models/repository/git_spec.rb +++ b/spec/models/repository/git_spec.rb @@ -85,7 +85,7 @@ describe Repository::Git, type: :model do end describe 'with an actual repository' do - with_filesystem_repository('git', 'git') do |repo_dir| + with_git_repository do |repo_dir| let(:url) { repo_dir } let(:instance) { FactoryGirl.create(:repository_git, path_encoding: encoding, url: url) } diff --git a/spec/models/repository/subversion_spec.rb b/spec/models/repository/subversion_spec.rb index 510929c935c..d0abc058fe0 100644 --- a/spec/models/repository/subversion_spec.rb +++ b/spec/models/repository/subversion_spec.rb @@ -98,7 +98,7 @@ describe Repository::Subversion, type: :model do end describe 'with an actual repository' do - with_filesystem_repository('subversion', 'svn') do |repo_dir| + with_subversion_repository do |repo_dir| let(:url) { "file://#{repo_dir}" } let(:instance) { FactoryGirl.create(:repository_subversion, url: url) } diff --git a/spec/models/user_deletion_spec.rb b/spec/models/user_deletion_spec.rb index 7726101f001..8cfff0ed72f 100644 --- a/spec/models/user_deletion_spec.rb +++ b/spec/models/user_deletion_spec.rb @@ -397,7 +397,7 @@ describe User, 'deletion', type: :model do end describe 'WHEN the user has created a changeset' do - with_created_subversion_repository do + with_virtual_subversion_repository do let(:associated_instance) do FactoryGirl.build(:changeset, repository_id: repository.id, @@ -412,7 +412,7 @@ describe User, 'deletion', type: :model do end describe 'WHEN the user has updated a changeset' do - with_created_subversion_repository do + with_virtual_subversion_repository do let(:associated_instance) do FactoryGirl.build(:changeset, repository_id: repository.id, diff --git a/spec/support/repository_helpers.rb b/spec/support/repository_helpers.rb index f94603ed875..ab1d4bf01da 100644 --- a/spec/support/repository_helpers.rb +++ b/spec/support/repository_helpers.rb @@ -26,6 +26,14 @@ # # See doc/COPYRIGHT.rdoc for more details. #++ + +## +# Create a temporary +vendor+ repository from the stored fixture. +# Automatically extracts and destroys said repository, +# however does not provide single example isolation +# due to performance. +# As we do not write to the repository, we don't need this kind +# of isolation. def with_filesystem_repository(vendor, command = nil, &block) repo_dir = File.join(Rails.root, 'tmp', 'test', "#{vendor}_repository") fixture = File.join(Rails.root, "spec/fixtures/repositories/#{vendor}_repository.tar.gz") @@ -52,7 +60,21 @@ def with_filesystem_repository(vendor, command = nil, &block) block.call(repo_dir) end -def with_created_subversion_repository(&block) +def with_subversion_repository(&block) + with_filesystem_repository('subversion', 'svn', &block) +end + +def with_git_repository(&block) + with_filesystem_repository('git', 'git', &block) +end + +## +# Many specs required any repository to be available, +# often Filesystem adapter was used, even though +# no actual filesystem access occured. +# Instead, we wrap these repository specs in a virtual +# subversion repository which does not exist on disk. +def with_virtual_subversion_repository(&block) let(:repository) { FactoryGirl.create(:repository_subversion) } before do diff --git a/spec/support/tempdir.rb b/spec/support/tempdir.rb new file mode 100644 index 00000000000..90a63da3ed0 --- /dev/null +++ b/spec/support/tempdir.rb @@ -0,0 +1,37 @@ +#-- encoding: UTF-8 +#-- copyright +# OpenProject is a project management system. +# Copyright (C) 2012-2015 the OpenProject Foundation (OPF) +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See doc/COPYRIGHT.rdoc for more details. +#++ +shared_context 'with tmpdir' do + around do |example| + Dir.mktmpdir do |dir| + @tmpdir = dir + example.run + end + end + attr_reader :tmpdir +end