Code cleanup.

git-svn-id: http://svn.redmine.org/redmine/trunk@13508 e93f8b46-1217-0410-a6f0-8f06a7374b81

4 files changed, 96 insertions, 19 deletions

diff --git a/app/models/changeset.rb b/app/models/changeset.rb
index c54ad0bc3..4eedae145 100644
--- a/app/models/changeset.rb
+++ b/app/models/changeset.rb
@@ -227,7 +227,7 @@ class Changeset < ActiveRecord::Base
     # the issue may have been updated by the closure of another one (eg. duplicate)
     issue.reload
     # don't change the status is the issue is closed
-    return if issue.status && issue.status.is_closed?
+    return if issue.closed?
 
     journal = issue.init_journal(user || User.anonymous,
                                  ll(Setting.default_language,
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 910359bf5..78854568a 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -588,7 +588,7 @@ class Issue < ActiveRecord::Base
     if fixed_version
       if !assignable_versions.include?(fixed_version)
         errors.add :fixed_version_id, :inclusion
-      elsif reopened? && fixed_version.closed?
+      elsif reopening? && fixed_version.closed?
         errors.add :base, I18n.t(:error_can_not_reopen_issue_on_closed_version)
       end
     end
@@ -691,31 +691,33 @@ class Issue < ActiveRecord::Base
     status.present? && status.is_closed?
   end
 
+  # Returns true if the issue was closed when loaded
+  def was_closed?
+    status_was.present? && status_was.is_closed?
+  end
+
   # Return true if the issue is being reopened
-  def reopened?
-    if !new_record? && status_id_changed?
-      status_was = IssueStatus.find_by_id(status_id_was)
-      status_new = IssueStatus.find_by_id(status_id)
-      if status_was && status_new && status_was.is_closed? && !status_new.is_closed?
-        return true
-      end
+  def reopening?
+    if new_record?
+      false
+    else
+      status_id_changed? && !closed? && was_closed?
     end
-    false
   end
+  alias :reopened? :reopening?
 
   # Return true if the issue is being closed
   def closing?
-    if !new_record? && status_id_changed?
-      if closed? && status_was && !status_was.is_closed?
-        return true
-      end
+    if new_record?
+      closed?
+    else
+      status_id_changed? && closed? && !was_closed?
     end
-    false
   end
 
   # Returns true if the issue is overdue
   def overdue?
-    !due_date.nil? && (due_date < Date.today) && !status.is_closed?
+    due_date.present? && (due_date < Date.today) && !closed?
   end
 
   # Is the amount of work done less than it should for the due date
@@ -1511,7 +1513,7 @@ class Issue < ActiveRecord::Base
   # The closed_on attribute stores the time of the last closing
   # and is preserved when the issue is reopened.
   def update_closed_on
-    if closing? || (new_record? && closed?)
+    if closing?
       self.closed_on = updated_on
     end
   end
diff --git a/test/unit/issue_test.rb b/test/unit/issue_test.rb
index 2cf00f5ff..834792458 100644
--- a/test/unit/issue_test.rb
+++ b/test/unit/issue_test.rb
@@ -2398,6 +2398,13 @@ class IssueTest < ActiveSupport::TestCase
     assert_equal IssueStatus.find(1), issue.status_was
   end
 
+  def test_status_was_should_return_status_before_change_with_status_id
+    issue = Issue.find(1)
+    assert_equal IssueStatus.find(1), issue.status
+    issue.status_id = 2
+    assert_equal IssueStatus.find(1), issue.status_was
+  end
+
   def test_status_was_should_be_reset_on_save
     issue = Issue.find(1)
     issue.status = IssueStatus.find(2)
@@ -2405,4 +2412,72 @@ class IssueTest < ActiveSupport::TestCase
     assert issue.save!
     assert_equal IssueStatus.find(2), issue.status_was
   end
+
+  def test_closing_should_return_true_when_closing_an_issue
+    issue = Issue.find(1)
+    issue.status = IssueStatus.find(2)
+    assert_equal false, issue.closing?
+    issue.status = IssueStatus.find(5)
+    assert_equal true, issue.closing?
+  end
+
+  def test_closing_should_return_true_when_closing_an_issue_with_status_id
+    issue = Issue.find(1)
+    issue.status_id = 2
+    assert_equal false, issue.closing?
+    issue.status_id = 5
+    assert_equal true, issue.closing?
+  end
+
+  def test_closing_should_return_true_for_new_closed_issue
+    issue = Issue.new
+    assert_equal false, issue.closing?
+    issue.status = IssueStatus.find(5)
+    assert_equal true, issue.closing?
+  end
+
+  def test_closing_should_return_true_for_new_closed_issue_with_status_id
+    issue = Issue.new
+    assert_equal false, issue.closing?
+    issue.status_id = 5
+    assert_equal true, issue.closing?
+  end
+
+  def test_closing_should_be_reset_after_save
+    issue = Issue.find(1)
+    issue.status_id = 5
+    assert_equal true, issue.closing?
+    issue.save!
+    assert_equal false, issue.closing?
+  end
+
+  def test_reopening_should_return_true_when_reopening_an_issue
+    issue = Issue.find(8)
+    issue.status = IssueStatus.find(6)
+    assert_equal false, issue.reopening?
+    issue.status = IssueStatus.find(2)
+    assert_equal true, issue.reopening?
+  end
+
+  def test_reopening_should_return_true_when_reopening_an_issue_with_status_id
+    issue = Issue.find(8)
+    issue.status_id = 6
+    assert_equal false, issue.reopening?
+    issue.status_id = 2
+    assert_equal true, issue.reopening?
+  end
+
+  def test_reopening_should_return_false_for_new_open_issue
+    issue = Issue.new
+    issue.status = IssueStatus.find(1)
+    assert_equal false, issue.reopening?
+  end
+
+  def test_reopening_should_be_reset_after_save
+    issue = Issue.find(8)
+    issue.status_id = 2
+    assert_equal true, issue.reopening?
+    issue.save!
+    assert_equal false, issue.reopening?
+  end
 end
diff --git a/test/unit/repository_test.rb b/test/unit/repository_test.rb
index 9537e50a6..54f72bbe8 100644
--- a/test/unit/repository_test.rb
+++ b/test/unit/repository_test.rb
@@ -212,7 +212,7 @@ class RepositoryTest < ActiveSupport::TestCase
 
     # make sure issue 1 is not already closed
     fixed_issue = Issue.find(1)
-    assert !fixed_issue.status.is_closed?
+    assert !fixed_issue.closed?
     old_status = fixed_issue.status
 
     with_settings :notified_events => %w(issue_added issue_updated) do
@@ -222,7 +222,7 @@ class RepositoryTest < ActiveSupport::TestCase
 
     # fixed issues
     fixed_issue.reload
-    assert fixed_issue.status.is_closed?
+    assert fixed_issue.closed?
     assert_equal 90, fixed_issue.done_ratio
     assert_equal [101], fixed_issue.changeset_ids