From 309106403108e71900c08166336519caf9551213 Mon Sep 17 00:00:00 2001 From: Jens Ulferts Date: Tue, 21 Aug 2018 13:59:33 +0200 Subject: [PATCH 1/3] linting --- app/models/journal_manager.rb | 5 +- lib/api/v3/activities/activity_representer.rb | 16 +++--- spec/models/journal_manager_spec.rb | 52 +++++++++---------- 3 files changed, 37 insertions(+), 36 deletions(-) diff --git a/app/models/journal_manager.rb b/app/models/journal_manager.rb index 340f04738e..81d9ac0420 100644 --- a/app/models/journal_manager.rb +++ b/app/models/journal_manager.rb @@ -60,8 +60,7 @@ class JournalManager # we generally ignore changes from blank to blank predecessor - .map { |k, v| current[k.to_s] != v && (v.present? || current[k.to_s].present?) } - .any? + .any? { |k, v| current[k.to_s] != v && (v.present? || current[k.to_s].present?) } end def self.association_changed?(journable, journal_association, association, id, key, value) @@ -79,7 +78,7 @@ class JournalManager changes.merge! JournalManager.removed_references(merged_journals, association.to_s, value.to_s) changes.merge! JournalManager.changed_references(merged_journals, association.to_s, value.to_s) - not changes.empty? + !changes.empty? else false end diff --git a/lib/api/v3/activities/activity_representer.rb b/lib/api/v3/activities/activity_representer.rb index 5754011c86..ca4b2fb789 100644 --- a/lib/api/v3/activities/activity_representer.rb +++ b/lib/api/v3/activities/activity_representer.rb @@ -37,7 +37,7 @@ module API self_link path: :activity, id_attribute: :notes_id, - title_getter: -> (*) { nil } + title_getter: ->(*) { nil } link :workPackage do { @@ -53,26 +53,28 @@ module API end link :update do + next unless current_user_allowed_to_edit? + { href: api_v3_paths.activity(represented.notes_id), method: :patch - } if current_user_allowed_to_edit? + } end property :id, - getter: -> (*) { notes_id }, + getter: ->(*) { notes_id }, render_nil: true property :comment, exec_context: :decorator, - getter: -> (*) { + getter: ->(*) { ::API::Decorators::Formattable.new(represented.notes, object: represented.journable) }, - setter: -> (value, *) { represented.notes = value['raw'] }, + setter: ->(value, *) { represented.notes = value['raw'] }, render_nil: true property :details, exec_context: :decorator, - getter: -> (*) { + getter: ->(*) { details = render_details(represented, no_html: true) html_details = render_details(represented) formattables = details.zip(html_details) @@ -81,7 +83,7 @@ module API }, render_nil: true property :version, render_nil: true - property :created_at, getter: -> (*) { DateTimeFormatter::format_datetime(created_at) } + property :created_at, getter: ->(*) { DateTimeFormatter::format_datetime(created_at) } def _type if represented.notes.blank? diff --git a/spec/models/journal_manager_spec.rb b/spec/models/journal_manager_spec.rb index 1478aa3701..074012a3be 100644 --- a/spec/models/journal_manager_spec.rb +++ b/spec/models/journal_manager_spec.rb @@ -104,35 +104,35 @@ describe JournalManager, type: :model do describe 'self.#update_user_references' do let!(:work_package) { FactoryBot.create :work_package } let!(:doomed_user) { work_package.author } - let!(:data1) { + let!(:data1) do FactoryBot.build(:journal_work_package_journal, - subject: work_package.subject, - status_id: work_package.status_id, - type_id: work_package.type_id, - author_id: doomed_user.id, - project_id: work_package.project_id) - } - let!(:data2) { + subject: work_package.subject, + status_id: work_package.status_id, + type_id: work_package.type_id, + author_id: doomed_user.id, + project_id: work_package.project_id) + end + let!(:data2) do FactoryBot.build(:journal_work_package_journal, - subject: work_package.subject, - status_id: work_package.status_id, - type_id: work_package.type_id, - author_id: doomed_user.id, - project_id: work_package.project_id) - } - let!(:doomed_user_journal) { + subject: work_package.subject, + status_id: work_package.status_id, + type_id: work_package.type_id, + author_id: doomed_user.id, + project_id: work_package.project_id) + end + let!(:doomed_user_journal) do FactoryBot.create :work_package_journal, - notes: '1', - user: doomed_user, - journable_id: work_package.id, - data: data1 - } - let!(:some_other_journal) { + notes: '1', + user: doomed_user, + journable_id: work_package.id, + data: data1 + end + let!(:some_other_journal) do FactoryBot.create :work_package_journal, - notes: '2', - journable_id: work_package.id, - data: data2 - } + notes: '2', + journable_id: work_package.id, + data: data2 + end before do doomed_user.destroy @@ -143,7 +143,7 @@ describe JournalManager, type: :model do end it "should not mark an unrelated journal's user as deleted" do - expect(some_other_journal.reload.user.is_a?(DeletedUser)).to be_falsy + expect(some_other_journal.reload.user.is_a?(DeletedUser)).to be_falsey end end end From 77b4863d09a95c5bc6501dfaec56e3955fc002e9 Mon Sep 17 00:00:00 2001 From: Jens Ulferts Date: Tue, 21 Aug 2018 14:01:51 +0200 Subject: [PATCH 2/3] fix journalizing detection for multi field cfs --- app/models/journal_manager.rb | 62 ++++++++++--------- .../lib/journal_changes.rb | 11 ++-- 2 files changed, 39 insertions(+), 34 deletions(-) diff --git a/app/models/journal_manager.rb b/app/models/journal_manager.rb index 81d9ac0420..2ca01d5d88 100644 --- a/app/models/journal_manager.rb +++ b/app/models/journal_manager.rb @@ -72,11 +72,11 @@ class JournalManager current = remove_empty_associations(current, value.to_s) - merged_journals = JournalManager.merge_reference_journals_by_id current, predecessor, key.to_s + merged_journals = JournalManager.merge_reference_journals_by_id(current, predecessor, key.to_s, value.to_s) - changes.merge! JournalManager.added_references(merged_journals, association.to_s, value.to_s) - changes.merge! JournalManager.removed_references(merged_journals, association.to_s, value.to_s) - changes.merge! JournalManager.changed_references(merged_journals, association.to_s, value.to_s) + changes.merge! JournalManager.added_references(merged_journals, association.to_s) + changes.merge! JournalManager.removed_references(merged_journals, association.to_s) + changes.merge! JournalManager.changed_references(merged_journals, association.to_s) !changes.empty? else @@ -91,45 +91,39 @@ class JournalManager # This would lead to false change information, otherwise. # We need to be careful though, because we want to accept false (and false.blank? == true) def self.remove_empty_associations(associations, value) - associations.reject { |association| + associations.reject do |association| association.has_key?(value) && association[value].blank? && association[value] != false - } + end end - def self.merge_reference_journals_by_id(new_journals, old_journals, id_key) + def self.merge_reference_journals_by_id(new_journals, old_journals, id_key, value) all_associated_journal_ids = new_journals.map { |j| j[id_key] } | old_journals.map { |j| j[id_key] } - all_associated_journal_ids.each_with_object({}) { |id, result| - result[id] = [old_journals.detect { |j| j[id_key] == id }, - new_journals.detect { |j| j[id_key] == id }] - } + all_associated_journal_ids.each_with_object({}) do |id, result| + result[id] = [select_and_combine(old_journals, id, id_key, value), + select_and_combine(new_journals, id, id_key, value)] + end end - def self.added_references(merged_references, key, value) - merged_references.select { |_, (old_attributes, new_attributes)| - old_attributes.nil? && !new_attributes.nil? - }.each_with_object({}) { |(id, (_, new_attributes)), result| - result["#{key}_#{id}"] = [nil, new_attributes[value]] - } + def self.added_references(merged_references, key) + merged_references + .select { |_, (old_value, new_value)| old_value.nil? && new_value.present? } + .each_with_object({}) { |(id, (_, new_value)), result| result["#{key}_#{id}"] = [nil, new_value] } end - def self.removed_references(merged_references, key, value) - merged_references.select { |_, (old_attributes, new_attributes)| - !old_attributes.nil? && new_attributes.nil? - }.each_with_object({}) { |(id, (old_attributes, _)), result| - result["#{key}_#{id}"] = [old_attributes[value], nil] - } + def self.removed_references(merged_references, key) + merged_references + .select { |_, (old_value, new_value)| old_value.present? && new_value.nil? } + .each_with_object({}) { |(id, (old_value, _)), result| result["#{key}_#{id}"] = [old_value, nil] } end - def self.changed_references(merged_references, key, value) - merged_references.select { |_, (old_attributes, new_attributes)| - !old_attributes.nil? && !new_attributes.nil? && old_attributes[value] != new_attributes[value] - }.each_with_object({}) { |(id, (old_attributes, new_attributes)), result| - result["#{key}_#{id}"] = [old_attributes[value], new_attributes[value]] - } + def self.changed_references(merged_references, key) + merged_references + .select { |_, (old_value, new_value)| old_value.present? && new_value.present? && old_value != new_value } + .each_with_object({}) { |(id, (old_value, new_value)), result| result["#{key}_#{id}"] = [old_value, new_value] } end def self.recreate_initial_journal(type, journal, changed_data) @@ -326,4 +320,14 @@ class JournalManager def self.reset_notification @send_notification = true end + + def self.select_and_combine(journals, id, key, value) + selected_journals = journals.select { |j| j[key] == id }.map { |j| j[value] } + + if selected_journals.empty? + nil + else + selected_journals.join(',') + end + end end diff --git a/lib/plugins/acts_as_journalized/lib/journal_changes.rb b/lib/plugins/acts_as_journalized/lib/journal_changes.rb index a14dc74c17..53846c3bd1 100644 --- a/lib/plugins/acts_as_journalized/lib/journal_changes.rb +++ b/lib/plugins/acts_as_journalized/lib/journal_changes.rb @@ -72,13 +72,14 @@ module JournalChanges new_journals = send(journal_assoc_name).map(&:attributes) old_journals = predecessor.send(journal_assoc_name).map(&:attributes) - merged_journals = JournalManager.merge_reference_journals_by_id new_journals, + merged_journals = JournalManager.merge_reference_journals_by_id(new_journals, old_journals, - key.to_s + key.to_s, + value.to_s) - changes.merge! JournalManager.added_references(merged_journals, association, value.to_s) - changes.merge! JournalManager.removed_references(merged_journals, association, value.to_s) - changes.merge! JournalManager.changed_references(merged_journals, association, value.to_s) + changes.merge! JournalManager.added_references(merged_journals, association) + changes.merge! JournalManager.removed_references(merged_journals, association) + changes.merge! JournalManager.changed_references(merged_journals, association) end changes From f3622b2ac87d5983354417181f51cd4a3adc4a18 Mon Sep 17 00:00:00 2001 From: Jens Ulferts Date: Tue, 21 Aug 2018 14:21:33 +0200 Subject: [PATCH 3/3] remove code duplication --- app/models/journal_manager.rb | 228 +++++++++--------- .../lib/journal_changes.rb | 23 +- 2 files changed, 124 insertions(+), 127 deletions(-) diff --git a/app/models/journal_manager.rb b/app/models/journal_manager.rb index 2ca01d5d88..a1464d0148 100644 --- a/app/models/journal_manager.rb +++ b/app/models/journal_manager.rb @@ -30,6 +30,122 @@ class JournalManager class << self attr_accessor :send_notification + + def changes_on_association(current, predecessor, association, key, value) + merged_journals = merge_reference_journals_by_id(current, predecessor, key.to_s, value.to_s) + + changes = added_references(merged_journals) + .merge(removed_references(merged_journals)) + .merge(changed_references(merged_journals)) + + to_changes_format(changes, association.to_s) + end + + def reset_notification + @send_notification = true + end + + private + + def merge_reference_journals_by_id(new_journals, old_journals, id_key, value) + all_associated_journal_ids = new_journals.map { |j| j[id_key] } | old_journals.map { |j| j[id_key] } + + all_associated_journal_ids.each_with_object({}) do |id, result| + result[id] = [select_and_combine(old_journals, id, id_key, value), + select_and_combine(new_journals, id, id_key, value)] + end + end + + def added_references(merged_references) + merged_references + .select { |_, (old_value, new_value)| old_value.nil? && new_value.present? } + end + + def removed_references(merged_references) + merged_references + .select { |_, (old_value, new_value)| old_value.present? && new_value.nil? } + end + + def changed_references(merged_references) + merged_references + .select { |_, (old_value, new_value)| old_value.present? && new_value.present? && old_value != new_value } + end + + def to_changes_format(references, key) + references.each_with_object({}) do |(id, (old_value, new_value)), result| + result["#{key}_#{id}"] = [old_value, new_value] + end + end + + def journable_details(journable) + calculated_proc = journable.class.vestal_journals_options[:calculate] + + attributes = journable.attributes.symbolize_keys + + if calculated_proc + attributes + .merge!(journable.instance_exec(&calculated_proc)) + end + + attributes + end + + def journal_class_name(type) + "#{base_class(type).name}Journal" + end + + def base_class(type) + type.base_class + end + + def create_association_data(journable, journal) + create_attachment_data journable, journal if journable.respond_to? :attachments + create_custom_field_data journable, journal if journable.respond_to? :custom_values + end + + def create_attachment_data(journable, journal) + journable.attachments.each do |a| + journal.attachable_journals.build attachment: a, filename: a.filename + end + end + + def create_custom_field_data(journable, journal) + journable.custom_values.group_by(&:custom_field).each do |custom_field, custom_values| + # Consider only custom values with non-blank values. Otherwise, + # non-existing custom values are different to custom values with an empty + # value. + # Mind that false.present? == false, but we don't consider false this being "blank"... + # This does not matter when we use stringly typed values (as in the database), + # but it matters when we use real types + valid_values = custom_values.select { |cv| cv.value.present? || cv.value == false } + + if custom_field.multi_value? && valid_values.any? + build_multi_value_custom_field_journal! journal, custom_field, valid_values + elsif valid_values.any? + build_custom_field_journal! journal, custom_field, valid_values.first + end + end + end + + def build_multi_value_custom_field_journal!(journal, custom_field, custom_values) + value = custom_values.map(&:value).join(",") # comma separated custom option IDs + + journal.customizable_journals.build custom_field_id: custom_field.id, value: value + end + + def build_custom_field_journal!(journal, custom_field, custom_value) + journal.customizable_journals.build custom_field_id: custom_field.id, value: custom_value.value + end + + def select_and_combine(journals, id, key, value) + selected_journals = journals.select { |j| j[key] == id }.map { |j| j[value] } + + if selected_journals.empty? + nil + else + selected_journals.join(',') + end + end end self.send_notification = true @@ -66,17 +182,12 @@ class JournalManager def self.association_changed?(journable, journal_association, association, id, key, value) if journable.respond_to? association journal_assoc_name = "#{journal_association}_journals" - changes = {} current = journable.send(association).map { |a| { key.to_s => a.send(id), value.to_s => a.send(value) } } predecessor = journable.journals.last.send(journal_assoc_name).map(&:attributes) current = remove_empty_associations(current, value.to_s) - merged_journals = JournalManager.merge_reference_journals_by_id(current, predecessor, key.to_s, value.to_s) - - changes.merge! JournalManager.added_references(merged_journals, association.to_s) - changes.merge! JournalManager.removed_references(merged_journals, association.to_s) - changes.merge! JournalManager.changed_references(merged_journals, association.to_s) + changes = JournalManager.changes_on_association(current, predecessor, association, key, value) !changes.empty? else @@ -98,34 +209,6 @@ class JournalManager end end - def self.merge_reference_journals_by_id(new_journals, old_journals, id_key, value) - all_associated_journal_ids = new_journals.map { |j| j[id_key] } | - old_journals.map { |j| j[id_key] } - - all_associated_journal_ids.each_with_object({}) do |id, result| - result[id] = [select_and_combine(old_journals, id, id_key, value), - select_and_combine(new_journals, id, id_key, value)] - end - end - - def self.added_references(merged_references, key) - merged_references - .select { |_, (old_value, new_value)| old_value.nil? && new_value.present? } - .each_with_object({}) { |(id, (_, new_value)), result| result["#{key}_#{id}"] = [nil, new_value] } - end - - def self.removed_references(merged_references, key) - merged_references - .select { |_, (old_value, new_value)| old_value.present? && new_value.nil? } - .each_with_object({}) { |(id, (old_value, _)), result| result["#{key}_#{id}"] = [old_value, nil] } - end - - def self.changed_references(merged_references, key) - merged_references - .select { |_, (old_value, new_value)| old_value.present? && new_value.present? && old_value != new_value } - .each_with_object({}) { |(id, (old_value, new_value)), result| result["#{key}_#{id}"] = [old_value, new_value] } - end - def self.recreate_initial_journal(type, journal, changed_data) if journal.data.nil? journal.data = create_journal_data journal.id, type, changed_data.except(:id) @@ -253,81 +336,4 @@ class JournalManager result end - - private - - def self.journable_details(journable) - calculated_proc = journable.class.vestal_journals_options[:calculate] - - attributes = journable.attributes.symbolize_keys - - if calculated_proc - attributes - .merge!(journable.instance_exec(&calculated_proc)) - end - - attributes - end - private_class_method :journable_details - - def self.journal_class_name(type) - "#{base_class(type).name}Journal" - end - - def self.base_class(type) - type.base_class - end - - def self.create_association_data(journable, journal) - create_attachment_data journable, journal if journable.respond_to? :attachments - create_custom_field_data journable, journal if journable.respond_to? :custom_values - end - - def self.create_attachment_data(journable, journal) - journable.attachments.each do |a| - journal.attachable_journals.build attachment: a, filename: a.filename - end - end - - def self.create_custom_field_data(journable, journal) - journable.custom_values.group_by(&:custom_field).each do |custom_field, custom_values| - # Consider only custom values with non-blank values. Otherwise, - # non-existing custom values are different to custom values with an empty - # value. - # Mind that false.present? == false, but we don't consider false this being "blank"... - # This does not matter when we use stringly typed values (as in the database), - # but it matters when we use real types - valid_values = custom_values.select { |cv| cv.value.present? || cv.value == false } - - if custom_field.multi_value? && valid_values.any? - build_multi_value_custom_field_journal! journal, custom_field, valid_values - elsif valid_values.any? - build_custom_field_journal! journal, custom_field, valid_values.first - end - end - end - - def self.build_multi_value_custom_field_journal!(journal, custom_field, custom_values) - value = custom_values.map(&:value).join(",") # comma separated custom option IDs - - journal.customizable_journals.build custom_field_id: custom_field.id, value: value - end - - def self.build_custom_field_journal!(journal, custom_field, custom_value) - journal.customizable_journals.build custom_field_id: custom_field.id, value: custom_value.value - end - - def self.reset_notification - @send_notification = true - end - - def self.select_and_combine(journals, id, key, value) - selected_journals = journals.select { |j| j[key] == id }.map { |j| j[value] } - - if selected_journals.empty? - nil - else - selected_journals.join(',') - end - end end diff --git a/lib/plugins/acts_as_journalized/lib/journal_changes.rb b/lib/plugins/acts_as_journalized/lib/journal_changes.rb index 53846c3bd1..3d13fd94e6 100644 --- a/lib/plugins/acts_as_journalized/lib/journal_changes.rb +++ b/lib/plugins/acts_as_journalized/lib/journal_changes.rb @@ -1,3 +1,5 @@ +#-- encoding: UTF-8 + #-- copyright # OpenProject is a project management system. # Copyright (C) 2012-2018 the OpenProject Foundation (OPF) @@ -26,7 +28,6 @@ # See docs/COPYRIGHT.rdoc for more details. #++ -#-- encoding: UTF-8 module JournalChanges def get_changes return @changes if @changes @@ -54,34 +55,24 @@ module JournalChanges end end - @changes.merge!(get_association_changes predecessor, 'attachable', 'attachments', :attachment_id, :filename) - @changes.merge!(get_association_changes predecessor, 'customizable', 'custom_fields', :custom_field_id, :value) + @changes.merge!(get_association_changes(predecessor, 'attachable', 'attachments', :attachment_id, :filename)) + @changes.merge!(get_association_changes(predecessor, 'customizable', 'custom_fields', :custom_field_id, :value)) end def get_association_changes(predecessor, journal_association, association, key, value) - changes = {} journal_assoc_name = "#{journal_association}_journals" if predecessor.nil? - send(journal_assoc_name).each_with_object(changes) { |associated_journal, h| + send(journal_assoc_name).each_with_object({}) do |associated_journal, h| changed_attribute = "#{association}_#{associated_journal.send(key)}" new_value = associated_journal.send(value) h[changed_attribute] = [nil, new_value] - } + end else new_journals = send(journal_assoc_name).map(&:attributes) old_journals = predecessor.send(journal_assoc_name).map(&:attributes) - merged_journals = JournalManager.merge_reference_journals_by_id(new_journals, - old_journals, - key.to_s, - value.to_s) - - changes.merge! JournalManager.added_references(merged_journals, association) - changes.merge! JournalManager.removed_references(merged_journals, association) - changes.merge! JournalManager.changed_references(merged_journals, association) + JournalManager.changes_on_association(new_journals, old_journals, association, key, value) end - - changes end end