From 39bed6b157a4052822af38b67d99fc1da9aa28b7 Mon Sep 17 00:00:00 2001 From: Emi Matchu Date: Tue, 19 Nov 2024 14:26:06 -0800 Subject: [PATCH] Make Item's `update_cached_fields` callback more reliable In our tests, I discovered an unexpected behavior where calling `item.swf_assets << swf_asset` wasn't updating computed fields correctly. This isn't something we actually do in-app, I think the modeling system happened to trigger the callbacks in a way that still worked fine? But I think this is a good idea for reliability, since caching is such a notoriously difficult thing to get right anyway! And it makes our tests simpler and clearer. Specifically, `compatible_body_ids` references `swf_assets`, which, I'm kinda surprised, *doesn't* include the newly-added asset yet when the `ParentSwfAssetRelationship.after_save` hook runs while calling `item.swf_assets << swf_asset`. Reloading it fixes this! --- app/models/item.rb | 4 ++++ spec/models/item_spec.rb | 12 ------------ 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/app/models/item.rb b/app/models/item.rb index 4b7a4feb..51fab75a 100644 --- a/app/models/item.rb +++ b/app/models/item.rb @@ -263,6 +263,10 @@ class Item < ApplicationRecord end def update_cached_fields + # Reload our associations, so they include any new records. + swf_assets.reload + + # Then, compute and save our cached fields. self.cached_occupied_zone_ids = occupied_zone_ids self.cached_compatible_body_ids = compatible_body_ids(use_cached: false) self.save! diff --git a/spec/models/item_spec.rb b/spec/models/item_spec.rb index 08d1f8bd..c76ff167 100644 --- a/spec/models/item_spec.rb +++ b/spec/models/item_spec.rb @@ -90,9 +90,6 @@ RSpec.describe Item do before do item.swf_assets << build_item_asset(zones(:wings), body_id: 1) item.swf_assets << build_item_asset(zones(:wings), body_id: 2) - # HACK: I don't understand why the first asset is triggering the hooks - # for cached fields, but the second isn't? Idk, force an update. - item.update_cached_fields end it_behaves_like "a not-fully-modeled item" @@ -112,9 +109,6 @@ RSpec.describe Item do item.swf_assets << build_item_asset(zones(:wings), body_id: 2) item.swf_assets << build_item_asset(zones(:wings), body_id: 3) item.swf_assets << build_item_asset(zones(:wings), body_id: 4) - # HACK: I don't understand why the first asset is triggering the hooks - # for cached fields, but the second isn't? Idk, force an update. - item.update_cached_fields end it_behaves_like "a fully-modeled item" @@ -155,9 +149,6 @@ RSpec.describe Item do before do item.swf_assets << build_item_asset(zones(:wings), body_id: 11) item.swf_assets << build_item_asset(zones(:wings), body_id: 12) - # HACK: I don't understand why the first asset is triggering the hooks - # for cached fields, but the second isn't? Idk, force an update. - item.update_cached_fields end it_behaves_like "a not-fully-modeled item" @@ -177,9 +168,6 @@ RSpec.describe Item do item.swf_assets << build_item_asset(zones(:wings), body_id: 12) item.swf_assets << build_item_asset(zones(:wings), body_id: 13) item.swf_assets << build_item_asset(zones(:wings), body_id: 4) - # HACK: I don't understand why the first asset is triggering the hooks - # for cached fields, but the second isn't? Idk, force an update. - item.update_cached_fields end it_behaves_like "a fully-modeled item"