[#441] Journals should touch their journaled to update updated_on/at fields

bfba5e21 · Eric Davis · 85c7cfd0 · bfba5e21 · bfba5e21 · bfba5e21
Commit bfba5e21 authored Jun 02, 2011 by Eric Davis
9 changed files
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -529,6 +529,20 @@ class Issue < ActiveRecord::Base
    "#{tracker} ##{id}: #{subject}"
  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}"

--- a/app/models/journal.rb
+++ b/app/models/journal.rb
@@ -23,7 +23,7 @@ class Journal < ActiveRecord::Base
  # Make sure each journaled model instance only has unique version ids
  validates_uniqueness_of :version, :scope => [:journaled_id, :type]
-  belongs_to :journaled
+  belongs_to :journaled, :touch => true
  belongs_to :user
  # ActiveRecord::Base#changes is an existing method, so before serializing the +changes+ column,

--- a/app/models/journal_observer.rb
+++ b/app/models/journal_observer.rb
@@ -15,7 +15,7 @@ class JournalObserver < ActiveRecord::Observer
  attr_accessor :send_notification
  def after_create(journal)
-    if journal.type == "IssueJournal" and journal.version > 1 and self.send_notification
+    if journal.type == "IssueJournal" and !journal.initial? and send_notification
      after_create_issue_journal(journal)
    end
    clear_notification

--- a/test/unit/issue_nested_set_test.rb
+++ b/test/unit/issue_nested_set_test.rb
@@ -30,8 +30,10 @@ class IssueNestedSetTest < ActiveSupport::TestCase
    issue1.reload
    issue2.reload
-    assert_equal [issue1.id, nil, 1, 2], [issue1.root_id, issue1.parent_id, issue1.lft, issue1.rgt]
+    assert_equal issue1.id, issue1.root_id
-    assert_equal [issue2.id, nil, 1, 2], [issue2.root_id, issue2.parent_id, issue2.lft, issue2.rgt]
+    assert issue1.leaf?
+    assert_equal issue2.id, issue2.root_id
+    assert issue2.leaf?
  end
  def test_create_child_issue
@@ -40,8 +42,8 @@ class IssueNestedSetTest < ActiveSupport::TestCase
    parent.reload
    child.reload
-    assert_equal [parent.id, nil, 1, 4], [parent.root_id, parent.parent_id, parent.lft, parent.rgt]
+    assert_equal [parent.id, nil, 3], [parent.root_id, parent.parent_id, parent.rgt - parent.lft]
-    assert_equal [parent.id, parent.id, 2, 3], [child.root_id, child.parent_id, child.lft, child.rgt]
+    assert_equal [parent.id, parent.id, 1], [child.root_id, child.parent_id, child.rgt - child.lft]
  end
  def test_creating_a_child_in_different_project_should_not_validate
@@ -62,9 +64,9 @@ class IssueNestedSetTest < ActiveSupport::TestCase
    parent1.reload
    parent2.reload
-    assert_equal [parent1.id, 1, 6], [parent1.root_id, parent1.lft, parent1.rgt]
+    assert_equal [parent1.id, 5], [parent1.root_id, parent1.nested_set_span]
-    assert_equal [parent1.id, 4, 5], [parent2.root_id, parent2.lft, parent2.rgt]
+    assert_equal [parent1.id, 1], [parent2.root_id, parent2.nested_set_span]
-    assert_equal [parent1.id, 2, 3], [child.root_id, child.lft, child.rgt]
+    assert_equal [parent1.id, 1], [child.root_id, child.nested_set_span]
  end
  def test_move_a_child_to_root
@@ -78,9 +80,9 @@ class IssueNestedSetTest < ActiveSupport::TestCase
    parent1.reload
    parent2.reload
-    assert_equal [parent1.id, 1, 2], [parent1.root_id, parent1.lft, parent1.rgt]
+    assert_equal [parent1.id, 1], [parent1.root_id, parent1.nested_set_span]
-    assert_equal [parent2.id, 1, 2], [parent2.root_id, parent2.lft, parent2.rgt]
+    assert_equal [parent2.id, 1], [parent2.root_id, parent2.nested_set_span]
-    assert_equal [child.id, 1, 2], [child.root_id, child.lft, child.rgt]
+    assert_equal [child.id, 1], [child.root_id, child.nested_set_span]
  end
  def test_move_a_child_to_another_issue
@@ -94,9 +96,9 @@ class IssueNestedSetTest < ActiveSupport::TestCase
    parent1.reload
    parent2.reload
-    assert_equal [parent1.id, 1, 2], [parent1.root_id, parent1.lft, parent1.rgt]
+    assert_equal [parent1.id, 1], [parent1.root_id, parent1.nested_set_span]
-    assert_equal [parent2.id, 1, 4], [parent2.root_id, parent2.lft, parent2.rgt]
+    assert_equal [parent2.id, 3], [parent2.root_id, parent2.nested_set_span]
-    assert_equal [parent2.id, 2, 3], [child.root_id, child.lft, child.rgt]
+    assert_equal [parent2.id, 1], [child.root_id, child.nested_set_span]
  end
  def test_move_a_child_with_descendants_to_another_issue
@@ -110,10 +112,10 @@ class IssueNestedSetTest < ActiveSupport::TestCase
    child.reload
    grandchild.reload
-    assert_equal [parent1.id, 1, 6], [parent1.root_id, parent1.lft, parent1.rgt]
+    assert_equal [parent1.id, 5], [parent1.root_id, parent1.nested_set_span]
-    assert_equal [parent2.id, 1, 2], [parent2.root_id, parent2.lft, parent2.rgt]
+    assert_equal [parent2.id, 1], [parent2.root_id, parent2.nested_set_span]
-    assert_equal [parent1.id, 2, 5], [child.root_id, child.lft, child.rgt]
+    assert_equal [parent1.id, 3], [child.root_id, child.nested_set_span]
-    assert_equal [parent1.id, 3, 4], [grandchild.root_id, grandchild.lft, grandchild.rgt]
+    assert_equal [parent1.id, 1], [grandchild.root_id, grandchild.nested_set_span]
    child.reload.parent_issue_id = parent2.id
    child.save!
@@ -122,10 +124,10 @@ class IssueNestedSetTest < ActiveSupport::TestCase
    parent1.reload
    parent2.reload
-    assert_equal [parent1.id, 1, 2], [parent1.root_id, parent1.lft, parent1.rgt]
+    assert_equal [parent1.id, 1], [parent1.root_id, parent1.nested_set_span]
-    assert_equal [parent2.id, 1, 6], [parent2.root_id, parent2.lft, parent2.rgt]
+    assert_equal [parent2.id, 5], [parent2.root_id, parent2.nested_set_span]
-    assert_equal [parent2.id, 2, 5], [child.root_id, child.lft, child.rgt]
+    assert_equal [parent2.id, 3], [child.root_id, child.nested_set_span]
-    assert_equal [parent2.id, 3, 4], [grandchild.root_id, grandchild.lft, grandchild.rgt]
+    assert_equal [parent2.id, 1], [grandchild.root_id, grandchild.nested_set_span]
  end
  def test_move_a_child_with_descendants_to_another_project
@@ -138,9 +140,9 @@ class IssueNestedSetTest < ActiveSupport::TestCase
    grandchild.reload
    parent1.reload
-    assert_equal [1, parent1.id, 1, 2], [parent1.project_id, parent1.root_id, parent1.lft, parent1.rgt]
+    assert_equal [1, parent1.id, 1], [parent1.project_id, parent1.root_id, parent1.nested_set_span]
-    assert_equal [2, child.id, 1, 4], [child.project_id, child.root_id, child.lft, child.rgt]
+    assert_equal [2, child.id, 3], [child.project_id, child.root_id, child.nested_set_span]
-    assert_equal [2, child.id, 2, 3], [grandchild.project_id, grandchild.root_id, grandchild.lft, grandchild.rgt]
+    assert_equal [2, child.id, 1], [grandchild.project_id, grandchild.root_id, grandchild.nested_set_span]
  end
  def test_invalid_move_to_another_project
@@ -150,7 +152,7 @@ class IssueNestedSetTest < ActiveSupport::TestCase
    Project.find(2).tracker_ids = [1]
    parent1.reload
-    assert_equal [1, parent1.id, 1, 6], [parent1.project_id, parent1.root_id, parent1.lft, parent1.rgt]
+    assert_equal [1, parent1.id, 5], [parent1.project_id, parent1.root_id, parent1.nested_set_span]
    # child can not be moved to Project 2 because its child is on a disabled tracker
    assert_equal false, Issue.find(child.id).move_to_project(Project.find(2))
@@ -159,9 +161,9 @@ class IssueNestedSetTest < ActiveSupport::TestCase
    parent1.reload
    # no change
-    assert_equal [1, parent1.id, 1, 6], [parent1.project_id, parent1.root_id, parent1.lft, parent1.rgt]
+    assert_equal [1, parent1.id, 5], [parent1.project_id, parent1.root_id, parent1.nested_set_span]
-    assert_equal [1, parent1.id, 2, 5], [child.project_id, child.root_id, child.lft, child.rgt]
+    assert_equal [1, parent1.id, 3], [child.project_id, child.root_id, child.nested_set_span]
-    assert_equal [1, parent1.id, 3, 4], [grandchild.project_id, grandchild.root_id, grandchild.lft, grandchild.rgt]
+    assert_equal [1, parent1.id, 1], [grandchild.project_id, grandchild.root_id, grandchild.nested_set_span]
  end
  def test_moving_an_issue_to_a_descendant_should_not_validate
@@ -212,8 +214,8 @@ class IssueNestedSetTest < ActiveSupport::TestCase
    issue4.reload
    assert !Issue.exists?(issue2.id)
    assert !Issue.exists?(issue3.id)
-    assert_equal [issue1.id, 1, 4], [issue1.root_id, issue1.lft, issue1.rgt]
+    assert_equal [issue1.id, 3], [issue1.root_id, issue1.nested_set_span]
-    assert_equal [issue1.id, 2, 3], [issue4.root_id, issue4.lft, issue4.rgt]
+    assert_equal [issue1.id, 1], [issue4.root_id, issue4.nested_set_span]
  end
  def test_destroy_parent_issue_updated_during_children_destroy

--- a/test/unit/issue_test.rb
+++ b/test/unit/issue_test.rb
@@ -372,6 +372,7 @@ class IssueTest < ActiveSupport::TestCase
  def test_move_to_another_project_should_clear_fixed_version_when_not_shared
    issue = Issue.find(1)
    issue.update_attribute(:fixed_version_id, 1)
+    issue.reload
    assert issue.move_to_project(Project.find(2))
    issue.reload
    assert_equal 2, issue.project_id
@@ -382,6 +383,7 @@ class IssueTest < ActiveSupport::TestCase
  def test_move_to_another_project_should_keep_fixed_version_when_shared_with_the_target_project
    issue = Issue.find(1)
    issue.update_attribute(:fixed_version_id, 4)
+    issue.reload
    assert issue.move_to_project(Project.find(5))
    issue.reload
    assert_equal 5, issue.project_id
@@ -392,6 +394,7 @@ class IssueTest < ActiveSupport::TestCase
  def test_move_to_another_project_should_clear_fixed_version_when_not_shared_with_the_target_project
    issue = Issue.find(1)
    issue.update_attribute(:fixed_version_id, 1)
+    issue.reload
    assert issue.move_to_project(Project.find(5))
    issue.reload
    assert_equal 5, issue.project_id
@@ -402,6 +405,7 @@ class IssueTest < ActiveSupport::TestCase
  def test_move_to_another_project_should_keep_fixed_version_when_shared_systemwide
    issue = Issue.find(1)
    issue.update_attribute(:fixed_version_id, 7)
+    issue.reload
    assert issue.move_to_project(Project.find(2))
    issue.reload
    assert_equal 2, issue.project_id

--- a/test/unit/journal_observer_test.rb
+++ b/test/unit/journal_observer_test.rb
@@ -13,103 +13,97 @@
 require File.expand_path('../../test_helper', __FILE__)
 class JournalObserverTest < ActiveSupport::TestCase
-  fixtures :issues, :issue_statuses, :journals
  def setup
+    @user = User.generate!(:mail_notification => 'all')
+    @project = Project.generate!
+    User.add_to_project(@user, @project, Role.generate!(:permissions => [:view_issues, :edit_issues]))
+    @issue = Issue.generate_for_project!(@project)
    ActionMailer::Base.deliveries.clear
-    @journal = Journal.find 1
-    if (i = Issue.find(:first)).journals.empty?
-      i.init_journal(User.current, 'Creation') # Make sure the initial journal is created
-      i.save
-    end
-  end
-  # context: issue_updated notified_events
-  def test_create_should_send_email_notification_with_issue_updated
-    Setting.notified_events = ['issue_updated']
-    issue = Issue.find(:first)
-    user = User.find(:first)
-    issue.init_journal(user)
-    assert issue.send(:create_journal)
-    assert_equal 1, ActionMailer::Base.deliveries.size
-  end
-  def test_create_should_not_send_email_notification_without_issue_updated
-    Setting.notified_events = []
-    issue = Issue.find(:first)
-    user = User.find(:first)
-    issue.init_journal(user)
-    assert issue.save
-    assert_equal 0, ActionMailer::Base.deliveries.size
-  end
-  # context: issue_note_added notified_events
-  def test_create_should_send_email_notification_with_issue_note_added
-    Setting.notified_events = ['issue_note_added']
-    issue = Issue.find(:first)
-    user = User.find(:first)
-    issue.init_journal(user, 'This update has a note')
-    assert issue.save
-    assert_equal 1, ActionMailer::Base.deliveries.size
  end
-  def test_create_should_not_send_email_notification_without_issue_note_added
+  context "#after_create for 'issue_updated'" do
-    Setting.notified_events = []
+    should "should send a notification when configured as a notification" do
-    issue = Issue.find(:first)
+      Setting.notified_events = ['issue_updated']
-    user = User.find(:first)
+      assert_difference('ActionMailer::Base.deliveries.size') do
-    issue.init_journal(user, 'This update has a note')
+        @issue.init_journal(@user)
+        @issue.subject = "A change to the issue"
+        assert @issue.save
+      end
+    end
-    assert issue.save
+    should "not send a notification with not configured" do
-    assert_equal 0, ActionMailer::Base.deliveries.size
+      Setting.notified_events = []
+      assert_no_difference('ActionMailer::Base.deliveries.size') do
+        @issue.init_journal(@user)
+        @issue.subject = "A change to the issue"
+        assert @issue.save
+      end
+    end
  end
-  # context: issue_status_updated notified_events
+  context "#after_create for 'issue_note_added'" do
-  def test_create_should_send_email_notification_with_issue_status_updated
+    should "should send a notification when configured as a notification" do
-    Setting.notified_events = ['issue_status_updated']
+      Setting.notified_events = ['issue_note_added']
-    issue = Issue.find(:first)
+      assert_difference('ActionMailer::Base.deliveries.size') do
-    user = User.find(:first)
+        @issue.init_journal(@user, 'This update has a note')
-    issue.init_journal(user)
+        assert @issue.save
-    issue.status = IssueStatus.last
+      end
+    end
-    assert issue.save
+    should "not send a notification with not configured" do
-    assert_equal 1, ActionMailer::Base.deliveries.size
+      Setting.notified_events = []
+      assert_no_difference('ActionMailer::Base.deliveries.size') do
+        @issue.init_journal(@user, 'This update has a note')
+        assert @issue.save
+      end
+    end
  end
-  def test_create_should_not_send_email_notification_without_issue_status_updated
+  context "#after_create for 'issue_status_updated'" do
-    Setting.notified_events = []
+    should "should send a notification when configured as a notification" do
-    issue = Issue.find(:first)
+      Setting.notified_events = ['issue_status_updated']
-    user = User.find(:first)
+      assert_difference('ActionMailer::Base.deliveries.size') do
-    issue.init_journal(user)
+        @issue.init_journal(@user)
-    issue.status = IssueStatus.last
+        @issue.status = IssueStatus.generate!
+        assert @issue.save
-    assert issue.save
+      end
-    assert_equal 0, ActionMailer::Base.deliveries.size
-  end
+    end
-  # context: issue_priority_updated notified_events
+    should "not send a notification with not configured" do
-  def test_create_should_send_email_notification_with_issue_priority_updated
+      Setting.notified_events = []
-    Setting.notified_events = ['issue_priority_updated']
+      assert_no_difference('ActionMailer::Base.deliveries.size') do
-    issue = Issue.find(:first)
+        @issue.init_journal(@user)
-    user = User.find(:first)
+        @issue.status = IssueStatus.generate!
-    issue.init_journal(user)
+        assert @issue.save
-    issue.priority = IssuePriority.last
-    assert issue.save
+      end
-    assert_equal 1, ActionMailer::Base.deliveries.size
+    end
  end
-  def test_create_should_not_send_email_notification_without_issue_priority_updated
+  context "#after_create for 'issue_priority_updated'" do
-    Setting.notified_events = []
+    should "should send a notification when configured as a notification" do
-    issue = Issue.find(:first)
+      Setting.notified_events = ['issue_priority_updated']
-    user = User.find(:first)
+      assert_difference('ActionMailer::Base.deliveries.size') do
-    issue.init_journal(user)
+        @issue.init_journal(@user)
-    issue.priority = IssuePriority.last
+        @issue.priority = IssuePriority.generate!
+        assert @issue.save
+      end
+    end
-    assert issue.save
+    should "not send a notification with not configured" do
-    assert_equal 0, ActionMailer::Base.deliveries.size
+      Setting.notified_events = []
+      assert_no_difference('ActionMailer::Base.deliveries.size') do
+        @issue.init_journal(@user)
+        @issue.priority = IssuePriority.generate!
+        assert @issue.save
+      end
+    end
  end
 end
--- a/test/unit/journal_test.rb
+++ b/test/unit/journal_test.rb
@@ -36,12 +36,13 @@ class JournalTest < ActiveSupport::TestCase
    ActionMailer::Base.deliveries.clear
    issue = Issue.find(:first)
    if issue.journals.empty?
-      issue.init_journal(User.current, "This journal represents the creational journal version 1")
+      issue.init_journal(User.current, "This journal represents the creationa of journal version 1")
      issue.save
    end
    user = User.find(:first)
    assert_equal 0, ActionMailer::Base.deliveries.size
+    issue.reload
    issue.update_attribute(:subject, "New subject to trigger automatic journal entry")
    assert_equal 1, ActionMailer::Base.deliveries.size
  end
@@ -58,4 +59,18 @@ class JournalTest < ActiveSupport::TestCase
    end
    assert_equal 0, ActionMailer::Base.deliveries.size
  end
+  test "creating a journal should update the updated_on value of the parent record (touch)" do
+    @user = User.generate!
+    @project = Project.generate!
+    @issue = Issue.generate_for_project!(@project).reload
+    start = @issue.updated_on
+    assert_difference("Journal.count") do
+      @issue.init_journal(@user, "A note")
+      @issue.save
+    end
+    assert_not_equal start, @issue.reload.updated_on
+  end
 end
--- a/test/unit/mail_handler_test.rb
+++ b/test/unit/mail_handler_test.rb
@@ -60,7 +60,8 @@ class MailHandlerTest < ActiveSupport::TestCase
    assert_equal Version.find_by_name('alpha'), issue.fixed_version
    assert_equal 2.5, issue.estimated_hours
    assert_equal 30, issue.done_ratio
-    assert_equal [issue.id, 1, 2], [issue.root_id, issue.lft, issue.rgt]
+    assert_equal issue.id, issue.root_id
+    assert issue.leaf?
    # keywords should be removed from the email body
    assert !issue.description.match(/^Project:/i)
    assert !issue.description.match(/^Status:/i)
@@ -208,7 +209,8 @@ class MailHandlerTest < ActiveSupport::TestCase
        assert issue.is_a?(Issue)
        assert issue.author.anonymous?
        assert !issue.project.is_public?
-        assert_equal [issue.id, 1, 2], [issue.root_id, issue.lft, issue.rgt]
+        assert_equal issue.id, issue.root_id
+        assert issue.leaf?
      end
    end
  end

--- a/vendor/plugins/acts_as_journalized/lib/redmine/acts/journalized/reversion.rb
+++ b/vendor/plugins/acts_as_journalized/lib/redmine/acts/journalized/reversion.rb
@@ -107,4 +107,4 @@ module Redmine::Acts::Journalized
        end
    end
  end
 end
\ No newline at end of file