feat(exports): add rule operand references#1726
Merged
jjmata merged 11 commits intoMay 12, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFamily::DataExporter emits structured ChangesRule Value Reference Serialization
Sequence DiagramsequenceDiagram
participant Exporter as Family::DataExporter
participant Resolver as resolve_rule_operand_record / rule_operand
participant Store as Category/Merchant/Tag
participant Exported as Exported Rule NDJSON
participant Importer as Family::DataImporter
participant OperandPicker as rule_operand_value
Exporter->>Resolver: request operand for condition/action (input value)
Resolver->>Store: lookup by id (if uuid_like?) or by name
Store-->>Resolver: return record (id, name, type) or nil
Resolver-->>Exporter: return { value, value_ref? }
Exporter->>Exported: emit value + optional value_ref (type/id/name)
Importer->>OperandPicker: supply exported condition/action data
OperandPicker-->>Importer: return operand (value or value_ref.name)
Importer->>Store: resolve operand to record ID
Store-->>Importer: return ID
Importer->>Importer: persist rule with resolved ID
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
|
The One thing in category = @family.categories.find_by(id: action.value) || @family.categories.find_by(name: action.value)When Generated by Claude Code |
superagent-security
Bot
added
contributor:verified
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/models/family/data_exporter.rb (1)
434-450:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve each operand record only once.
These
value_refadditions re-query the same category/merchant/tag record thatresolve_condition_value/resolve_action_valuealready looked up, so rule export now adds 2-4 queries per mapped operand. On larger exports this becomes an avoidable N+1 path. Please derive bothvalueandvalue_reffrom a single resolved record; at minimum, skip the name fallback when the raw value already looks like an unresolved UUID.As per coding guidelines,
app/models/**/*.rb: Optimize database access with proper indexes and prevent N+1 queries via includes/joins.Also applies to: 473-485, 511-528
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/family/data_exporter.rb` around lines 434 - 450, The serialize methods (serialize_condition and serialize_action) currently call resolve_*_value and then separately call resolve_*_value_ref which re-queries the same referenced record; change them to resolve the operand once and derive both value and value_ref from that single resolved record by: (1) add/modify a helper that returns the resolved record (category/merchant/tag) plus the formatted value, (2) have serialize_condition use that helper to set both data[:value] and data[:value_ref] without extra DB lookups, and (3) when the raw stored value already matches an UUID format, skip the name-fallback lookup to avoid an unnecessary query. Apply the same refactor to the other occurrences noted (the blocks around lines 473-485 and 511-528) so all value/value_ref pairs come from one resolved object.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/models/family/data_importer.rb`:
- Around line 691-693: The current rule_operand_value method returns
data["value"] even when it contains a non-portable legacy UUID; change it to
prefer data.dig("value_ref","name") when data["value"] is a raw UUID (i.e.
matches the UUID hex-with-hyphens 36-char format) and otherwise keep the
existing behavior (use data["value"] if present, else value_ref.name). Update
rule_operand_value to detect UUID-like values and fall back to value_ref.name in
that case; keep references to data["value"] and data.dig("value_ref","name") in
the implementation.
---
Outside diff comments:
In `@app/models/family/data_exporter.rb`:
- Around line 434-450: The serialize methods (serialize_condition and
serialize_action) currently call resolve_*_value and then separately call
resolve_*_value_ref which re-queries the same referenced record; change them to
resolve the operand once and derive both value and value_ref from that single
resolved record by: (1) add/modify a helper that returns the resolved record
(category/merchant/tag) plus the formatted value, (2) have serialize_condition
use that helper to set both data[:value] and data[:value_ref] without extra DB
lookups, and (3) when the raw stored value already matches an UUID format, skip
the name-fallback lookup to avoid an unnecessary query. Apply the same refactor
to the other occurrences noted (the blocks around lines 473-485 and 511-528) so
all value/value_ref pairs come from one resolved object.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd5a93be-f080-41bb-9a68-cb9f9bacd138
📒 Files selected for processing (4)
app/models/family/data_exporter.rbapp/models/family/data_importer.rbtest/models/family/data_exporter_test.rbtest/models/family/data_importer_test.rb
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/models/family/data_exporter.rb (2)
475-494: 💤 Low valueConsider removing redundant presence checks.
Lines 479, 484, and 489 check
action.value.present?again after the early return on line 476 already ensured value is present. While harmless, these checks are redundant.♻️ Optional simplification
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? + 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? + 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? + if action.action_type == "set_transaction_tags" return rule_operand(action.value, type: "Tag", relation: `@family.tags`, fallback_to_name: true) end rule_operand(action.value) end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/family/data_exporter.rb` around lines 475 - 494, The method resolve_action_operand redundantly re-checks action.value.present? inside three branches after the early guard return; remove the extra presence checks so each branch only tests action.action_type and calls rule_operand with the same params (e.g., for "set_transaction_category" call rule_operand(action.value, type: "Category", relation: `@family.categories`, fallback_to_name: true), similarly for "set_transaction_merchant" and "set_transaction_tags"), leaving the initial return rule_operand(action.value) unless action.value.present? guard intact.
459-473: 💤 Low valueConsider removing redundant presence checks.
Lines 463 and 468 check
condition.value.present?again after the early return on line 460 already ensured value is present. While harmless, these checks are redundant.♻️ Optional simplification
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? + 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? + if condition.condition_type == "transaction_merchant" return rule_operand(condition.value, type: "Merchant", relation: `@family.merchants`) end rule_operand(condition.value) end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/family/data_exporter.rb` around lines 459 - 473, The method resolve_condition_operand redundantly checks condition.value.present? inside the transaction_category and transaction_merchant branches even though there's an early return for missing value; remove the repeated presence checks so the branches simply inspect condition.condition_type and call rule_operand(condition.value, type: "Category", relation: `@family.categories`) or rule_operand(condition.value, type: "Merchant", relation: `@family.merchants`) as appropriate, leaving the initial guard (return rule_operand(condition.value) unless condition.value.present?) and the final fallback call to rule_operand(condition.value) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/models/family/data_exporter.rb`:
- Around line 475-494: The method resolve_action_operand redundantly re-checks
action.value.present? inside three branches after the early guard return; remove
the extra presence checks so each branch only tests action.action_type and calls
rule_operand with the same params (e.g., for "set_transaction_category" call
rule_operand(action.value, type: "Category", relation: `@family.categories`,
fallback_to_name: true), similarly for "set_transaction_merchant" and
"set_transaction_tags"), leaving the initial return rule_operand(action.value)
unless action.value.present? guard intact.
- Around line 459-473: The method resolve_condition_operand redundantly checks
condition.value.present? inside the transaction_category and
transaction_merchant branches even though there's an early return for missing
value; remove the repeated presence checks so the branches simply inspect
condition.condition_type and call rule_operand(condition.value, type:
"Category", relation: `@family.categories`) or rule_operand(condition.value, type:
"Merchant", relation: `@family.merchants`) as appropriate, leaving the initial
guard (return rule_operand(condition.value) unless condition.value.present?) and
the final fallback call to rule_operand(condition.value) intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0cb1c7bd-4f82-48d5-908e-220af541139d
📒 Files selected for processing (4)
app/models/family/data_exporter.rbapp/models/family/data_importer.rbtest/models/family/data_exporter_test.rbtest/models/family/data_importer_test.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- app/models/family/data_importer.rb
- test/models/family/data_exporter_test.rb
github-project-automation
Bot
moved this to Todo
in Latest release train (v0.7.1) - May 2026
jjmata
reviewed
May 11, 2026
Collaborator
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Can the UUID_FORMATs be imported from somewhere else?
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/models/family/data_importer.rb`:
- Around line 691-698: In rule_operand_value, avoid using .presence directly on
data["value"] (which treats false as blank); instead preserve explicit
false/non-string values by first extracting raw = data["value"], then normalize
only strings with raw.is_a?(String) ? raw.presence : raw to produce value;
change the uuid fallback check to only run for string values (e.g. if
value.is_a?(String) && uuid_like?(value) && value_ref_name.present? then return
value_ref_name), and finally return value unless it is nil (fall back to
value_ref_name) so boolean false is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e253969b-6ef7-4503-977e-c2f89c2eb185
📒 Files selected for processing (6)
app/controllers/api/v1/base_controller.rbapp/controllers/api/v1/valuations_controller.rbapp/models/family/data_exporter.rbapp/models/family/data_importer.rblib/uuid_format.rbtest/lib/uuid_format_test.rb
💤 Files with no reviewable changes (1)
- app/controllers/api/v1/valuations_controller.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/models/family/data_exporter.rb
jjmata
approved these changes
May 12, 2026
github-project-automation
Bot
moved this from Todo
to Done
in Latest release train (v0.7.1) - May 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
What changed
value_refobjects for category, merchant, and tag operands when the source record can be resolved.value_ref.namewhenvalueis absent.Why
Names alone are not enough for adapter-grade parity, and raw UUIDs are not portable across restored families. Exporting normalized operand references gives clients a machine-comparable contract without breaking existing import behavior.
Validation
docker compose -f .devcontainer/docker-compose.yml exec -T app bash -lc 'cd /workspace && bin/rails test test/models/family/data_exporter_test.rb test/models/family/data_importer_test.rb'docker compose -f .devcontainer/docker-compose.yml exec -T app bash -lc 'cd /workspace && bin/rubocop app/models/family/data_exporter.rb app/models/family/data_importer.rb test/models/family/data_exporter_test.rb test/models/family/data_importer_test.rb'git diff --checkNotes
valuefields remain present in exports;value_refis additive metadata.Summary by CodeRabbit
New Features
Tests
Chores