Fixed bug: Regular Model list imports (e.g. Tickets) sequences fail import because of attribute that is not required and can't be provided.

lib/assets_set.rb

@@ -0,0 +1,61 @@
+require_dependency 'assets_set/proxy'
+
+# The name AssetsSet combines the Assets term in Zammad with the Set class from the Ruby StdLib.
+# Zammad Assets serve the purpose to limit requests to the REST API. For a requested object all
+# related and relevant objects are collected recursively and then send to the client in one go.
+# A Ruby Set implements a collection of unordered values with no duplicates.
+#
+# This class implements a collection of Zammad Assets with no duplicates.
+# This is done by having two structures:
+#
+# 1st: The actual Assets Hash (e.g. `assets[model_name][object_id] = object_attributes`)
+# 2nd: A lookup table for keeping track which elements were added to the actual Assets Hash
+#
+# The actual Assets Hash should be flushed after sending it to the client. This will keep the
+# lookup table untouched. The next time a object that was already send to the client
+# should be added to the actual Assets Hash the lookup table will recognize the object
+# and will prevent the addition to the actual Assets Hash.
+# This way Assets will be send only once to the client for the lifetime of a AssetsSet instance.
+class AssetsSet < SimpleDelegator
+
+  # This method overwrites the SimpleDelegator initializer
+  # to be able to have the actual Assets Hash as an optional argument.
+  def initialize(assets = {})
+    super(assets)
+  end
+
+  # This method initializes the the global lookup table.
+  # Each (accessed) Model gets it's own sub structure in it.
+  def lookup_table
+    @lookup_table ||= {}
+  end
+
+  # This method flushes the actual Assets Hash.
+  def flush
+    __setobj__({})
+  end
+
+  # This method intercepts `assets[model_name]` calls by registering a AssetsSet::Proxy.
+  # Instead of creating an entry in the actual Assets Hash a AssetsSet::Proxy.
+  # See AssetsSet::Proxy for further information.
+  # Existing proxies will be reused.
+  def [](model)
+    __getobj__[model] ||= proxy(model)
+  end
+
+  # This method is used to convert the AssetsSet into a regular Hash which then can be send to the client.
+  # It cleans up empty Model sub structures in the internal structure.
+  def to_h
+    super.delete_if { |_model, assets| assets.blank? }.transform_values!(&:to_h)
+  end
+
+  private
+
+  def proxy(model)
+    lookup_table[model] ||= {}
+
+    ::AssetsSet::Proxy.new.tap do |proxy|
+      proxy.lookup_table = lookup_table[model]
+    end
+  end
+end

lib/assets_set/proxy.rb

@@ -0,0 +1,50 @@
+# This class defines a Proxy for accessing objects inside of a AssetsSet Model sub structure.
+#
+# The Zammad Assets logic works by having an Assets Hash that contains a sub structure for
+# each model that is relevant. Before an object gets added to the Model sub structure the
+# Model sub structure is checked for the presence of the object by its id. If the object is
+# already present it will be skipped. However, if the model is not yet present in the matching
+# Model sub structure the Zammad Assets will be collected and added.
+#
+# We make use of this lookup / add if not present functionality by intercepting calls to the
+# actual Assets Hash.
+#
+# If an object is not yet present in the Model sub structure and should be added
+# it will added to the lookup table of the AssetsSet first. After that the object will
+# be stored to the actual Assets Hash.
+#
+# The next time a lookup for an object in the Model sub structure is performed it will find the
+# actual object data. However, if the actual Assets Hash is send to the client and the AssetsSet
+# is flushed the lookup table is still present. The next time a lookup for an object in the
+# Model sub is performed it will NOT find the actual object data. In this case a fall back
+# to the lookup table will be performed which will will just return `true` to satisfy the
+# "is present" condition
+class AssetsSet < SimpleDelegator
+  class Proxy < SimpleDelegator
+
+    attr_accessor :lookup_table
+
+    # This method overwrites the SimpleDelegator initializer
+    # to be able to have the actual Assets Hash as an optional argument.
+    def initialize(assets = {})
+      super(assets)
+    end
+
+    # This method intercepts `assets[model_name][object_id]` calls and return the actual objects data.
+    # If the object it not present the lookup table of the AssetsSet will be queried.
+    # If the object was present before a previously performed `flush` it will return true and
+    # satisfy the "is present" condition in the `assets` of the given model.
+    # If the object is not and never was present the `assets` logic will be performed as normal.
+    def [](id)
+      __getobj__[id] || lookup_table[id]
+    end
+
+    # This method intercepts `assets[model_name][object_id] = object_attributes` calls.
+    # It stores an entry in the lookup the of the AssetsSet and then performs the regular call
+    # to store the data in the actual Assets Hash Model sub structure.
+    def []=(id, _value)
+      lookup_table[id] = true
+      super
+    end
+  end
+end

lib/sessions/backend/ticket_overview_list.rb

@@ -86,6 +86,7 @@ class Sessions::Backend::TicketOverviewList < Sessions::Backend::Base
 
     # push overviews
     results = []
+    assets  = AssetsSet.new
     index_and_lists.each do |data|
 
       # do not deliver unchanged lists
@@ -93,7 +94,6 @@ class Sessions::Backend::TicketOverviewList < Sessions::Backend::Base
 
       @last_overview[data[:overview][:id]] = [data[:tickets], data[:overview]]
 
-      assets = {}
       overview = Overview.lookup(id: data[:overview][:id])
       next if !overview
 
@@ -108,7 +108,8 @@ class Sessions::Backend::TicketOverviewList < Sessions::Backend::Base
 
         assets = asset_push(ticket, assets)
       end
-      data[:assets] = assets
+
+      data[:assets] = assets.to_h
 
       if !@client
         result = {
@@ -125,6 +126,8 @@ class Sessions::Backend::TicketOverviewList < Sessions::Backend::Base
           data:  data,
         )
       end
+
+      assets.flush
     end
     return results if !@client
 

spec/lib/sessions/backend/ticket_overview_list_spec.rb

@@ -32,9 +32,50 @@ RSpec.describe Sessions::Backend::TicketOverviewList do
           .to match_array(Ticket::Overviews.all(current_user: admin).map(&:name))
       end
 
+      it 'is optimized to not send duplicate asset entries over all events' do
+        collection_assets = collection.push.map { |hash| hash[:data][:assets] }
+
+        # match all event assets against the assets of the other events
+        # and make sure that each asset entry is unique over all events assets
+        unique_asssets = collection_assets.each_with_index.all? do |lookup_assets, lookup_index|
+
+          collection_assets.each_with_index.none? do |comparison_assets, comparison_index|
+
+            # skip assets comparison for same event
+            next if comparison_index == lookup_index
+
+            # check that none of the lookup_assets assets is present
+            # in the comparison_assets
+            lookup_assets.keys.any? do |model|
+              # skip Models that are only present in our lookup_assets entry
+              next if !comparison_assets.key?(model)
+
+              # check if there are no intersect Model record IDs
+              # aka IDs present in both hashes
+              intersection_ids = lookup_assets[model].keys & comparison_assets[model].keys
+              intersection_ids.present?
+            end
+          end
+        end
+
+        expect(unique_asssets).to be(true)
+      end
+
       it 'includes FE assets for all overviews and tickets not pushed in the last two hours' do
-        expect(collection.push.map { |hash| hash[:data][:assets] })
+
-          .to match_array(Ticket::Overviews.all(current_user: admin).map { |overview| overview.assets({}) })
+        # ATTENTION: we can't compare the arrays of assets directly
+        # because the Ticket::Overviews backend contain an optimization logic that sends an asset entry only once
+        # while the Sessions::Backend::* classes results contain all assets for each entry.
+        # Therefore we merge all assets for each of the both arrays to have two big Hashes that contains all assets.
+        # See previous example for the matching spec.
+
+        collection_assets        = collection.push.map { |hash| hash[:data][:assets] }
+        collection_assets_merged = collection_assets.each_with_object({}) { |assets, result| result.deep_merge!(assets) }
+
+        overviews_all_assets        = Ticket::Overviews.all(current_user: admin).map { |overview| overview.assets({}) }
+        overviews_all_assets_merged = overviews_all_assets.each_with_object({}) { |assets, result| result.deep_merge!(assets) }
+
+        expect(collection_assets_merged).to eq(overviews_all_assets_merged)
       end
 
       context 'when called twice, with no changes to Ticket and Overview tables' do