diff --git a/app/controllers/api/v1/base_controller.rb b/app/controllers/api/v1/base_controller.rb index ef175bf17..f1631ec1c 100644 --- a/app/controllers/api/v1/base_controller.rb +++ b/app/controllers/api/v1/base_controller.rb @@ -3,14 +3,11 @@ class Api::V1::BaseController < ApplicationController include Doorkeeper::Rails::Helpers - UUID_PATTERN = /\A[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\z/i - private_constant :UUID_PATTERN - InvalidFilterError = Class.new(StandardError) class << self def valid_uuid?(value) - value.to_s.match?(UUID_PATTERN) + UuidFormat.valid?(value) end end diff --git a/app/controllers/api/v1/valuations_controller.rb b/app/controllers/api/v1/valuations_controller.rb index d5ce1842b..24c9cfd21 100644 --- a/app/controllers/api/v1/valuations_controller.rb +++ b/app/controllers/api/v1/valuations_controller.rb @@ -269,10 +269,6 @@ def parse_date_param(key) raise InvalidFilterError, "#{key} must be an ISO 8601 date" end - def valid_uuid?(value) - value.to_s.match?(/\A[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\z/i) - end - def safe_page_param page = params[:page].to_i page > 0 ? page : 1 diff --git a/app/models/family/data_exporter.rb b/app/models/family/data_exporter.rb index 34b6beba5..da3f8ac36 100644 --- a/app/models/family/data_exporter.rb +++ b/app/models/family/data_exporter.rb @@ -493,11 +493,14 @@ def serialize_rule_for_export(rule) end def serialize_condition(condition) + operand = resolve_condition_operand(condition) data = { condition_type: condition.condition_type, operator: condition.operator, - value: resolve_condition_value(condition) + value: operand[:value] } + value_ref = operand[:value_ref] + data[:value_ref] = value_ref if value_ref.present? if condition.compound? && condition.sub_conditions.any? data[:sub_conditions] = condition.sub_conditions.map { |sub| serialize_condition(sub) } @@ -507,52 +510,79 @@ def serialize_condition(condition) end def serialize_action(action) - { + operand = resolve_action_operand(action) + data = { action_type: action.action_type, - value: resolve_action_value(action) + value: operand[:value] } + value_ref = operand[:value_ref] + data[:value_ref] = value_ref if value_ref.present? + + data end - def resolve_condition_value(condition) - return condition.value unless condition.value.present? + def resolve_condition_operand(condition) + return rule_operand(condition.value) unless condition.value.present? # Map category UUIDs to names for portability - if condition.condition_type == "transaction_category" && condition.value.present? - category = @family.categories.find_by(id: condition.value) - return category&.name || condition.value + if condition.condition_type == "transaction_category" + return rule_operand(condition.value, type: "Category", relation: @family.categories) end # Map merchant UUIDs to names for portability - if condition.condition_type == "transaction_merchant" && condition.value.present? - merchant = @family.merchants.find_by(id: condition.value) - return merchant&.name || condition.value + if condition.condition_type == "transaction_merchant" + return rule_operand(condition.value, type: "Merchant", relation: @family.merchants) end - condition.value + rule_operand(condition.value) end - def resolve_action_value(action) - return action.value unless action.value.present? + def resolve_action_operand(action) + return rule_operand(action.value) unless action.value.present? # Map category UUIDs to names for portability - if action.action_type == "set_transaction_category" && action.value.present? - category = @family.categories.find_by(id: action.value) || @family.categories.find_by(name: action.value) - return category&.name || action.value + if action.action_type == "set_transaction_category" + return rule_operand(action.value, type: "Category", relation: @family.categories, fallback_to_name: true) end # Map merchant UUIDs to names for portability - if action.action_type == "set_transaction_merchant" && action.value.present? - merchant = @family.merchants.find_by(id: action.value) || @family.merchants.find_by(name: action.value) - return merchant&.name || action.value + if action.action_type == "set_transaction_merchant" + return rule_operand(action.value, type: "Merchant", relation: @family.merchants, fallback_to_name: true) end # Map tag UUIDs to names for portability - if action.action_type == "set_transaction_tags" && action.value.present? - tag = @family.tags.find_by(id: action.value) || @family.tags.find_by(name: action.value) - return tag&.name || action.value + if action.action_type == "set_transaction_tags" + return rule_operand(action.value, type: "Tag", relation: @family.tags, fallback_to_name: true) end - action.value + rule_operand(action.value) + end + + def rule_operand(value, type: nil, relation: nil, fallback_to_name: false) + record = relation && resolve_rule_operand_record(relation, value, fallback_to_name: fallback_to_name) + + { + value: record&.name || value, + value_ref: record ? rule_value_ref(type, record) : nil + } + end + + def resolve_rule_operand_record(relation, value, fallback_to_name:) + return relation.find_by(id: value) if uuid_like?(value) + + relation.find_by(name: value) if fallback_to_name + end + + def rule_value_ref(type, record) + { + type: type, + id: record.id, + name: record.name + } + end + + def uuid_like?(value) + UuidFormat.valid?(value) end def serialize_conditions_for_csv(conditions) diff --git a/app/models/family/data_importer.rb b/app/models/family/data_importer.rb index 7601516ce..8ee468dc8 100644 --- a/app/models/family/data_importer.rb +++ b/app/models/family/data_importer.rb @@ -671,7 +671,7 @@ def build_rule_action(rule, action_data) def resolve_rule_condition_value(condition_data) condition_type = condition_data["condition_type"] - value = condition_data["value"] + value = rule_operand_value(condition_data) return value unless value.present? @@ -699,7 +699,7 @@ def resolve_rule_condition_value(condition_data) def resolve_rule_action_value(action_data) action_type = action_data["action_type"] - value = action_data["value"] + value = rule_operand_value(action_data) return value unless value.present? @@ -732,6 +732,21 @@ def resolve_rule_action_value(action_data) value end + def rule_operand_value(data) + raw_value = data["value"] + value = raw_value.is_a?(String) ? raw_value.presence : raw_value + value_ref_name = data.dig("value_ref", "name") + + return value_ref_name if value.is_a?(String) && uuid_like?(value) && value_ref_name.present? + return value unless value.nil? + + value_ref_name + end + + def uuid_like?(value) + UuidFormat.valid?(value) + end + def importable_cost_basis_source(value) source = value.to_s Holding::COST_BASIS_SOURCES.include?(source) ? source : nil diff --git a/lib/uuid_format.rb b/lib/uuid_format.rb new file mode 100644 index 000000000..6e4890f9c --- /dev/null +++ b/lib/uuid_format.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module UuidFormat + PATTERN = /\A[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\z/i.freeze + + module_function + + def valid?(value) + PATTERN.match?(value.to_s) + end +end diff --git a/test/lib/uuid_format_test.rb b/test/lib/uuid_format_test.rb new file mode 100644 index 000000000..0d7389b97 --- /dev/null +++ b/test/lib/uuid_format_test.rb @@ -0,0 +1,17 @@ +require "test_helper" + +class UuidFormatTest < ActiveSupport::TestCase + test "valid matches canonical UUID values" do + uuid = SecureRandom.uuid + + assert UuidFormat.valid?(uuid) + assert UuidFormat.valid?(uuid.upcase) + end + + test "valid rejects non UUID values" do + refute UuidFormat.valid?(nil) + refute UuidFormat.valid?("") + refute UuidFormat.valid?("not-a-uuid") + refute UuidFormat.valid?("#{SecureRandom.uuid}-extra") + end +end diff --git a/test/models/family/data_exporter_test.rb b/test/models/family/data_exporter_test.rb index 03376e08e..5a8ff650a 100644 --- a/test/models/family/data_exporter_test.rb +++ b/test/models/family/data_exporter_test.rb @@ -318,9 +318,56 @@ class Family::DataExporterTest < ActiveSupport::TestCase assert_equal "set_transaction_category", actions[0]["action_type"] # Should export category name instead of UUID assert_equal "Test Category", actions[0]["value"] + assert_equal({ "type" => "Category", "id" => @category.id, "name" => "Test Category" }, actions[0]["value_ref"]) end end + test "exports rule condition value refs for mapped operands" do + category_rule = @family.rules.build( + name: "Category Condition Rule", + resource_type: "transaction", + active: true + ) + category_rule.conditions.build( + condition_type: "transaction_category", + operator: "=", + value: @category.id + ) + category_rule.actions.build(action_type: "auto_categorize") + category_rule.save! + + zip_data = @exporter.generate_export + + Zip::File.open_buffer(zip_data) do |zip| + rule_data = zip.read("all.ndjson").split("\n").filter_map do |line| + parsed = JSON.parse(line) + parsed if parsed["type"] == "Rule" && parsed["data"]["name"] == "Category Condition Rule" + end.first + + condition = rule_data["data"]["conditions"].first + assert_equal "Test Category", condition["value"] + assert_equal({ "type" => "Category", "id" => @category.id, "name" => "Test Category" }, condition["value_ref"]) + end + end + + test "rule operand lookup skips name fallback for stale UUID values" do + stale_uuid = SecureRandom.uuid + relation = mock + relation.expects(:find_by).with(id: stale_uuid).once.returns(nil) + relation.expects(:find_by).with(name: stale_uuid).never + + operand = @exporter.send( + :rule_operand, + stale_uuid, + type: "Category", + relation: relation, + fallback_to_name: true + ) + + assert_equal stale_uuid, operand[:value] + assert_nil operand[:value_ref] + end + test "exports rule actions and maps tag UUIDs to names" do # Create a rule with a tag action tag_rule = @family.rules.build( @@ -359,6 +406,7 @@ class Family::DataExporterTest < ActiveSupport::TestCase assert_equal "set_transaction_tags", actions[0]["action_type"] # Should export tag name instead of UUID assert_equal "Test Tag", actions[0]["value"] + assert_equal({ "type" => "Tag", "id" => @tag.id, "name" => "Test Tag" }, actions[0]["value_ref"]) end end diff --git a/test/models/family/data_importer_test.rb b/test/models/family/data_importer_test.rb index 0d8efd0be..c6c75dda1 100644 --- a/test/models/family/data_importer_test.rb +++ b/test/models/family/data_importer_test.rb @@ -1498,6 +1498,119 @@ class Family::DataImporterTest < ActiveSupport::TestCase assert_equal category.id, action.value end + test "imports rules from normalized operand value refs" do + ndjson = build_ndjson([ + { + type: "Rule", + version: 1, + data: { + name: "Map Merchant To Dining", + resource_type: "transaction", + active: true, + conditions: [ + { + condition_type: "transaction_merchant", + operator: "=", + value_ref: { + type: "Merchant", + id: "source-merchant-id", + name: "Coffee Bar" + } + } + ], + actions: [ + { + action_type: "set_transaction_category", + value_ref: { + type: "Category", + id: "source-category-id", + name: "Dining" + } + } + ] + } + } + ]) + + importer = Family::DataImporter.new(@family, ndjson) + importer.import! + + rule = @family.rules.find_by!(name: "Map Merchant To Dining") + merchant = @family.merchants.find_by!(name: "Coffee Bar") + category = @family.categories.find_by!(name: "Dining") + + assert_equal merchant.id, rule.conditions.first.value + assert_equal category.id, rule.actions.first.value + end + + test "imports rule value refs when legacy operand values are stale UUIDs" do + stale_merchant_id = SecureRandom.uuid + stale_category_id = SecureRandom.uuid + ndjson = build_ndjson([ + { + type: "Rule", + version: 1, + data: { + name: "Map Stale UUID Operands", + resource_type: "transaction", + active: true, + conditions: [ + { + condition_type: "transaction_merchant", + operator: "=", + value: stale_merchant_id, + value_ref: { + type: "Merchant", + id: stale_merchant_id, + name: "Coffee Bar" + } + } + ], + actions: [ + { + action_type: "set_transaction_category", + value: stale_category_id, + value_ref: { + type: "Category", + id: stale_category_id, + name: "Dining" + } + } + ] + } + } + ]) + + importer = Family::DataImporter.new(@family, ndjson) + importer.import! + + rule = @family.rules.find_by!(name: "Map Stale UUID Operands") + merchant = @family.merchants.find_by!(name: "Coffee Bar") + category = @family.categories.find_by!(name: "Dining") + + assert_equal merchant.id, rule.conditions.first.value + assert_equal category.id, rule.actions.first.value + assert_not @family.merchants.exists?(name: stale_merchant_id) + assert_not @family.categories.exists?(name: stale_category_id) + end + + test "preserves explicit false rule operand values" do + importer = Family::DataImporter.new(@family, "") + + value = importer.send( + :rule_operand_value, + { + "value" => false, + "value_ref" => { + "type" => "Category", + "name" => "Fallback" + } + } + ) + + assert_equal false, value + end + test "imports rules with compound conditions" do ndjson = build_ndjson([ {