From 57285d2b035902d4e79a6f00d6c99c83894df622 Mon Sep 17 00:00:00 2001 From: Jens Ulferts Date: Fri, 26 Jul 2013 15:42:39 +0200 Subject: [PATCH 01/29] adds spec for nested set attributes on issues --- spec/models/work_package_nested_set_spec.rb | 165 ++++++++++++++++++++ 1 file changed, 165 insertions(+) create mode 100644 spec/models/work_package_nested_set_spec.rb diff --git a/spec/models/work_package_nested_set_spec.rb b/spec/models/work_package_nested_set_spec.rb new file mode 100644 index 0000000000..be72a36e6e --- /dev/null +++ b/spec/models/work_package_nested_set_spec.rb @@ -0,0 +1,165 @@ +#-- copyright +# OpenProject is a project management system. +# +# Copyright (C) 2012-2013 the OpenProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# See doc/COPYRIGHT.rdoc for more details. +#++ + +require 'spec_helper' + +# TODO: this spec is for now targeting each WorkPackage subclass +# independently. Once only WorkPackage exist, this can safely be consolidated. +describe WorkPackage do + let(:project) { FactoryGirl.build(:project_with_types) } + let(:issue) { FactoryGirl.build(:issue, :project => project, :type => project.types.first) } + let(:issue2) { FactoryGirl.build(:issue, :project => project, :type => project.types.first) } + let(:issue3) { FactoryGirl.build(:issue, :project => project, :type => project.types.first) } + let(:planning_element) { FactoryGirl.build(:planning_element, :project => project) } + let(:planning_element2) { FactoryGirl.build(:planning_element, :project => project) } + let(:planning_element3) { FactoryGirl.build(:planning_element, :project => project) } + + [:issue].each do |subclass| + + describe "(#{subclass})" do + let(:instance) { send(subclass) } + let(:parent) { send(:"#{subclass}2") } + let(:parent2) { send(:"#{subclass}3") } + + shared_examples_for "root" do + it "should set root_id to the id of the #{subclass}" do + instance.root_id.should == instance.id + end + + it "should set lft to 1" do + instance.lft.should == 1 + end + + it "should set rgt to 2" do + instance.rgt.should == 2 + end + end + + shared_examples_for "first child" do + it "should set root_id to the id of the parent #{subclass}" do + instance.root_id.should == parent.id + end + + it "should set lft to 2" do + instance.lft.should == 2 + end + + it "should set rgt to 3" do + instance.rgt.should == 3 + end + end + + describe "creating a new instance without a parent" do + + before do + instance.save! + end + + it_should_behave_like "root" + end + + describe "creating a new instance without a parent" do + + before do + parent.save! + instance.parent_issue_id = parent.id + + instance.save! + end + + it_should_behave_like "first child" + end + + describe "an existant instance receives a parent" do + + before do + parent.save! + instance.save! + instance.parent_issue_id = parent.id + instance.save! + end + + it_should_behave_like "first child" + end + + describe "an existant instance becomes a root" do + + before do + parent.save! + instance.parent_issue_id = parent.id + instance.save! + instance.parent_issue_id = nil + instance.save! + end + + it_should_behave_like "root" + end + + describe "an existant instance receives a new parent (new tree)" do + + before do + parent.save! + parent2.save! + instance.parent_issue_id = parent2.id + instance.save! + + instance.parent_issue_id = parent.id + instance.save! + end + + it_should_behave_like "first child" + end + + describe "an existant instance receives a new parent (same tree)" do + + before do + parent.save! + parent2.save! + instance.parent_issue_id = parent2.id + instance.save! + + instance.parent_issue_id = parent.id + instance.save! + end + + it_should_behave_like "first child" + end + + describe "an existant instance with children receives a new parent (itself)" do + let(:child) { send(:"#{subclass}3") } + + before do + parent.save! + instance.parent_issue_id = parent.id + instance.save! + child.parent_issue_id = instance.id + child.save! + + instance.parent_issue_id = nil + instance.save! + child.reload + end + + it "the child should have the root_id of the #{subclass}" do + child.root_id.should == instance.id + end + + it "the child should have a lft of 2" do + child.lft.should == 2 + end + + it "the child should have a rgt of 3" do + child.rgt.should == 3 + end + end + end + end +end From 247436ca38c866ac9dd16f21f6162e183fb66f5e Mon Sep 17 00:00:00 2001 From: Jens Ulferts Date: Fri, 26 Jul 2013 23:24:32 +0200 Subject: [PATCH 02/29] moves scoped nested set into own module, applies to WorkPackage --- app/models/issue.rb | 112 +------------ app/models/planning_element.rb | 11 -- app/models/work_package.rb | 3 +- .../nested_set/with_root_id_scope.rb | 151 ++++++++++++++++++ spec/models/work_package_nested_set_spec.rb | 2 +- 5 files changed, 156 insertions(+), 123 deletions(-) create mode 100644 lib/open_project/nested_set/with_root_id_scope.rb diff --git a/app/models/issue.rb b/app/models/issue.rb index 928402778c..1dbfa36bea 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -35,7 +35,6 @@ class Issue < WorkPackage validate :validate_fixed_version_is_assignable validate :validate_fixed_version_is_still_open validate :validate_enabled_type - validate :validate_correct_parent scope :open, :conditions => ["#{IssueStatus.table_name}.is_closed = ?", false], :include => :status @@ -90,7 +89,7 @@ class Issue < WorkPackage before_create :default_assign before_save :close_duplicates, :update_done_ratio_from_issue_status - after_save :reschedule_following_issues, :update_nested_set_attributes, :update_parent_attributes + after_save :reschedule_following_issues, :update_parent_attributes after_destroy :update_parent_attributes before_destroy :remove_attachments @@ -111,7 +110,7 @@ class Issue < WorkPackage # Moves/copies an issue to a new project and type # Returns the moved/copied issue on success, false on failure def move_to_project(*args) - ret = Issue.transaction do + Issue.transaction do move_to_project_without_transaction(*args) || raise(ActiveRecord::Rollback) end || false end @@ -324,24 +323,6 @@ class Issue < WorkPackage end end - def validate_correct_parent - # Checks parent issue assignment - if @parent_issue - if !Setting.cross_project_issue_relations? && @parent_issue.project_id != self.project_id - errors.add :parent_issue_id, :not_a_valid_parent - elsif !new_record? - # moving an existing issue - if @parent_issue.root_id != root_id - # we can always move to another tree - elsif move_possible?(@parent_issue) - # move accepted inside tree - else - errors.add :parent_issue_id, :not_a_valid_parent - end - end - end - end - # Set the done_ratio using the status if that setting is set. This will keep the done_ratios # even if the user turns off the setting later def update_done_ratio_from_issue_status @@ -386,12 +367,6 @@ class Issue < WorkPackage return done_date <= Date.today end - # Does this issue have children? - def children? - !leaf? - end - - # Returns an array of status that user is able to apply def new_statuses_allowed_to(user, include_default=false) return [] if status.nil? @@ -454,20 +429,6 @@ class Issue < WorkPackage end end - # The number of "items" this issue spans in it's nested set - # - # A parent issue would span all of it's children + 1 left + 1 right (3) - # - # | parent | - # || child || - # - # A child would span only itself (1) - # - # |child| - def nested_set_span - rgt - lft - end - # Returns a string of css classes that apply to the issue def css_classes s = "issue status-#{status.position} priority-#{priority.position}" @@ -540,26 +501,6 @@ class Issue < WorkPackage Issue.update_versions(["#{Version.table_name}.project_id IN (?) OR #{Issue.table_name}.project_id IN (?)", moved_project_ids, moved_project_ids]) end - def parent_issue_id=(arg) - parent_issue_id = arg.blank? ? nil : arg.to_i - if parent_issue_id && @parent_issue = Issue.find_by_id(parent_issue_id) - journal_changes["parent_id"] = [self.parent_id, @parent_issue.id] - @parent_issue.id - else - @parent_issue = nil - journal_changes["parent_id"] = [self.parent_id, nil] - nil - end - end - - def parent_issue_id - if instance_variable_defined? :@parent_issue - @parent_issue.nil? ? nil : @parent_issue.id - else - parent_id - end - end - # Extracted from the ReportsController. def self.by_type(project) count_and_group_by(:project => project, @@ -682,55 +623,6 @@ class Issue < WorkPackage private - def update_nested_set_attributes - if root_id.nil? - # issue was just created - self.root_id = (@parent_issue.nil? ? id : @parent_issue.root_id) - set_default_left_and_right - Issue.update_all("root_id = #{root_id}, lft = #{lft}, rgt = #{rgt}", ["id = ?", id]) - if @parent_issue - move_to_child_of(@parent_issue) - end - reload - elsif parent_issue_id != parent_id - former_parent_id = parent_id - # moving an existing issue - if @parent_issue && @parent_issue.root_id == root_id - # inside the same tree - move_to_child_of(@parent_issue) - else - # to another tree - unless root? - move_to_right_of(root) - reload - end - old_root_id = root_id - self.root_id = (@parent_issue.nil? ? id : @parent_issue.root_id ) - target_maxright = nested_set_scope.maximum(right_column_name) || 0 - offset = target_maxright + 1 - lft - Issue.update_all("root_id = #{root_id}, lft = lft + #{offset}, rgt = rgt + #{offset}", - ["root_id = ? AND lft >= ? AND rgt <= ? ", old_root_id, lft, rgt]) - self[left_column_name] = lft + offset - self[right_column_name] = rgt + offset - if @parent_issue - move_to_child_of(@parent_issue) - end - end - reload - - # delete invalid relations of all descendants - self_and_descendants.each do |issue| - issue.relations.each do |relation| - relation.destroy unless relation.valid? - end - end - - # update former parent - recalculate_attributes_for(former_parent_id) if former_parent_id - end - remove_instance_variable(:@parent_issue) if instance_variable_defined?(:@parent_issue) - end - # this removes all attachments separately before destroying the issue # avoids getting a ActiveRecord::StaleObjectError when deleting an issue def remove_attachments diff --git a/app/models/planning_element.rb b/app/models/planning_element.rb index 27e959f61d..2b7c7692d5 100644 --- a/app/models/planning_element.rb +++ b/app/models/planning_element.rb @@ -249,17 +249,6 @@ class PlanningElement < WorkPackage !!read_attribute(:deleted_at) end - # Aliasing the parent_issue_id methods here in order - # to improve compatibility between - # planning elments and issues - alias_method :parent_issue_id, :parent_id - - # I am not sure why it is not possible to - # alias_method :parent_issue_id=, :parent_id= - def parent_issue_id=(arg) - parent_id = arg - end - protected def update_parent_attributes diff --git a/app/models/work_package.rb b/app/models/work_package.rb index f3020fc7ae..908b8b7d9b 100644 --- a/app/models/work_package.rb +++ b/app/models/work_package.rb @@ -50,7 +50,8 @@ class WorkPackage < ActiveRecord::Base acts_as_watchable - acts_as_nested_set :scope => 'root_id', :dependent => :destroy + include OpenProject::NestedSet::WithRootIdScope + acts_as_customizable acts_as_searchable :columns => ['subject', "#{table_name}.description", "#{Journal.table_name}.notes"], diff --git a/lib/open_project/nested_set/with_root_id_scope.rb b/lib/open_project/nested_set/with_root_id_scope.rb new file mode 100644 index 0000000000..c1b8c0c98d --- /dev/null +++ b/lib/open_project/nested_set/with_root_id_scope.rb @@ -0,0 +1,151 @@ +#-- encoding: UTF-8 +#-- copyright +# OpenProject is a project management system. +# +# Copyright (C) 2012-2013 the OpenProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# See doc/COPYRIGHT.rdoc for more details. +#++ + +module OpenProject::NestedSet + module WithRootIdScope + def self.included(base) + base.class_eval do + acts_as_nested_set :scope => 'root_id', :dependent => :destroy + after_save :update_nested_set_attributes + + validate :validate_correct_parent + + include InstanceMethods + end + end + + module InstanceMethods + + # The number of "items" this issue spans in it's nested set + # + # A parent issue would span all of it's children + 1 left + 1 right (3) + # + # | parent | + # || child || + # + # A child would span only itself (1) + # + # |child| + def nested_set_span + rgt - lft + end + + # Does this issue have children? + def children? + !leaf? + end + + def validate_correct_parent + # Checks parent issue assignment + if @parent_issue + if !Setting.cross_project_issue_relations? && @parent_issue.project_id != self.project_id + errors.add :parent_issue_id, :not_a_valid_parent + elsif !new_record? + # moving an existing issue + if @parent_issue.root_id != root_id + # we can always move to another tree + elsif move_possible?(@parent_issue) + # move accepted inside tree + else + errors.add :parent_issue_id, :not_a_valid_parent + end + end + end + end + + def parent_issue_id=(arg) + parent_issue_id = arg.blank? ? nil : arg.to_i + if parent_issue_id && @parent_issue = self.class.find_by_id(parent_issue_id) + journal_changes["parent_id"] = [self.parent_id, @parent_issue.id] + @parent_issue.id + else + @parent_issue = nil + journal_changes["parent_id"] = [self.parent_id, nil] + nil + end + end + + def parent_issue_id + if instance_variable_defined? :@parent_issue + @parent_issue.nil? ? nil : @parent_issue.id + else + parent_id + end + end + + private + + def update_nested_set_attributes + if root_id.nil? + set_initial_root_id + elsif parent_issue_id != parent_id + update_existing_tree_node_attributes + end + remove_instance_variable(:@parent_issue) if instance_variable_defined?(:@parent_issue) + end + + def set_initial_root_id + self.root_id = (@parent_issue.nil? ? id : @parent_issue.root_id) + set_default_left_and_right + self.class.update_all("root_id = #{root_id}, lft = #{lft}, rgt = #{rgt}", ["id = ?", id]) + if @parent_issue + move_to_child_of(@parent_issue) + end + reload + end + + def move_in_tree(new_parent) + if new_parent && new_parent.root_id == root_id + # inside the same tree + move_to_child_of(new_parent) + else + # to another tree + unless root? + move_to_right_of(root) + reload + end + old_root_id = root_id + self.root_id = (new_parent.nil? ? id : new_parent.root_id ) + target_maxright = nested_set_scope.maximum(right_column_name) || 0 + offset = target_maxright + 1 - lft + + self.class.update_all("root_id = #{root_id}, lft = lft + #{offset}, rgt = rgt + #{offset}", + ["root_id = ? AND lft >= ? AND rgt <= ? ", old_root_id, lft, rgt]) + self[left_column_name] = lft + offset + self[right_column_name] = rgt + offset + if new_parent + move_to_child_of(new_parent) + end + end + + reload + end + + def update_existing_tree_node_attributes + former_parent_id = parent_id + # moving an existing issue + + move_in_tree(@parent_issue) + + # delete invalid relations of all descendants + self_and_descendants.each do |issue| + issue.relations.each do |relation| + relation.destroy unless relation.valid? + end + end + + # update former parent + recalculate_attributes_for(former_parent_id) if former_parent_id + end + end + end +end diff --git a/spec/models/work_package_nested_set_spec.rb b/spec/models/work_package_nested_set_spec.rb index be72a36e6e..ff097547ad 100644 --- a/spec/models/work_package_nested_set_spec.rb +++ b/spec/models/work_package_nested_set_spec.rb @@ -22,7 +22,7 @@ describe WorkPackage do let(:planning_element2) { FactoryGirl.build(:planning_element, :project => project) } let(:planning_element3) { FactoryGirl.build(:planning_element, :project => project) } - [:issue].each do |subclass| + [:issue, :planning_element].each do |subclass| describe "(#{subclass})" do let(:instance) { send(subclass) } From b03a412cef32723d68c397efaba0a7fc56bfbc11 Mon Sep 17 00:00:00 2001 From: Jens Ulferts Date: Mon, 29 Jul 2013 14:18:29 +0200 Subject: [PATCH 03/29] internal restructurings of nested_set patch --- .../nested_set/with_root_id_scope.rb | 66 +++++++++---------- 1 file changed, 32 insertions(+), 34 deletions(-) diff --git a/lib/open_project/nested_set/with_root_id_scope.rb b/lib/open_project/nested_set/with_root_id_scope.rb index c1b8c0c98d..35b3efab00 100644 --- a/lib/open_project/nested_set/with_root_id_scope.rb +++ b/lib/open_project/nested_set/with_root_id_scope.rb @@ -100,51 +100,49 @@ module OpenProject::NestedSet if @parent_issue move_to_child_of(@parent_issue) end - reload end - def move_in_tree(new_parent) + def update_existing_tree_node_attributes + former_parent_id = parent_id + # moving an existing instance + + move_to_new_parent_node(@parent_issue) + + after_update_of_existing_tree_node(former_parent_id) + end + + def move_to_new_parent_node(new_parent) if new_parent && new_parent.root_id == root_id - # inside the same tree - move_to_child_of(new_parent) + move_within_set(new_parent) else - # to another tree - unless root? - move_to_right_of(root) - reload - end - old_root_id = root_id - self.root_id = (new_parent.nil? ? id : new_parent.root_id ) - target_maxright = nested_set_scope.maximum(right_column_name) || 0 - offset = target_maxright + 1 - lft - - self.class.update_all("root_id = #{root_id}, lft = lft + #{offset}, rgt = rgt + #{offset}", - ["root_id = ? AND lft >= ? AND rgt <= ? ", old_root_id, lft, rgt]) - self[left_column_name] = lft + offset - self[right_column_name] = rgt + offset - if new_parent - move_to_child_of(new_parent) - end + move_to_different_set(new_parent) end + end - reload + def move_within_set(new_parent) + inside the same tree + move_to_child_of(new_parent) end - def update_existing_tree_node_attributes - former_parent_id = parent_id - # moving an existing issue + def move_to_different_set(new_parent) + unless root? + move_to_right_of(root) + end + old_root_id = root_id - move_in_tree(@parent_issue) + self.root_id = (new_parent.nil? ? id : new_parent.root_id ) - # delete invalid relations of all descendants - self_and_descendants.each do |issue| - issue.relations.each do |relation| - relation.destroy unless relation.valid? - end - end + target_maxright = nested_set_scope.maximum(right_column_name) || 0 + offset = target_maxright + 1 - lft + + self.class.update_all("root_id = #{root_id}, lft = lft + #{offset}, rgt = rgt + #{offset}", + ["root_id = ? AND lft >= ? AND rgt <= ? ", old_root_id, lft, rgt]) + self[left_column_name] = lft + offset + self[right_column_name] = rgt + offset - # update former parent - recalculate_attributes_for(former_parent_id) if former_parent_id + if new_parent + move_to_child_of(new_parent) + end end end end From 426e1d712e4ba537eac83aa8fee0858c1d70c6e8 Mon Sep 17 00:00:00 2001 From: Christian Ratz Date: Wed, 31 Jul 2013 09:40:13 +0200 Subject: [PATCH 04/29] Re enable additional environment config file This file is used to load instance specific configuration like cache store, log file location, etc. so we should load it. --- config/application.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/config/application.rb b/config/application.rb index 6005840b6f..c3e2b679b3 100644 --- a/config/application.rb +++ b/config/application.rb @@ -86,12 +86,11 @@ module OpenProject # like if you have constraints or database-specific column types # config.active_record.schema_format = :sql - # needed? # Load any local configuration that is kept out of source control # (e.g. patches). - #if File.exists?(File.join(File.dirname(__FILE__), 'additional_environment.rb')) - # instance_eval File.read(File.join(File.dirname(__FILE__), 'additional_environment.rb')) - #end + if File.exists?(File.join(File.dirname(__FILE__), 'additional_environment.rb')) + instance_eval File.read(File.join(File.dirname(__FILE__), 'additional_environment.rb')) + end # Enforce whitelist mode for mass assignment. # This will create an empty whitelist of attributes available for mass-assignment for all models From 5d3b70928c84c8ae69ffe4063e279b63bc1f2df5 Mon Sep 17 00:00:00 2001 From: Jens Ulferts Date: Mon, 29 Jul 2013 15:13:55 +0200 Subject: [PATCH 05/29] moves after save hooks to work_package --- app/models/issue.rb | 5 - app/models/planning_element.rb | 39 ------ app/models/work_package.rb | 123 ++++++++++++------ .../nested_set/with_root_id_scope.rb | 3 + spec/models/planning_element_spec.rb | 34 ++--- spec/models/work_package_nested_set_spec.rb | 2 +- 6 files changed, 103 insertions(+), 103 deletions(-) diff --git a/app/models/issue.rb b/app/models/issue.rb index 1dbfa36bea..536d0a90bb 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -89,8 +89,6 @@ class Issue < WorkPackage before_create :default_assign before_save :close_duplicates, :update_done_ratio_from_issue_status - after_save :reschedule_following_issues, :update_parent_attributes - after_destroy :update_parent_attributes before_destroy :remove_attachments after_initialize :set_default_values @@ -631,9 +629,6 @@ class Issue < WorkPackage reload # important end - def update_parent_attributes - recalculate_attributes_for(parent_id) if parent_id - end # Update issues so their versions are not pointing to a # fixed_version that is not shared with the issue's project diff --git a/app/models/planning_element.rb b/app/models/planning_element.rb index 2b7c7692d5..6e4fcfc542 100644 --- a/app/models/planning_element.rb +++ b/app/models/planning_element.rb @@ -119,9 +119,6 @@ class PlanningElement < WorkPackage before_save :append_scenario_dates_to_journal - after_save :update_parent_attributes - after_save :create_alternate_date - validates_presence_of :subject, :project validates_length_of :subject, :maximum => 255, :unless => lambda { |e| e.subject.blank? } @@ -217,14 +214,6 @@ class PlanningElement < WorkPackage end end - def note - @journal_notes - end - - def note=(text) - @journal_notes = text - end - def trash unless new_record? or self.deleted_at self.children.each{|child| child.trash} @@ -248,32 +237,4 @@ class PlanningElement < WorkPackage def deleted? !!read_attribute(:deleted_at) end - - protected - - def update_parent_attributes - if parent.present? - parent.reload - - unless parent.children.without_deleted.empty? - children = parent.children.without_deleted - - parent.start_date = [children.minimum(:start_date), children.minimum(:due_date)].reject(&:nil?).min - parent.due_date = [children.maximum(:start_date), children.maximum(:due_date)].reject(&:nil?).max - - if parent.changes.present? - parent.note = I18n.t('timelines.planning_element_updated_automatically_by_child_changes', :child => "*#{id}") - - # Ancestors will be updated by parent's after_save hook. - parent.save(:validate => false) - end - end - end - end - - def create_alternate_date - if start_date_changed? or due_date_changed? - alternate_dates.create(:start_date => start_date, :due_date => due_date) - end - end end diff --git a/app/models/work_package.rb b/app/models/work_package.rb index 908b8b7d9b..8dd764d4b8 100644 --- a/app/models/work_package.rb +++ b/app/models/work_package.rb @@ -51,6 +51,10 @@ class WorkPackage < ActiveRecord::Base acts_as_watchable include OpenProject::NestedSet::WithRootIdScope + after_save :reschedule_following_issues, + :update_parent_attributes, + :create_alternate_date + after_destroy :update_parent_attributes acts_as_customizable @@ -283,47 +287,8 @@ class WorkPackage < ActiveRecord::Base end end - def recalculate_attributes_for(work_package_id) - if work_package_id.is_a? WorkPackage - p = work_package_id - else - p = WorkPackage.find_by_id(work_package_id) - end - - if p - # priority = highest priority of children - if priority_position = p.children.joins(:priority).maximum("#{IssuePriority.table_name}.position") - p.priority = IssuePriority.find_by_position(priority_position) - end - - # start/due dates = lowest/highest dates of children - p.start_date = p.children.minimum(:start_date) - p.due_date = p.children.maximum(:due_date) - if p.start_date && p.due_date && p.due_date < p.start_date - p.start_date, p.due_date = p.due_date, p.start_date - end - - # done ratio = weighted average ratio of leaves - unless WorkPackage.use_status_for_done_ratio? && p.status && p.status.default_done_ratio - leaves_count = p.leaves.count - if leaves_count > 0 - average = p.leaves.average(:estimated_hours).to_f - if average == 0 - average = 1 - end - done = p.leaves.joins(:status).sum("COALESCE(estimated_hours, #{average}) * (CASE WHEN is_closed = #{connection.quoted_true} THEN 100 ELSE COALESCE(done_ratio, 0) END)").to_f - progress = done / (average * leaves_count) - p.done_ratio = progress.round - end - end - - # estimate = sum of leaves estimates - p.estimated_hours = p.leaves.sum(:estimated_hours).to_f - p.estimated_hours = nil if p.estimated_hours == 0.0 - - # ancestors will be recursively updated - p.save(:validate => false) if p.changed? - end + def is_milestone? + planning_element_type && planning_element_type.is_milestone? end # This is a dummy implementation that is currently overwritten @@ -347,6 +312,82 @@ class WorkPackage < ActiveRecord::Base .sum("#{TimeEntry.table_name}.hours").to_f || 0.0 end + protected + + def recalculate_attributes_for(work_package_id) + p = if work_package_id.is_a? WorkPackage + work_package_id + else + WorkPackage.find_by_id(work_package_id) + end + + return unless p + + # priority = highest priority of children + if priority_position = p.children.joins(:priority).maximum("#{IssuePriority.table_name}.position") + p.priority = IssuePriority.find_by_position(priority_position) + end + + p.inherit_dates_from_children + + # done ratio = weighted average ratio of leaves + unless WorkPackage.use_status_for_done_ratio? && p.status && p.status.default_done_ratio + leaves_count = p.leaves.count + if leaves_count > 0 + average = p.leaves.average(:estimated_hours).to_f + if average == 0 + average = 1 + end + done = p.leaves.joins(:status).sum("COALESCE(estimated_hours, #{average}) * (CASE WHEN is_closed = #{connection.quoted_true} THEN 100 ELSE COALESCE(done_ratio, 0) END)").to_f + progress = done / (average * leaves_count) + p.done_ratio = progress.round + end + end + + # estimate = sum of leaves estimates + p.estimated_hours = p.leaves.sum(:estimated_hours).to_f + p.estimated_hours = nil if p.estimated_hours == 0.0 + + # ancestors will be recursively updated + if p.changed? + p.journal_notes = I18n.t('timelines.planning_element_updated_automatically_by_child_changes', :child => "*#{id}") + + # Ancestors will be updated by parent's after_save hook. + p.save(:validate => false) + end + end + + def update_parent_attributes + recalculate_attributes_for(parent_id) if parent_id.present? + end + + def inherit_dates_from_children + active_children = children.without_deleted + + unless active_children.empty? + self.start_date = [active_children.minimum(:start_date), active_children.minimum(:due_date)].compact.min + self.due_date = [active_children.maximum(:start_date), active_children.maximum(:due_date)].compact.max + end + end + + def create_alternate_date + if start_date_changed? or due_date_changed? + alternate_dates.create(:start_date => start_date, :due_date => due_date) + end + end + + def after_update_of_existing_tree_node(former_parent_id) + # delete invalid relations of all descendants + self_and_descendants.each do |issue| + issue.relations.each do |relation| + relation.destroy unless relation.valid? + end + end + + # update former parent + recalculate_attributes_for(former_parent_id) if former_parent_id + end + private def add_time_entry_for(user, attributes) diff --git a/lib/open_project/nested_set/with_root_id_scope.rb b/lib/open_project/nested_set/with_root_id_scope.rb index 35b3efab00..8b1dad4998 100644 --- a/lib/open_project/nested_set/with_root_id_scope.rb +++ b/lib/open_project/nested_set/with_root_id_scope.rb @@ -84,6 +84,9 @@ module OpenProject::NestedSet private + # just here to be overwritten + def after_update_of_existing_tree_node(former_parent_id) end; + def update_nested_set_attributes if root_id.nil? set_initial_root_id diff --git a/spec/models/planning_element_spec.rb b/spec/models/planning_element_spec.rb index 1ab86c1eaf..e133012df5 100644 --- a/spec/models/planning_element_spec.rb +++ b/spec/models/planning_element_spec.rb @@ -181,8 +181,8 @@ describe PlanningElement do describe 'derived attributes' do before do @pe1 = FactoryGirl.create(:planning_element, :project_id => project.id) - @pe11 = FactoryGirl.create(:planning_element, :project_id => project.id, :parent_id => @pe1.id) - @pe12 = FactoryGirl.create(:planning_element, :project_id => project.id, :parent_id => @pe1.id) + @pe11 = FactoryGirl.create(:planning_element, :project_id => project.id, :parent_issue_id => @pe1.id) + @pe12 = FactoryGirl.create(:planning_element, :project_id => project.id, :parent_issue_id => @pe1.id) end describe 'start_date' do @@ -603,7 +603,7 @@ describe PlanningElement do describe 'planning element hierarchies' do let(:child_pe) { FactoryGirl.create(:planning_element, - :parent_id => pe.id, + :parent_issue_id => pe.id, :subject => "Plan B", :description => "This will work out", # interval is the same as parent, so that @@ -682,7 +682,7 @@ describe PlanningElement do end it "should create seperate journal entries for start_date and due_date if only one of 'em is modified" do - # PlanningElements create a alternate_date on save by default, so just use that + # PlanningElements create an alternate_date on save by default, so just use that alternate_date = journal_planning_element.alternate_dates.first.tap do |ad| ad.start_date = Date.today ad.due_date = Date.today + 1.month @@ -743,7 +743,7 @@ describe PlanningElement do pe11 = FactoryGirl.create(:planning_element, :project_id => project.id, - :parent_id => @pe1.id, + :parent_issue_id => @pe1.id, :start_date => Date.new(2011, 1, 1), :due_date => Date.new(2011, 2, 1)) update_journal = @pe1.journals.last @@ -770,17 +770,17 @@ describe PlanningElement do pe11 = FactoryGirl.create(:planning_element, :project_id => project.id, - :parent_id => @pe1.id, + :parent_issue_id => @pe1.id, :start_date => nil, :due_date => Date.new(2011, 1, 1)) pe12 = FactoryGirl.create(:planning_element, :project_id => project.id, - :parent_id => @pe1.id, + :parent_issue_id => @pe1.id, :start_date => Date.new(2012, 2, 1), :due_date => Date.new(2012, 6, 1)) pe13 = FactoryGirl.create(:planning_element, :project_id => project.id, - :parent_id => @pe1.id, + :parent_issue_id => @pe1.id, :start_date => Date.new(2013, 2, 1), :due_date => nil) @@ -808,7 +808,7 @@ describe PlanningElement do :project_id => project.id, :start_date => start_date, :due_date => due_date, - :parent_id => @pe1.id) + :parent_issue_id => @pe1.id) @pe1.reload @@ -827,7 +827,7 @@ describe PlanningElement do it 'should not restore child elements whose parent is deleted' do pe11 = FactoryGirl.create(:planning_element, :project_id => project.id, - :parent_id => @pe1.id, + :parent_issue_id => @pe1.id, :start_date => Date.new(2011, 1, 1), :due_date => Date.new(2011, 2, 1)) @pe1.reload @@ -841,7 +841,7 @@ describe PlanningElement do it 'should restore elements without touching the children' do pe11 = FactoryGirl.create(:planning_element, :project_id => project.id, - :parent_id => @pe1.id, + :parent_issue_id => @pe1.id, :start_date => Date.new(2011, 1, 1), :due_date => Date.new(2011, 2, 1)) @@ -859,9 +859,9 @@ describe PlanningElement do it 'moves all child elements to trash' do pe1 = FactoryGirl.create(:planning_element, :project_id => project.id) - pe11 = FactoryGirl.create(:planning_element, :project_id => project.id, :parent_id => pe1.id) - pe12 = FactoryGirl.create(:planning_element, :project_id => project.id, :parent_id => pe1.id) - pe121 = FactoryGirl.create(:planning_element, :project_id => project.id, :parent_id => pe12.id) + pe11 = FactoryGirl.create(:planning_element, :project_id => project.id, :parent_issue_id => pe1.id) + pe12 = FactoryGirl.create(:planning_element, :project_id => project.id, :parent_issue_id => pe1.id) + pe121 = FactoryGirl.create(:planning_element, :project_id => project.id, :parent_issue_id => pe12.id) pe2 = FactoryGirl.create(:planning_element, :project_id => project.id) pe1.reload @@ -886,9 +886,9 @@ describe PlanningElement do it 'destroys all child elements' do pe1 = FactoryGirl.create(:planning_element, :project_id => project.id) - pe11 = FactoryGirl.create(:planning_element, :project_id => project.id, :parent_id => pe1.id) - pe12 = FactoryGirl.create(:planning_element, :project_id => project.id, :parent_id => pe1.id) - pe121 = FactoryGirl.create(:planning_element, :project_id => project.id, :parent_id => pe12.id) + pe11 = FactoryGirl.create(:planning_element, :project_id => project.id, :parent_issue_id => pe1.id) + pe12 = FactoryGirl.create(:planning_element, :project_id => project.id, :parent_issue_id => pe1.id) + pe121 = FactoryGirl.create(:planning_element, :project_id => project.id, :parent_issue_id => pe12.id) pe2 = FactoryGirl.create(:planning_element, :project_id => project.id) pe1.destroy diff --git a/spec/models/work_package_nested_set_spec.rb b/spec/models/work_package_nested_set_spec.rb index ff097547ad..5121401b1d 100644 --- a/spec/models/work_package_nested_set_spec.rb +++ b/spec/models/work_package_nested_set_spec.rb @@ -66,7 +66,7 @@ describe WorkPackage do it_should_behave_like "root" end - describe "creating a new instance without a parent" do + describe "creating a new instance with a parent" do before do parent.save! From eb350344cafab2502b3bc5f8f7ca2006cd46c3cb Mon Sep 17 00:00:00 2001 From: Jens Ulferts Date: Mon, 29 Jul 2013 15:22:09 +0200 Subject: [PATCH 06/29] moves code into their own methods --- app/models/work_package.rb | 53 ++++++++++++++++++++++++-------------- 1 file changed, 33 insertions(+), 20 deletions(-) diff --git a/app/models/work_package.rb b/app/models/work_package.rb index 8dd764d4b8..0ec6fc6686 100644 --- a/app/models/work_package.rb +++ b/app/models/work_package.rb @@ -323,30 +323,13 @@ class WorkPackage < ActiveRecord::Base return unless p - # priority = highest priority of children - if priority_position = p.children.joins(:priority).maximum("#{IssuePriority.table_name}.position") - p.priority = IssuePriority.find_by_position(priority_position) - end + p.inherit_priority_from_children p.inherit_dates_from_children - # done ratio = weighted average ratio of leaves - unless WorkPackage.use_status_for_done_ratio? && p.status && p.status.default_done_ratio - leaves_count = p.leaves.count - if leaves_count > 0 - average = p.leaves.average(:estimated_hours).to_f - if average == 0 - average = 1 - end - done = p.leaves.joins(:status).sum("COALESCE(estimated_hours, #{average}) * (CASE WHEN is_closed = #{connection.quoted_true} THEN 100 ELSE COALESCE(done_ratio, 0) END)").to_f - progress = done / (average * leaves_count) - p.done_ratio = progress.round - end - end + p.inherit_done_ratio_from_leaves - # estimate = sum of leaves estimates - p.estimated_hours = p.leaves.sum(:estimated_hours).to_f - p.estimated_hours = nil if p.estimated_hours == 0.0 + p.inherit_estimated_hours_from_leaves # ancestors will be recursively updated if p.changed? @@ -361,6 +344,13 @@ class WorkPackage < ActiveRecord::Base recalculate_attributes_for(parent_id) if parent_id.present? end + def inherit_priority_from_children + # priority = highest priority of children + if priority_position = children.joins(:priority).maximum("#{IssuePriority.table_name}.position") + self.priority = IssuePriority.find_by_position(priority_position) + end + end + def inherit_dates_from_children active_children = children.without_deleted @@ -370,6 +360,29 @@ class WorkPackage < ActiveRecord::Base end end + def inherit_done_ratio_from_leaves + # done ratio = weighted average ratio of leaves + unless WorkPackage.use_status_for_done_ratio? && status && status.default_done_ratio + leaves_count = leaves.count + if leaves_count > 0 + average = leaves.average(:estimated_hours).to_f + if average == 0 + average = 1 + end + done = leaves.joins(:status).sum("COALESCE(estimated_hours, #{average}) * (CASE WHEN is_closed = #{connection.quoted_true} THEN 100 ELSE COALESCE(done_ratio, 0) END)").to_f + progress = done / (average * leaves_count) + + self.done_ratio = progress.round + end + end + end + + def inherit_estimated_hours_from_leaves + # estimate = sum of leaves estimates + self.estimated_hours = leaves.sum(:estimated_hours).to_f + self.estimated_hours = nil if estimated_hours == 0.0 + end + def create_alternate_date if start_date_changed? or due_date_changed? alternate_dates.create(:start_date => start_date, :due_date => due_date) From c0b57509e42767b2c41dc73afbcf1c2e471d5c55 Mon Sep 17 00:00:00 2001 From: Jens Ulferts Date: Mon, 29 Jul 2013 18:06:50 +0200 Subject: [PATCH 07/29] alternate implementation of nested set with root id --- .../nested_set/with_root_id_scope.rb | 109 ++++++++++++------ spec/models/work_package_nested_set_spec.rb | 79 ++++++++++++- 2 files changed, 148 insertions(+), 40 deletions(-) diff --git a/lib/open_project/nested_set/with_root_id_scope.rb b/lib/open_project/nested_set/with_root_id_scope.rb index 8b1dad4998..c85c0ce61c 100644 --- a/lib/open_project/nested_set/with_root_id_scope.rb +++ b/lib/open_project/nested_set/with_root_id_scope.rb @@ -14,8 +14,10 @@ module OpenProject::NestedSet module WithRootIdScope def self.included(base) base.class_eval do + skip_callback :create, :before, :set_default_left_and_right + before_save :set_parent_id_to_parent_issue_id + after_save :manage_root_id acts_as_nested_set :scope => 'root_id', :dependent => :destroy - after_save :update_nested_set_attributes validate :validate_correct_parent @@ -84,69 +86,100 @@ module OpenProject::NestedSet private - # just here to be overwritten - def after_update_of_existing_tree_node(former_parent_id) end; - - def update_nested_set_attributes - if root_id.nil? - set_initial_root_id - elsif parent_issue_id != parent_id - update_existing_tree_node_attributes + def manage_root_id + if root_id.nil? # new node + initial_root_id + elsif parent_id_changed? + update_root_id end + remove_instance_variable(:@parent_issue) if instance_variable_defined?(:@parent_issue) end - def set_initial_root_id - self.root_id = (@parent_issue.nil? ? id : @parent_issue.root_id) - set_default_left_and_right - self.class.update_all("root_id = #{root_id}, lft = #{lft}, rgt = #{rgt}", ["id = ?", id]) + def initial_root_id if @parent_issue - move_to_child_of(@parent_issue) + self.root_id = @parent_issue.root_id + else + self.root_id = id end + + set_default_left_and_right + persist_nested_set_attributes end - def update_existing_tree_node_attributes - former_parent_id = parent_id - # moving an existing instance + def update_root_id + if !@parent_issue || @parent_issue.root_id != root_id + # as the following actions depend on the + # node having current values, we reload them here + self.reload_nested_set - move_to_new_parent_node(@parent_issue) + # and save them in order to be save between removing the node from + # the set and fixing the former set's attributes + old_root_id = root_id + old_rgt = rgt - after_update_of_existing_tree_node(former_parent_id) - end + new_root_id = @parent_issue.nil? ? id : @parent_issue.root_id + moved_span = nested_set_span + 1 - def move_to_new_parent_node(new_parent) - if new_parent && new_parent.root_id == root_id - move_within_set(new_parent) - else - move_to_different_set(new_parent) + move_subtree_to_new_set(new_root_id, old_root_id) + correct_former_set_attributes(old_root_id, moved_span, old_rgt) end end - def move_within_set(new_parent) - inside the same tree - move_to_child_of(new_parent) + def persist_nested_set_attributes + self.class.update_all("root_id = #{root_id}, lft = #{lft}, rgt = #{rgt}", ["id = ?", id]) end - def move_to_different_set(new_parent) - unless root? - move_to_right_of(root) - end - old_root_id = root_id - - self.root_id = (new_parent.nil? ? id : new_parent.root_id ) + def move_subtree_to_new_set(new_root_id, old_root_id) + self.root_id = new_root_id target_maxright = nested_set_scope.maximum(right_column_name) || 0 offset = target_maxright + 1 - lft self.class.update_all("root_id = #{root_id}, lft = lft + #{offset}, rgt = rgt + #{offset}", ["root_id = ? AND lft >= ? AND rgt <= ? ", old_root_id, lft, rgt]) + self[left_column_name] = lft + offset self[right_column_name] = rgt + offset + end - if new_parent - move_to_child_of(new_parent) - end + # Update all nodes left and right values in the former set having a right + # value larger than self's former right value. + # + # It calculates what will have to be subtracted from the left and right + # values of the nodes in question. Then it will always subtract this + # value from the right value of every node. It will only subtract the + # value from the left value if the left value is larger than the removed + # node's right value. + # + # Given a set: + # 1*6 + # / \ + # 2*3 4*5 + # for wich the node with lft = 2 and rgt = 3 is self and was removed, the + # resulting set will be: + # 1*4 + # | + # 2*3 + + def correct_former_set_attributes(old_root_id, removed_span, rgt_offset) + # As every node takes two integers we can multiply the amount of + # removed_nodes by 2 to calculate the value by which right and left + # will have to be reduced. + #removed_span = removed_nodes * 2 + + self.class.update_all("#{quoted_right_column_name} = #{quoted_right_column_name} - #{removed_span}, " + + "#{quoted_left_column_name} = CASE " + + "WHEN #{quoted_left_column_name} > #{rgt_offset} " + + "THEN #{quoted_left_column_name} - #{removed_span} " + + "ELSE #{quoted_left_column_name} END", + ["root_id = ? AND #{quoted_right_column_name} > ?", old_root_id, rgt_offset]) end + + def set_parent_id_to_parent_issue_id + self.parent_id = parent_issue_id + end + end end end diff --git a/spec/models/work_package_nested_set_spec.rb b/spec/models/work_package_nested_set_spec.rb index 5121401b1d..8258d28ca7 100644 --- a/spec/models/work_package_nested_set_spec.rb +++ b/spec/models/work_package_nested_set_spec.rb @@ -101,6 +101,10 @@ describe WorkPackage do end it_should_behave_like "root" + + it "should set parent_id to nil" do + instance.parent_id.should == nil + end end describe "an existant instance receives a new parent (new tree)" do @@ -116,6 +120,57 @@ describe WorkPackage do end it_should_behave_like "first child" + + it "should set parent_id to new parent" do + instance.parent_id.should == parent.id + end + end + + describe "an existant instance + with a right sibling receives a new parent" do + + let(:other_child) { send(:"#{subclass}3") } + + before do + parent.save! + instance.parent_issue_id = parent.id + instance.save! + other_child.parent_issue_id = parent.id + other_child.save! + + instance.parent_issue_id = nil + instance.save! + end + + it "former roots's root_id should be unchanged" do + parent.reload + parent.root_id.should == parent.id + end + + it "former roots's lft should be 1" do + parent.reload + parent.lft.should == 1 + end + + it "former roots's rgt should be 4" do + parent.reload + parent.rgt.should == 4 + end + + it "former right siblings's root_id should be unchanged" do + other_child.reload + other_child.root_id.should == parent.id + end + + it "former right siblings's left should be 2" do + other_child.reload + other_child.lft.should == 2 + end + + it "former right siblings's rgt should be 3" do + other_child.reload + other_child.rgt.should == 3 + end end describe "an existant instance receives a new parent (same tree)" do @@ -143,20 +198,40 @@ describe WorkPackage do child.parent_issue_id = instance.id child.save! + # reloading as instance's nested set attributes (lft, rgt) where + # updated by adding child to the set + instance.reload instance.parent_issue_id = nil instance.save! - child.reload end - it "the child should have the root_id of the #{subclass}" do + it "former parent's root_id should be unchanged" do + parent.reload + parent.root_id.should == parent.id + end + + it "former parent's left should be 1" do + parent.reload + parent.lft.should == 1 + end + + it "former parent's right should be 2" do + parent.reload + parent.rgt.should == 2 + end + + it "the child should have the root_id of the parent #{subclass}" do + child.reload child.root_id.should == instance.id end it "the child should have a lft of 2" do + child.reload child.lft.should == 2 end it "the child should have a rgt of 3" do + child.reload child.rgt.should == 3 end end From d88c1467aee1943445c169b8f240ed1f929905b9 Mon Sep 17 00:00:00 2001 From: Jens Ulferts Date: Tue, 30 Jul 2013 15:27:11 +0200 Subject: [PATCH 08/29] renames parent to parent_issue_id for planning element - preliminary --- spec/controllers/work_packages_controller_spec.rb | 2 +- .../v2/planning_elements/_planning_element_api_rsb_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/controllers/work_packages_controller_spec.rb b/spec/controllers/work_packages_controller_spec.rb index 1bc3a75277..db763330ca 100644 --- a/spec/controllers/work_packages_controller_spec.rb +++ b/spec/controllers/work_packages_controller_spec.rb @@ -655,7 +655,7 @@ describe WorkPackagesController do describe "when work_package is a planning element" do let(:descendant_planning_element) { FactoryGirl.create(:planning_element, :project => project, - :parent => planning_element) } + :parent_issue_id => planning_element.id) } it "should return the work_packages ancestors" do controller.stub!(:work_package).and_return(descendant_planning_element) diff --git a/spec/views/api/v2/planning_elements/_planning_element_api_rsb_spec.rb b/spec/views/api/v2/planning_elements/_planning_element_api_rsb_spec.rb index a0b5353363..a45b66f6ff 100644 --- a/spec/views/api/v2/planning_elements/_planning_element_api_rsb_spec.rb +++ b/spec/views/api/v2/planning_elements/_planning_element_api_rsb_spec.rb @@ -143,12 +143,12 @@ describe 'api/v2/planning_elements/_planning_element.api' do before do FactoryGirl.create(:planning_element, :project_id => project.id, - :parent_id => planning_element.id, + :parent_issue_id => planning_element.id, :id => 1339, :subject => 'Child #1') FactoryGirl.create(:planning_element, :project_id => project.id, - :parent_id => planning_element.id, + :parent_issue_id => planning_element.id, :id => 1340, :subject => 'Child #2') end From 12de6e45b97bde75052d2717cc571d9f4a40f4b1 Mon Sep 17 00:00:00 2001 From: Jens Ulferts Date: Tue, 30 Jul 2013 15:27:50 +0200 Subject: [PATCH 09/29] removes reference to plugin from spec --- spec/models/issue_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index fd3f34eac6..5daf96ba62 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -81,7 +81,6 @@ describe Issue do @issue.status = @status_rejected @issue.priority = @priority_low @issue.estimated_hours = 3 - @issue.remaining_hours = 43 if Redmine::Plugin.all.collect(&:id).include?(:backlogs) @issue.save! initial_journal = @issue.journals.first From 6431fee0aac5d6a0fe28e05ecc5f71c19a702399 Mon Sep 17 00:00:00 2001 From: Jens Ulferts Date: Tue, 30 Jul 2013 15:28:21 +0200 Subject: [PATCH 10/29] reloads alternate_dates in specs where necessary --- spec/models/planning_element_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/models/planning_element_spec.rb b/spec/models/planning_element_spec.rb index e133012df5..e03f00d761 100644 --- a/spec/models/planning_element_spec.rb +++ b/spec/models/planning_element_spec.rb @@ -642,7 +642,7 @@ describe PlanningElement do let(:journal_scenario) { FactoryGirl.create(:scenario) } # PlanningElements create a alternate_date on save by default, so just use that it "should create a journal entry if a scenario gets assigned to an alternate date" do - alternate_date = journal_planning_element.alternate_dates.first.tap do |ad| + alternate_date = journal_planning_element.alternate_dates(true).first.tap do |ad| ad.scenario = journal_scenario end journal_planning_element.save! @@ -662,7 +662,7 @@ describe PlanningElement do it "should create a journal entry if a scenario gets removed from an alternate date" do # PlanningElements create a alternate_date on save by default, so just use that - alternate_date = journal_planning_element.alternate_dates.first.tap do |ad| + alternate_date = journal_planning_element.alternate_dates(true).first.tap do |ad| ad.scenario = journal_scenario end journal_planning_element.save! @@ -683,7 +683,7 @@ describe PlanningElement do it "should create seperate journal entries for start_date and due_date if only one of 'em is modified" do # PlanningElements create an alternate_date on save by default, so just use that - alternate_date = journal_planning_element.alternate_dates.first.tap do |ad| + alternate_date = journal_planning_element.alternate_dates(true).first.tap do |ad| ad.start_date = Date.today ad.due_date = Date.today + 1.month ad.scenario = journal_scenario From 58da8064e41a8df100d367fb632901951ca46c01 Mon Sep 17 00:00:00 2001 From: Jens Ulferts Date: Tue, 30 Jul 2013 15:58:38 +0200 Subject: [PATCH 11/29] reorders and breaks up before filter --- app/models/work_package.rb | 44 ++++++++++++++++++++++++++++++++------ 1 file changed, 37 insertions(+), 7 deletions(-) diff --git a/app/models/work_package.rb b/app/models/work_package.rb index 0ec6fc6686..02594d9419 100644 --- a/app/models/work_package.rb +++ b/app/models/work_package.rb @@ -50,10 +50,13 @@ class WorkPackage < ActiveRecord::Base acts_as_watchable + before_save :store_former_parent_id include OpenProject::NestedSet::WithRootIdScope after_save :reschedule_following_issues, :update_parent_attributes, :create_alternate_date + after_move :remove_invalid_relations, + :recalculate_attributes_for_former_parent after_destroy :update_parent_attributes acts_as_customizable @@ -113,6 +116,17 @@ class WorkPackage < ActiveRecord::Base :responsible_id register_on_journal_formatter :scenario_date, /^scenario_(\d+)_(start|due)_date$/ + # acts_as_journalized will create an initial journal on wp creation + # and touch the journaled object: + # journal.rb:47 + # + # This will result in optimistic locking increasing the lock_version attribute to 1. + # In order to avoid stale object errors we reload the attributes in question + # after the wp is created. + # As after_create is run before after_save, and journal creation is triggered by an + # after_save hook, we rely on after_save and a specific version, here. + after_save :reload_lock_and_timestamps, :if => Proc.new { |wp| wp.lock_version == 0 } + # Returns a SQL conditions string used to find all work units visible by the specified user def self.visible_condition(user, options={}) Project.allowed_to_condition(user, :view_work_packages, options) @@ -383,22 +397,26 @@ class WorkPackage < ActiveRecord::Base self.estimated_hours = nil if estimated_hours == 0.0 end - def create_alternate_date - if start_date_changed? or due_date_changed? - alternate_dates.create(:start_date => start_date, :due_date => due_date) - end + def store_former_parent_id + @former_parent_id = (parent_issue_id != parent_id) ? parent_id : false + true # force callback to return true end - def after_update_of_existing_tree_node(former_parent_id) + def remove_invalid_relations # delete invalid relations of all descendants self_and_descendants.each do |issue| issue.relations.each do |relation| relation.destroy unless relation.valid? end end + end + + def recalculate_attributes_for_former_parent + recalculate_attributes_for(@former_parent_id) if @former_parent_id + end - # update former parent - recalculate_attributes_for(former_parent_id) if former_parent_id + def reload_lock_and_timestamps + reload(:select => [:lock_version, :created_at, :updated_at]) end private @@ -411,4 +429,16 @@ class WorkPackage < ActiveRecord::Base time_entries.build(attributes) end + + def create_alternate_date + # This is a hack. + # It is required as long as alternate dates exist/are not moved up to work_packages. + # Its purpose is to allow for setting the after_save filter in the correct order + # before acts as journalized and the cleanup method reload_lock_and_timestamps. + return true unless respond_to?(:alternate_dates) + + if start_date_changed? or due_date_changed? + alternate_dates.create(:start_date => start_date, :due_date => due_date) + end + end end From a391d12f128f7abfeb761daa9979a87c00f9f6d8 Mon Sep 17 00:00:00 2001 From: Jens Ulferts Date: Tue, 30 Jul 2013 16:01:33 +0200 Subject: [PATCH 12/29] adapts form to use journal notes --- app/views/planning_elements/_form.html.erb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/views/planning_elements/_form.html.erb b/app/views/planning_elements/_form.html.erb index abdb187277..2dfcf1fb50 100644 --- a/app/views/planning_elements/_form.html.erb +++ b/app/views/planning_elements/_form.html.erb @@ -146,11 +146,11 @@ See doc/COPYRIGHT.rdoc for more details. <% unless planning_element.new_record? %>

<%= l(:field_notes) %>

-