From 334f6ce277bd1eaeaba166af19a424e6ee816e46 Mon Sep 17 00:00:00 2001 From: Jacob Kiers Date: Thu, 27 Nov 2025 21:04:41 +0100 Subject: [PATCH] fix: Prevent duplicate account links Before, for each sync run, accounts were re-linked together. That is incorrect, they should only be linked once. Furthermore, the account status for Gocardless was incorrect: it always defaulted to "linked". That is now fixed: it now defaults to "active". --- banks2ff/src/adapters/gocardless/client.rs | 11 ++-- banks2ff/src/core/linking.rs | 69 +++++++++++++++++++++- banks2ff/src/core/models.rs | 2 +- banks2ff/src/core/sync.rs | 6 +- banks2ff/src/main.rs | 15 +++-- 5 files changed, 88 insertions(+), 15 deletions(-) diff --git a/banks2ff/src/adapters/gocardless/client.rs b/banks2ff/src/adapters/gocardless/client.rs index a3591a4..ee749f1 100644 --- a/banks2ff/src/adapters/gocardless/client.rs +++ b/banks2ff/src/adapters/gocardless/client.rs @@ -258,18 +258,19 @@ impl TransactionSource for GoCardlessAdapter { if let Some(req_accounts) = req.accounts { for acc_id in req_accounts { - let iban = if let Some(iban) = cache.get_iban(&acc_id) { - iban + let (iban, status) = if let Some(iban) = cache.get_iban(&acc_id) { + (iban, "active".to_string()) // Cached accounts are active } else { // Fetch if not cached match client.get_account(&acc_id).await { Ok(details) => { let iban = details.iban.unwrap_or_default(); + let status = details.status.unwrap_or_else(|| "unknown".to_string()); cache.insert(acc_id.clone(), iban.clone()); cache.save(); - iban + (iban, status) } - Err(_) => "Unknown".to_string(), + Err(_) => ("Unknown".to_string(), "error".to_string()), } }; @@ -277,7 +278,7 @@ impl TransactionSource for GoCardlessAdapter { id: acc_id, iban, currency: "EUR".to_string(), // Assuming EUR for now - status: "linked".to_string(), + status, }); } } diff --git a/banks2ff/src/core/linking.rs b/banks2ff/src/core/linking.rs index 33ac719..202ab41 100644 --- a/banks2ff/src/core/linking.rs +++ b/banks2ff/src/core/linking.rs @@ -59,7 +59,12 @@ impl LinkStore { source_account: &Account, dest_account: &Account, auto_linked: bool, - ) -> String { + ) -> Option { + // Check if link already exists + if self.links.iter().any(|l| l.source_account_id == source_account.id && l.dest_account_id == dest_account.id) { + return None; // Link already exists + } + let id = format!("link_{}", self.next_id); self.next_id += 1; let link = AccountLink { @@ -70,7 +75,7 @@ impl LinkStore { auto_linked, }; self.links.push(link); - id + Some(id) } pub fn set_alias(&mut self, link_id: &str, alias: String) -> Result<()> { @@ -125,3 +130,63 @@ pub fn auto_link_accounts( // Could add name similarity matching here links } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_add_link_prevents_duplicates() { + let mut store = LinkStore::default(); + let src = Account { + id: "src1".to_string(), + iban: "NL01".to_string(), + currency: "EUR".to_string(), + }; + let dest = Account { + id: "dest1".to_string(), + iban: "NL01".to_string(), + currency: "EUR".to_string(), + }; + + // First call should create a link + let first_result = store.add_link(&src, &dest, true); + assert!(first_result.is_some()); + assert_eq!(store.links.len(), 1); + + // Second call should not create a duplicate + let second_result = store.add_link(&src, &dest, true); + assert!(second_result.is_none()); + assert_eq!(store.links.len(), 1); // Still only one link + } + + #[test] + fn test_add_link_allows_different_accounts() { + let mut store = LinkStore::default(); + let src1 = Account { + id: "src1".to_string(), + iban: "NL01".to_string(), + currency: "EUR".to_string(), + }; + let dest1 = Account { + id: "dest1".to_string(), + iban: "NL01".to_string(), + currency: "EUR".to_string(), + }; + let dest2 = Account { + id: "dest2".to_string(), + iban: "NL02".to_string(), + currency: "EUR".to_string(), + }; + + // Link src1 to dest1 + let result1 = store.add_link(&src1, &dest1, true); + assert!(result1.is_some()); + assert_eq!(store.links.len(), 1); + + // Link src1 to dest2 (different destination) + let result2 = store.add_link(&src1, &dest2, true); + assert!(result2.is_some()); + assert_eq!(store.links.len(), 2); // Two different links + } +} diff --git a/banks2ff/src/core/models.rs b/banks2ff/src/core/models.rs index 6beac6a..091da13 100644 --- a/banks2ff/src/core/models.rs +++ b/banks2ff/src/core/models.rs @@ -121,7 +121,7 @@ pub struct AccountSummary { pub id: String, pub iban: String, pub currency: String, - pub status: String, // e.g., "active", "expired", "linked" + pub status: String, // e.g., "active", "blocked", "suspended" } #[derive(Clone, Debug, Serialize)] diff --git a/banks2ff/src/core/sync.rs b/banks2ff/src/core/sync.rs index 1720384..f08b71b 100644 --- a/banks2ff/src/core/sync.rs +++ b/banks2ff/src/core/sync.rs @@ -58,7 +58,11 @@ pub async fn run_sync( for (src_idx, dest_idx) in links { let src = &all_source_accounts[src_idx]; let dest = &all_dest_accounts[dest_idx]; - link_store.add_link(src, dest, true); + if let Some(_link_id) = link_store.add_link(src, dest, true) { + info!("Created new account link: {} -> {}", src.id, dest.id); + } else { + info!("Account link already exists: {} -> {}", src.id, dest.id); + } } link_store.save().map_err(SyncError::SourceError)?; diff --git a/banks2ff/src/main.rs b/banks2ff/src/main.rs index 9729773..3e26741 100644 --- a/banks2ff/src/main.rs +++ b/banks2ff/src/main.rs @@ -375,12 +375,15 @@ async fn handle_link(subcommand: LinkCommands) -> anyhow::Result<()> { .cloned(); if let (Some(src), Some(dst)) = (source_acc, dest_acc) { - let link_id = link_store.add_link(&src, &dst, false); - link_store.save()?; - println!( - "Created link {} between {} and {}", - link_id, src.iban, dst.iban - ); + if let Some(link_id) = link_store.add_link(&src, &dst, false) { + link_store.save()?; + println!( + "Created link {} between {} and {}", + link_id, src.iban, dst.iban + ); + } else { + println!("Link between {} and {} already exists", src.iban, dst.iban); + } } else { println!("Account not found. Ensure accounts are discovered via sync first."); }