fix(place/bookings): PPT-2426 resolve asset names from Asset records

drivers/place/at_capacity_mailer_spec.cr

@@ -2,6 +2,50 @@ require "placeos-driver/spec"
 require "placeos-driver/interface/mailer"
 
 class StaffAPI < DriverSpecs::MockDriver
+  getter metadata_shape : String = "legacy"
+  getter failing_type : String? = nil
+  getter asset_calls = 0
+  getter type_calls = 0
+
+  def configure(metadata_shape : String, failing_type : String? = nil)
+    @metadata_shape = metadata_shape
+    @failing_type = failing_type
+  end
+
+  def asset_categories(hidden : Bool? = nil)
+    JSON.parse([
+      {id: "category-desk", name: "_DESKS_", hidden: true},
+      {id: "category-parking", name: "_PARKING_", hidden: true},
+      {id: "category-parking-legacy", name: "_PARKING_SPACES_", hidden: true},
+    ].to_json)
+  end
+
+  def asset_types(category_id : String? = nil, zone_id : String? = nil, brand : String? = nil, model_number : String? = nil)
+    @type_calls += 1
+    JSON.parse([
+      {id: "type-desk", name: "_DESKS_", category_id: "category-desk"},
+      {id: "type-parking", name: "_PARKING_SPACES_", category_id: "category-parking"},
+      {id: "type-parking-duplicate", name: "_PARKING_SPACES_", category_id: "category-parking"},
+      {id: "type-parking-legacy", name: "_PARKING_SPACES_", category_id: "category-parking-legacy"},
+      {id: "type-parking-users", name: "_PARKING_USERS_", category_id: "category-parking"},
+      {id: "type-locker", name: "_LOCKERS_", category_id: "category-desk"},
+    ].select { |type| category_id.nil? || type[:category_id] == category_id }.to_json)
+  end
+
+  def assets(type_id : String? = nil, zone_id : String? = nil)
+    @asset_calls += 1
+    raise "assets unavailable" if type_id == failing_type
+    ids = case type_id
+          when "type-desk"              then ["asset-desk-1", "asset-desk-2"]
+          when "type-parking"           then ["asset-park-1"]
+          when "type-parking-duplicate" then ["asset-park-2"]
+          when "type-parking-legacy"    then ["asset-park-3"]
+          else                               ["unrelated-asset"]
+          end
+    ids = [] of String unless zone_id == "level-1" || zone_id == "level-2"
+    JSON.parse(ids.map { |id| {id: id, identifier: "Name #{id}", name: nil, zone_id: zone_id, zones: [] of String} }.to_json)
+  end
+
   ZONES = [
     {
       created_at:   1660537814,
@@ -54,6 +98,19 @@ class StaffAPI < DriverSpecs::MockDriver
     zone = ZONES.find! { |z| z["id"] == id }
     key = key.not_nil!
 
+    return JSON.parse("{}") if metadata_shape == "missing"
+    raise "metadata unavailable" if metadata_shape == "error"
+    unless metadata_shape == "legacy"
+      details = case metadata_shape
+                when "migrated" then JSON.parse(%({"migrated":true,"migrated_at":1765438509624}))
+                when "empty"    then JSON.parse("[]")
+                when "null"     then JSON.parse("null")
+                when "object"   then JSON.parse("{}")
+                else                 JSON.parse(%(""))
+                end
+      return JSON.parse({key => {name: key, parent_id: id, details: details}}.to_json)
+    end
+
     details = case key
               when "desks"
                 [
@@ -140,6 +197,9 @@ class StaffAPI < DriverSpecs::MockDriver
              when "parking"
                ["park-1"]
              end
+    if metadata_shape == "migrated"
+      assets = type == "desk" ? ["asset-desk-1", "asset-desk-2"] : ["asset-park-1", "asset-park-2", "asset-park-3"]
+    end
     JSON.parse(assets.to_json)
   end
 end
@@ -244,4 +304,71 @@ DriverSpecs.mock_driver "Place::AtCapacityMailer" do
 
   #####################################
   # End of tests for: #check_capacity
+
+  api = system(:StaffAPI_1).as(StaffAPI)
+
+  it "keeps legacy capacity lists metadata-only when Asset records also exist" do
+    settings({booking_type: "desk", zones: ["level-1"]})
+    exec(:get_asset_ids).get.should eq({"level-1" => ["desk-1", "desk-2"]})
+    api.asset_calls.should eq 0
+    api.type_calls.should eq 0
+  end
+
+  it "sends at-capacity mail for a migrated desk zone" do
+    api.configure("migrated")
+    settings({booking_type: "desk", zones: ["level-2"]})
+    exec(:get_asset_ids).get.should eq({"level-2" => ["asset-desk-1", "asset-desk-2"]})
+    exec(:check_capacity).get
+    system(:Mailer_1)[:sent].should eq 2
+  end
+
+  it "includes duplicate parking types from current and legacy categories" do
+    api.configure("migrated")
+    settings({booking_type: "parking", zones: ["level-1"]})
+    exec(:get_asset_ids).get.should eq({"level-1" => ["asset-park-1", "asset-park-2", "asset-park-3"]})
+  end
+
+  ["missing", "null", "object", "string", "error"].each do |shape|
+    it "uses Asset records for #{shape} metadata" do
+      api.configure(shape)
+      settings({booking_type: "desk", zones: ["level-1"]})
+      exec(:get_asset_ids).get.should eq({"level-1" => ["asset-desk-1", "asset-desk-2"]})
+    end
+  end
+
+  it "keeps an empty metadata array as an empty capacity list" do
+    api.configure("empty")
+    settings({booking_type: "desk", zones: ["level-1"]})
+    calls = api.asset_calls
+    exec(:get_asset_ids).get.should eq({"level-1" => [] of String})
+    api.asset_calls.should eq calls
+  end
+
+  it "caches Asset lists per zone and shares type discovery between zones" do
+    api.configure("migrated")
+    settings({booking_type: "parking", zones: ["level-1", "level-2"]})
+    asset_calls = api.asset_calls
+    type_calls = api.type_calls
+    exec(:get_asset_ids).get
+    exec(:get_asset_ids).get
+    api.asset_calls.should eq asset_calls + 6
+    api.type_calls.should eq type_calls + 1
+  end
+
+  it "expires the Asset list and type caches using asset_cache_timeout" do
+    api.configure("migrated")
+    settings({booking_type: "desk", zones: ["level-1"], asset_cache_timeout: 0})
+    asset_calls = api.asset_calls
+    type_calls = api.type_calls
+    2.times { exec(:get_asset_ids).get }
+    api.asset_calls.should eq asset_calls + 2
+    api.type_calls.should eq type_calls + 2
+  end
+
+  it "retains available assets when one duplicate type cannot be queried" do
+    api.configure("migrated", "type-parking-duplicate")
+    settings({booking_type: "parking", zones: ["level-1"]})
+    exec(:get_asset_ids).get.should eq({"level-1" => ["asset-park-1", "asset-park-3"]})
+    api.configure("migrated")
+  end
 end

drivers/place/auto_release_spec.cr

@@ -2,6 +2,46 @@ require "placeos-driver/spec"
 require "placeos-driver/interface/mailer"
 
 class StaffAPI < DriverSpecs::MockDriver
+  getter metadata_shape : String = "migrated"
+  getter asset_identifier : String? = "Desk from Asset"
+  getter asset_name : String? = "Fallback name"
+  getter assets_fail : Bool = false
+  getter asset_calls = 0
+
+  def configure(metadata_shape : String, identifier : String? = "Desk from Asset", name : String? = "Fallback name", assets_fail : Bool = false)
+    @metadata_shape = metadata_shape
+    @asset_identifier = identifier
+    @asset_name = name
+    @assets_fail = assets_fail
+  end
+
+  def metadata(id : String, key : String? = nil)
+    details = if metadata_shape == "migrated"
+                JSON.parse(%({"migrated":true,"migrated_at":1765438509624}))
+              else
+                JSON.parse([{id: "legacy-desk", name: "Legacy desk"}].to_json)
+              end
+    JSON.parse({key.not_nil! => {name: key, parent_id: id, details: details}}.to_json)
+  end
+
+  def asset_categories(hidden : Bool? = nil)
+    JSON.parse([{id: "category-desk", name: "_DESKS_", hidden: true}].to_json)
+  end
+
+  def asset_types(category_id : String? = nil, zone_id : String? = nil, brand : String? = nil, model_number : String? = nil)
+    JSON.parse([{id: "type-desk", name: "_DESKS_", category_id: "category-desk"}].to_json)
+  end
+
+  def assets(type_id : String? = nil, zone_id : String? = nil)
+    @asset_calls += 1
+    raise "assets unavailable" if assets_fail
+    return JSON.parse("[]") unless type_id == "type-desk" && zone_id == "zone-1234"
+    JSON.parse([
+      {id: "asset-desk", identifier: asset_identifier, name: asset_name, zone_id: zone_id, zones: [] of String},
+      {id: "legacy-desk", identifier: "Asset name for legacy desk", name: nil, zone_id: zone_id, zones: [] of String},
+    ].to_json)
+  end
+
   def on_load
     self[:rejected] = 0
   end
@@ -799,6 +839,7 @@ class Mailer < DriverSpecs::MockDriver
   )
     self[:sent] = self[:sent].as_i + 1
     self[:reply_to] = reply_to
+    self[:last_args] = args
   end
 
   def send_mail(
@@ -1306,4 +1347,43 @@ DriverSpecs.mock_driver "Place::AutoRelease" do
 
   #############################
   # End of tests for: #enabled?
+
+  api = system(:StaffAPI_1).as(StaffAPI)
+  [
+    {"migrated", "Asset identifier", "Asset name", "asset-desk", "Asset identifier"},
+    {"migrated", "", "Asset name", "asset-desk", "Asset name"},
+    {"migrated", nil, nil, "asset-desk", "asset-desk"},
+    {"migrated", "", "", "asset-desk", "asset-desk"},
+    {"legacy", "Asset identifier", "Asset name", "asset-desk", "Asset identifier"},
+    {"legacy", "Asset identifier", "Asset name", "legacy-desk", "Legacy desk"},
+    {"legacy", "Asset identifier", "Asset name", "unknown-desk", "unknown-desk"},
+  ].each do |shape, identifier, name, asset_id, expected|
+    it "resolves #{shape} #{asset_id} with identifier #{identifier.inspect} and name #{name.inspect}" do
+      api.configure(shape, identifier, name)
+      settings({auto_release: {time_before: 10, time_after: 10, resources: ["desk"]}})
+      status[:pending_release] = [StaffAPI::BOOKINGS[1].merge({asset_id: asset_id})]
+      status[:released_booking_ids] = [] of Int64
+      status[:emailed_booking_ids] = [] of Int64
+      calls = api.asset_calls
+      exec(:send_release_emails).get.should eq [2]
+      system(:Mailer_1)[:last_args]["asset_name"].should eq expected
+      api.asset_calls.should eq calls if asset_id == "legacy-desk"
+
+      calls = api.asset_calls
+      status[:emailed_booking_ids] = [] of Int64
+      exec(:send_release_emails).get.should eq [2]
+      system(:Mailer_1)[:last_args]["asset_name"].should eq expected
+      api.asset_calls.should eq calls
+    end
+  end
+
+  it "still sends the email with the raw id when the Asset request fails" do
+    api.configure("migrated", assets_fail: true)
+    settings({auto_release: {time_before: 10, time_after: 10, resources: ["desk"]}})
+    status[:pending_release] = [StaffAPI::BOOKINGS[1].merge({asset_id: "asset-desk"})]
+    status[:released_booking_ids] = [] of Int64
+    status[:emailed_booking_ids] = [] of Int64
+    exec(:send_release_emails).get.should eq [2]
+    system(:Mailer_1)[:last_args]["asset_name"].should eq "asset-desk"
+  end
 end

drivers/place/bookings/asset_name_resolver.cr

@@ -5,12 +5,16 @@ module Place::AssetNameResolver
   include Place::LockerMetadataParser
 
   @asset_cache : AssetCache = AssetCache.new
+  @asset_record_cache : AssetCache = AssetCache.new
+  @asset_type_cache = {} of String => Tuple(Int64, Array(String))
   @asset_cache_timeout : Int64 = 3600_i64 # 1 hour
 
   private getter asset_cache : AssetCache
 
   private def clear_asset_cache
     @asset_cache = AssetCache.new
+    @asset_record_cache = AssetCache.new
+    @asset_type_cache.clear
   end
 
   private def lookup_asset(asset_id : String, type : String, zones : Array(String) = [building_id]) : String
@@ -19,14 +23,10 @@ module Place::AssetNameResolver
       return locker.name if locker
     else
       zones.each do |zone_id|
-        asset = if (cache = asset_cache[{zone_id, type}]?) && cache[0] > Time.utc.to_unix
-                  cache[1].find { |asset| asset.id == asset_id }
-                else
-                  assets = lookup_assets(zone_id, type)
-                  @asset_cache[{zone_id, type}] = {Time.utc.to_unix + @asset_cache_timeout, assets}
-                  assets.find { |asset| asset.id == asset_id }
-                end
+        asset = lookup_assets(zone_id, type).find { |asset| asset.id == asset_id }
+        return asset.name if asset
 
+        asset = lookup_asset_records(zone_id, type).find { |asset| asset.id == asset_id }
         return asset.name if asset
       end
     end
@@ -46,8 +46,24 @@ module Place::AssetNameResolver
                      end
 
     if metadata_field
-      metadata = Metadata.from_json staff_api.metadata(zone_id, metadata_field).get[metadata_field].to_json
-      assets = metadata.details.as_a.map { |asset| Asset.from_json asset.to_json }
+      if (cache = asset_cache[{zone_id, type}]?) && cache[0] > Time.utc.to_unix
+        return cache[1]
+      end
+
+      details = begin
+        metadata = Metadata.from_json staff_api.metadata(zone_id, metadata_field).get[metadata_field].to_json
+        metadata.details.as_a?
+      rescue error
+        logger.debug { "unable to get #{metadata_field} from zone #{zone_id} metadata" }
+        nil
+      end
+
+      assets = if details
+                 details.map { |asset| Asset.from_json asset.to_json }
+               else
+                 lookup_asset_records(zone_id, type)
+               end
+      @asset_cache[{zone_id, type}] = {Time.utc.to_unix + @asset_cache_timeout, assets}
     elsif type == "locker"
       assets = locker_details.map { |id, locker| Asset.new(id, locker.name) }
     end
@@ -58,6 +74,48 @@ module Place::AssetNameResolver
     [] of Asset
   end
 
+  private def lookup_asset_records(zone_id : String, type : String) : Array(Asset)
+    assets = [] of Asset
+    type_name = case type
+                when "desk"    then "_DESKS_"
+                when "parking" then "_PARKING_SPACES_"
+                end
+    return assets unless type_name
+
+    if (cache = @asset_record_cache[{zone_id, type}]?) && cache[0] > Time.utc.to_unix
+      return cache[1]
+    end
+
+    begin
+      type_ids = if (cache = @asset_type_cache[type]?) && cache[0] > Time.utc.to_unix
+                   cache[1]
+                 else
+                   ids = staff_api.asset_types.get.as_a.select { |asset_type| asset_type["name"]?.try(&.as_s?) == type_name }
+                     .map { |asset_type| asset_type["id"].as_s }.uniq!
+                   @asset_type_cache[type] = {Time.utc.to_unix + @asset_cache_timeout, ids}
+                   ids
+                 end
+
+      type_ids.each do |type_id|
+        begin
+          staff_api.assets(type_id: type_id, zone_id: zone_id).get.as_a.each do |asset|
+            id = asset["id"].as_s
+            name = asset["identifier"]?.try(&.as_s?).presence || asset["name"]?.try(&.as_s?).presence || id
+            assets << Asset.new(id, name)
+          end
+        rescue error
+          logger.warn(exception: error) { "unable to get #{type_id} assets from zone #{zone_id}" }
+        end
+      end
+    rescue error
+      logger.warn(exception: error) { "unable to get #{type} asset types for zone #{zone_id}" }
+    end
+
+    assets.uniq!(&.id)
+    @asset_record_cache[{zone_id, type}] = {Time.utc.to_unix + @asset_cache_timeout, assets}
+    assets
+  end
+
   #                            zone_id, type         timeout, assets
   alias AssetCache = Hash(Tuple(String, String), Tuple(Int64, Array(Asset)))