TheDude
/
openproject
2
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge pull request #6559 from opf/fix/multi_value_cf_journalizing

Browse Source 
Fix/multi value cf journalizing

[ci skip]
pull/6565/head
Oliver Günther 6 years ago committed by GitHub
parent 9a5e52b461 f3622b2ac8
commit 568bb9aeb0
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 163 additions and 160 deletions
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Show Stats Download Patch File Download Diff File
  1. 231
      app/models/journal_manager.rb
  2. 16
      lib/api/v3/activities/activity_representer.rb
  3. 22
      lib/plugins/acts_as_journalized/lib/journal_changes.rb
  4. 18
      spec/models/journal_manager_spec.rb

231
app/models/journal_manager.rb
Unescape View File

@ -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
@ -60,26 +176,20 @@ 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)
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
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 = JournalManager.changes_on_association(current, predecessor, association, key, value)
not changes.empty?
!changes.empty?
else
false
end
@ -92,45 +202,11 @@ 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
def self.merge_reference_journals_by_id(new_journals, old_journals, id_key)
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 }]
}
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]]
}
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]
}
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]]
}
end
def self.recreate_initial_journal(type, journal, changed_data)
@ -260,71 +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
end

16
lib/api/v3/activities/activity_representer.rb
Unescape View File

@ -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?

22
lib/plugins/acts_as_journalized/lib/journal_changes.rb
Unescape View File

@ -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,33 +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
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)
JournalManager.changes_on_association(new_journals, old_journals, association, key, value)
end
changes
end
end

18
spec/models/journal_manager_spec.rb
Unescape View File

@ -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) {
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) {
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) {
end
let!(:some_other_journal) do
FactoryBot.create :work_package_journal,
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

Write Preview
Loading…
Cancel
Save