Merge pull request #190 from ixti/fix/bulk_edit_journals

Fix/bulk edit journals

.rubocop.yml

@@ -1,21 +1,18 @@
 ## Styles ######################################################################
 
-Style/AlignParameters:
+Layout/AlignParameters:
   EnforcedStyle: with_fixed_indentation
 
 Style/BracesAroundHashParameters:
   Enabled: false
 
-Style/EmptyLineBetweenDefs:
+Layout/EmptyLineBetweenDefs:
   AllowAdjacentOneLineDefs: true
 
-Style/Encoding:
-  EnforcedStyle: when_needed
-
 Style/HashSyntax:
   EnforcedStyle: hash_rockets
 
-Style/IndentHash:
+Layout/IndentHash:
   EnforcedStyle: consistent
 
 # New lambda syntax is as ugly to me as new syntax of Hash.
@@ -28,7 +25,7 @@ Style/Lambda:
 #     conn.hset    :k1, now
 #     conn.hincrby :k2, 123
 #   end
-SingleSpaceBeforeFirstArg:
+Layout/SingleSpaceBeforeFirstArg:
   Enabled: false
 
 Style/StringLiterals:

db/migrate/20180122193833_remove_wrong_tag_list_journals.rb

@@ -0,0 +1,12 @@
+class RemoveWrongTagListJournals < ActiveRecord::Migration
+  def change
+    # First, remove from journal details the tag changes where the new value is the same with the old_value
+    JournalDetail.where("prop_key = 'tag_list' AND old_value = value").destroy_all
+
+    # Remove all journals that do not have notes and neither attributes changes in the journal_details table
+    Journal.joins("LEFT OUTER JOIN #{JournalDetail.table_name} ON #{JournalDetail.table_name}.journal_id = #{Journal.table_name}.id")
+      .where("(#{Journal.table_name}.notes IS NULL OR #{Journal.table_name}.notes = '') AND #{Journal.table_name}.journalized_type = 'Issue'")
+      .where("#{JournalDetail.table_name}.journal_id IS NULL")
+      .destroy_all
+  end
+end

lib/redmine_tags/hooks/model_issue_hook.rb

@@ -6,7 +6,7 @@ def controller_issues_edit_before_save(context = {})
       end
 
       def controller_issues_bulk_edit_before_save(context = {})
-        bulk_update_tags_to_issues context, true
+        bulk_update_tags_to_issues context
       end
 
       # Issue has an after_save method that calls reload (update_nested_set_attributes)
@@ -34,7 +34,7 @@ def save_tags_to_issue(context, create_journal)
         end
       end
 
-      def bulk_update_tags_to_issues(context, create_journal)
+      def bulk_update_tags_to_issues(context)
         params = context[:params]
         issue = context[:issue]
         common_tags = []
@@ -49,17 +49,19 @@ def bulk_update_tags_to_issues(context, create_journal)
           tags_to_add = tag_list - common_tags
           tags_to_remove = common_tags - tag_list
 
-          # variables for journal entry
-          old_tags = current_tags.to_s
-          new_tags = current_tags.add(tags_to_add).remove(tags_to_remove)
+          if tags_to_add.any? || tags_to_remove.any?
+            # variables for journal entry
+            old_tags = current_tags.to_s
+            new_tags = current_tags.add(tags_to_add).remove(tags_to_remove)
 
-          issue.tag_list = new_tags
-          # without this when reload called in Issue#save all changes will be
-          # gone :(
-          issue.save_tags
-          create_journal_entry(issue, old_tags, new_tags) if create_journal
+            issue.tag_list = new_tags
+            # without this when reload called in Issue#save all changes will be
+            # gone :(
+            issue.save_tags
+            create_journal_entry(issue, old_tags, new_tags)
 
-          Issue.remove_unused_tags!
+            Issue.remove_unused_tags!
+          end
         end
       end
 

test/functional/issues_controller_test.rb

@@ -184,4 +184,36 @@ def test_bulk_edit_with_common_tag_and_remove_common_tag
     assert_equal [], Issue.find(4).tag_list
     assert_equal ['Front End'], Issue.find(6).tag_list
   end
+
+  def test_bulk_edit_journal_without_tag_changing
+    # journal should not log tags changing when tags were not changed
+    @request.session[:user_id] = 2
+    post :bulk_update,
+         :ids => [2, 7],
+         :issue => {
+           :new_tag_list => '',
+           :priority_id => 7
+         },
+         :common_tags => ''
+    assert_response 302
+
+    assert_equal ['priority_id'], Issue.find(2).journals.last.details.map(&:prop_key)
+    assert_equal ['priority_id'], Issue.find(7).journals.last.details.map(&:prop_key)
+    assert_equal 7, Issue.find(2).priority_id
+    assert_equal 7, Issue.find(7).priority_id
+  end
+
+  def test_bulk_edit_journal_with_tag_changing
+    # journal should log tags changing when tags were changed
+    @request.session[:user_id] = 2
+    post :bulk_update,
+         :ids => [2, 7],
+         :issue => {
+           :new_tag_list => ['Production', 'Security']
+         }
+    assert_response 302
+
+    assert_equal ['tag_list'], Issue.find(2).journals.last.details.map(&:prop_key)
+    assert_equal ['tag_list'], Issue.find(7).journals.last.details.map(&:prop_key)
+  end
 end