From 1dfa9c1cf249e531179889e77f0d41c635c44ba7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Wed, 24 Jul 2019 14:07:44 +0200 Subject: [PATCH 1/3] [30594] Implement separate version table for journals This ensures inserts to the journal versions are atomtic, so we can avoid locking the journals table. https://community.openproject.com/wp/30594 --- app/models/changeset.rb | 19 +- app/models/journal.rb | 25 -- app/models/journal_manager.rb | 83 ++++- app/models/journal_version.rb | 31 ++ app/services/changesets/log_time_service.rb | 3 +- ...190724093332_add_journal_versions_table.rb | 28 ++ db/migrate/tables/journals.rb | 3 +- .../lib/redmine/acts/journalized/creation.rb | 2 + .../redmine/acts/journalized/save_hooks.rb | 7 + .../reporting/spec/features/my_time_spec.rb | 4 +- spec/models/changeset_spec.rb | 323 +++++++++++++++++- spec/models/journal_version_spec.rb | 73 ++++ spec/models/repository/subversion_spec.rb | 5 + .../functional/wiki_controller_spec.rb | 1 + spec_legacy/unit/changeset_spec.rb | 279 --------------- spec_legacy/unit/repository_spec.rb | 168 --------- 16 files changed, 550 insertions(+), 504 deletions(-) create mode 100644 app/models/journal_version.rb create mode 100644 db/migrate/20190724093332_add_journal_versions_table.rb create mode 100644 spec/models/journal_version_spec.rb delete mode 100644 spec_legacy/unit/changeset_spec.rb delete mode 100644 spec_legacy/unit/repository_spec.rb diff --git a/app/models/changeset.rb b/app/models/changeset.rb index 8db4ef4b3f..829e609a5c 100644 --- a/app/models/changeset.rb +++ b/app/models/changeset.rb @@ -196,13 +196,21 @@ class Changeset < ActiveRecord::Base # i.e. a work_package that belong to the repository project, a subproject or a parent project def find_referenced_work_package_by_id(id) return nil if id.blank? + work_package = WorkPackage.includes(:project).find_by(id: id.to_i) - if work_package - unless work_package.project && (project == work_package.project || project.is_ancestor_of?(work_package.project) || project.is_descendant_of?(work_package.project)) - work_package = nil - end + + # Check that the work package is either in the same, + # a parent or child project of the given changeset + if in_ancestor_chain?(work_package, project) + work_package end - work_package + end + + def in_ancestor_chain?(work_package, project) + work_package&.project && + (project == work_package.project || + project.is_ancestor_of?(work_package.project) || + project.is_descendant_of?(work_package.project)) end def fix_work_package(work_package) @@ -227,6 +235,7 @@ class Changeset < ActiveRecord::Base unless work_package.save(validate: false) logger.warn("Work package ##{work_package.id} could not be saved by changeset #{id}: #{work_package.errors.full_messages}") if logger end + work_package end diff --git a/app/models/journal.rb b/app/models/journal.rb index 97d742e9c6..af571bb02c 100644 --- a/app/models/journal.rb +++ b/app/models/journal.rb @@ -54,31 +54,6 @@ class Journal < ActiveRecord::Base # logs like the history on issue#show scope :changing, -> { where(['version > 1']) } - # Ensure that no INSERT/UPDATE/DELETE statements as well as other code inside :with_write_lock - # is run concurrently to the code inside this block, by using database locking. - # Note for PostgreSQL: If this is called from inside a transaction, the lock will last until the - # end of that transaction. - # Note for MySQL: THis method does not currently change anything (no locking at all) - def self.with_write_lock(journable) - lock_name = - if OpenProject::Database.mysql? - # MySQL only supports a single lock - "journals.write_lock" - else - "journal.#{journable.class}.#{journable.id}" - end - - result = Journal.with_advisory_lock_result(lock_name, timeout_seconds: 60) do - yield - end - - unless result.lock_was_acquired? - raise "Failed to acquire write lock to journable #{journable.class} #{journable.id}" - end - - result.result - end - def changed_data=(changed_attributes) attributes = changed_attributes diff --git a/app/models/journal_manager.rb b/app/models/journal_manager.rb index 5524b6ef24..a99b94907e 100644 --- a/app/models/journal_manager.rb +++ b/app/models/journal_manager.rb @@ -91,13 +91,17 @@ class JournalManager end def journal_class_name(type) - "#{base_class(type).name}Journal" + "#{base_class_name(type)}Journal" end def base_class(type) type.base_class end + def base_class_name(type) + base_class(type).name + end + def create_association_data(journable, journal) create_attachment_data journable, journal if journable.respond_to? :attachments create_custom_field_data journable, journal if journable.respond_to? :custom_values @@ -225,30 +229,75 @@ class JournalManager end def self.add_journal!(journable, user = User.current, notes = '') - if journalized? journable - # Obtain a table lock to ensure consistent version numbers - Journal.with_write_lock(journable) do + return unless journalized?(journable) - # Maximum version might be nil, so use to_i here. - version = journable.journals.maximum(:version).to_i + 1 + # Ensure a version exists for this journable type + # since no version is changed here, in case of concurrency, one + # of the calls is allowed to fail + journable_type = base_class_name(journable.class) + ::JournalVersion.find_or_create_by(journable_type: journable_type, journable_id: journable.id) - journal_attributes = { journable_id: journable.id, - journable_type: journal_class_name(journable.class), - version: version, - activity_type: journable.send(:activity_type), - details: journable_details(journable) } + version = get_next_journal_version(journable_type, journable) - journal = create_journal journable, journal_attributes, user, notes + Rails.logger.debug "Inserting new journal for #{journable_type} ##{journable.id} @ #{version}" - # FIXME: this is required for the association to be correctly saved... - journable.journals.select(&:new_record?) + journal_attributes = { journable_id: journable.id, + journable_type: journal_class_name(journable.class), + version: version, + activity_type: journable.send(:activity_type), + details: journable_details(journable) } - journal.save! - journal - end + journal = create_journal journable, journal_attributes, user, notes + + # FIXME: this is required for the association to be correctly saved... + journable.journals.select(&:new_record?) + + journal.save! + journal + end + + def self.get_next_journal_version(journable_type, journable) + # Increment the version according to our DB + if OpenProject::Database.postgresql? + increment_version!(journable_type, journable.id) + else + lock_and_increment_version!(journable_type, journable.id) end end + ## + # In case of MySQL, we cannot return the value just inserted with RETURNING, + # but have to insert and select separately, which is not atomic + def self.lock_and_increment_version!(journable_type, journable_id) + result = Journal.with_advisory_lock_result('journals.write_lock', timeout_seconds: 30) do + entry = ::JournalVersion.find_by!(journable_type: journable_type, journable_id: journable_id) + entry.increment!(:version) + + entry.version + end + + unless result.lock_was_acquired? + raise "Failed to acquire write lock to journable #{journable_type} ##{journable_id}" + end + + result.result + end + + def self.increment_version!(journable_type, journable_id) + sql = <<~SQL + UPDATE #{JournalVersion.table_name} + SET version = version + 1 + WHERE journable_type = :journable_type AND :journable_id = journable_id + RETURNING version + SQL + + sanitized = ::OpenProject::SqlSanitization.sanitize(sql, journable_type: journable_type, journable_id: journable_id) + ::JournalVersion + .connection + .execute(sanitized) + .first['version'] + end + def self.create_journal(journable, journal_attributes, user = User.current, notes = '') type = base_class(journable.class) extended_journal_attributes = journal_attributes diff --git a/app/models/journal_version.rb b/app/models/journal_version.rb new file mode 100644 index 0000000000..b05359a58a --- /dev/null +++ b/app/models/journal_version.rb @@ -0,0 +1,31 @@ +#-- encoding: UTF-8 +#-- copyright +# OpenProject is a project management system. +# Copyright (C) 2012-2018 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-2017 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 docs/COPYRIGHT.rdoc for more details. +#++ + +class JournalVersion < ActiveRecord::Base +end diff --git a/app/services/changesets/log_time_service.rb b/app/services/changesets/log_time_service.rb index da01286167..63d5a1b1bc 100644 --- a/app/services/changesets/log_time_service.rb +++ b/app/services/changesets/log_time_service.rb @@ -67,7 +67,8 @@ module Changesets def log_error(service_result) unless service_result.success? - logger&.warn("TimeEntry could not be created by changeset #{id}: #{service_result.errors.full_messages}") + errors = service_result.errors.full_messages.join(", ") + Rails.logger.warn("TimeEntry could not be created by changeset #{changeset.id}: #{errors}") end end diff --git a/db/migrate/20190724093332_add_journal_versions_table.rb b/db/migrate/20190724093332_add_journal_versions_table.rb new file mode 100644 index 0000000000..990a78f166 --- /dev/null +++ b/db/migrate/20190724093332_add_journal_versions_table.rb @@ -0,0 +1,28 @@ +require_relative './migration_utils/utils' + +class AddJournalVersionsTable < ActiveRecord::Migration[5.2] + include ::Migration::Utils + + def up + create_table :journal_versions do |t| + t.string :journable_type + t.integer :journable_id + t.integer :version, default: 0 + t.index %i[journable_type journable_id version], + name: 'unique_journal_version', + unique: true + end + + ActiveRecord::Base.connection.execute <<-SQL + INSERT INTO journal_versions (journable_type, journable_id, version) + (SELECT + journable_type, journable_id, MAX(version) + FROM journals + GROUP BY journable_type, journable_id); + SQL + end + + def down + drop_table :journal_versions + end +end diff --git a/db/migrate/tables/journals.rb b/db/migrate/tables/journals.rb index 41cb708d4c..512b9ab414 100644 --- a/db/migrate/tables/journals.rb +++ b/db/migrate/tables/journals.rb @@ -45,7 +45,8 @@ class Tables::Journals < Tables::Base t.index :journable_type t.index :user_id t.index :activity_type - t.index %i[journable_type journable_id version], unique: true + t.index %i[journable_type journable_id version], + unique: true end end end diff --git a/lib/plugins/acts_as_journalized/lib/redmine/acts/journalized/creation.rb b/lib/plugins/acts_as_journalized/lib/redmine/acts/journalized/creation.rb index 7c7145f5e2..9bed0d1b66 100644 --- a/lib/plugins/acts_as_journalized/lib/redmine/acts/journalized/creation.rb +++ b/lib/plugins/acts_as_journalized/lib/redmine/acts/journalized/creation.rb @@ -156,6 +156,8 @@ module Redmine::Acts::Journalized new_journal.user_id = user.id end + # Ensure journal version exists + ::JournalVersion.find_or_create_by(journable_type: self.class.name, journable_id: id, version: 1) JournalManager.recreate_initial_journal self.class, new_journal, changed_data # Backdate journal diff --git a/lib/plugins/acts_as_journalized/lib/redmine/acts/journalized/save_hooks.rb b/lib/plugins/acts_as_journalized/lib/redmine/acts/journalized/save_hooks.rb index 09adea4876..8698df3018 100644 --- a/lib/plugins/acts_as_journalized/lib/redmine/acts/journalized/save_hooks.rb +++ b/lib/plugins/acts_as_journalized/lib/redmine/acts/journalized/save_hooks.rb @@ -56,11 +56,18 @@ module Redmine::Acts::Journalized base.class_eval do after_save :save_journals + after_destroy :remove_journal_version attr_accessor :journal_notes, :journal_user, :extra_journal_attributes end end + def remove_journal_version + ::JournalVersion + .where(journable_type: self.class.name, journable_id: id) + .delete_all + end + def save_journals @journal_user ||= User.current @journal_notes ||= '' diff --git a/modules/reporting/spec/features/my_time_spec.rb b/modules/reporting/spec/features/my_time_spec.rb index b61d38e7ff..5986d438ef 100644 --- a/modules/reporting/spec/features/my_time_spec.rb +++ b/modules/reporting/spec/features/my_time_spec.rb @@ -25,7 +25,7 @@ describe 'Cost report showing my own times', type: :feature, js: true do context 'as user with logged time' do let(:current_user) { user } it 'shows my time' do - expect(page).to have_selector('.report', text: '10.00') + expect(page).to have_selector('.report', text: '10.0') end end @@ -37,4 +37,4 @@ describe 'Cost report showing my own times', type: :feature, js: true do expect(page).not_to have_text '10.00' # 1 EUR x 10 end end -end \ No newline at end of file +end diff --git a/spec/models/changeset_spec.rb b/spec/models/changeset_spec.rb index 9f38c9cddf..a0f28d6bfd 100644 --- a/spec/models/changeset_spec.rb +++ b/spec/models/changeset_spec.rb @@ -34,10 +34,10 @@ describe Changeset, type: :model do with_virtual_subversion_repository do let(:changeset) { FactoryBot.build(:changeset, - repository: repository, - revision: '1', - committer: email, - comments: 'Initial commit') + repository: repository, + revision: '1', + committer: email, + comments: 'Initial commit') } end @@ -57,9 +57,318 @@ describe Changeset, type: :model do end end + describe 'empty comment' do + it 'should comments empty' do + changeset.comments = '' + expect(changeset.save).to eq true + expect(changeset.comments).to eq '' + + if changeset.comments.respond_to?(:force_encoding) + assert_equal 'UTF-8', changeset.comments.encoding.to_s + end + end + + it 'should comments nil' do + changeset.comments = nil + expect(changeset.save).to eq true + expect(changeset.comments).to eq '' + + if changeset.comments.respond_to?(:force_encoding) + assert_equal 'UTF-8', changeset.comments.encoding.to_s + end + end + end + + describe 'stripping commit' do + let(:comment) { 'This is a looooooooooooooong comment' + (' ' * 80 + "\n") * 5 } + with_virtual_subversion_repository do + let(:changeset) { + FactoryBot.build(:changeset, + repository: repository, + revision: '1', + committer: email, + comments: comment) + } + end + + it 'should for changeset comments strip' do + expect(changeset.save).to eq true + expect(comment).to_not eq changeset.comments + expect(changeset.comments).to eq 'This is a looooooooooooooong comment' + end + end + + describe 'mapping' do + let!(:user) { FactoryBot.create :user, login: 'jsmith', mail: 'jsmith@somenet.foo' } + let!(:repository) { FactoryBot.create(:repository_subversion) } + + it 'should manual user mapping' do + c = Changeset.create! repository: repository, + committer: 'foo', + committed_on: Time.now, + revision: 100, + comments: 'Committed by foo.' + + expect(c.user).to be_nil + repository.committer_ids = { 'foo' => user.id } + expect(c.reload.user).to eq user + + # committer is now mapped + c = Changeset.create! repository: repository, + committer: 'foo', + committed_on: Time.now, + revision: 101, + comments: 'Another commit by foo.' + expect(c.user).to eq user + end + + it 'should auto user mapping by username' do + c = Changeset.create! repository: repository, + committer: 'jsmith', + committed_on: Time.now, + revision: 100, + comments: 'Committed by john.' + expect(c.user).to eq user + end + + it 'should auto user mapping by email' do + c = Changeset.create! repository: repository, + committer: 'john ', + committed_on: Time.now, + revision: 100, + comments: 'Committed by john.' + + expect(c.user).to eq user + end + end + + describe '#scan_comment_for_work_package_ids', + with_settings: { + commit_fix_done_ratio: '90', + commit_ref_keywords: 'refs , references, IssueID', + commit_fix_keywords: 'fixes , closes', + default_language: 'en' + } do + let!(:user) { FactoryBot.create :admin, login: 'dlopper' } + let!(:open_status) { FactoryBot.create :status } + let!(:closed_status) { FactoryBot.create :closed_status } + + let!(:other_work_package) { FactoryBot.create :work_package, status: open_status } + let(:comments) { "Some fix made, fixes ##{work_package.id} and fixes ##{other_work_package.id}" } + + with_virtual_subversion_repository do + let!(:work_package) { FactoryBot.create :work_package, project: repository.project, status: open_status } + let(:changeset) { + FactoryBot.create(:changeset, + repository: repository, + revision: '123', + committer: user.login, + comments: comments) + } + end + + before do + # choosing a status to apply to fix issues + allow(Setting).to receive(:commit_fix_status_id).and_return closed_status.id + end + + describe 'with any matching', with_settings: { commit_ref_keywords: '*' } do + describe 'reference with brackets' do + let(:comments) { "[##{work_package.id}] Worked on this work_package" } + + it 'should reference' do + changeset.scan_comment_for_work_package_ids + work_package.reload + + expect(work_package.changesets).to eq [changeset] + end + end + + describe 'reference at line start' do + let(:comments) { "##{work_package.id} Worked on this work_package" } + + it 'should reference' do + changeset.scan_comment_for_work_package_ids + work_package.reload + + expect(work_package.changesets).to eq [changeset] + end + end + end + + describe 'non matching ref' do + let(:comments) { "Some fix ignores ##{work_package.id}" } + it 'should reference the work package id' do + changeset.scan_comment_for_work_package_ids + work_package.reload + + expect(work_package.changesets).to eq [] + end + end + + describe 'with timelogs' do + let!(:activity) { FactoryBot.create :activity, is_default: true } + before do + repository.project.enabled_module_names += ['time_tracking'] + repository.project.save! + end + + it 'should ref keywords any with timelog' do + allow(Setting).to receive(:commit_ref_keywords).and_return '*' + allow(Setting).to receive(:commit_logtime_enabled?).and_return true + + { + '2' => 2.0, + '2h' => 2.0, + '2hours' => 2.0, + '15m' => 0.25, + '15min' => 0.25, + '3h15' => 3.25, + '3h15m' => 3.25, + '3h15min' => 3.25, + '3:15' => 3.25, + '3.25' => 3.25, + '3.25h' => 3.25, + '3,25' => 3.25, + '3,25h' => 3.25 + }.each do |syntax, expected_hours| + c = Changeset.new repository: repository, + committed_on: 24.hours.ago, + comments: "Worked on this work_package ##{work_package.id} @#{syntax}", + revision: '520', + user: user + + expect { c.scan_comment_for_work_package_ids } + .to change { TimeEntry.count }.by(1) + + expect(c.work_package_ids).to eq [work_package.id] + + time = TimeEntry.order(Arel.sql('id DESC')).first + assert_equal work_package.id, time.work_package_id + assert_equal work_package.project_id, time.project_id + assert_equal user.id, time.user_id + + expect(time.hours).to eq expected_hours + expect(time.spent_on).to eq Date.yesterday + + expect(time.activity.is_default).to eq true + expect(time.comments).to include 'r520' + end + end + + context 'with a second work package' do + let!(:work_package2) { FactoryBot.create :work_package, project: repository.project, status: open_status } + it 'should ref keywords closing with timelog' do + allow(Setting).to receive(:commit_fix_status_id).and_return closed_status.id + allow(Setting).to receive(:commit_ref_keywords).and_return '*' + allow(Setting).to receive(:commit_fix_keywords).and_return 'fixes , closes' + allow(Setting).to receive(:commit_logtime_enabled?).and_return true + + c = Changeset.new repository: repository, + committed_on: Time.now, + comments: "This is a comment. Fixes ##{work_package.id} @4.5, ##{work_package2.id} @1", + revision: '520', + user: user + + expect { c.scan_comment_for_work_package_ids } + .to change { TimeEntry.count }.by(2) + + expect(c.work_package_ids).to match_array [work_package.id, work_package2.id] + + work_package.reload + work_package2.reload + expect(work_package).to be_closed + expect(work_package2).to be_closed + + times = TimeEntry.order(Arel.sql('id desc')).limit(2) + expect(times.map(&:work_package_id)).to match_array [work_package.id, work_package2.id] + end + end + end + + it 'should reference the work package id' do + # make sure work package 1 is not already closed + expect(work_package.status.is_closed?).to eq false + + changeset.scan_comment_for_work_package_ids + work_package.reload + + expect(work_package.changesets).to eq [changeset] + + expect(work_package.status).to eq closed_status + expect(work_package.done_ratio).to eq 90 + + # issue change + journal = work_package.journals.last + + expect(journal.user).to eq user + expect(journal.notes).to eq 'Applied in changeset r123.' + + # Expect other work package to be unchanged + # due to other project + other_work_package.reload + expect(other_work_package.changesets).to eq [] + end + + describe 'with work package in parent project' do + let(:parent) { FactoryBot.create :project } + let!(:work_package) { FactoryBot.create :work_package, project: parent, status: open_status } + + before do + repository.project.parent = parent + repository.project.save! + end + + it 'can reference it' do + # make sure work package 1 is not already closed + expect(work_package.status.is_closed?).to eq false + + changeset.scan_comment_for_work_package_ids + work_package.reload + + expect(work_package.changesets).to eq [changeset] + + # Expect other work package to be unchanged + # due to other project + other_work_package.reload + expect(other_work_package.changesets).to eq [] + end + end + + describe 'with work package in sub project' do + let(:sub) { FactoryBot.create :project } + let!(:work_package) { FactoryBot.create :work_package, project: sub, status: open_status } + + before do + sub.parent = repository.project + sub.save! + + repository.project.reload + sub.reload + end + + it 'can reference it' do + # make sure work package 1 is not already closed + expect(work_package.status.is_closed?).to eq false + + changeset.scan_comment_for_work_package_ids + work_package.reload + + expect(work_package.changesets).to eq [changeset] + + # Expect other work package to be unchanged + # due to other project + other_work_package.reload + expect(other_work_package.changesets).to eq [] + end + end + end + describe 'assign_openproject user' do describe 'w/o user' do - before do changeset.save! end + before do + changeset.save! + end it_behaves_like 'valid changeset' do let(:journal_user) { User.anonymous } @@ -69,7 +378,9 @@ describe Changeset, type: :model do describe 'with user is committer' do let!(:committer) { FactoryBot.create(:user, login: email) } - before do changeset.save! end + before do + changeset.save! + end it_behaves_like 'valid changeset' do let(:journal_user) { committer } diff --git a/spec/models/journal_version_spec.rb b/spec/models/journal_version_spec.rb new file mode 100644 index 0000000000..c715a7c83b --- /dev/null +++ b/spec/models/journal_version_spec.rb @@ -0,0 +1,73 @@ +#-- copyright +# OpenProject is a project management system. +# Copyright (C) 2012-2018 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-2017 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 docs/COPYRIGHT.rdoc for more details. +#++ + +require 'spec_helper' + +describe JournalVersion, type: :model do + let!(:work_package) do + wp = FactoryBot.build(:work_package) + wp.journal_notes = 'foobar!' + + wp.save! + wp + end + + subject { ::JournalVersion.find_by!(journable_type: 'WorkPackage', id: work_package.id) } + + before do + work_package + subject + end + + it 'is created when the work package is created' do + expect(subject.version).to eq 1 + end + + it 'is incremented when the work package is journaled' do + work_package.subject = 'Foobar!' + work_package.journal_notes = 'My comment' + work_package.save! + + work_package.reload + + expect(work_package.journals.count).to eq 2 + expect(work_package.journals.first.version).to eq 1 + expect(work_package.journals.last.version).to eq 2 + + subject.reload + expect(subject.version).to eq 2 + end + + it 'is removed when the work package is removed' do + expect(subject).to be_present + + work_package.destroy! + + expect { subject.reload }.to raise_error ActiveRecord::RecordNotFound + end +end diff --git a/spec/models/repository/subversion_spec.rb b/spec/models/repository/subversion_spec.rb index f61a7f1b09..014c414414 100644 --- a/spec/models/repository/subversion_spec.rb +++ b/spec/models/repository/subversion_spec.rb @@ -46,6 +46,11 @@ describe Repository::Subversion, type: :model do it 'does not allow creating a repository' do expect { instance.save! }.to raise_error ActiveRecord::RecordInvalid end + + it 'returns an error when trying to save' do + expect(instance.save).to eq false + expect(instance.errors[:type]).to include I18n.translate('activerecord.errors.models.repository.not_available') + end end describe 'default Subversion' do diff --git a/spec_legacy/functional/wiki_controller_spec.rb b/spec_legacy/functional/wiki_controller_spec.rb index abbe47fb21..7571e04b4a 100644 --- a/spec_legacy/functional/wiki_controller_spec.rb +++ b/spec_legacy/functional/wiki_controller_spec.rb @@ -176,6 +176,7 @@ describe WikiController, type: :controller do # because running whole suite is fine, but running only this test # results in failure it 'should update stale page should not raise an error' do + ::JournalVersion.create!(journable_type: 'WikiContent', journable_id: 2, version: 1) journal = FactoryBot.create :wiki_content_journal, journable_id: 2, version: 1, diff --git a/spec_legacy/unit/changeset_spec.rb b/spec_legacy/unit/changeset_spec.rb deleted file mode 100644 index a5d96170c0..0000000000 --- a/spec_legacy/unit/changeset_spec.rb +++ /dev/null @@ -1,279 +0,0 @@ -#-- encoding: UTF-8 -#-- copyright -# OpenProject is a project management system. -# Copyright (C) 2012-2018 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-2017 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 docs/COPYRIGHT.rdoc for more details. -#++ -require_relative '../legacy_spec_helper' - -describe Changeset, type: :model do - fixtures :all - - context 'with notified events', with_settings: { notified_events: %w(work_package_updated) } do - it 'should ref keywords any' do - WorkPackage.all.each(&:recreate_initial_journal!) - - Setting.commit_fix_status_id = Status.where(['is_closed = ?', true]).first.id - Setting.commit_fix_done_ratio = '90' - Setting.commit_ref_keywords = '*' - Setting.commit_fix_keywords = 'fixes , closes' - - c = Changeset.new(repository: Project.find(1).repository, - committed_on: Time.now, - comments: 'New commit (#2). Fixes #1') - c.scan_comment_for_work_package_ids - - assert_equal [1, 2], c.work_package_ids.sort - fixed = WorkPackage.find(1) - assert fixed.closed? - assert_equal 90, fixed.done_ratio - assert_equal 2, ActionMailer::Base.deliveries.size - end - - it 'should ref keywords' do - Setting.commit_ref_keywords = 'refs' - Setting.commit_fix_keywords = '' - - c = Changeset.new(repository: Project.find(1).repository, - committed_on: Time.now, - comments: 'Ignores #2. Refs #1') - c.scan_comment_for_work_package_ids - - assert_equal [1], c.work_package_ids.sort - end - - it 'should ref keywords any only' do - Setting.commit_ref_keywords = '*' - Setting.commit_fix_keywords = '' - - c = Changeset.new(repository: Project.find(1).repository, - committed_on: Time.now, - comments: 'Ignores #2. Refs #1') - c.scan_comment_for_work_package_ids - - assert_equal [1, 2], c.work_package_ids.sort - end - - it 'should ref keywords any with timelog' do - Setting.commit_ref_keywords = '*' - Setting.commit_logtime_enabled = '1' - - { - '2' => 2.0, - '2h' => 2.0, - '2hours' => 2.0, - '15m' => 0.25, - '15min' => 0.25, - '3h15' => 3.25, - '3h15m' => 3.25, - '3h15min' => 3.25, - '3:15' => 3.25, - '3.25' => 3.25, - '3.25h' => 3.25, - '3,25' => 3.25, - '3,25h' => 3.25 - }.each do |syntax, expected_hours| - c = Changeset.new(repository: Project.find(1).repository, - committed_on: 24.hours.ago, - comments: "Worked on this work_package #1 @#{syntax}", - revision: '520', - user: User.find(2)) - assert_difference 'TimeEntry.count' do - c.scan_comment_for_work_package_ids - end - assert_equal [1], c.work_package_ids.sort - - time = TimeEntry.order(Arel.sql('id DESC')).first - assert_equal 1, time.work_package_id - assert_equal 1, time.project_id - assert_equal 2, time.user_id - assert_equal expected_hours, - time.hours, - "@#{syntax} should be logged as #{expected_hours} hours but was #{time.hours}" - assert_equal Date.yesterday, time.spent_on - assert time.activity.is_default? - assert time.comments.include?('r520'), - "r520 was expected in time_entry comments: #{time.comments}" - end - end - - it 'should ref keywords closing with timelog' do - Setting.commit_fix_status_id = Status.where(['is_closed = ?', true]).first.id - Setting.commit_ref_keywords = '*' - Setting.commit_fix_keywords = 'fixes , closes' - Setting.commit_logtime_enabled = '1' - - c = Changeset.new(repository: Project.find(1).repository, - committed_on: Time.now, - comments: 'This is a comment. Fixes #1 @4.5, #2 @1', - user: User.find(2)) - assert_difference 'TimeEntry.count', 2 do - c.scan_comment_for_work_package_ids - end - - assert_equal [1, 2], c.work_package_ids.sort - assert WorkPackage.find(1).closed? - assert WorkPackage.find(2).closed? - - times = TimeEntry.order(Arel.sql('id desc')).limit(2) - assert_equal [1, 2], times.map(&:work_package_id).sort - end - - it 'should ref keywords any line start' do - Setting.commit_ref_keywords = '*' - - c = Changeset.new(repository: Project.find(1).repository, - committed_on: Time.now, - comments: '#1 is the reason of this commit') - c.scan_comment_for_work_package_ids - - assert_equal [1], c.work_package_ids.sort - end - - it 'should ref keywords allow brackets around a work package number' do - Setting.commit_ref_keywords = '*' - - c = Changeset.new(repository: Project.find(1).repository, - committed_on: Time.now, - comments: '[#1] Worked on this work_package') - c.scan_comment_for_work_package_ids - - assert_equal [1], c.work_package_ids.sort - end - - it 'should ref keywords allow brackets around multiple work package numbers' do - Setting.commit_ref_keywords = '*' - - c = Changeset.new(repository: Project.find(1).repository, - committed_on: Time.now, - comments: '[#1 #2, #3] Worked on these') - c.scan_comment_for_work_package_ids - - assert_equal [1, 2, 3], c.work_package_ids.sort - end - - it 'should commit referencing a subproject work package' do - c = Changeset.new(repository: Project.find(1).repository, - committed_on: Time.now, - comments: 'refs #5, a subproject work_package') - c.scan_comment_for_work_package_ids - - assert_equal [5], c.work_package_ids.sort - assert c.work_packages.first.project != c.project - end - - it 'should commit referencing a parent project work package' do - # repository of child project - r = Repository::Subversion.create!( - project: Project.find(3), - scm_type: 'existing', - url: 'svn://localhost/test') - - c = Changeset.new(repository: r, - committed_on: Time.now, - comments: 'refs #2, an work_package of a parent project') - c.scan_comment_for_work_package_ids - - assert_equal [2], c.work_package_ids.sort - assert c.work_packages.first.project != c.project - end - - it 'should text tag revision' do - c = Changeset.new(revision: '520') - assert_equal 'r520', c.text_tag - end - - it 'should text tag hash' do - c = Changeset.new( - scmid: '7234cb2750b63f47bff735edc50a1c0a433c2518', - revision: '7234cb2750b63f47bff735edc50a1c0a433c2518') - assert_equal 'commit:7234cb2750b63f47bff735edc50a1c0a433c2518', c.text_tag - end - - it 'should text tag hash all number' do - c = Changeset.new(scmid: '0123456789', revision: '0123456789') - assert_equal 'commit:0123456789', c.text_tag - end - - it 'should previous' do - changeset = Changeset.find_by(revision: '3') - assert_equal Changeset.find_by(revision: '2'), changeset.previous - end - - it 'should previous nil' do - changeset = Changeset.find_by(revision: '1') - assert_nil changeset.previous - end - - it 'should next' do - changeset = Changeset.find_by(revision: '2') - assert_equal Changeset.find_by(revision: '3'), changeset.next - end - - it 'should next nil' do - changeset = Changeset.find_by(revision: '10') - assert_nil changeset.next - end - end - - context 'enabled scm', with_settings: { enabled_scm: ['subversion'] } do - it 'should comments empty' do - r = FactoryBot.create(:repository_subversion) - - assert r - c = Changeset.new(repository: r, - committed_on: Time.now, - revision: '123', - scmid: '12345', - comments: '') - assert(c.save) - assert_equal '', c.comments - if c.comments.respond_to?(:force_encoding) - assert_equal 'UTF-8', c.comments.encoding.to_s - end - end - - it 'should comments nil' do - r = FactoryBot.create(:repository_subversion) - assert r - - c = Changeset.new(repository: r, - committed_on: Time.now, - revision: '123', - scmid: '12345', - comments: nil) - assert(c.save) - assert_equal '', c.comments - if c.comments.respond_to?(:force_encoding) - assert_equal 'UTF-8', c.comments.encoding.to_s - end - end - - it 'should identifier' do - c = Changeset.find_by(revision: '1') - assert_equal c.revision, c.identifier - end - end -end diff --git a/spec_legacy/unit/repository_spec.rb b/spec_legacy/unit/repository_spec.rb deleted file mode 100644 index a65e404373..0000000000 --- a/spec_legacy/unit/repository_spec.rb +++ /dev/null @@ -1,168 +0,0 @@ -#-- encoding: UTF-8 - -#-- copyright -# OpenProject is a project management system. -# Copyright (C) 2012-2018 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-2017 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 docs/COPYRIGHT.rdoc for more details. -#++ -require_relative '../legacy_spec_helper' - -describe Repository, type: :model do - fixtures :all - - before do - @repository = Project.find(1).repository - Setting.enabled_scm = %w(subversion) - end - - it 'should create' do - repository = Repository::Subversion.new(project: Project.find(3), scm_type: 'existing') - assert !repository.save - - repository.url = 'svn://localhost' - assert repository.save - repository.reload - - project = Project.find(3) - assert_equal repository, project.repository - end - - it 'should destroy' do - changesets = Changeset.where('repository_id = 10').size - changes = Change.includes(:changeset) - .where("#{Changeset.table_name}.repository_id = 10") - .references(:changesets) - .size - assert_difference 'Changeset.count', -changesets do - assert_difference 'Change.count', -changes do - Repository.find(10).destroy - end - end - end - - it 'should not create with disabled scm' do - Setting.enabled_scm = ['Git'] # disable Subversion - repository = Repository::Subversion.new(project: Project.find(3), scm_type: 'existing', url: 'svn://localhost') - assert !repository.save - assert_includes repository.errors[:type], I18n.translate('activerecord.errors.models.repository.not_available') - # re-enable Subversion for following tests - Setting.delete_all - end - - it 'should scan changesets for work package ids' do - WorkPackage.all.each(&:recreate_initial_journal!) - - Setting.default_language = 'en' - Setting.notified_events = ['work_package_added', 'work_package_updated'] - - # choosing a status to apply to fix issues - Setting.commit_fix_status_id = Status.where(['is_closed = ?', true]).first.id - Setting.commit_fix_done_ratio = '90' - Setting.commit_ref_keywords = 'refs , references, IssueID' - Setting.commit_fix_keywords = 'fixes , closes' - Setting.default_language = 'en' - - # make sure work package 1 is not already closed - fixed_work_package = WorkPackage.find(1) - assert !fixed_work_package.status.is_closed? - old_status = fixed_work_package.status - - Repository.scan_changesets_for_work_package_ids - assert_equal [101, 102], WorkPackage.find(3).changeset_ids - - # fixed issues - fixed_work_package.reload - assert fixed_work_package.status.is_closed? - assert_equal 90, fixed_work_package.done_ratio - assert_equal [101], fixed_work_package.changeset_ids - - # issue change - journal = fixed_work_package.journals.last - assert_equal User.find_by_login('dlopper'), journal.user - assert_equal 'Applied in changeset r2.', journal.notes - - # 2 email notifications to 5 users - assert_equal 5, ActionMailer::Base.deliveries.size - mail = ActionMailer::Base.deliveries.first - assert_kind_of Mail::Message, mail - assert mail.subject.starts_with?("#{fixed_work_package.project.name} - #{fixed_work_package.status.name} #{fixed_work_package.type.name} ##{fixed_work_package.id}") - assert mail.body.encoded.include?( - "Status changed from #{old_status}
to #{fixed_work_package.status}" - ) - - # ignoring commits referencing an issue of another project - assert_equal [], WorkPackage.find(4).changesets - end - - it 'should for changeset comments strip' do - repository = Repository::Subversion.create( - project: Project.find(4), - scm_type: 'existing', - url: 'svn://:login:password@host:/path/to/the/repository' - ) - comment = 'This is a looooooooooooooong comment' + (' ' * 80 + "\n") * 5 - changeset = Changeset.new( - comments: comment, commit_date: Time.now, revision: 0, scmid: 'f39b7922fb3c', - committer: 'foo ', committed_on: Time.now, repository: repository) - assert(changeset.save) - refute_equal(comment, changeset.comments) - assert_equal('This is a looooooooooooooong comment', changeset.comments) - end - - it 'should for urls strip' do - repository = Repository::Subversion.create( - project: Project.find(4), - url: ' svn://:login:password@host:/path/to/the/repository', - scm_type: 'existing', - log_encoding: 'UTF-8') - repository.root_url = 'foo ' # can't mass-assign this attr - assert repository.save - repository.reload - assert_equal 'svn://:login:password@host:/path/to/the/repository', repository.url - assert_equal 'foo', repository.root_url - end - - it 'should manual user mapping' do - assert_no_difference "Changeset.where('user_id <> 2').count" do - c = Changeset.create!(repository: @repository, committer: 'foo', committed_on: Time.now, revision: 100, comments: 'Committed by foo.') - assert_nil c.user - @repository.committer_ids = { 'foo' => '2' } - assert_equal User.find(2), c.reload.user - # committer is now mapped - c = Changeset.create!(repository: @repository, committer: 'foo', committed_on: Time.now, revision: 101, comments: 'Another commit by foo.') - assert_equal User.find(2), c.user - end - end - - it 'should auto user mapping by username' do - c = Changeset.create!(repository: @repository, committer: 'jsmith', committed_on: Time.now, revision: 100, comments: 'Committed by john.') - assert_equal User.find(2), c.user - end - - it 'should auto user mapping by email' do - c = Changeset.create!(repository: @repository, committer: 'john ', committed_on: Time.now, revision: 100, comments: 'Committed by john.') - assert_equal User.find(2), c.user - end -end From e2e8fd12d1936d6a6f28457d009bda3b28f19aae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20G=C3=BCnther?= Date: Fri, 26 Jul 2019 16:43:06 +0200 Subject: [PATCH 2/3] Try to debug missing journal version --- app/models/journal_manager.rb | 4 +++- .../lib/redmine/acts/journalized/save_hooks.rb | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/models/journal_manager.rb b/app/models/journal_manager.rb index a99b94907e..f1ac4a8ba4 100644 --- a/app/models/journal_manager.rb +++ b/app/models/journal_manager.rb @@ -235,11 +235,12 @@ class JournalManager # since no version is changed here, in case of concurrency, one # of the calls is allowed to fail journable_type = base_class_name(journable.class) + warn "Setting version #{journable_type} #{journable.id}" ::JournalVersion.find_or_create_by(journable_type: journable_type, journable_id: journable.id) version = get_next_journal_version(journable_type, journable) - Rails.logger.debug "Inserting new journal for #{journable_type} ##{journable.id} @ #{version}" + warn "Inserting new journal for #{journable_type} ##{journable.id} @ #{version}" journal_attributes = { journable_id: journable.id, journable_type: journal_class_name(journable.class), @@ -247,6 +248,7 @@ class JournalManager activity_type: journable.send(:activity_type), details: journable_details(journable) } + warn "SAVING JOURNAL" journal = create_journal journable, journal_attributes, user, notes # FIXME: this is required for the association to be correctly saved... diff --git a/lib/plugins/acts_as_journalized/lib/redmine/acts/journalized/save_hooks.rb b/lib/plugins/acts_as_journalized/lib/redmine/acts/journalized/save_hooks.rb index 8698df3018..c85b85a8a8 100644 --- a/lib/plugins/acts_as_journalized/lib/redmine/acts/journalized/save_hooks.rb +++ b/lib/plugins/acts_as_journalized/lib/redmine/acts/journalized/save_hooks.rb @@ -74,6 +74,7 @@ module Redmine::Acts::Journalized add_journal = journals.empty? || JournalManager.changed?(self) || !@journal_notes.empty? + warn "Adding journal? #{add_journal}" journal = JournalManager.add_journal! self, @journal_user, @journal_notes if add_journal if add_journal From 993f779dfcacf9085e9ac30d396a3be269b50f11 Mon Sep 17 00:00:00 2001 From: Markus Kahl Date: Tue, 30 Jul 2019 10:35:02 +0100 Subject: [PATCH 3/3] Revert "Try to debug missing journal version" This reverts commit e2e8fd12d1936d6a6f28457d009bda3b28f19aae. --- app/models/journal_manager.rb | 4 +--- .../lib/redmine/acts/journalized/save_hooks.rb | 1 - 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/app/models/journal_manager.rb b/app/models/journal_manager.rb index f1ac4a8ba4..a99b94907e 100644 --- a/app/models/journal_manager.rb +++ b/app/models/journal_manager.rb @@ -235,12 +235,11 @@ class JournalManager # since no version is changed here, in case of concurrency, one # of the calls is allowed to fail journable_type = base_class_name(journable.class) - warn "Setting version #{journable_type} #{journable.id}" ::JournalVersion.find_or_create_by(journable_type: journable_type, journable_id: journable.id) version = get_next_journal_version(journable_type, journable) - warn "Inserting new journal for #{journable_type} ##{journable.id} @ #{version}" + Rails.logger.debug "Inserting new journal for #{journable_type} ##{journable.id} @ #{version}" journal_attributes = { journable_id: journable.id, journable_type: journal_class_name(journable.class), @@ -248,7 +247,6 @@ class JournalManager activity_type: journable.send(:activity_type), details: journable_details(journable) } - warn "SAVING JOURNAL" journal = create_journal journable, journal_attributes, user, notes # FIXME: this is required for the association to be correctly saved... diff --git a/lib/plugins/acts_as_journalized/lib/redmine/acts/journalized/save_hooks.rb b/lib/plugins/acts_as_journalized/lib/redmine/acts/journalized/save_hooks.rb index c85b85a8a8..8698df3018 100644 --- a/lib/plugins/acts_as_journalized/lib/redmine/acts/journalized/save_hooks.rb +++ b/lib/plugins/acts_as_journalized/lib/redmine/acts/journalized/save_hooks.rb @@ -74,7 +74,6 @@ module Redmine::Acts::Journalized add_journal = journals.empty? || JournalManager.changed?(self) || !@journal_notes.empty? - warn "Adding journal? #{add_journal}" journal = JournalManager.add_journal! self, @journal_user, @journal_notes if add_journal if add_journal