diff --git a/app/javascript/controllers/rule/conditions_controller.js b/app/javascript/controllers/rule/conditions_controller.js index fccabd527..ad23c862c 100644 --- a/app/javascript/controllers/rule/conditions_controller.js +++ b/app/javascript/controllers/rule/conditions_controller.js @@ -2,7 +2,7 @@ import { Controller } from "@hotwired/stimulus"; // Connects to data-controller="rule--conditions" export default class extends Controller { - static values = { conditionFilters: Array }; + static values = { conditionFilters: Array, valuelessOperators: Array }; static targets = [ "destroyField", "filterValue", @@ -12,7 +12,7 @@ export default class extends Controller { ]; connect() { - // Hide value field on initial load if operator is "is_null" + // Hide value field on initial load for valueless operators (e.g. "is_null") this.#toggleValueFieldVisibility(); } @@ -129,7 +129,7 @@ export default class extends Controller { #toggleValueFieldVisibility() { const operator = this.operatorSelectTarget.value; - if (operator === "is_null") { + if (this.valuelessOperatorsValue.includes(operator)) { this.filterValueTarget.classList.add("hidden"); // Clear the value since it's not needed if (this.valueInputEl) { diff --git a/app/models/rule/condition.rb b/app/models/rule/condition.rb index 1fece12df..67429f95c 100644 --- a/app/models/rule/condition.rb +++ b/app/models/rule/condition.rb @@ -14,7 +14,7 @@ class Rule::Condition < ApplicationRecord validates :condition_type, presence: true, inclusion: { in: SUPPORTED_CONDITION_TYPES, allow_blank: true } validates :operator, presence: true - validates :value, presence: true, unless: -> { compound? || operator == "is_null" } + validates :value, presence: true, unless: -> { compound? || Rule::ConditionFilter::VALUELESS_OPERATORS.include?(operator) } accepts_nested_attributes_for :sub_conditions, allow_destroy: true diff --git a/app/models/rule/condition_filter.rb b/app/models/rule/condition_filter.rb index fdabcfa34..2c3ee2f51 100644 --- a/app/models/rule/condition_filter.rb +++ b/app/models/rule/condition_filter.rb @@ -3,10 +3,32 @@ class Rule::ConditionFilter TYPES = [ "text", "number", "select" ] + # Operators that don't require a value (and that the form hides the value field for) + VALUELESS_OPERATORS = [ "is_null", "is_not_null" ].freeze + OPERATORS_MAP = { - "text" => [ [ "Contains", "like" ], [ "Equal to", "=" ], [ "Is empty", "is_null" ] ], - "number" => [ [ "Greater than", ">" ], [ "Greater or equal to", ">=" ], [ "Less than", "<" ], [ "Less than or equal to", "<=" ], [ "Is equal to", "=" ], [ "Is not equal to", "!=" ] ], - "select" => [ [ "Equal to", "=" ], [ "Is empty", "is_null" ] ] + "text" => [ + [ :contains, "like" ], + [ :does_not_contain, "not_like" ], + [ :equal_to, "=" ], + [ :not_equal_to, "!=" ], + [ :is_empty, "is_null" ], + [ :is_not_empty, "is_not_null" ] + ], + "number" => [ + [ :greater_than, ">" ], + [ :greater_or_equal_to, ">=" ], + [ :less_than, "<" ], + [ :less_than_or_equal_to, "<=" ], + [ :is_equal_to, "=" ], + [ :not_equal_to, "!=" ] + ], + "select" => [ + [ :equal_to, "=" ], + [ :not_equal_to, "!=" ], + [ :is_empty, "is_null" ], + [ :is_not_empty, "is_not_null" ] + ] } def initialize(rule) @@ -39,7 +61,7 @@ class Rule::ConditionFilter end def operators - OPERATORS_MAP.dig(type) + OPERATORS_MAP.dig(type).map { |label_key, operator| [ operator_label(label_key), operator ] } end # Matchers can prepare the scope with joins by implementing this method @@ -70,20 +92,35 @@ class Rule::ConditionFilter rule.family end + def operator_label(key) + I18n.t("rules.condition_filters.operators.#{key}") + end + def build_sanitized_where_condition(field, operator, value) - if operator == "is_null" + if VALUELESS_OPERATORS.include?(operator) ActiveRecord::Base.sanitize_sql_for_conditions( "#{field} #{sanitize_operator(operator)}" ) else normalized_value = normalize_value(value) normalized_field = normalize_field(field) - sanitized_value = operator == "like" ? "%#{ActiveRecord::Base.sanitize_sql_like(normalized_value)}%" : normalized_value - ActiveRecord::Base.sanitize_sql_for_conditions([ - "#{normalized_field} #{sanitize_operator(operator)} ?", - sanitized_value - ]) + if operator == "like" || operator == "not_like" + sanitized_value = "%#{ActiveRecord::Base.sanitize_sql_like(normalized_value)}%" + expression = ActiveRecord::Base.sanitize_sql_for_conditions([ + "#{normalized_field} #{sanitize_operator(operator)} ?", + sanitized_value + ]) + + # "Does not contain" should also match rows where the field is absent (NULL), + # otherwise NOT ILIKE silently drops them due to SQL's three-valued logic. + operator == "not_like" ? "(#{expression} OR #{field} IS NULL)" : expression + else + ActiveRecord::Base.sanitize_sql_for_conditions([ + "#{normalized_field} #{sanitize_operator(operator)} ?", + normalized_value + ]) + end end end @@ -93,8 +130,18 @@ class Rule::ConditionFilter case operator when "like" "ILIKE" + when "not_like" + "NOT ILIKE" when "is_null" "IS NULL" + when "is_not_null" + "IS NOT NULL" + when "!=" + # IS DISTINCT FROM treats NULL as a regular value, so "not equal to X" also + # matches rows where the field is NULL. This is intentional for select-type + # fields (e.g. merchant_id, category_id) where NULL means "not set". For + # number-type fields (e.g. amount), NULL is impossible at the DB level. + "IS DISTINCT FROM" else operator end diff --git a/app/models/rule/condition_filter/transaction_details.rb b/app/models/rule/condition_filter/transaction_details.rb index b93e769c1..8484e2ab3 100644 --- a/app/models/rule/condition_filter/transaction_details.rb +++ b/app/models/rule/condition_filter/transaction_details.rb @@ -3,6 +3,16 @@ class Rule::ConditionFilter::TransactionDetails < Rule::ConditionFilter "text" end + # JSONB search only supports contains/equals/empty semantics, so we keep the + # original operator set rather than inheriting the extended text operators. + def operators + [ + [ I18n.t("rules.condition_filters.operators.contains"), "like" ], + [ I18n.t("rules.condition_filters.operators.equal_to"), "=" ], + [ I18n.t("rules.condition_filters.operators.is_empty"), "is_null" ] + ] + end + def prepare(scope) scope end diff --git a/app/views/rule/conditions/_condition.html.erb b/app/views/rule/conditions/_condition.html.erb index a3354dc6a..f2c9b7272 100644 --- a/app/views/rule/conditions/_condition.html.erb +++ b/app/views/rule/conditions/_condition.html.erb @@ -3,7 +3,10 @@ <% condition = form.object %> <% rule = condition.rule %> -
  • +
  • <%# Conditionally render the prefix %> <%# Condition groups pass in show_prefix: false for subconditions since the ANY/ALL selector makes that clear %> diff --git a/config/locales/views/rules/en.yml b/config/locales/views/rules/en.yml index 10229a22a..308f24698 100644 --- a/config/locales/views/rules/en.yml +++ b/config/locales/views/rules/en.yml @@ -104,6 +104,18 @@ en: expense: Expense transfer: Transfer equal_to: Equal to + operators: + contains: Contains + does_not_contain: Does not contain + equal_to: Equal to + is_equal_to: Is equal to + not_equal_to: Not equal to + is_empty: Is empty + is_not_empty: Is not empty + greater_than: Greater than + greater_or_equal_to: Greater or equal to + less_than: Less than + less_than_or_equal_to: Less than or equal to rule: conditions: condition_group: diff --git a/test/models/rule/condition_test.rb b/test/models/rule/condition_test.rb index 4eb18e50b..2e30ccc60 100644 --- a/test/models/rule/condition_test.rb +++ b/test/models/rule/condition_test.rb @@ -91,6 +91,98 @@ class Rule::ConditionTest < ActiveSupport::TestCase assert_equal 2, filtered.count end + test "applies not equal operator for select condition and includes nulls" do + scope = @rule_scope + + # Only transaction1 and transaction4 have a merchant (@whole_foods_merchant) + condition = Rule::Condition.new( + rule: @transaction_rule, + condition_type: "transaction_merchant", + operator: "!=", + value: @whole_foods_merchant.id + ) + + scope = condition.prepare(scope) + filtered = condition.apply(scope) + + # "not equal" includes the 3 transactions with no merchant (NULL) too + assert_equal 3, filtered.count + assert filtered.all? { |t| t.merchant_id != @whole_foods_merchant.id } + end + + test "applies is_not_null operator for select condition" do + scope = @rule_scope + + condition = Rule::Condition.new( + rule: @transaction_rule, + condition_type: "transaction_merchant", + operator: "is_not_null", + value: nil + ) + + scope = condition.prepare(scope) + filtered = condition.apply(scope) + + assert_equal 2, filtered.count + assert filtered.all? { |t| t.merchant_id.present? } + end + + test "applies not equal operator for number condition" do + scope = @rule_scope + + condition = Rule::Condition.new( + rule: @transaction_rule, + condition_type: "transaction_amount", + operator: "!=", + value: "100" + ) + + scope = condition.prepare(scope) + filtered = condition.apply(scope) + + # transaction1 has absolute amount 100, the other 4 differ + assert_equal 4, filtered.count + assert filtered.all? { |t| t.entry.amount.abs != 100 } + end + + test "applies not_like operator for text condition and includes nulls" do + scope = @rule_scope + + condition = Rule::Condition.new( + rule: @transaction_rule, + condition_type: "transaction_name", + operator: "not_like", + value: "transaction1" + ) + + scope = condition.prepare(scope) + filtered = condition.apply(scope) + + # Excludes only transaction1, keeps the other 4 + assert_equal 4, filtered.count + assert filtered.none? { |t| t.entry.name.include?("transaction1") } + end + + test "not_like operator keeps rows with NULL field value (OR IS NULL branch)" do + # entries.notes is nullable, so we can verify the OR IS NULL guard in not_like + noted_entry = @account.entries.first + noted_entry.update!(notes: "business trip") + + condition = Rule::Condition.new( + rule: @transaction_rule, + condition_type: "transaction_notes", + operator: "not_like", + value: "business trip" + ) + + scope = condition.prepare(@rule_scope) + filtered = condition.apply(scope) + + # The entry with matching notes is excluded; the 4 entries with NULL notes are kept + assert_equal 4, filtered.count + assert filtered.none? { |t| t.id == noted_entry.transaction.id } + end + test "applies compound and condition" do scope = @rule_scope