mirror of
https://github.com/we-promise/sure.git
synced 2026-09-02 05:11:05 +00:00
fix(enable-banking): tighten date-stamp regex and clean up the merchant line
Addresses two review threads on this PR:
1. jjmata (PR review): technical_remittance_line?'s date-stamp check required
exactly 2-digit day/month (\d{2}[./]\d{2}), so an un-padded date ("1.07."
instead of "01.07.") wasn't recognized as technical and the line would
resurface as the transaction name -- reproducing the original #2935 bug for
that date shape. Now accepts 1-2 digits for both.
2. john-frandsen (issue #2935 comment): suggested cleaning up the merchant line
further (e.g. "BILLA DANKT 0007114 SIEGENDORF 7011" -> "Billa"). Checked
point 1 (structured remittance fields) against Enable Banking's own API
docs -- no such field exists there, not applicable. Points 2/3/5 already
match current behavior. Point 4 (loyalty-marker cleanup) implemented as two
layers:
- Primary: match the line against merchants the family already knows
(Family#known_merchant_names) -- self-maintaining, no pattern-guessing,
and now also assigns the transaction's merchant when matched (previously
out of scope for blank-counterparty EB transactions). Case-insensitive,
regex-escaped, longest-match-wins, with a minimum length guard against
spurious short-name matches.
- Fallback (no known merchant yet): remove only the "DANKT"/"DANKE"
thank-you marker word itself, not a directional truncation -- the marker
can precede or follow the merchant name depending on phrasing ("X DANKT"
vs. "DANKE ... bei X"), so truncating at it risked deleting the real
merchant name in one of the two phrasings.
Verified against the full test suite, RuboCop, and Brakeman on a test-stack
Rails instance (0 RuboCop offenses, 0 Brakeman warnings; full-suite failures
present on that instance are pre-existing/environmental and unrelated to
these files).
Disclosure: this fix (investigation, implementation, and tests) was written
by Claude Code.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
a4a1029d34
commit
5e39ee6b1c
@@ -6,6 +6,21 @@ class EnableBankingEntry::Processor
|
||||
# Small-merchant card terminal providers that prefix the payee with "KEYWORD *"
|
||||
PAYMENT_PROCESSOR_PREFIX = /\A(SUMUP|SQ|IZETTLE|ZETTLE|PAYPAL)\s*\*\s*/i
|
||||
|
||||
# Austrian/German retail POS terminals often include a "DANKT"/"DANKE" thank-you
|
||||
# marker somewhere in the merchant line (e.g. "<CHAIN> DANKT ..." or
|
||||
# "DANKE ... <CHAIN>"). It's never part of the retailer's actual name, so it's
|
||||
# safe to remove -- but its position relative to the merchant name isn't fixed,
|
||||
# so only the marker word itself is removed rather than truncating the line at
|
||||
# it. Used as a last-resort fallback when no known merchant name matches (see
|
||||
# matched_known_merchant_name) -- generalizes across retailers and phrasings
|
||||
# without risking the real merchant name being cut off.
|
||||
LOYALTY_MARKER_WORD = /\b(DANKT|DANKE)\b/i
|
||||
|
||||
# Guard against spurious matches from very short known merchant names (e.g. a
|
||||
# 2-letter FamilyMerchant name matching inside unrelated text, like "IT" would
|
||||
# inside "NAME IT" -- a real chain name observed in this issue's own data).
|
||||
MIN_KNOWN_MERCHANT_MATCH_LENGTH = 3
|
||||
|
||||
# enable_banking_transaction is the raw hash fetched from Enable Banking API
|
||||
# Transaction structure from Enable Banking:
|
||||
# {
|
||||
@@ -224,7 +239,9 @@ class EnableBankingEntry::Processor
|
||||
def primary_remittance_information
|
||||
lines = remittance_information_lines
|
||||
descriptive = lines.find { |line| !technical_remittance_line?(line) } || lines.first
|
||||
strip_payment_processor_prefix(descriptive)
|
||||
return descriptive if descriptive.blank?
|
||||
|
||||
matched_known_merchant_name(descriptive) || strip_loyalty_marker(strip_payment_processor_prefix(descriptive))
|
||||
end
|
||||
|
||||
def remittance_information_lines
|
||||
@@ -243,8 +260,10 @@ class EnableBankingEntry::Processor
|
||||
# no separate technical-only element) would wrongly match on the prefix alone,
|
||||
# and a line like "Invoice paid 31.07. 10:27" would wrongly match on the date
|
||||
# suffix alone. Requiring both matches every real technical line observed in
|
||||
# production while leaving both of those legitimate shapes untouched.
|
||||
line.match?(/\A(POS|ATM)\s+\d+[.,]\d{2}\b.*\d{2}[.\/]\d{2}\.?\s+\d{2}:\d{2}\z/i)
|
||||
# production while leaving both of those legitimate shapes untouched. Day/month
|
||||
# accept 1-2 digits (not just 2) so an un-padded ASPSP date ("1.07." instead of
|
||||
# "01.07.") is still recognized as technical.
|
||||
line.match?(/\A(POS|ATM)\s+\d+[.,]\d{2}\b.*\d{1,2}[.\/]\d{1,2}\.?\s+\d{2}:\d{2}\z/i)
|
||||
end
|
||||
|
||||
def strip_payment_processor_prefix(value)
|
||||
@@ -252,12 +271,53 @@ class EnableBankingEntry::Processor
|
||||
value.sub(PAYMENT_PROCESSOR_PREFIX, "").strip.presence || value
|
||||
end
|
||||
|
||||
def strip_loyalty_marker(value)
|
||||
return value if value.blank?
|
||||
# Only touch the string when the marker is actually present -- squeeze/strip
|
||||
# would otherwise also collapse intentional multi-space formatting (e.g. the
|
||||
# raw technical POS line) on lines that never had a marker to remove.
|
||||
return value unless value.match?(LOYALTY_MARKER_WORD)
|
||||
value.sub(LOYALTY_MARKER_WORD, "").squeeze(" ").strip.presence || value
|
||||
end
|
||||
|
||||
# Prefer a merchant name the family already knows over any text heuristic: it's
|
||||
# already clean/trusted, and sidesteps guessing which parts of a POS line are
|
||||
# noise (store numbers, city, thank-you markers, ...) vs. part of the name.
|
||||
# Case-insensitive, whole-word match; the *stored* name (and its casing) wins,
|
||||
# so e.g. "BILLA DANKT 0007114" resolves to "Billa", not "BILLA". Longest match
|
||||
# wins when multiple known names match (prefer the more specific one).
|
||||
def matched_known_merchant_name(line)
|
||||
candidates = known_merchant_names.select { |name| name.length >= MIN_KNOWN_MERCHANT_MATCH_LENGTH }
|
||||
# Lookaround instead of \b at both ends: \b only fires on a word/non-word
|
||||
# transition, so it silently fails to match right after a name that itself
|
||||
# ends in punctuation (e.g. "A+B (Café)" ends in ")" -- a non-word char next
|
||||
# to another non-word char has no \b between them). Asserting "the boundary
|
||||
# character, if any, isn't alphanumeric" works regardless of how the known
|
||||
# name itself starts/ends.
|
||||
matches = candidates.select do |name|
|
||||
line.match?(/(?<![[:alnum:]_])#{Regexp.escape(name)}(?![[:alnum:]_])/i)
|
||||
end
|
||||
matches.max_by(&:length)
|
||||
end
|
||||
|
||||
def known_merchant_names
|
||||
@known_merchant_names ||= account&.family&.known_merchant_names || []
|
||||
end
|
||||
|
||||
def merchant_name_candidate
|
||||
counterparty = counterparty_name.to_s.strip
|
||||
return counterparty if counterparty.present? && !technical_card_counterparty?(counterparty)
|
||||
# For technical CARD-* counterparties, reuse remittance as the best merchant candidate
|
||||
|
||||
remittance = primary_remittance_information
|
||||
return remittance.truncate(100, omission: "") if remittance.present? && technical_card_counterparty?(counterparty)
|
||||
return nil if remittance.blank?
|
||||
|
||||
# Trust remittance-derived text as a merchant candidate when either the
|
||||
# counterparty was a technical CARD-* placeholder (existing Wise case), or
|
||||
# the text matched an ALREADY-KNOWN merchant for this family. Inventing a
|
||||
# brand-new merchant from raw noisy POS text (blank counterparty, no CARD-*
|
||||
# signal, no known-merchant match) stays out of scope, unchanged from #2935.
|
||||
return remittance.truncate(100, omission: "") if technical_card_counterparty?(counterparty)
|
||||
return remittance if known_merchant_names.include?(remittance)
|
||||
|
||||
nil
|
||||
end
|
||||
|
||||
@@ -267,6 +267,15 @@ class Family < ApplicationRecord
|
||||
Merchant.where(id: (assigned_ids + recently_unlinked_ids + family_merchant_ids).uniq)
|
||||
end
|
||||
|
||||
# Merchant names already associated with this family (via any provider, or a
|
||||
# manually created FamilyMerchant) -- used to recognize a merchant embedded in
|
||||
# noisy provider text (e.g. Enable Banking's remittance lines) without
|
||||
# inventing a new one from scratch. Deliberately excludes recently-unlinked
|
||||
# merchants (unlike available_merchants), since those were explicitly removed.
|
||||
def known_merchant_names
|
||||
(assigned_merchants.pluck(:name) + merchants.pluck(:name)).uniq
|
||||
end
|
||||
|
||||
def assigned_merchants_for(user)
|
||||
merchant_ids = Transaction.joins(:entry)
|
||||
.where(entries: { account_id: accounts.accessible_by(user).select(:id) })
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
require "test_helper"
|
||||
require "ostruct"
|
||||
|
||||
class EnableBankingEntry::ProcessorTest < ActiveSupport::TestCase
|
||||
setup do
|
||||
@@ -220,17 +221,118 @@ class EnableBankingEntry::ProcessorTest < ActiveSupport::TestCase
|
||||
credit_debit_indicator: "DBIT",
|
||||
remittance_information: [
|
||||
"POS 45,13 AT D6 31.07. 10:27",
|
||||
"BILLA DANKT 0007114 SIEGENDORF 7011"
|
||||
"BILLA DANKT 0007114"
|
||||
],
|
||||
status: "BOOK"
|
||||
}
|
||||
|
||||
EnableBankingEntry::Processor.new(tx, enable_banking_account: @enable_banking_account).process
|
||||
entry = @account.entries.find_by!(external_id: "enable_banking_ref_pos_2935")
|
||||
assert_equal "BILLA DANKT 0007114 SIEGENDORF 7011", entry.name
|
||||
assert_equal "BILLA 0007114", entry.name
|
||||
assert_includes entry.notes, "POS 45,13 AT D6 31.07. 10:27"
|
||||
end
|
||||
|
||||
test "uses the family's known merchant name instead of the raw remittance line when it matches, and assigns the merchant" do
|
||||
@family.merchants.create!(name: "Billa")
|
||||
|
||||
tx = {
|
||||
entry_reference: "ref_known_merchant",
|
||||
transaction_id: nil,
|
||||
booking_date: Date.current.to_s,
|
||||
transaction_amount: { amount: "45.13", currency: "EUR" },
|
||||
creditor: { name: "" },
|
||||
bank_transaction_code: nil,
|
||||
credit_debit_indicator: "DBIT",
|
||||
remittance_information: [
|
||||
"POS 45,13 AT D6 31.07. 10:27",
|
||||
"BILLA DANKT 0007114"
|
||||
],
|
||||
status: "BOOK"
|
||||
}
|
||||
|
||||
EnableBankingEntry::Processor.new(tx, enable_banking_account: @enable_banking_account).process
|
||||
entry = @account.entries.find_by!(external_id: "enable_banking_ref_known_merchant")
|
||||
|
||||
assert_equal "Billa", entry.name
|
||||
assert_equal "Billa", entry.transaction.merchant&.name
|
||||
end
|
||||
|
||||
test "matches known merchants regardless of retailer chain" do
|
||||
%w[Bipa Spar Hofer Lidl Penny].each do |chain_name|
|
||||
@family.merchants.create!(name: chain_name)
|
||||
end
|
||||
|
||||
[
|
||||
[ "bipa", "BIPA DANKT 0001234", "Bipa" ],
|
||||
[ "spar", "SPAR DANKT 0005678", "Spar" ],
|
||||
[ "hofer", "HOFER DANKT 0009876", "Hofer" ],
|
||||
[ "lidl", "LIDL DANKT 0004321", "Lidl" ],
|
||||
[ "penny", "DANKE 0009999 PENNY", "Penny" ]
|
||||
].each do |ref_suffix, raw_line, expected_name|
|
||||
tx = {
|
||||
entry_reference: "ref_known_#{ref_suffix}",
|
||||
transaction_id: nil,
|
||||
booking_date: Date.current.to_s,
|
||||
transaction_amount: { amount: "12.34", currency: "EUR" },
|
||||
creditor: { name: "" },
|
||||
bank_transaction_code: nil,
|
||||
credit_debit_indicator: "DBIT",
|
||||
remittance_information: [
|
||||
"POS 12,34 AT D6 01.01. 09:00",
|
||||
raw_line
|
||||
],
|
||||
status: "BOOK"
|
||||
}
|
||||
|
||||
EnableBankingEntry::Processor.new(tx, enable_banking_account: @enable_banking_account).process
|
||||
entry = @account.entries.find_by!(external_id: "enable_banking_ref_known_#{ref_suffix}")
|
||||
|
||||
assert_equal expected_name, entry.name, "expected #{raw_line.inspect} to resolve to #{expected_name.inspect}"
|
||||
end
|
||||
end
|
||||
|
||||
test "does not match a known merchant name shorter than the minimum match length" do
|
||||
@family.merchants.create!(name: "IT")
|
||||
|
||||
name = build_name_with_family(
|
||||
credit_debit_indicator: "DBIT",
|
||||
creditor: { name: "" },
|
||||
bank_transaction_code: nil,
|
||||
remittance_information: [ "NAME IT 1234" ]
|
||||
)
|
||||
|
||||
# No known merchant match attempted ("IT" is below MIN_KNOWN_MERCHANT_MATCH_LENGTH),
|
||||
# and no DANKT/DANKE marker present, so the line passes through unchanged.
|
||||
assert_equal "NAME IT 1234", name
|
||||
end
|
||||
|
||||
test "prefers the longest matching known merchant name when multiple match" do
|
||||
@family.merchants.create!(name: "Billa")
|
||||
@family.merchants.create!(name: "Billa Corso")
|
||||
|
||||
name = build_name_with_family(
|
||||
credit_debit_indicator: "DBIT",
|
||||
creditor: { name: "" },
|
||||
bank_transaction_code: nil,
|
||||
remittance_information: [ "Billa Corso 0001234" ]
|
||||
)
|
||||
|
||||
assert_equal "Billa Corso", name
|
||||
end
|
||||
|
||||
test "escapes regex metacharacters in a known merchant name so matching never raises" do
|
||||
@family.merchants.create!(name: "A+B (Café)")
|
||||
|
||||
name = build_name_with_family(
|
||||
credit_debit_indicator: "DBIT",
|
||||
creditor: { name: "" },
|
||||
bank_transaction_code: nil,
|
||||
remittance_information: [ "A+B (Café) 0001234" ]
|
||||
)
|
||||
|
||||
assert_equal "A+B (Café)", name
|
||||
end
|
||||
|
||||
test "stores exchange_rate in extra when present" do
|
||||
tx = {
|
||||
entry_reference: "ref_fx",
|
||||
@@ -305,13 +407,24 @@ class EnableBankingEntry::ProcessorTest < ActiveSupport::TestCase
|
||||
end
|
||||
|
||||
def build_processor(data)
|
||||
EnableBankingEntry::Processor.new(data, enable_banking_account: Object.new)
|
||||
# A minimal stand-in that responds to current_account (real EnableBankingAccount
|
||||
# always does) so `account`/`known_merchant_names` resolve safely to nil/[] instead
|
||||
# of raising -- these unit-level tests aren't wired to a real family, so they always
|
||||
# exercise the "no known merchant" fallback path.
|
||||
EnableBankingEntry::Processor.new(data, enable_banking_account: OpenStruct.new(current_account: nil))
|
||||
end
|
||||
|
||||
def build_name(data)
|
||||
build_processor(data).send(:name)
|
||||
end
|
||||
|
||||
# Unlike build_name/build_processor, wires up the real @enable_banking_account
|
||||
# (and thus the real @family) so known-merchant matching has something to match
|
||||
# against -- used by tests that create FamilyMerchant records via @family first.
|
||||
def build_name_with_family(data)
|
||||
EnableBankingEntry::Processor.new(data, enable_banking_account: @enable_banking_account).send(:name)
|
||||
end
|
||||
|
||||
test "skips technical card counterparty and falls back to remittance_information" do
|
||||
name = build_name(
|
||||
credit_debit_indicator: "CRDT",
|
||||
@@ -463,11 +576,11 @@ class EnableBankingEntry::ProcessorTest < ActiveSupport::TestCase
|
||||
bank_transaction_code: nil,
|
||||
remittance_information: [
|
||||
"POS 45,13 AT D6 31.07. 10:27",
|
||||
"BILLA DANKT 0007114 SIEGENDORF 7011"
|
||||
"BILLA DANKT 0007114"
|
||||
]
|
||||
)
|
||||
|
||||
assert_equal "BILLA DANKT 0007114 SIEGENDORF 7011", name
|
||||
assert_equal "BILLA 0007114", name
|
||||
end
|
||||
|
||||
test "skips generic ATM terminal line and uses real merchant from remittance_information" do
|
||||
@@ -490,12 +603,12 @@ class EnableBankingEntry::ProcessorTest < ActiveSupport::TestCase
|
||||
creditor: { name: "" },
|
||||
bank_transaction_code: nil,
|
||||
remittance_information: [
|
||||
"POS 45,13 BILLA DANKT 0007114 SIEGENDORF 7011",
|
||||
"POS 45,13 BILLA DANKT 0007114",
|
||||
"Reference 0394676"
|
||||
]
|
||||
)
|
||||
|
||||
assert_equal "POS 45,13 BILLA DANKT 0007114 SIEGENDORF 7011", name
|
||||
assert_equal "POS 45,13 BILLA 0007114", name
|
||||
end
|
||||
|
||||
test "does not treat a legitimate line as technical just because it ends with a date+time stamp" do
|
||||
@@ -526,6 +639,40 @@ class EnableBankingEntry::ProcessorTest < ActiveSupport::TestCase
|
||||
assert_equal "HERR DR. EISENSTADT 7000", name
|
||||
end
|
||||
|
||||
test "removes the DANKT/DANKE thank-you marker regardless of retailer chain or its position in the line" do
|
||||
[
|
||||
[ "BIPA DANKT 0001234", "BIPA 0001234" ],
|
||||
[ "SPAR DANKT 0005678", "SPAR 0005678" ],
|
||||
[ "HOFER DANKT 0009876", "HOFER 0009876" ],
|
||||
[ "LIDL DANKT 0004321", "LIDL 0004321" ],
|
||||
[ "PENNY DANKE 0002468", "PENNY 0002468" ],
|
||||
[ "DANKE 0009999 PENNY", "0009999 PENNY" ]
|
||||
].each do |raw_line, expected_name|
|
||||
name = build_name(
|
||||
credit_debit_indicator: "DBIT",
|
||||
creditor: { name: "" },
|
||||
bank_transaction_code: nil,
|
||||
remittance_information: [
|
||||
"POS 12,34 AT D6 01.01. 09:00",
|
||||
raw_line
|
||||
]
|
||||
)
|
||||
|
||||
assert_equal expected_name, name, "expected #{raw_line.inspect} to normalize to #{expected_name.inspect}"
|
||||
end
|
||||
end
|
||||
|
||||
test "does not alter a name that merely contains 'dank' as a substring" do
|
||||
name = build_name(
|
||||
credit_debit_indicator: "DBIT",
|
||||
creditor: { name: "" },
|
||||
bank_transaction_code: nil,
|
||||
remittance_information: [ "STEFAN DANKL GMBH" ]
|
||||
)
|
||||
|
||||
assert_equal "STEFAN DANKL GMBH", name
|
||||
end
|
||||
|
||||
test "falls back to the technical line when remittance_information has no descriptive line" do
|
||||
name = build_name(
|
||||
credit_debit_indicator: "DBIT",
|
||||
|
||||
@@ -173,6 +173,20 @@ class FamilyTest < ActiveSupport::TestCase
|
||||
assert_includes family.available_merchants, new_merchant
|
||||
end
|
||||
|
||||
test "known_merchant_names includes both assigned and family-owned merchant names, deduplicated" do
|
||||
family = families(:dylan_family)
|
||||
|
||||
provider_merchant = ProviderMerchant.create!(name: "Known Provider Merchant", source: "enable_banking")
|
||||
transactions(:one).update!(merchant: provider_merchant)
|
||||
unassigned_family_merchant = family.merchants.create!(name: "Unassigned Family Merchant")
|
||||
|
||||
names = family.known_merchant_names
|
||||
|
||||
assert_includes names, "Known Provider Merchant"
|
||||
assert_includes names, unassigned_family_merchant.name
|
||||
assert_equal names.uniq, names
|
||||
end
|
||||
|
||||
test "enabled currencies always include the base currency" do
|
||||
family = families(:dylan_family)
|
||||
family.update!(currency: "SGD", enabled_currencies: [ "USD" ])
|
||||
|
||||
Reference in New Issue
Block a user