Merge pull request #672 from opf/fix/ignore_empty_string_changes

app/models/journal.rb

@@ -149,7 +149,11 @@ class Journal < ActiveRecord::Base
         normalized_data = JournalManager.normalize_newlines(data.journaled_attributes)
         normalized_predecessor_data = JournalManager.normalize_newlines(predecessor.data.journaled_attributes)
 
-        normalized_data.select{|k,v| v != normalized_predecessor_data[k]}.each do |k, v|
+        normalized_data.select do |k,v|
+          # we dont record changes for changes from nil to empty strings and vice versa
+          pred = normalized_predecessor_data[k]
+          v != pred && (v.present? || pred.present?)
+        end.each do |k, v|
           @changes[k] = [normalized_predecessor_data[k], v]
         end
       end

app/models/journal_manager.rb

@@ -53,7 +53,8 @@ class JournalManager
     current = normalize_newlines(current)
     predecessor = normalize_newlines(predecessor)
 
-    return predecessor.map{|k,v| current[k.to_s] != v}
+    # we generally ignore changes from blank to blank
+    return predecessor.map{|k,v| current[k.to_s] != v && (v.present? || current[k.to_s].present?)}
                       .inject(false) { |r, c| r || c }
   end
 

doc/CHANGELOG.md

@@ -36,6 +36,7 @@ See doc/COPYRIGHT.rdoc for more details.
 * `#3020` Fix: E-mail Message-ID header is not unique for Work Package mails
 * `#3030` Users preferences for order of comments is ignored on wp comments
 * `#3032` Fix: Work package comments aren't editable by authors
+* `#3038` Fix: Journal changes are recorded for empty text fields
 * API v2: Improve timelines performance by not filtering statuses by visible projects
 * Add hook so that plugins can register assets, which are loaded on every page
 

spec/models/journal_manager_spec.rb

@@ -2,19 +2,74 @@ require 'spec_helper'
 
 describe JournalManager do
   describe '#self.changed?' do
-    context 'when only the newline character representation has changed' do
-      let(:description_with_newline) { "Description\nContains newline character" }
-      let(:description_with_other_newline) { description_with_newline.gsub("\n", "\r\n") }
-      let(:journable) do
-        FactoryGirl.create(:work_package, description: description_with_newline).tap do |journable|
-          # replace newline character and apply another change
-          journable.assign_attributes description: description_with_other_newline
-        end
+    let(:journable) do
+      FactoryGirl.create(:work_package, description: old).tap do |journable|
+        # replace newline character and apply another change
+        journable.assign_attributes description: changed
       end
+    end
+
+    context 'when only the newline character representation has changed' do
+      let(:old) { "Description\nContains newline character" }
+      let(:changed) { old.gsub("\n", "\r\n") }
+
+      subject { JournalManager.changed? journable }
+
+      it { should be_false }
+    end
+
+    context 'when old value is nil and changed value is an empty string' do
+      let(:old) { nil }
+      let(:changed) { '' }
 
       subject { JournalManager.changed? journable }
 
       it { should be_false }
     end
+
+    context 'when changed value is nil and old value is an empty string' do
+      let(:old) { '' }
+      let(:changed) { nil }
+
+      subject { JournalManager.changed? journable }
+
+      it { should be_false }
+    end
+
+    context 'when changed value has a value and old value is an empty string' do
+      let(:old) { '' }
+      let(:changed) { 'Changed text' }
+
+      subject { JournalManager.changed? journable }
+
+      it { should be_true }
+    end
+
+    context 'when changed value has a value and old value is nil' do
+      let(:old) { nil }
+      let(:changed) { 'Changed text' }
+
+      subject { JournalManager.changed? journable }
+
+      it { should be_true }
+    end
+
+    context 'when changed value is nil and old value was some text' do
+      let(:old) { 'Old text' }
+      let(:changed) { nil }
+
+      subject { JournalManager.changed? journable }
+
+      it { should be_true }
+    end
+
+    context 'when changed value is an empty string and old value was some text' do
+      let(:old) { 'Old text' }
+      let(:changed) { '' }
+
+      subject { JournalManager.changed? journable }
+
+      it { should be_true }
+    end
   end
 end