From 4b7fba71362398823caa2e36fa5ba55b3081ec17 Mon Sep 17 00:00:00 2001 From: Nathan Przybyszewski <172687074+nprzy@users.noreply.github.com> Date: Mon, 10 Jun 2024 10:01:30 -0600 Subject: [PATCH 01/16] Inject Plaid URL in wrapper rather than PlaidApi --- .../api/plaid/PlaidApiWrapper.kt | 16 +++++++++++----- .../api/plaid/apis/PlaidApi.kt | 1 - 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/PlaidApiWrapper.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/PlaidApiWrapper.kt index 10f69c7..c3dabf1 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/PlaidApiWrapper.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/PlaidApiWrapper.kt @@ -1,17 +1,19 @@ package net.djvk.fireflyPlaidConnector2.api.plaid -import io.ktor.client.call.* +import io.ktor.client.HttpClientConfig +import io.ktor.client.engine.HttpClientEngine import io.ktor.client.plugins.* import io.ktor.http.* import kotlinx.coroutines.delay import net.djvk.fireflyPlaidConnector2.api.plaid.apis.PlaidApi -import net.djvk.fireflyPlaidConnector2.api.plaid.infrastructure.clientIdHeader -import net.djvk.fireflyPlaidConnector2.api.plaid.infrastructure.secretHeader import org.slf4j.LoggerFactory import org.springframework.beans.factory.annotation.Value import org.springframework.stereotype.Component import kotlin.time.Duration.Companion.minutes +const val clientIdHeader = "PLAID-CLIENT-ID" +const val secretHeader = "PLAID-SECRET" + /** * A wrapper for Plaid API calls that provides additional services: * - rate limiting @@ -20,14 +22,18 @@ import kotlin.time.Duration.Companion.minutes */ @Component class PlaidApiWrapper( - private val plaidApi: PlaidApi, + @Value("\${fireflyPlaidConnector2.plaid.url}") + private val baseUrl: String, @Value("\${fireflyPlaidConnector2.plaid.maxRetries:3}") private val maxRetries: Int, @Value("\${fireflyPlaidConnector2.plaid.clientId}") private val plaidClientId: String, @Value("\${fireflyPlaidConnector2.plaid.secret}") private val plaidSecret: String, + httpClientEngine: HttpClientEngine? = null, + httpClientConfig: ((HttpClientConfig<*>) -> Unit)? = null, ) { + private val plaidApi = PlaidApi(baseUrl, httpClientEngine, httpClientConfig) private val logger = LoggerFactory.getLogger(this::class.java) init { @@ -62,4 +68,4 @@ class PlaidApiWrapper( return executeRequest(request, logString, remainingRetries - 1) } } -} \ No newline at end of file +} diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/apis/PlaidApi.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/apis/PlaidApi.kt index fac7f3b..198d478 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/apis/PlaidApi.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/apis/PlaidApi.kt @@ -31,7 +31,6 @@ import java.io.File @Component open class PlaidApi( - @Value("\${fireflyPlaidConnector2.plaid.url}") baseUrl: String = ApiClient.BASE_URL, httpClientEngine: HttpClientEngine? = null, httpClientConfig: ((HttpClientConfig<*>) -> Unit)? = null, From 188c6bf50cf784638c4f130b7bd8bcd5e9049327 Mon Sep 17 00:00:00 2001 From: Nathan Przybyszewski <172687074+nprzy@users.noreply.github.com> Date: Mon, 10 Jun 2024 10:53:29 -0600 Subject: [PATCH 02/16] Extract PersonalFinanceCategoryEnum from generated Plaid client The PersonalFinanceCategoryEnum existed as custom code that had been embedded into the generated Plaid client. This extracts the custom enum and associated logic so that the Plaid client may be more easily regenerated from the OpenAPI spec. --- .../plaid/models/PersonalFinanceCategory.kt | 18 +----------- .../PersonalFinanceCategoryEnum.kt | 24 +++++++++++++-- .../transactions/TransactionConverter.kt | 29 +++++++++---------- .../lib/PlaidFixtures.kt | 7 +++-- .../transactions/TransactionConverterTest.kt | 3 +- 5 files changed, 40 insertions(+), 41 deletions(-) rename src/main/kotlin/net/djvk/fireflyPlaidConnector2/{api/plaid/models => transactions}/PersonalFinanceCategoryEnum.kt (95%) diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/PersonalFinanceCategory.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/PersonalFinanceCategory.kt index 8b4332b..e08e15c 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/PersonalFinanceCategory.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/PersonalFinanceCategory.kt @@ -38,20 +38,4 @@ data class PersonalFinanceCategory( /* A granular category conveying the transaction's intent. This field can also be used as a unique identifier for the category. */ @field:JsonProperty("detailed") val detailed: kotlin.String -) : kotlin.collections.HashMap() { - constructor(enum: PersonalFinanceCategoryEnum) : this(enum.primary.name, enum.name) - - fun toEnum(): PersonalFinanceCategoryEnum { - // Special case to handle what I believe is a Plaid bug I saw in the wild - if (primary == PersonalFinanceCategoryEnum.Primary.TRAVEL.name && - detailed == PersonalFinanceCategoryEnum.TRANSPORTATION_PUBLIC_TRANSIT.name) { - return PersonalFinanceCategoryEnum.TRANSPORTATION_PUBLIC_TRANSIT - } - // Business as usual - return PersonalFinanceCategoryEnum.values().find { - it.primary.name == primary && it.name == detailed - } - ?: throw IllegalArgumentException("Failed to convert personal finance category $this to enum") - } -} - +) : kotlin.collections.HashMap() diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/PersonalFinanceCategoryEnum.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PersonalFinanceCategoryEnum.kt similarity index 95% rename from src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/PersonalFinanceCategoryEnum.kt rename to src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PersonalFinanceCategoryEnum.kt index 8987533..139b11a 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/PersonalFinanceCategoryEnum.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PersonalFinanceCategoryEnum.kt @@ -1,5 +1,6 @@ -package net.djvk.fireflyPlaidConnector2.api.plaid.models +package net.djvk.fireflyPlaidConnector2.transactions +import net.djvk.fireflyPlaidConnector2.api.plaid.models.PersonalFinanceCategory import net.djvk.fireflyPlaidConnector2.constants.Direction /** @@ -152,6 +153,25 @@ enum class PersonalFinanceCategoryEnum(val primary: Primary, val detailed: Detai RENT_AND_UTILITIES_OTHER_UTILITIES(Primary.RENT_AND_UTILITIES, RentAndUtilitiesDetailed.OTHER_UTILITIES), OTHER(Primary.OTHER, OtherDetailed.OTHER); + companion object { + fun from(categoryModel: PersonalFinanceCategory): PersonalFinanceCategoryEnum { + // Special case to handle what I believe is a Plaid bug I saw in the wild + if (categoryModel.primary == Primary.TRAVEL.name && + categoryModel.detailed == TRANSPORTATION_PUBLIC_TRANSIT.name) { + return TRANSPORTATION_PUBLIC_TRANSIT + } + // Business as usual + return values().find { + it.primary.name == categoryModel.primary && it.name == categoryModel.detailed + } + ?: throw IllegalArgumentException("Failed to convert personal finance category $categoryModel to enum") + } + } + + fun toPersonalFinanceCategory(): PersonalFinanceCategory { + return PersonalFinanceCategory(this.primary.name, this.name) + } + interface Detailed { val description: String val name: String @@ -333,5 +353,3 @@ enum class PersonalFinanceCategoryEnum(val primary: Primary, val detailed: Detai OTHER("Other"), } } - - diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt index 04a6d41..8d14a0f 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt @@ -4,10 +4,9 @@ import net.djvk.fireflyPlaidConnector2.api.firefly.apis.FireflyTransactionId import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionRead import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionSplit import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionTypeProperty -import net.djvk.fireflyPlaidConnector2.api.plaid.models.PersonalFinanceCategoryEnum -import net.djvk.fireflyPlaidConnector2.api.plaid.models.PersonalFinanceCategoryEnum.Primary.* import net.djvk.fireflyPlaidConnector2.api.plaid.models.PlaidTransactionId import net.djvk.fireflyPlaidConnector2.constants.Direction +import net.djvk.fireflyPlaidConnector2.transactions.PersonalFinanceCategoryEnum.Primary.* import org.slf4j.LoggerFactory import org.springframework.beans.factory.annotation.Value import org.springframework.stereotype.Component @@ -91,12 +90,11 @@ class TransactionConverter( ?: if (useNameForDestination) { tx.name.take(255) } else { - val cat = tx.personalFinanceCategory?.toEnum() - if (cat == null) { + if (tx.personalFinanceCategory == null) { return "Unknown" - } else { - getUnknownSourceOrDestinationName(cat, isSource) } + val cat = PersonalFinanceCategoryEnum.from(tx.personalFinanceCategory) + getUnknownSourceOrDestinationName(cat, isSource) } } @@ -294,7 +292,7 @@ class TransactionConverter( ): SortByPairsBatchedResult { // Split Plaid transactions based on whether they are transfers or not val (transfers, nonTransfers) = txs.partition { - transferTypes.contains(it.personalFinanceCategory?.toEnum()?.primary) + it.personalFinanceCategory != null && transferTypes.contains(PersonalFinanceCategoryEnum.from(it.personalFinanceCategory).primary) } val (singles, pairs) = sortByPairs(transfers, accountMap) @@ -625,17 +623,16 @@ class TransactionConverter( */ protected suspend fun getFireflyCategoryTags(tx: PlaidTransaction): List { val tagz = mutableListOf() + if (tx.personalFinanceCategory == null) { + return tagz + } if (enablePrimaryCategorization) { - val primaryCat = tx.personalFinanceCategory?.primary - if (primaryCat != null) { - tagz.add(primaryCategoryPrefix + convertScreamingSnakeCaseToKebabCase(primaryCat)) - } + val primaryCat = tx.personalFinanceCategory.primary + tagz.add(primaryCategoryPrefix + convertScreamingSnakeCaseToKebabCase(primaryCat)) } if (enableDetailedCategorization) { - val detailedCat = tx.personalFinanceCategory?.toEnum()?.detailed?.name - if (detailedCat != null) { - tagz.add(detailedCategoryPrefix + convertScreamingSnakeCaseToKebabCase(detailedCat)) - } + val detailedCat = PersonalFinanceCategoryEnum.from(tx.personalFinanceCategory).detailed.name + tagz.add(detailedCategoryPrefix + convertScreamingSnakeCaseToKebabCase(detailedCat)) } return tagz } @@ -674,4 +671,4 @@ class TransactionConverter( return TransactionTypeProperty.deposit } } -} \ No newline at end of file +} diff --git a/src/test/kotlin/net/djvk/fireflyPlaidConnector2/lib/PlaidFixtures.kt b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/lib/PlaidFixtures.kt index 5e35728..404b26c 100644 --- a/src/test/kotlin/net/djvk/fireflyPlaidConnector2/lib/PlaidFixtures.kt +++ b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/lib/PlaidFixtures.kt @@ -2,6 +2,7 @@ package net.djvk.fireflyPlaidConnector2.lib import net.djvk.fireflyPlaidConnector2.api.plaid.models.* import net.djvk.fireflyPlaidConnector2.transactions.FireflyAccountId +import net.djvk.fireflyPlaidConnector2.transactions.PersonalFinanceCategoryEnum import net.djvk.fireflyPlaidConnector2.transactions.PlaidAccountId import net.djvk.fireflyPlaidConnector2.transactions.TransactionConverter import net.djvk.fireflyPlaidConnector2.util.Utilities @@ -56,7 +57,7 @@ object PlaidFixtures { originalDescription: String? = null, merchantName: String? = null, checkNumber: String? = null, - personalFinanceCategory: PersonalFinanceCategory = PersonalFinanceCategory(PersonalFinanceCategoryEnum.TRANSFER_OUT_ACCOUNT_TRANSFER) + personalFinanceCategory: PersonalFinanceCategory = PersonalFinanceCategoryEnum.TRANSFER_OUT_ACCOUNT_TRANSFER.toPersonalFinanceCategory() ): Transaction { return Transaction( pendingTransactionId = pendingTransactionId, @@ -154,7 +155,7 @@ object PlaidFixtures { originalDescription = originalDescription, merchantName = merchantName, checkNumber = checkNumber, - personalFinanceCategory = PersonalFinanceCategory(personalFinanceCategory), + personalFinanceCategory = personalFinanceCategory.toPersonalFinanceCategory(), ) } @@ -241,4 +242,4 @@ object PlaidFixtures { } return out } -} \ No newline at end of file +} diff --git a/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt index 2dde027..9816fe5 100644 --- a/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt +++ b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt @@ -4,7 +4,6 @@ import kotlinx.coroutines.runBlocking import net.djvk.fireflyPlaidConnector2.api.firefly.models.ObjectLink import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionRead import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionTypeProperty -import net.djvk.fireflyPlaidConnector2.api.plaid.models.PersonalFinanceCategoryEnum import net.djvk.fireflyPlaidConnector2.api.plaid.models.PlaidTransactionId import net.djvk.fireflyPlaidConnector2.api.plaid.models.Transaction import net.djvk.fireflyPlaidConnector2.lib.FireflyFixtures @@ -338,4 +337,4 @@ internal class TransactionConverterTest { assertEquals(expectedDestinationName, tx.destinationName) } } -} \ No newline at end of file +} From ddea2f135c82e5b9bb134112dfd49875a024c522 Mon Sep 17 00:00:00 2001 From: Nathan Przybyszewski <172687074+nprzy@users.noreply.github.com> Date: Mon, 10 Jun 2024 11:28:56 -0600 Subject: [PATCH 03/16] Drop usage of custom Plaid Transaction.getTimestamp This getTimestamp method was custom-added on top of the generated client code. Dropping this usage will make it easier to regenerate the Plaid client from the OpenAPI spec. --- .../djvk/fireflyPlaidConnector2/sync/BatchSyncRunner.kt | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/sync/BatchSyncRunner.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/sync/BatchSyncRunner.kt index bdb18b2..39edd48 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/sync/BatchSyncRunner.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/sync/BatchSyncRunner.kt @@ -33,8 +33,6 @@ class BatchSyncRunner( private val setInitialBalance: Boolean, @Value("\${fireflyPlaidConnector2.plaid.batchSize}") private val plaidBatchSize: Int, - @Value("\${fireflyPlaidConnector2.timeZone}") - private val timeZoneString: String, private val plaidApiWrapper: PlaidApiWrapper, private val syncHelper: SyncHelper, @@ -44,7 +42,6 @@ class BatchSyncRunner( ) : Runner { private val logger = LoggerFactory.getLogger(this::class.java) - private val timeZone = TimeZone.getTimeZone(timeZoneString) override fun run() { val allPlaidTxs = mutableMapOf>() @@ -202,7 +199,7 @@ class BatchSyncRunner( } val earliestTimestamp = txs.fold(OffsetDateTime.now()) { acc, tx -> - val ts = tx.getTimestamp(timeZone.toZoneId()) + val ts = converter.getTxTimestamp(tx) if (ts < acc) { ts } else { @@ -235,4 +232,4 @@ class BatchSyncRunner( } } } -} \ No newline at end of file +} From f84db3bb91aca14f4bc2f14bdf9c73e5042b21fa Mon Sep 17 00:00:00 2001 From: Nathan Przybyszewski <172687074+nprzy@users.noreply.github.com> Date: Mon, 10 Jun 2024 11:34:41 -0600 Subject: [PATCH 04/16] Revert usage of PlaidTransactionId to String The PlaidTransactionId type alias is custom, added manually to the generated client code. This reverts PlaidTransactionId usage back to string to make it easier to regenerate the Plaid client. --- .../api/plaid/models/Transaction.kt | 4 +--- .../fireflyPlaidConnector2/sync/PolledSyncRunner.kt | 2 +- .../FireflyTransactionExternalIdIndexer.kt | 7 +++---- .../transactions/TransactionConverter.kt | 7 +++---- .../transactions/TransactionConverterTest.kt | 11 +++++------ 5 files changed, 13 insertions(+), 18 deletions(-) diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/Transaction.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/Transaction.kt index c356112..d57f91a 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/Transaction.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/Transaction.kt @@ -29,8 +29,6 @@ import net.djvk.fireflyPlaidConnector2.transactions.TransactionConverter import java.time.OffsetDateTime import java.time.ZoneId -typealias PlaidTransactionId = String - /** * A representation of a transaction * @@ -114,7 +112,7 @@ data class Transaction( /* The unique ID of the transaction. Like all Plaid identifiers, the `transaction_id` is case sensitive. */ @field:JsonProperty("transaction_id") - override val transactionId: PlaidTransactionId, + override val transactionId: kotlin.String, /* The channel used to make a payment. `online:` transactions that took place online. `in store:` transactions that were made at a physical location. `other:` transactions that relate to banks, e.g. fees or deposits. This field replaces the `transaction_type` field. */ @field:JsonProperty("payment_channel") diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/sync/PolledSyncRunner.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/sync/PolledSyncRunner.kt index 93e93ce..8b4c92a 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/sync/PolledSyncRunner.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/sync/PolledSyncRunner.kt @@ -136,7 +136,7 @@ class PolledSyncRunner( val plaidCreatedTxs = mutableListOf() val plaidUpdatedTxs = mutableListOf() - val plaidDeletedTxs = mutableListOf() + val plaidDeletedTxs = mutableListOf() accessTokenLoop@ for ((accessToken, accountIds) in accountAccessTokenSequence) { logger.debug( diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/FireflyTransactionExternalIdIndexer.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/FireflyTransactionExternalIdIndexer.kt index 87cd024..88cef63 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/FireflyTransactionExternalIdIndexer.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/FireflyTransactionExternalIdIndexer.kt @@ -3,7 +3,6 @@ package net.djvk.fireflyPlaidConnector2.transactions import net.djvk.fireflyPlaidConnector2.api.firefly.apis.FireflyExternalId import net.djvk.fireflyPlaidConnector2.api.firefly.apis.FireflyTransactionId import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionRead -import net.djvk.fireflyPlaidConnector2.api.plaid.models.PlaidTransactionId class FireflyTransactionExternalIdIndexer( existingFireflyTxs: List, @@ -24,14 +23,14 @@ class FireflyTransactionExternalIdIndexer( } fun findExistingFireflyTx( - plaidTransactionId: PlaidTransactionId, + plaidTransactionId: String, ): TransactionRead? { return fireflyTxsByExternalId[getExternalId(plaidTransactionId)] } companion object { - fun getExternalId(txId: PlaidTransactionId): String { + fun getExternalId(txId: String): String { return "plaid-${txId}" } } -} \ No newline at end of file +} diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt index 8d14a0f..a6a6fe2 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt @@ -4,7 +4,6 @@ import net.djvk.fireflyPlaidConnector2.api.firefly.apis.FireflyTransactionId import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionRead import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionSplit import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionTypeProperty -import net.djvk.fireflyPlaidConnector2.api.plaid.models.PlaidTransactionId import net.djvk.fireflyPlaidConnector2.constants.Direction import net.djvk.fireflyPlaidConnector2.transactions.PersonalFinanceCategoryEnum.Primary.* import org.slf4j.LoggerFactory @@ -163,7 +162,7 @@ class TransactionConverter( accountMap: Map, plaidCreatedTxs: List, plaidUpdatedTxs: List, - plaidDeletedTxs: List, + plaidDeletedTxs: List, existingFireflyTxs: List, ): ConvertPollSyncResult { logger.trace("Starting ${::convertPollSync.name}") @@ -403,8 +402,8 @@ class TransactionConverter( * This is an issue because the [CandidatePair] array we make matches every A transaction to every B * transaction, so each transaction appears more than once in the array. */ - val usedATxIds = mutableSetOf() - val usedBTxIds = mutableSetOf() + val usedATxIds = mutableSetOf() + val usedBTxIds = mutableSetOf() for ((_, aTx, bTx) in sortedPairs) { // If we don't have any remaining possible transactions in the input sets, then we're done here diff --git a/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt index 9816fe5..0d43c20 100644 --- a/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt +++ b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt @@ -4,7 +4,6 @@ import kotlinx.coroutines.runBlocking import net.djvk.fireflyPlaidConnector2.api.firefly.models.ObjectLink import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionRead import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionTypeProperty -import net.djvk.fireflyPlaidConnector2.api.plaid.models.PlaidTransactionId import net.djvk.fireflyPlaidConnector2.api.plaid.models.Transaction import net.djvk.fireflyPlaidConnector2.lib.FireflyFixtures import net.djvk.fireflyPlaidConnector2.lib.PlaidFixtures @@ -37,8 +36,8 @@ internal class TransactionConverterTest { ), // plaidUpdatedTxs: List, listOf(), -// plaidDeletedTxs: List, - listOf(), +// plaidDeletedTxs: List, + listOf(), // existingFireflyTxs: List, listOf( TransactionRead( @@ -91,8 +90,8 @@ internal class TransactionConverterTest { ), // plaidUpdatedTxs: List, listOf(), -// plaidDeletedTxs: List, - listOf(), +// plaidDeletedTxs: List, + listOf(), // existingFireflyTxs: List, listOf( TransactionRead( @@ -244,7 +243,7 @@ internal class TransactionConverterTest { accountMap: Map, plaidCreatedTxs: List, plaidUpdatedTxs: List, - plaidDeletedTxs: List, + plaidDeletedTxs: List, existingFireflyTxs: List, expectedResult: TransactionConverter.ConvertPollSyncResult, ) { From ab4805e45a92419fca76ccf6dd35176bcd70fb58 Mon Sep 17 00:00:00 2001 From: Nathan Przybyszewski <172687074+nprzy@users.noreply.github.com> Date: Mon, 10 Jun 2024 11:54:16 -0600 Subject: [PATCH 05/16] Drop custom getDirection from generated Plaid Transaction --- .../api/plaid/models/Transaction.kt | 9 --------- .../transactions/TransactionConverter.kt | 10 +++++++++- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/Transaction.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/Transaction.kt index d57f91a..2055457 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/Transaction.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/Transaction.kt @@ -21,7 +21,6 @@ package net.djvk.fireflyPlaidConnector2.api.plaid.models import com.fasterxml.jackson.annotation.JsonProperty -import net.djvk.fireflyPlaidConnector2.constants.Direction import net.djvk.fireflyPlaidConnector2.transactions.FireflyAccountId import net.djvk.fireflyPlaidConnector2.transactions.PlaidAccountId import net.djvk.fireflyPlaidConnector2.transactions.SortableTransaction @@ -173,14 +172,6 @@ data class Transaction( unresolved("unresolved"); } - fun getDirection(): Direction { - return if (amount > 0) { - Direction.OUT - } else { - Direction.IN - } - } - override fun getTimestamp(zoneId: ZoneId): OffsetDateTime { return datetime ?: authorizedDatetime diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt index a6a6fe2..2442ddb 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt @@ -73,6 +73,14 @@ class TransactionConverter( else -> throw IllegalArgumentException("Can't get Plaid amount for a Firefly transaction of type ${tx.tx.type}") } } + + fun getTransactionDirection(tx: PlaidTransaction): Direction { + return if (tx.amount > 0) { + Direction.OUT + } else { + Direction.IN + } + } } fun getTxTimestamp(tx: PlaidTransaction): OffsetDateTime { @@ -470,7 +478,7 @@ class TransactionConverter( val sourceName: String? val destinationId: String? val destinationName: String? - if (tx.getDirection() == Direction.IN) { + if (getTransactionDirection(tx) == Direction.IN) { destinationId = fireflyAccountId destinationName = null From 5d9ec9d48181efc8613e89dc8f3cc1015b73886b Mon Sep 17 00:00:00 2001 From: Nathan Przybyszewski <172687074+nprzy@users.noreply.github.com> Date: Mon, 10 Jun 2024 14:01:58 -0600 Subject: [PATCH 06/16] Revert changes to Transaction PaymentChannel enum Commit a31977ff dropped the Transaction-scoped PaymentChannel enums in favor of the top-level model.PaymentChannel enum. This commit reverts that change. Although I think that those customizations make more sense than what the code generator did, reverting this change will make it easier to auto-generate the Plaid client from new versions of the OpenAPI spec. --- .../api/plaid/models/Transaction.kt | 15 +++++++++++++++ .../api/plaid/models/TransactionAllOf.kt | 16 +++++++++++++++- .../fireflyPlaidConnector2/lib/PlaidFixtures.kt | 6 +++--- 3 files changed, 33 insertions(+), 4 deletions(-) diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/Transaction.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/Transaction.kt index 2055457..c9f7d49 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/Transaction.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/Transaction.kt @@ -156,6 +156,21 @@ data class Transaction( @field:JsonProperty("personal_finance_category") val personalFinanceCategory: PersonalFinanceCategory? ) : SortableTransaction { + + /** + * The channel used to make a payment. `online:` transactions that took place online. `in store:` transactions that were made at a physical location. `other:` transactions that relate to banks, e.g. fees or deposits. This field replaces the `transaction_type` field. + * + * Values: online,inStore,other + */ + enum class PaymentChannel(val value: kotlin.String) { + @JsonProperty(value = "online") + online("online"), + @JsonProperty(value = "in store") + inStore("in store"), + @JsonProperty(value = "other") + other("other"); + } + /** * Please use the `payment_channel` field, `transaction_type` will be deprecated in the future. `digital:` transactions that took place online. `place:` transactions that were made at a physical location. `special:` transactions that relate to banks, e.g. fees or deposits. `unresolved:` transactions that do not fit into the other three types. * diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/TransactionAllOf.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/TransactionAllOf.kt index 0f0d88b..634be17 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/TransactionAllOf.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/TransactionAllOf.kt @@ -56,5 +56,19 @@ data class TransactionAllOf( @field:JsonProperty("personal_finance_category") val personalFinanceCategory: PersonalFinanceCategory? = null -) +) { + /** + * The channel used to make a payment. `online:` transactions that took place online. `in store:` transactions that were made at a physical location. `other:` transactions that relate to banks, e.g. fees or deposits. This field replaces the `transaction_type` field. + * + * Values: online,inStore,other + */ + enum class PaymentChannel(val value: kotlin.String) { + @JsonProperty(value = "online") + online("online"), + @JsonProperty(value = "in store") + inStore("in store"), + @JsonProperty(value = "other") + other("other"); + } +} diff --git a/src/test/kotlin/net/djvk/fireflyPlaidConnector2/lib/PlaidFixtures.kt b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/lib/PlaidFixtures.kt index 404b26c..d0337bd 100644 --- a/src/test/kotlin/net/djvk/fireflyPlaidConnector2/lib/PlaidFixtures.kt +++ b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/lib/PlaidFixtures.kt @@ -48,7 +48,7 @@ object PlaidFixtures { date: LocalDate = defaultLocalNow, pending: Boolean = false, transactionId: String = "ccccccccccccccccccccccccccccccccccccc", - paymentChannel: PaymentChannel = PaymentChannel.other, + paymentChannel: Transaction.PaymentChannel = Transaction.PaymentChannel.other, authorizedDate: LocalDate? = null, authorizedDatetime: java.time.OffsetDateTime? = null, datetime: java.time.OffsetDateTime? = null, @@ -122,7 +122,7 @@ object PlaidFixtures { date: LocalDate = defaultLocalNow, pending: Boolean = false, transactionId: String = Utilities.getRandomAlphabeticalString(30), - paymentChannel: PaymentChannel = PaymentChannel.other, + paymentChannel: Transaction.PaymentChannel = Transaction.PaymentChannel.other, authorizedDate: LocalDate? = null, authorizedDatetime: java.time.OffsetDateTime? = null, transactionCode: TransactionCode? = null, @@ -192,7 +192,7 @@ object PlaidFixtures { date: LocalDate = defaultLocalNow, pending: Boolean = false, transactionId: String = "ccccccccccccccccccccccccccccccccccccc", - paymentChannel: PaymentChannel, + paymentChannel: Transaction.PaymentChannel, authorizedDate: LocalDate? = null, authorizedDatetime: java.time.OffsetDateTime? = null, datetime: java.time.OffsetDateTime? = null, From b045db6de75f62c4d5eff68961df39b795af240f Mon Sep 17 00:00:00 2001 From: Nathan Przybyszewski <172687074+nprzy@users.noreply.github.com> Date: Mon, 17 Jun 2024 14:42:10 -0600 Subject: [PATCH 07/16] Strengthen Firefly/Plaid transfer matching test Add a few new Firefly transactions to the input to try and "trick" the matching logic. --- .../lib/FireflyFixtures.kt | 3 +- .../transactions/TransactionConverterTest.kt | 35 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/src/test/kotlin/net/djvk/fireflyPlaidConnector2/lib/FireflyFixtures.kt b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/lib/FireflyFixtures.kt index 80f4f80..af0f5d6 100644 --- a/src/test/kotlin/net/djvk/fireflyPlaidConnector2/lib/FireflyFixtures.kt +++ b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/lib/FireflyFixtures.kt @@ -14,6 +14,7 @@ object FireflyFixtures { groupTitle: String? = null, type: TransactionTypeProperty = TransactionTypeProperty.withdrawal, date: java.time.OffsetDateTime = defaultOffsetNow, + dateSubtractHours: Long = 0, amount: String = "1111.22", description: String = "Test Firefly Transaction", sourceId: String? = null, @@ -77,7 +78,7 @@ object FireflyFixtures { transactions = listOf( TransactionSplit( type = type, - date = date, + date = date.minusHours(dateSubtractHours), amount = amount, description = description, sourceId = sourceId, diff --git a/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt index 0d43c20..6f7072d 100644 --- a/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt +++ b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt @@ -48,12 +48,47 @@ internal class TransactionConverterTest { sourceId = "3", ), ObjectLink() ), + // This is identical to the expected matching transaction, except for the amount. + TransactionRead( + "thing", "wrongAmountFireflyTransactionId", + FireflyFixtures.getTransaction( + type = TransactionTypeProperty.withdrawal, + amount = "1234.56", + sourceId = "2", + ), ObjectLink() + ), + // This is identical to the expected matching transaction, except that it's the same account + // as the Plaid transaction. + TransactionRead( + "thing", "wrongAccountfireflyTransactionId", + FireflyFixtures.getTransaction( + type = TransactionTypeProperty.withdrawal, + amount = "1111.22", + sourceId = "1", + ), ObjectLink() + ), + // This is identical to the expected matching transaction, except that it's a deposit. + TransactionRead( + "thing", "wrongTypeFireflyDepositTransactionId", + FireflyFixtures.getTransaction( + type = TransactionTypeProperty.deposit, + amount = "1111.22", + destinationId = "2", + ), ObjectLink() + ), + // This is the transaction that we expect to match with the Plaid transaction to be converted + // into a transfer. TransactionRead( "thing", "fireflyTransactionId", FireflyFixtures.getTransaction( type = TransactionTypeProperty.withdrawal, amount = "1111.22", sourceId = "2", + // The transfer matching logic attempts to find the closest (by date) matching + // transaction. Offset this date compared to the other Firefly transactions above so + // that when this one "wins" we know it's not incidental just because this one was + // processed first. + dateSubtractHours = 12 ), ObjectLink() ), ), From 0d3036366de817f717f68d3c475c9f25044fb39a Mon Sep 17 00:00:00 2001 From: Nathan Przybyszewski <172687074+nprzy@users.noreply.github.com> Date: Sun, 23 Jun 2024 20:23:30 -0600 Subject: [PATCH 08/16] Refactor transfer matching logic This commit aims to progress us towards eliminating the SortableTransaction interface, which is only used here in this transaction matching logic. The new logic uses wrapper classes as an alternative to adding custom behavior to the generated Plaid client's Transaction model. --- .../transactions/PlaidFireflyTransaction.kt | 127 ++++++++++ .../transactions/TransactionConverter.kt | 238 +++++------------- .../transactions/TransferMatcher.kt | 144 +++++++++++ .../transactions/TransferMatcherTest.kt | 226 +++++++++++++++++ 4 files changed, 557 insertions(+), 178 deletions(-) create mode 100644 src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PlaidFireflyTransaction.kt create mode 100644 src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcher.kt create mode 100644 src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcherTest.kt diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PlaidFireflyTransaction.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PlaidFireflyTransaction.kt new file mode 100644 index 0000000..4bf7c17 --- /dev/null +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PlaidFireflyTransaction.kt @@ -0,0 +1,127 @@ +package net.djvk.fireflyPlaidConnector2.transactions + +import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionTypeProperty +import net.djvk.fireflyPlaidConnector2.api.plaid.models.Transaction +import java.time.OffsetDateTime +import java.time.ZoneId + +/** + * Represents a single non-transfer transaction, including both the Plaid and the Firefly representation of it. + */ +sealed interface PlaidFireflyTransaction { + val amount: Double + val transactionId: String + val fireflyAccountId: Int + val fireflyTransaction: FireflyTransactionDto? + val plaidTransaction: Transaction? + fun getTimestamp(zoneId: ZoneId): OffsetDateTime + + companion object { + /** + * Wraps Plaid and Firefly transactions into PlaidFireflyTransaction objects, joining them together when + * matching transactions are identified. + */ + fun match( + plaidTxs: List, + fireflyTxs: List, + accountMap: Map + ): List { + val plaidById = plaidTxs.groupBy { FireflyTransactionExternalIdIndexer.getExternalId(it.transactionId) } + val fireflyByExtId = fireflyTxs.groupBy { it.tx.externalId } + val externalIds = plaidById.keys.union(fireflyByExtId.keys) + + return externalIds.flatMap { + val matchingPlaid = plaidById[it] ?: listOf() + val matchingFirefly = fireflyByExtId[it] ?: listOf() + + // For all transactions that do not have an external ID, or if we've found more matching transactions + // than we expected to find, return them without attempting to combine. + if (it == null || matchingFirefly.size > 1 || matchingPlaid.size > 1) { + val convertedFirefly = matchingFirefly.map { FireflyTransaction(it) } + val convertedPlaid = matchingPlaid.map { + val accountId = accountMap[it.accountId] + if (accountId == null) { + throw throw IllegalArgumentException("Can not match Plaid transactions from accounts not mapped " + + "to a Firefly account id") + } + PlaidTransaction(it, accountId) + } + val converted: List = convertedPlaid + convertedFirefly + return@flatMap converted + } + + val plaidTx = matchingPlaid.getOrNull(0) + val fireflyTx = matchingFirefly.getOrNull(0) + + val converted = if (plaidTx != null && fireflyTx != null) { + listOf(MatchedTransaction(plaidTx, fireflyTx)) + } else if (plaidTx != null) { + val accountId = accountMap[plaidTx.accountId] + if (accountId == null) { + throw throw IllegalArgumentException("Can not match Plaid transactions from accounts not mapped " + + "to a Firefly account id") + } + listOf(PlaidTransaction(plaidTx, accountId)) + } else if (fireflyTx != null) { + listOf(FireflyTransaction(fireflyTx)) + } else { + throw RuntimeException("Unexpected transaction combination") + } + + converted + } + } + + fun getPlaidTransactionDate(tx: Transaction, zoneId: ZoneId): OffsetDateTime { + return tx.datetime + ?: tx.authorizedDatetime + ?: TransactionConverter.getOffsetDateTimeForDate(zoneId, tx.date) + } + + private fun getFireflyAccountId(dto: FireflyTransactionDto): Int { + return when (dto.tx.type) { + TransactionTypeProperty.deposit -> dto.tx.destinationId?.toInt() + ?: throw IllegalArgumentException("Firefly deposit ${dto.id} is missing the " + + "required destinationId field") + TransactionTypeProperty.withdrawal -> dto.tx.sourceId?.toInt() + ?: throw IllegalArgumentException("Firefly withdrawal ${dto.id} is missing the " + + "required sourceId field") + else -> throw IllegalArgumentException("Only withdrawal or deposit transaction types are supported") + } + } + } + + data class PlaidTransaction( + override val plaidTransaction: Transaction, + override val fireflyAccountId: Int, + ): PlaidFireflyTransaction { + override val amount = plaidTransaction.amount + override val transactionId = plaidTransaction.transactionId + override fun getTimestamp(zoneId: ZoneId): OffsetDateTime { + return getPlaidTransactionDate(plaidTransaction, zoneId) + } + override val fireflyTransaction = null + } + + data class FireflyTransaction(override val fireflyTransaction: FireflyTransactionDto): PlaidFireflyTransaction { + override val amount = fireflyTransaction.amount + override val fireflyAccountId get() = getFireflyAccountId(fireflyTransaction) + override val transactionId get() = fireflyTransaction.id ?: throw IllegalArgumentException("Firefly transaction does not yet have an ID") + override fun getTimestamp(zoneId: ZoneId): OffsetDateTime { + return fireflyTransaction.tx.date + } + override val plaidTransaction = null + } + + data class MatchedTransaction( + override val plaidTransaction: Transaction, + override val fireflyTransaction: FireflyTransactionDto, + ): PlaidFireflyTransaction { + override val amount = plaidTransaction.amount + override val transactionId = plaidTransaction.transactionId + override val fireflyAccountId get() = getFireflyAccountId(fireflyTransaction) + override fun getTimestamp(zoneId: ZoneId): OffsetDateTime { + return getPlaidTransactionDate(plaidTransaction, zoneId) + } + } +} diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt index 2442ddb..d5fcb4e 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt @@ -48,7 +48,7 @@ class TransactionConverter( private val logger = LoggerFactory.getLogger(this::class.java) private val timeZone = TimeZone.getTimeZone(timeZoneString) private val zoneId = timeZone.toZoneId() - private val transferMatchWindowSeconds = transferMatchWindowDays * 24 * 60 * 60 + private val transferMatcher = TransferMatcher(timeZoneString, transferMatchWindowDays) companion object { fun convertScreamingSnakeCaseToKebabCase(input: String): String { @@ -81,6 +81,14 @@ class TransactionConverter( Direction.IN } } + + private fun requirePlaidTransaction(tx: PlaidFireflyTransaction?): PlaidTransaction { + return tx?.plaidTransaction ?: throw RuntimeException("Transaction missing required Plaid information") + } + + private fun requireFireflyTransaction(tx: PlaidFireflyTransaction?): FireflyTransactionDto { + return tx?.fireflyTransaction ?: throw RuntimeException("Transaction missing required Firefly information") + } } fun getTxTimestamp(tx: PlaidTransaction): OffsetDateTime { @@ -139,15 +147,21 @@ class TransactionConverter( accountMap: Map, ): List { logger.debug("Batch sync converting Plaid transactions to Firefly transactions") - val (singles, pairs) = sortByPairsBatched(txs, accountMap) + val (singles, pairs) = transferMatcher.match(PlaidFireflyTransaction.match(txs, listOf(), accountMap)) val out = mutableListOf() for (single in singles) { - out.add(convertSingle(single, accountMap)) + out.add(convertSingle(requirePlaidTransaction(single), accountMap)) } for (pair in pairs) { - out.add(convertDoublePlaid(pair.first, pair.second, accountMap)) + out.add( + convertDoublePlaid( + requirePlaidTransaction(pair.first), + requirePlaidTransaction(pair.second), + accountMap + ) + ) } return out @@ -186,17 +200,29 @@ class TransactionConverter( * Don't pass in [plaidUpdatedTxs] here because we're not going to try to update transfers for now * because it's more complexity than I want to deal with, and I haven't seen any Plaid updates in the wild yet */ - val (singles, pairs) = sortByPairs(plaidCreatedTxs + transferCandidateExistingFireflyTxs, accountMap) - logger.debug("${::convertPollSync.name} call to ${::sortByPairs.name} received ${singles.size} singles and ${pairs.size} pairs") + val (singles, pairs) = transferMatcher.match( + PlaidFireflyTransaction.match(plaidCreatedTxs, transferCandidateExistingFireflyTxs, accountMap) + ) + logger.debug("${::convertPollSync.name} call to transferMatcher returned ${singles.size} singles and ${pairs.size} pairs") /** * Handle singles, which are transactions that didn't have any transfer pair matches */ for (single in singles) { val convertedSingle = when (single) { - is PlaidTransaction -> convertSingle(single, accountMap) - is FireflyTransactionDto -> single - else -> throw RuntimeException("Failed to convert transaction of type ${single::class} in poll sync") + is PlaidFireflyTransaction.PlaidTransaction -> convertSingle(single.plaidTransaction, accountMap) + + // In both of these cases a Firefly transaction already exists. We don't need to do anything to it. + // If we have an associated Plaid transaction, log a message. Otherwise, silently ignore it. + is PlaidFireflyTransaction.FireflyTransaction -> continue + is PlaidFireflyTransaction.MatchedTransaction -> { + logger.debug( + "Ignoring Plaid transaction id {} because it already has a corresponding Firefly transaction {}", + single.plaidTransaction.transactionId, + single.fireflyTransaction.id, + ) + continue + } } if (convertedSingle.id == null) { @@ -210,37 +236,31 @@ class TransactionConverter( * Handle pairs that we think should become Firefly transfers */ for (pair in pairs) { + val (hasFireflyTx, noFireflyTx) = pair.toList().partition { it.fireflyTransaction != null } + if (hasFireflyTx.size > 1) { + logger.debug("TransferMatcher found multiple existing Firefly transactions that appear to " + + "be a transfer. Converting multiple existing Firefly transactions to a transfer is not " + + "supported. Skipping: {}", pair) + continue + } + + val plaidComponent = requirePlaidTransaction(noFireflyTx.getOrNull(0)) + val out = when { - pair.first is FireflyTransactionDto && pair.second is FireflyTransactionDto -> - throw IllegalArgumentException("Sorted pair illegally has two Firefly transactions: $pair") - - pair.first is FireflyTransactionDto || pair.second is FireflyTransactionDto -> { - val (plaid, firefly) = if (pair.first is FireflyTransactionDto) Pair( - pair.second, - pair.first - ) else pair + hasFireflyTx.isNotEmpty() -> convertDoubleFirefly( - plaid as PlaidTransaction, - firefly as FireflyTransactionDto, + plaidComponent, + requireFireflyTransaction(hasFireflyTx.getOrNull(0)), accountMap, ) - } - pair.first is PlaidTransaction && pair.second is PlaidTransaction -> { + else -> convertDoublePlaid( - pair.first as PlaidTransaction, - pair.second as PlaidTransaction, + plaidComponent, + requirePlaidTransaction(noFireflyTx.getOrNull(1)), accountMap, ) - } - - else -> throw IllegalArgumentException("Unexpected sorted pair state: $pair") } - val plaidComponent = when { - pair.first is PlaidTransaction -> pair.first - pair.second is PlaidTransaction -> pair.second - else -> throw IllegalArgumentException("Sorted pair illegally has two Firefly transactions: $pair") - } as PlaidTransaction if (out.id != null) { updates.add(out) @@ -293,19 +313,20 @@ class TransactionConverter( val pairs: List>, ) + // This function is no longer used. It is only retained here in this commit to demonstrate that the existing + // unit test ("sortByPairs") still passes when transferMatcher subbed-in for the old code. This function to + // be removed in the following commit. suspend fun sortByPairsBatched( txs: List, accountMap: Map, ): SortByPairsBatchedResult { - // Split Plaid transactions based on whether they are transfers or not - val (transfers, nonTransfers) = txs.partition { - it.personalFinanceCategory != null && transferTypes.contains(PersonalFinanceCategoryEnum.from(it.personalFinanceCategory).primary) - } - val (singles, pairs) = sortByPairs(transfers, accountMap) + val (singles, pairs) = transferMatcher.match(PlaidFireflyTransaction.match(txs, listOf(), accountMap)) return SortByPairsBatchedResult( - singles as List + nonTransfers, - pairs as List>, + (singles as List) + .map { it.plaidTransaction }, + (pairs as List>) + .map { Pair(it.first.plaidTransaction, it.second.plaidTransaction) }, ) } @@ -327,150 +348,11 @@ class TransactionConverter( } - data class CandidatePair( - val secondsDiff: Long, - val aTx: SortableTransaction, - val bTx: SortableTransaction, - ) - - val transferTypes = - setOf( - PersonalFinanceCategoryEnum.Primary.TRANSFER_IN, - PersonalFinanceCategoryEnum.Primary.TRANSFER_OUT, - PersonalFinanceCategoryEnum.Primary.LOAN_PAYMENTS, - PersonalFinanceCategoryEnum.Primary.BANK_FEES, - ) - - data class SortByPairsResult( - val singles: List, - val pairs: List>, - ) - - /** - * Attempt to find pairs of Plaid and/or Firefly transactions that make up one actual transfer transaction - * - * Only visible for testing - */ - protected suspend fun sortByPairs( - txs: List, - accountMap: Map, - ): SortByPairsResult { - logger.trace("Starting ${::sortByPairs.name}") - val pairsOut = mutableListOf>() - val singlesOut = mutableListOf() - - val amountIndexedTxs = txs.groupBy { it.amount } - // The loop below will process an amount value and its inverse, so we use this to mark the inverse - // as processed so we don't double process amount sets - val processedAmounts = mutableSetOf() - - for ((amount, groupTxs) in amountIndexedTxs) { - logger.trace("${::sortByPairs.name} processing amount $amount with ${groupTxs.size} transactions") - if (processedAmounts.contains(amount)) { - continue - } - // Get all transfer txs that all have a currency amount inverse to groupTxs, which should theoretically be matching txs - val matchingGroupTxs = amountIndexedTxs[-amount] - processedAmounts.add(-amount) - - // If there are no matching txs, then this group has no soulmates and we should move on - if (matchingGroupTxs == null) { - singlesOut.addAll(groupTxs) - continue - } - val aTxs = groupTxs - val bTxs = matchingGroupTxs - val aTxIds = aTxs.map { it.transactionId }.toHashSet() - val bTxIds = bTxs.map { it.transactionId }.toHashSet() - val txsSecondsDiff = mutableListOf() - - // Index txs by their temporal difference from each other so we can match up the closest pairs - logger.trace("${::sortByPairs.name} indexing transactions by time diff for amount $amount") - for (aTx in aTxs) { - for (bTx in bTxs) { - txsSecondsDiff.add( - CandidatePair( - abs( - aTx.getTimestamp(zoneId).toEpochSecond() - - bTx.getTimestamp(zoneId).toEpochSecond() - ), - aTx, - bTx - ) - ) - } - } - - val sortedPairs = txsSecondsDiff - .filter { it.secondsDiff < transferMatchWindowSeconds } - .sortedBy { it.secondsDiff } - - /** - * Ids of transactions we've already used from either group so we don't use them again. - * This is an issue because the [CandidatePair] array we make matches every A transaction to every B - * transaction, so each transaction appears more than once in the array. - */ - val usedATxIds = mutableSetOf() - val usedBTxIds = mutableSetOf() - - for ((_, aTx, bTx) in sortedPairs) { - // If we don't have any remaining possible transactions in the input sets, then we're done here - if ((aTxIds.size - usedATxIds.size) < 1 || - (bTxIds.size - usedBTxIds.size) < 1 - ) { - break - } - // If we already used either transaction A or B, then move on to the next candidate - if (usedATxIds.contains(aTx.transactionId) || usedBTxIds.contains(bTx.transactionId)) { - continue - } - - // If either transaction is "on" the same account, they're not a valid transfer candidate and we should move on - if (aTx.getFireflyAccountId(accountMap) == bTx.getFireflyAccountId(accountMap)) { - continue - } - - /** - * Check if one and only one tx is Firefly - * If one and only one tx is Firefly, this is a candidate for updating the existing Firefly transaction - * to a transfer. - */ - val (fireflyTx, plaidTx) = if (aTx is FireflyTransactionDto && bTx is PlaidTransaction) { - Pair(aTx, bTx) - } else if (aTx is PlaidTransaction && bTx is FireflyTransactionDto) { - Pair(bTx, aTx) - } else if (aTx is FireflyTransactionDto && bTx is FireflyTransactionDto) { - // Transfer can't be composed of two Firefly transactions - continue - } else { - // Two Plaid transactions, which is fine - Pair(null, null) - } - - // Otherwise let's peel off the next pair of transactions - logger.trace("${::sortByPairs.name} found valid pair with timestamps ${aTx.getTimestamp(zoneId)};" + - "${bTx.getTimestamp(zoneId)} and amount $amount") - pairsOut.add(Pair(aTx, bTx)) - usedATxIds.add(aTx.transactionId) - usedBTxIds.add(bTx.transactionId) - } - // Output all leftover transactions as singles - singlesOut.addAll(aTxs.filter { !usedATxIds.contains(it.transactionId) }) - singlesOut.addAll(bTxs.filter { !usedBTxIds.contains(it.transactionId) }) - } - - /** - * Existing Firefly transactions are only used here to create transfers, so filter them out - * of the output - */ - return SortByPairsResult(singlesOut.filter { it !is FireflyTransactionDto }, pairsOut) - } - protected suspend fun convertSingle( tx: PlaidTransaction, accountMap: Map, ): FireflyTransactionDto { - logger.trace("Starting ${::sortByPairs.name}") + logger.trace("Starting ${::convertSingle.name}") val fireflyAccountId = accountMap[tx.accountId]?.toString() ?: throw RuntimeException("Failed to find Firefly account mapping for Plaid account ${tx.accountId}") diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcher.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcher.kt new file mode 100644 index 0000000..7df68da --- /dev/null +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcher.kt @@ -0,0 +1,144 @@ +package net.djvk.fireflyPlaidConnector2.transactions + +import org.slf4j.LoggerFactory +import org.springframework.beans.factory.annotation.Value +import org.springframework.stereotype.Component +import java.time.ZoneId +import kotlin.math.abs + +/** + * Identifies transaction pairs that represent a transfer from one account into another. + */ +@Component +class TransferMatcher( + @Value("\${fireflyPlaidConnector2.timeZone}") + private val timeZoneString: String, + + @Value("\${fireflyPlaidConnector2.transferMatchWindowDays}") + private val transferMatchWindowDays: Long, +) { + private val zoneId = ZoneId.of(timeZoneString) + private val transferMatchWindowSeconds = transferMatchWindowDays * 24 * 60 * 60 + private val logger = LoggerFactory.getLogger(this::class.java) + + val transferTypes = + setOf( + PersonalFinanceCategoryEnum.Primary.TRANSFER_IN, + PersonalFinanceCategoryEnum.Primary.TRANSFER_OUT, + PersonalFinanceCategoryEnum.Primary.LOAN_PAYMENTS, + PersonalFinanceCategoryEnum.Primary.BANK_FEES, + ) + + data class SortByPairsResult( + val singles: List, + val pairs: List>, + ) + + /** + * Identify matching transaction pairs that can be converted to a single "transfer" in Firefly. + * + * Note that this method will not perform any filtering. The caller is expected to filter-out any transactions + * that it would not make sense to act on, such as matching pairs of Firefly transactions that do not have + * corresponding Plaid transactions. + */ + fun match(txs: List): SortByPairsResult { + logger.trace("Starting ${::match.name}") + + // Split-out the transactions that are unlikely to be transfers based on their category. If we're not sure, + // we'll try to match it as a transfer anyway. + val (possibleTransfers, nonTransfers) = txs.partition { + val category = it.plaidTransaction?.personalFinanceCategory + category == null || transferTypes.contains(PersonalFinanceCategoryEnum.from(category).primary) + } + + val pairsOut = mutableListOf>() + val singlesOut = nonTransfers.toMutableList() + + val amountIndexedTxs = possibleTransfers.groupBy { it.amount } + // The loop below will process an amount value and its inverse, so we use this to mark the inverse + // as processed so we don't double process amount sets + val processedAmounts = mutableSetOf() + + for ((amount, groupTxs) in amountIndexedTxs) { + if (processedAmounts.contains(amount)) { + continue + } + logger.trace("${::match.name} processing amount $amount with ${groupTxs.size} transactions") + + // Get all transfer txs that all have a currency amount inverse to groupTxs, which should theoretically be matching txs + val matchingGroupTxs = amountIndexedTxs[-amount] + processedAmounts.add(-amount) + + // If there are no matching txs, then this group has no soulmates and we should move on + if (matchingGroupTxs == null) { + singlesOut.addAll(groupTxs) + continue + } + val aTxs = groupTxs + val bTxs = matchingGroupTxs + val txsSecondsDiff = mutableListOf() + + // Index txs by their temporal difference from each other so we can match up the closest pairs + logger.trace("${::match.name} indexing transactions by time diff for amount $amount") + for (aTx in aTxs) { + for (bTx in bTxs) { + txsSecondsDiff.add( + CandidatePair( + abs( + aTx.getTimestamp(zoneId).toEpochSecond() - + bTx.getTimestamp(zoneId).toEpochSecond() + ), + aTx, + bTx + ) + ) + } + } + + val sortedPairs = txsSecondsDiff + .filter { it.secondsDiff < transferMatchWindowSeconds } + .sortedBy { it.secondsDiff } + + /** + * Ids of transactions we've already used from either group so we don't use them again. + * This is an issue because the [CandidatePair] array we make matches every A transaction to every B + * transaction, so each transaction appears more than once in the array. + */ + val usedATxIds = mutableSetOf() + val usedBTxIds = mutableSetOf() + + sortedPairs.filter { (_, aTx, bTx) -> + // Skip any pairs where we've already successfully matched one of the transactions + val match = !usedATxIds.contains(aTx.transactionId) + && !usedBTxIds.contains(bTx.transactionId) + + // The transfers we're looking for are from one account to another; skip any pairs where both + // transactions are for the same account. + && aTx.fireflyAccountId != bTx.fireflyAccountId + + if (match) { + usedATxIds.add(aTx.transactionId) + usedBTxIds.add(bTx.transactionId) + } + + match + }.map { (_, aTx, bTx) -> + logger.trace("${::match.name} found valid pair with timestamps ${aTx.getTimestamp(zoneId)};" + + "${bTx.getTimestamp(zoneId)} and amount $amount") + pairsOut.add(Pair(aTx, bTx)) + } + + // Output all leftover transactions as singles + singlesOut.addAll(aTxs.filter { !usedATxIds.contains(it.transactionId) }) + singlesOut.addAll(bTxs.filter { !usedBTxIds.contains(it.transactionId) }) + } + + return SortByPairsResult(singlesOut, pairsOut) + } + + private data class CandidatePair( + val secondsDiff: Long, + val aTx: PlaidFireflyTransaction, + val bTx: PlaidFireflyTransaction, + ) +} diff --git a/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcherTest.kt b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcherTest.kt new file mode 100644 index 0000000..12ee150 --- /dev/null +++ b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcherTest.kt @@ -0,0 +1,226 @@ +package net.djvk.fireflyPlaidConnector2.transactions + +import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionTypeProperty +import net.djvk.fireflyPlaidConnector2.api.plaid.models.Transaction +import net.djvk.fireflyPlaidConnector2.lib.FireflyFixtures +import net.djvk.fireflyPlaidConnector2.lib.PlaidFixtures +import org.junit.jupiter.api.Assertions.assertIterableEquals +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.Arguments +import org.junit.jupiter.params.provider.MethodSource +import java.time.OffsetDateTime +import java.time.ZoneOffset + +internal class TransferMatcherTest { + companion object { + val baseDateTime = OffsetDateTime.of(2022, 10, 1, 0, 0, 0, 0, ZoneOffset.ofHours(4)) + var matchWindowDays = 3L + var transactionIndex = 0 + + fun getFireflyDto( + type: TransactionTypeProperty, + amount: String, + date: OffsetDateTime = baseDateTime, + sourceId: String? = null, + destinationId: String? = null, + description: String = "TestTx", + ): FireflyTransactionDto { + val tx = FireflyFixtures.getTransaction( + type = type, + amount = amount, + date = date, + sourceId = sourceId, + destinationId = destinationId, + description = description, + ) + val txSplit = tx.transactions.getOrNull(0) ?: throw RuntimeException("Missing transaction") + return FireflyTransactionDto("tx-${transactionIndex++}", txSplit) + } + + private fun sortListOfPairs(input: List>): List> { + return input.map { pair -> + pair.toList().sortedBy { tx -> tx.transactionId } + }.sortedBy { it.first().transactionId } + } + + @JvmStatic + fun provideTransferPairTestCases(): List { + val plaidAcctA = "aaa" + val fireflyAcctA = 1 + val plaidAcctB = "bbb" + val fireflyAcctB = 2 + + val pairPlaid = Pair( + PlaidFireflyTransaction.PlaidTransaction( + PlaidFixtures.getTransferTestTransaction( + name = "Plaid Transfer Source", + datetime = baseDateTime.minusHours(48), + personalFinanceCategory = PersonalFinanceCategoryEnum.BANK_FEES_ATM_FEES, + accountId = plaidAcctA, + amount = 100.0, + ), + fireflyAcctA, + ), + PlaidFireflyTransaction.PlaidTransaction( + PlaidFixtures.getTransferTestTransaction( + name = "Plaid Transfer Dest", + datetime = baseDateTime, + personalFinanceCategory = PersonalFinanceCategoryEnum.BANK_FEES_ATM_FEES, + accountId = plaidAcctB, + amount = -100.0, + ), + fireflyAcctB, + ), + ) + + val pairMixedA = Pair( + PlaidFireflyTransaction.FireflyTransaction( + getFireflyDto( + description = "Firefly Transfer Source", + date = baseDateTime.minusHours(48), + type = TransactionTypeProperty.withdrawal, + sourceId = fireflyAcctA.toString(), + amount = "100.00", + ), + ), + PlaidFireflyTransaction.PlaidTransaction( + PlaidFixtures.getTransferTestTransaction( + name = "Plaid Transfer Destination", + datetime = baseDateTime, + personalFinanceCategory = PersonalFinanceCategoryEnum.BANK_FEES_ATM_FEES, + accountId = plaidAcctB, + amount = -100.0, + ), + fireflyAcctB, + ), + ) + + val pairMixedB = Pair( + PlaidFireflyTransaction.PlaidTransaction( + PlaidFixtures.getTransferTestTransaction( + name = "Plaid Transfer Source", + datetime = baseDateTime.minusHours(48), + personalFinanceCategory = PersonalFinanceCategoryEnum.BANK_FEES_ATM_FEES, + accountId = plaidAcctA, + amount = 100.0, + ), + fireflyAcctA, + ), + PlaidFireflyTransaction.FireflyTransaction( + getFireflyDto( + description = "Firefly Transfer Destination", + date = baseDateTime, + type = TransactionTypeProperty.deposit, + destinationId = fireflyAcctB.toString(), + amount = "100.00", + ), + ), + ) + + val singles = listOf( + PlaidFireflyTransaction.PlaidTransaction( + PlaidFixtures.getTransferTestTransaction( + name = "Category makes this not a transfer", + datetime = baseDateTime, + personalFinanceCategory = PersonalFinanceCategoryEnum.INCOME_WAGES, + accountId = plaidAcctA, + amount = 100.0, + ), + fireflyAcctB, + ), + PlaidFireflyTransaction.PlaidTransaction( + PlaidFixtures.getTransferTestTransaction( + name = "Date is not as close as the real matching transaction", + datetime = baseDateTime.minusHours(49), + personalFinanceCategory = PersonalFinanceCategoryEnum.BANK_FEES_ATM_FEES, + accountId = plaidAcctA, + amount = 100.0, + ), + fireflyAcctB, + ), + // Same account as matching transaction + PlaidFireflyTransaction.PlaidTransaction( + PlaidFixtures.getTransferTestTransaction( + name = "Can't match because the account ID is same as the destination", + datetime = baseDateTime, + personalFinanceCategory = PersonalFinanceCategoryEnum.BANK_FEES_ATM_FEES, + accountId = plaidAcctB, + amount = 100.0, + ), + fireflyAcctB, + ), + // Amount does not match + PlaidFireflyTransaction.PlaidTransaction( + PlaidFixtures.getTransferTestTransaction( + name = "Amount does not match", + datetime = baseDateTime, + personalFinanceCategory = PersonalFinanceCategoryEnum.BANK_FEES_ATM_FEES, + accountId = plaidAcctA, + amount = 101.0, + ), + fireflyAcctB, + ), + // These two transactions would match each other if their dates were a little closer together. + PlaidFireflyTransaction.PlaidTransaction( + PlaidFixtures.getTransferTestTransaction( + name = "Expected single because date is too far away from pair (1)", + datetime = baseDateTime, + personalFinanceCategory = PersonalFinanceCategoryEnum.BANK_FEES_ATM_FEES, + accountId = plaidAcctA, + amount = -200.0, + ), + fireflyAcctA, + ), + PlaidFireflyTransaction.PlaidTransaction( + PlaidFixtures.getTransferTestTransaction( + name = "Expected single because date is too far away from pair (2)", + datetime = baseDateTime.minusDays(matchWindowDays).minusMinutes(1), + personalFinanceCategory = PersonalFinanceCategoryEnum.BANK_FEES_ATM_FEES, + accountId = plaidAcctB, + amount = 200.0, + ), + fireflyAcctB, + ), + ) + + return listOf( + Arguments.of( + "Matching Plaid Source/Dest", + (singles + sequenceOf(pairPlaid.first, pairPlaid.second)).shuffled(), + singles, + listOf(pairPlaid), + ), + Arguments.of( + "Matching Plaid Source and Firefly Destination", + (singles + sequenceOf(pairMixedA.first, pairMixedA.second)).shuffled(), + singles, + listOf(pairMixedA), + ), + Arguments.of( + "Matching Firefly Source and Plaid Destination", + (singles + sequenceOf(pairMixedB.first, pairMixedB.second)).shuffled(), + singles, + listOf(pairMixedB), + ), + ) + } + } + + @ParameterizedTest(name = "{index} => {0}") + @MethodSource("provideTransferPairTestCases") + fun match( + testName: String, + input: List, + expectedSingles: List, + expectedPairs: List>, + ) { + val matcher = TransferMatcher( + timeZoneString = "America/New_York", + transferMatchWindowDays = matchWindowDays, + ) + + val (actualSingles, actualPairs) = matcher.match(input) + assertIterableEquals(sortListOfPairs(expectedPairs), sortListOfPairs(actualPairs)) + assertIterableEquals(expectedSingles.sortedBy { it.transactionId }, actualSingles.sortedBy { it.transactionId }) + } +} \ No newline at end of file From 3e75c5adbf0f2652b1149e2b58d88c38c15818e5 Mon Sep 17 00:00:00 2001 From: Nathan Przybyszewski <172687074+nprzy@users.noreply.github.com> Date: Sun, 23 Jun 2024 20:37:41 -0600 Subject: [PATCH 09/16] Remove SortableTransaction and related orphaned code --- .../api/firefly/models/TransactionRead.kt | 28 ---- .../api/plaid/models/Transaction.kt | 22 +--- .../transactions/FireflyTransactionDto.kt | 23 +--- .../transactions/SortableTransaction.kt | 21 --- .../transactions/TransactionConverter.kt | 22 ---- .../transactions/TransactionConverterTest.kt | 122 ------------------ 6 files changed, 6 insertions(+), 232 deletions(-) delete mode 100644 src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/SortableTransaction.kt diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/firefly/models/TransactionRead.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/firefly/models/TransactionRead.kt index 8ed5a43..a7baecb 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/firefly/models/TransactionRead.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/firefly/models/TransactionRead.kt @@ -21,7 +21,6 @@ package net.djvk.fireflyPlaidConnector2.api.firefly.models import com.fasterxml.jackson.annotation.JsonProperty -import net.djvk.fireflyPlaidConnector2.transactions.SortableTransaction import java.time.OffsetDateTime import java.time.ZoneId @@ -50,30 +49,3 @@ data class TransactionRead( val links: ObjectLink ) - -// : SortableTransaction { -// override val transactionId: String -// get() = id -// -// override val amount: Double -// get() { -// if (attributes.transactions.size != 1) { -// throw IllegalArgumentException( -// "Cannot resolve transaction id for Transaction with " + -// " ${attributes.transactions.size} splits" -// ) -// } -// return attributes.transactions.first().amount.toDouble() -// } -// -// override fun getTimestamp(zoneId: ZoneId): OffsetDateTime { -// if (attributes.transactions.size != 1) { -// throw IllegalArgumentException( -// "Cannot resolve transaction id for Transaction with " + -// " ${attributes.transactions.size} splits" -// ) -// } -// return attributes.transactions.first().date -// } -//} - diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/Transaction.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/Transaction.kt index c9f7d49..bc9c4e5 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/Transaction.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/Transaction.kt @@ -21,10 +21,6 @@ package net.djvk.fireflyPlaidConnector2.api.plaid.models import com.fasterxml.jackson.annotation.JsonProperty -import net.djvk.fireflyPlaidConnector2.transactions.FireflyAccountId -import net.djvk.fireflyPlaidConnector2.transactions.PlaidAccountId -import net.djvk.fireflyPlaidConnector2.transactions.SortableTransaction -import net.djvk.fireflyPlaidConnector2.transactions.TransactionConverter import java.time.OffsetDateTime import java.time.ZoneId @@ -91,7 +87,7 @@ data class Transaction( /* The settled value of the transaction, denominated in the transactions's currency, as stated in `iso_currency_code` or `unofficial_currency_code`. Positive values when money moves out of the account; negative values when money moves in. For example, debit card purchases are positive; credit card payments, direct deposits, and refunds are negative. */ @field:JsonProperty("amount") - override val amount: kotlin.Double, + val amount: kotlin.Double, /* The ISO-4217 currency code of the transaction. Always `null` if `unofficial_currency_code` is non-null. */ @field:JsonProperty("iso_currency_code") @@ -111,7 +107,7 @@ data class Transaction( /* The unique ID of the transaction. Like all Plaid identifiers, the `transaction_id` is case sensitive. */ @field:JsonProperty("transaction_id") - override val transactionId: kotlin.String, + val transactionId: kotlin.String, /* The channel used to make a payment. `online:` transactions that took place online. `in store:` transactions that were made at a physical location. `other:` transactions that relate to banks, e.g. fees or deposits. This field replaces the `transaction_type` field. */ @field:JsonProperty("payment_channel") @@ -155,7 +151,7 @@ data class Transaction( */ @field:JsonProperty("personal_finance_category") val personalFinanceCategory: PersonalFinanceCategory? -) : SortableTransaction { +) { /** * The channel used to make a payment. `online:` transactions that took place online. `in store:` transactions that were made at a physical location. `other:` transactions that relate to banks, e.g. fees or deposits. This field replaces the `transaction_type` field. @@ -186,17 +182,5 @@ data class Transaction( @JsonProperty(value = "unresolved") unresolved("unresolved"); } - - override fun getTimestamp(zoneId: ZoneId): OffsetDateTime { - return datetime - ?: authorizedDatetime - ?: TransactionConverter.getOffsetDateTimeForDate(zoneId, date) - } - - override fun getFireflyAccountId(accountMap: Map): FireflyAccountId { - return accountMap[accountId] - ?: throw IllegalArgumentException("SortableTransaction.getFireflyAccountId can't be called on a Plaid " + - "transaction with an account id that isn't mapped to a Firefly account id") - } } diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/FireflyTransactionDto.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/FireflyTransactionDto.kt index 2ac0867..0642f3a 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/FireflyTransactionDto.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/FireflyTransactionDto.kt @@ -18,30 +18,13 @@ data class FireflyTransactionDto( */ val id: FireflyTransactionId?, val tx: TransactionSplit, -) : SortableTransaction { - override val transactionId: String +) { + val transactionId: String get() = id ?: throw RuntimeException("Can't use a Firefly transaction without an id for sorting") - override val amount: Double + val amount: Double get() = TransactionConverter.getPlaidAmount(this) - override fun getTimestamp(zoneId: ZoneId): OffsetDateTime { - return tx.date - } - - override fun getFireflyAccountId(accountMap: Map): FireflyAccountId { - return when (tx.type) { - TransactionTypeProperty.deposit -> tx.destinationId?.toInt() - ?: throw IllegalArgumentException("SortableTransaction.getFireflyAccountId can't be called on a deposit " + - "with a null destination id") - TransactionTypeProperty.withdrawal -> tx.sourceId?.toInt() - ?: throw IllegalArgumentException("SortableTransaction.getFireflyAccountId can't be called on a withdrawal " + - "with a null source id") - else -> throw IllegalArgumentException("SortableTransaction.getFireflyAccountId isn't valid to call on " + - "FireflyTransactionDtos that are not withdrawals or deposits") - } - } - fun toTransactionStore(): TransactionStore { return TransactionStore( listOf(tx), diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/SortableTransaction.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/SortableTransaction.kt deleted file mode 100644 index 176d9a3..0000000 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/SortableTransaction.kt +++ /dev/null @@ -1,21 +0,0 @@ -package net.djvk.fireflyPlaidConnector2.transactions - -import java.time.OffsetDateTime -import java.time.ZoneId - -interface SortableTransaction { - val transactionId: String - - /** - * This should be the Plaid amount, as returned by [TransactionConverter.getPlaidAmount] for Firefly - * transactions. - */ - val amount: Double - fun getTimestamp(zoneId: ZoneId): OffsetDateTime - - /** - * Returns the Firefly account id that this transaction is "on," which is the Firefly source id for Firefly withdrawals, - * Firefly destination id for Firefly deposits, and the account id for Plaid transactions. - */ - fun getFireflyAccountId(accountMap: Map): FireflyAccountId -} \ No newline at end of file diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt index d5fcb4e..945b976 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt @@ -308,28 +308,6 @@ class TransactionConverter( ) } - data class SortByPairsBatchedResult( - val singles: List, - val pairs: List>, - ) - - // This function is no longer used. It is only retained here in this commit to demonstrate that the existing - // unit test ("sortByPairs") still passes when transferMatcher subbed-in for the old code. This function to - // be removed in the following commit. - suspend fun sortByPairsBatched( - txs: List, - accountMap: Map, - ): SortByPairsBatchedResult { - val (singles, pairs) = transferMatcher.match(PlaidFireflyTransaction.match(txs, listOf(), accountMap)) - - return SortByPairsBatchedResult( - (singles as List) - .map { it.plaidTransaction }, - (pairs as List>) - .map { Pair(it.first.plaidTransaction, it.second.plaidTransaction) }, - ) - } - fun filterFireflyCandidateTransferTxs( input: List, ): List { diff --git a/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt index 6f7072d..f8d3610 100644 --- a/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt +++ b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt @@ -4,7 +4,6 @@ import kotlinx.coroutines.runBlocking import net.djvk.fireflyPlaidConnector2.api.firefly.models.ObjectLink import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionRead import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionTypeProperty -import net.djvk.fireflyPlaidConnector2.api.plaid.models.Transaction import net.djvk.fireflyPlaidConnector2.lib.FireflyFixtures import net.djvk.fireflyPlaidConnector2.lib.PlaidFixtures import org.junit.jupiter.api.Assertions.assertEquals @@ -12,8 +11,6 @@ import org.junit.jupiter.api.Test import org.junit.jupiter.params.ParameterizedTest import org.junit.jupiter.params.provider.Arguments import org.junit.jupiter.params.provider.MethodSource -import java.time.OffsetDateTime -import java.time.ZoneOffset import net.djvk.fireflyPlaidConnector2.api.plaid.models.Transaction as PlaidTransaction internal class TransactionConverterTest { @@ -182,93 +179,6 @@ internal class TransactionConverterTest { ) ) } - - @JvmStatic - fun provideSortByPairs(): List { - val baseDateTime = OffsetDateTime.of(2022, 10, 1, 0, 0, 0, 0, ZoneOffset.ofHours(4)) - // Single because not a transfer - val z = PlaidFixtures.getTransferTestTransaction( - datetime = baseDateTime.minusHours(18), - personalFinanceCategory = PersonalFinanceCategoryEnum.BANK_FEES_ATM_FEES, - accountId = "zzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz", - amount = -100.0, - ) - // Single because not a transfer, even though it has a matching amount - val y = PlaidFixtures.getTransferTestTransaction( - datetime = baseDateTime.minusHours(19), - personalFinanceCategory = PersonalFinanceCategoryEnum.INCOME_WAGES, - accountId = "yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy", - amount = -100.0, - ) - // Single because no matching amount - val x = PlaidFixtures.getTransferTestTransaction( - datetime = baseDateTime.minusHours(19), - personalFinanceCategory = PersonalFinanceCategoryEnum.TRANSFER_IN_ACCOUNT_TRANSFER, - accountId = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", - amount = 19.0, - ) - // Single because while it's a transfer and has a matching amount, it's on the same account as - // the matching transaction - val bSingle = PlaidFixtures.getTransferTestTransaction( - datetime = baseDateTime.minusHours(5), - personalFinanceCategory = PersonalFinanceCategoryEnum.TRANSFER_IN_ACCOUNT_TRANSFER, - accountId = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", - amount = -100.0, - ) - // Single because while it's a transfer and has a matching amount, its timestamp is farther away than - // all the other candidates - val w = PlaidFixtures.getTransferTestTransaction( - datetime = baseDateTime.minusHours(20), - personalFinanceCategory = PersonalFinanceCategoryEnum.TRANSFER_IN_ACCOUNT_TRANSFER, - accountId = "wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww", - amount = -100.0, - ) - - val singles = listOf(z, y, x, bSingle, w) - - val a = PlaidFixtures.getTransferTestTransaction( - datetime = baseDateTime.minusHours(4), - personalFinanceCategory = PersonalFinanceCategoryEnum.TRANSFER_IN_ACCOUNT_TRANSFER, - accountId = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", - amount = -100.0, - ) - val b = PlaidFixtures.getTransferTestTransaction( - datetime = baseDateTime.minusHours(5), - personalFinanceCategory = PersonalFinanceCategoryEnum.TRANSFER_OUT_ACCOUNT_TRANSFER, - accountId = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", - amount = 100.0, - ) - val c = PlaidFixtures.getTransferTestTransaction( - datetime = baseDateTime.minusHours(6), - personalFinanceCategory = PersonalFinanceCategoryEnum.TRANSFER_IN_DEPOSIT, - accountId = "ccccccccccccccccccccccccccccccccccccc", - amount = -200.0, - ) - val d = PlaidFixtures.getTransferTestTransaction( - datetime = baseDateTime.minusHours(7), - personalFinanceCategory = PersonalFinanceCategoryEnum.TRANSFER_OUT_WITHDRAWAL, - accountId = "ddddddddddddddddddddddddddddddddddddd", - amount = 200.0, - ) - val pairs = listOf( - Pair(a, b), - Pair(c, d), - ) - return listOf( - Arguments.of( -// testName: String, - "Base case", -// input: List, - singles + pairs.flatMap { sequenceOf(it.first, it.second) }.shuffled(), -// accountMap: Map, - PlaidFixtures.getStandardAccountMapping(), -// expectedSingles: List, - singles, -// expectedPairs: List>, - pairs, - ), - ) - } } @ParameterizedTest(name = "{index} => {0}") @@ -304,38 +214,6 @@ internal class TransactionConverterTest { } } - @ParameterizedTest(name = "{index} => {0}") - @MethodSource("provideSortByPairs") - fun sortByPairs( - testName: String, - input: List, - accountMap: Map, - expectedSingles: List, - expectedPairs: List>, - ) { - runBlocking { - val converter = TransactionConverter( - false, - enablePrimaryCategorization = false, - primaryCategoryPrefix = "a", - enableDetailedCategorization = false, - detailedCategoryPrefix = "b", - timeZoneString = "America/New_York", - transferMatchWindowDays = 10L, - ) - val (actualSingles, actualPairs) = converter.sortByPairsBatched(input, accountMap) - - assertEquals(expectedSingles.sortedBy { it.transactionId }, actualSingles.sortedBy { it.transactionId }) - assertEquals(sortListOfPairs(expectedPairs), sortListOfPairs(actualPairs)) - } - } - - private fun sortListOfPairs(input: List>): List> { - return input.map { pair -> - pair.toList().sortedBy { tx -> tx.transactionId } - }.sortedBy { it.first().transactionId } - } - @Test fun getSourceKey() { } From 56ad8570a34c629aa0089ede7387604dfaaae3bc Mon Sep 17 00:00:00 2001 From: Nathan Przybyszewski <172687074+nprzy@users.noreply.github.com> Date: Sun, 30 Jun 2024 16:11:56 -0600 Subject: [PATCH 10/16] Re-introduce PlaidTransactionId typealias --- .../api/plaid/PlaidApiWrapper.kt | 2 ++ .../fireflyPlaidConnector2/sync/PolledSyncRunner.kt | 3 ++- .../FireflyTransactionExternalIdIndexer.kt | 6 +++--- .../transactions/TransactionConverter.kt | 3 ++- .../transactions/TransactionConverterTest.kt | 11 ++++++----- 5 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/PlaidApiWrapper.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/PlaidApiWrapper.kt index c3dabf1..880e28f 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/PlaidApiWrapper.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/PlaidApiWrapper.kt @@ -11,6 +11,8 @@ import org.springframework.beans.factory.annotation.Value import org.springframework.stereotype.Component import kotlin.time.Duration.Companion.minutes +typealias PlaidTransactionId = String + const val clientIdHeader = "PLAID-CLIENT-ID" const val secretHeader = "PLAID-SECRET" diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/sync/PolledSyncRunner.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/sync/PolledSyncRunner.kt index 8b4c92a..d496640 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/sync/PolledSyncRunner.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/sync/PolledSyncRunner.kt @@ -6,6 +6,7 @@ import net.djvk.fireflyPlaidConnector2.api.firefly.apis.TransactionsApi import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionRead import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionTypeFilter import net.djvk.fireflyPlaidConnector2.api.plaid.PlaidApiWrapper +import net.djvk.fireflyPlaidConnector2.api.plaid.PlaidTransactionId import net.djvk.fireflyPlaidConnector2.api.plaid.models.* import net.djvk.fireflyPlaidConnector2.transactions.TransactionConverter import org.slf4j.LoggerFactory @@ -136,7 +137,7 @@ class PolledSyncRunner( val plaidCreatedTxs = mutableListOf() val plaidUpdatedTxs = mutableListOf() - val plaidDeletedTxs = mutableListOf() + val plaidDeletedTxs = mutableListOf() accessTokenLoop@ for ((accessToken, accountIds) in accountAccessTokenSequence) { logger.debug( diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/FireflyTransactionExternalIdIndexer.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/FireflyTransactionExternalIdIndexer.kt index 88cef63..55a5599 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/FireflyTransactionExternalIdIndexer.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/FireflyTransactionExternalIdIndexer.kt @@ -1,8 +1,8 @@ package net.djvk.fireflyPlaidConnector2.transactions import net.djvk.fireflyPlaidConnector2.api.firefly.apis.FireflyExternalId -import net.djvk.fireflyPlaidConnector2.api.firefly.apis.FireflyTransactionId import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionRead +import net.djvk.fireflyPlaidConnector2.api.plaid.PlaidTransactionId class FireflyTransactionExternalIdIndexer( existingFireflyTxs: List, @@ -23,13 +23,13 @@ class FireflyTransactionExternalIdIndexer( } fun findExistingFireflyTx( - plaidTransactionId: String, + plaidTransactionId: PlaidTransactionId, ): TransactionRead? { return fireflyTxsByExternalId[getExternalId(plaidTransactionId)] } companion object { - fun getExternalId(txId: String): String { + fun getExternalId(txId: String): PlaidTransactionId { return "plaid-${txId}" } } diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt index 945b976..36c705b 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt @@ -4,6 +4,7 @@ import net.djvk.fireflyPlaidConnector2.api.firefly.apis.FireflyTransactionId import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionRead import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionSplit import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionTypeProperty +import net.djvk.fireflyPlaidConnector2.api.plaid.PlaidTransactionId import net.djvk.fireflyPlaidConnector2.constants.Direction import net.djvk.fireflyPlaidConnector2.transactions.PersonalFinanceCategoryEnum.Primary.* import org.slf4j.LoggerFactory @@ -184,7 +185,7 @@ class TransactionConverter( accountMap: Map, plaidCreatedTxs: List, plaidUpdatedTxs: List, - plaidDeletedTxs: List, + plaidDeletedTxs: List, existingFireflyTxs: List, ): ConvertPollSyncResult { logger.trace("Starting ${::convertPollSync.name}") diff --git a/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt index f8d3610..e083cc5 100644 --- a/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt +++ b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt @@ -4,6 +4,7 @@ import kotlinx.coroutines.runBlocking import net.djvk.fireflyPlaidConnector2.api.firefly.models.ObjectLink import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionRead import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionTypeProperty +import net.djvk.fireflyPlaidConnector2.api.plaid.PlaidTransactionId import net.djvk.fireflyPlaidConnector2.lib.FireflyFixtures import net.djvk.fireflyPlaidConnector2.lib.PlaidFixtures import org.junit.jupiter.api.Assertions.assertEquals @@ -33,8 +34,8 @@ internal class TransactionConverterTest { ), // plaidUpdatedTxs: List, listOf(), -// plaidDeletedTxs: List, - listOf(), +// plaidDeletedTxs: List, + listOf(), // existingFireflyTxs: List, listOf( TransactionRead( @@ -122,8 +123,8 @@ internal class TransactionConverterTest { ), // plaidUpdatedTxs: List, listOf(), -// plaidDeletedTxs: List, - listOf(), +// plaidDeletedTxs: List, + listOf(), // existingFireflyTxs: List, listOf( TransactionRead( @@ -188,7 +189,7 @@ internal class TransactionConverterTest { accountMap: Map, plaidCreatedTxs: List, plaidUpdatedTxs: List, - plaidDeletedTxs: List, + plaidDeletedTxs: List, existingFireflyTxs: List, expectedResult: TransactionConverter.ConvertPollSyncResult, ) { From a1fc26cbe9641b94e500301e37b4d24adba14d54 Mon Sep 17 00:00:00 2001 From: Nathan Przybyszewski <172687074+nprzy@users.noreply.github.com> Date: Sun, 30 Jun 2024 18:42:45 -0600 Subject: [PATCH 11/16] Clarify usage of PlaidFireflyTransaction and subtypes --- .../transactions/PlaidFireflyTransaction.kt | 21 ++++++++++++++++--- .../transactions/TransactionConverter.kt | 4 ++-- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PlaidFireflyTransaction.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PlaidFireflyTransaction.kt index 4bf7c17..0f2f42e 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PlaidFireflyTransaction.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PlaidFireflyTransaction.kt @@ -7,6 +7,8 @@ import java.time.ZoneId /** * Represents a single non-transfer transaction, including both the Plaid and the Firefly representation of it. + * + * See the descriptions of the individual subtypes for more details about the situations in which each is used. */ sealed interface PlaidFireflyTransaction { val amount: Double @@ -18,10 +20,11 @@ sealed interface PlaidFireflyTransaction { companion object { /** - * Wraps Plaid and Firefly transactions into PlaidFireflyTransaction objects, joining them together when - * matching transactions are identified. + * Normalizes Plaid and Firefly transactions into PlaidFireflyTransaction objects, joining them together as a + * MatchedTransaction when a Plaid transaction has a transactionId that matches a Firefly transaction's + * externalId. */ - fun match( + fun normalizeByTransactionId( plaidTxs: List, fireflyTxs: List, accountMap: Map @@ -91,6 +94,10 @@ sealed interface PlaidFireflyTransaction { } } + /** + * This subtype is used when we have a transaction from Plaid that either hasn't been created yet in Firefly, or + * has been created but we haven't found/loaded it. + */ data class PlaidTransaction( override val plaidTransaction: Transaction, override val fireflyAccountId: Int, @@ -103,6 +110,10 @@ sealed interface PlaidFireflyTransaction { override val fireflyTransaction = null } + /** + * This subtype is used when we've loaded a transaction from Firefly that we haven't seen in the current set of + * transactions from Plaid. + */ data class FireflyTransaction(override val fireflyTransaction: FireflyTransactionDto): PlaidFireflyTransaction { override val amount = fireflyTransaction.amount override val fireflyAccountId get() = getFireflyAccountId(fireflyTransaction) @@ -113,6 +124,10 @@ sealed interface PlaidFireflyTransaction { override val plaidTransaction = null } + /** + * This subtype is used when we've received a transaction from Plaid that already has a corresponding transaction + * in Firefly. Note that this is NOT related to + */ data class MatchedTransaction( override val plaidTransaction: Transaction, override val fireflyTransaction: FireflyTransactionDto, diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt index 36c705b..4b5240e 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt @@ -148,7 +148,7 @@ class TransactionConverter( accountMap: Map, ): List { logger.debug("Batch sync converting Plaid transactions to Firefly transactions") - val (singles, pairs) = transferMatcher.match(PlaidFireflyTransaction.match(txs, listOf(), accountMap)) + val (singles, pairs) = transferMatcher.match(PlaidFireflyTransaction.normalizeByTransactionId(txs, listOf(), accountMap)) val out = mutableListOf() for (single in singles) { @@ -202,7 +202,7 @@ class TransactionConverter( * because it's more complexity than I want to deal with, and I haven't seen any Plaid updates in the wild yet */ val (singles, pairs) = transferMatcher.match( - PlaidFireflyTransaction.match(plaidCreatedTxs, transferCandidateExistingFireflyTxs, accountMap) + PlaidFireflyTransaction.normalizeByTransactionId(plaidCreatedTxs, transferCandidateExistingFireflyTxs, accountMap) ) logger.debug("${::convertPollSync.name} call to transferMatcher returned ${singles.size} singles and ${pairs.size} pairs") From bcb576c2e70402d7fbcb77bd3ee822b51869d7f3 Mon Sep 17 00:00:00 2001 From: Nathan Przybyszewski <172687074+nprzy@users.noreply.github.com> Date: Sun, 30 Jun 2024 18:45:47 -0600 Subject: [PATCH 12/16] Remove unnecessary variable declarations --- .../transactions/PlaidFireflyTransaction.kt | 3 +-- .../transactions/TransferMatcher.kt | 10 ++++------ 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PlaidFireflyTransaction.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PlaidFireflyTransaction.kt index 0f2f42e..9b53842 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PlaidFireflyTransaction.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PlaidFireflyTransaction.kt @@ -49,8 +49,7 @@ sealed interface PlaidFireflyTransaction { } PlaidTransaction(it, accountId) } - val converted: List = convertedPlaid + convertedFirefly - return@flatMap converted + return@flatMap convertedPlaid + convertedFirefly } val plaidTx = matchingPlaid.getOrNull(0) diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcher.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcher.kt index 7df68da..fcc5e7c 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcher.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcher.kt @@ -74,14 +74,12 @@ class TransferMatcher( singlesOut.addAll(groupTxs) continue } - val aTxs = groupTxs - val bTxs = matchingGroupTxs val txsSecondsDiff = mutableListOf() // Index txs by their temporal difference from each other so we can match up the closest pairs logger.trace("${::match.name} indexing transactions by time diff for amount $amount") - for (aTx in aTxs) { - for (bTx in bTxs) { + for (aTx in groupTxs) { + for (bTx in matchingGroupTxs) { txsSecondsDiff.add( CandidatePair( abs( @@ -129,8 +127,8 @@ class TransferMatcher( } // Output all leftover transactions as singles - singlesOut.addAll(aTxs.filter { !usedATxIds.contains(it.transactionId) }) - singlesOut.addAll(bTxs.filter { !usedBTxIds.contains(it.transactionId) }) + singlesOut.addAll(groupTxs.filter { !usedATxIds.contains(it.transactionId) }) + singlesOut.addAll(matchingGroupTxs.filter { !usedBTxIds.contains(it.transactionId) }) } return SortByPairsResult(singlesOut, pairsOut) From 9819fa9cb1429d2bc7a66d0a240fe82aae8b4bbe Mon Sep 17 00:00:00 2001 From: Nathan Przybyszewski <172687074+nprzy@users.noreply.github.com> Date: Sun, 30 Jun 2024 18:47:12 -0600 Subject: [PATCH 13/16] Replace unnecessary use of map with forEach --- .../djvk/fireflyPlaidConnector2/transactions/TransferMatcher.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcher.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcher.kt index fcc5e7c..2f13a10 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcher.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcher.kt @@ -120,7 +120,7 @@ class TransferMatcher( } match - }.map { (_, aTx, bTx) -> + }.forEach { (_, aTx, bTx) -> logger.trace("${::match.name} found valid pair with timestamps ${aTx.getTimestamp(zoneId)};" + "${bTx.getTimestamp(zoneId)} and amount $amount") pairsOut.add(Pair(aTx, bTx)) From 9713978338a6b6631f3e38df5e60e4f92578ae34 Mon Sep 17 00:00:00 2001 From: Nathan Przybyszewski <172687074+nprzy@users.noreply.github.com> Date: Sun, 30 Jun 2024 18:48:27 -0600 Subject: [PATCH 14/16] Name lambda variables to avoid shadowing --- .../transactions/PlaidFireflyTransaction.kt | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PlaidFireflyTransaction.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PlaidFireflyTransaction.kt index 9b53842..da1e6c2 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PlaidFireflyTransaction.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PlaidFireflyTransaction.kt @@ -33,21 +33,19 @@ sealed interface PlaidFireflyTransaction { val fireflyByExtId = fireflyTxs.groupBy { it.tx.externalId } val externalIds = plaidById.keys.union(fireflyByExtId.keys) - return externalIds.flatMap { - val matchingPlaid = plaidById[it] ?: listOf() - val matchingFirefly = fireflyByExtId[it] ?: listOf() + return externalIds.flatMap { externalId -> + val matchingPlaid = plaidById[externalId] ?: listOf() + val matchingFirefly = fireflyByExtId[externalId] ?: listOf() // For all transactions that do not have an external ID, or if we've found more matching transactions // than we expected to find, return them without attempting to combine. - if (it == null || matchingFirefly.size > 1 || matchingPlaid.size > 1) { + if (externalId == null || matchingFirefly.size > 1 || matchingPlaid.size > 1) { val convertedFirefly = matchingFirefly.map { FireflyTransaction(it) } - val convertedPlaid = matchingPlaid.map { - val accountId = accountMap[it.accountId] - if (accountId == null) { - throw throw IllegalArgumentException("Can not match Plaid transactions from accounts not mapped " + val convertedPlaid = matchingPlaid.map { matchingPlaidTx -> + val accountId = accountMap[matchingPlaidTx.accountId] + ?: throw throw IllegalArgumentException("Can not match Plaid transactions from accounts not mapped " + "to a Firefly account id") - } - PlaidTransaction(it, accountId) + PlaidTransaction(matchingPlaidTx, accountId) } return@flatMap convertedPlaid + convertedFirefly } From 630128257630c19ab738660135435d1561a87fc7 Mon Sep 17 00:00:00 2001 From: Nathan Przybyszewski <172687074+nprzy@users.noreply.github.com> Date: Fri, 5 Jul 2024 01:24:34 -0600 Subject: [PATCH 15/16] Add polled TransactionConverter test cases --- .../lib/PlaidFixtures.kt | 2 +- .../transactions/TransactionConverterTest.kt | 416 +++++++++++++++++- 2 files changed, 411 insertions(+), 7 deletions(-) diff --git a/src/test/kotlin/net/djvk/fireflyPlaidConnector2/lib/PlaidFixtures.kt b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/lib/PlaidFixtures.kt index d0337bd..f0bfbef 100644 --- a/src/test/kotlin/net/djvk/fireflyPlaidConnector2/lib/PlaidFixtures.kt +++ b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/lib/PlaidFixtures.kt @@ -57,7 +57,7 @@ object PlaidFixtures { originalDescription: String? = null, merchantName: String? = null, checkNumber: String? = null, - personalFinanceCategory: PersonalFinanceCategory = PersonalFinanceCategoryEnum.TRANSFER_OUT_ACCOUNT_TRANSFER.toPersonalFinanceCategory() + personalFinanceCategory: PersonalFinanceCategory? = PersonalFinanceCategoryEnum.TRANSFER_OUT_ACCOUNT_TRANSFER.toPersonalFinanceCategory() ): Transaction { return Transaction( pendingTransactionId = pendingTransactionId, diff --git a/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt index e083cc5..f73fd96 100644 --- a/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt +++ b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverterTest.kt @@ -7,11 +7,13 @@ import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionTypePropert import net.djvk.fireflyPlaidConnector2.api.plaid.PlaidTransactionId import net.djvk.fireflyPlaidConnector2.lib.FireflyFixtures import net.djvk.fireflyPlaidConnector2.lib.PlaidFixtures +import org.assertj.core.api.Assertions.assertThat +import org.assertj.core.api.Assertions.assertThatThrownBy import org.junit.jupiter.api.Assertions.assertEquals -import org.junit.jupiter.api.Test import org.junit.jupiter.params.ParameterizedTest import org.junit.jupiter.params.provider.Arguments import org.junit.jupiter.params.provider.MethodSource +import org.junit.jupiter.params.provider.ValueSource import net.djvk.fireflyPlaidConnector2.api.plaid.models.Transaction as PlaidTransaction internal class TransactionConverterTest { @@ -153,7 +155,290 @@ internal class TransactionConverterTest { ), deletes = listOf(), ), - ) + ), + Arguments.of( +// testName: String, + "Incoming Plaid Deposit and Withdrawal", +// accountMap: Map, + PlaidFixtures.getStandardAccountMapping(), +// plaidCreatedTxs: List, + listOf( + PlaidFixtures.getPaymentTransaction( + accountId = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + transactionId = "plaidWithdrawalId", + name = "Plaid Withdrawal Tx", + amount = 1111.22, + ), + PlaidFixtures.getPaymentTransaction( + accountId = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + transactionId = "plaidDepositId", + name = "Plaid Deposit Tx", + amount = -1111.22, + ), + ), +// plaidUpdatedTxs: List, + listOf(), +// plaidDeletedTxs: List, + listOf(), +// existingFireflyTxs: List, + listOf( + TransactionRead( + "thing", "unrelatedFireflyTransactionId", + FireflyFixtures.getTransaction( + type = TransactionTypeProperty.deposit, + amount = "123.45", + destinationId = "2", + ), ObjectLink() + ), + ), +// expectedResult: TransactionConverter.ConvertPollSyncResult, + TransactionConverter.ConvertPollSyncResult( + creates = listOf( + FireflyTransactionDto( + null, + FireflyFixtures.getTransaction( + type = TransactionTypeProperty.transfer, + amount = "1111.22", + externalId = "plaid-plaidDepositId", + description = "Plaid Deposit Tx", + sourceId = "1", + destinationId = "2", + ).transactions.first() + ), + ), + updates = listOf(), + deletes = listOf(), + ), + ), + Arguments.of( +// testName: String, + "Update without matching Firefly Transaction", +// accountMap: Map, + PlaidFixtures.getStandardAccountMapping(), +// plaidCreatedTxs: List, + listOf(), +// plaidUpdatedTxs: List, + listOf( + PlaidFixtures.getPaymentTransaction( + accountId = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + transactionId = "plaidUpdateId", + amount = 1111.22, + ), + ), +// plaidDeletedTxs: List, + listOf(), +// existingFireflyTxs: List, + listOf( + TransactionRead( + "thing", "unrelatedFireflyTransactionId", + FireflyFixtures.getTransaction( + type = TransactionTypeProperty.deposit, + amount = "123.45", + destinationId = "2", + externalId = "unrelated" + ), ObjectLink() + ), + ), +// expectedResult: TransactionConverter.ConvertPollSyncResult, + TransactionConverter.ConvertPollSyncResult( + creates = listOf(), + updates = listOf(), + deletes = listOf(), + ), + ), + Arguments.of( +// testName: String, + "Update with matching Firefly Transaction", +// accountMap: Map, + PlaidFixtures.getStandardAccountMapping(), +// plaidCreatedTxs: List, + listOf(), +// plaidUpdatedTxs: List, + listOf( + PlaidFixtures.getPaymentTransaction( + accountId = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + name = "Updated transaction name", + transactionId = "plaidUpdateId", + amount = 1111.22, + ), + ), +// plaidDeletedTxs: List, + listOf(), +// existingFireflyTxs: List, + listOf( + TransactionRead( + "thing", "updatedFireflyId", + FireflyFixtures.getTransaction( + type = TransactionTypeProperty.deposit, + description = "Old transaction name", + amount = "123.45", + sourceId = "1", + destinationName = "Unknown Transfer Recipient", + externalId = "plaid-plaidUpdateId", + ), ObjectLink() + ), + ), +// expectedResult: TransactionConverter.ConvertPollSyncResult, + TransactionConverter.ConvertPollSyncResult( + creates = listOf(), + updates = listOf( + FireflyTransactionDto( + "updatedFireflyId", + FireflyFixtures.getTransaction( + type = TransactionTypeProperty.withdrawal, + description = "Updated transaction name", + amount = "1111.22", + sourceId = "1", + destinationName = "Unknown Transfer Recipient", + externalId = "plaid-plaidUpdateId", + ).transactions.first() + ), + ), + deletes = listOf(), + ), + ), + Arguments.of( +// testName: String, + "Delete without matching Firefly Transaction", +// accountMap: Map, + PlaidFixtures.getStandardAccountMapping(), +// plaidCreatedTxs: List, + listOf(), +// plaidUpdatedTxs: List, + listOf(), +// plaidDeletedTxs: List, + listOf( + "deletedTransactionId", + ), +// existingFireflyTxs: List, + listOf( + TransactionRead( + "thing", "updatedFireflyId", + FireflyFixtures.getTransaction( + type = TransactionTypeProperty.deposit, + amount = "123.45", + destinationId = "2", + externalId = "unrelated" + ), ObjectLink() + ), + ), +// expectedResult: TransactionConverter.ConvertPollSyncResult, + TransactionConverter.ConvertPollSyncResult( + creates = listOf(), + updates = listOf(), + deletes = listOf(), + ), + ), + Arguments.of( +// testName: String, + "Delete with matching Firefly Transaction", +// accountMap: Map, + PlaidFixtures.getStandardAccountMapping(), +// plaidCreatedTxs: List, + listOf(), +// plaidUpdatedTxs: List, + listOf(), +// plaidDeletedTxs: List, + listOf( + "deletedTransactionId", + ), +// existingFireflyTxs: List, + listOf( + TransactionRead( + "thing", "deletedFireflyId", + FireflyFixtures.getTransaction( + type = TransactionTypeProperty.deposit, + description = "Old transaction name", + amount = "123.45", + sourceId = "1", + destinationName = "Unknown Transfer Recipient", + externalId = "plaid-deletedTransactionId", + ), ObjectLink() + ), + ), +// expectedResult: TransactionConverter.ConvertPollSyncResult, + TransactionConverter.ConvertPollSyncResult( + creates = listOf(), + updates = listOf(), + deletes = listOf( + "deletedFireflyId", + ), + ), + ), + Arguments.of( +// testName: String, + "Create singles", +// accountMap: Map, + PlaidFixtures.getStandardAccountMapping(), +// plaidCreatedTxs: List, + listOf( + PlaidFixtures.getPaymentTransaction( + accountId = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + name = "Plaid deposit", + transactionId = "plaidDepositId", + amount = -1111.22, + ), + PlaidFixtures.getPaymentTransaction( + accountId = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + name = "Plaid withdrawal", + transactionId = "plaidWithdrawalId", + amount = 123.45, + ), + PlaidFixtures.getPaymentTransaction( + accountId = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + name = "Plaid withdrawal with matching FF", + transactionId = "plaidWithdrawalWithMatchingFfId", + amount = 234.56, + ), + ), +// plaidUpdatedTxs: List, + listOf(), +// plaidDeletedTxs: List, + listOf(), +// existingFireflyTxs: List, + listOf( + TransactionRead( + "thing", "matchingFireflyTxId", + FireflyFixtures.getTransaction( + type = TransactionTypeProperty.withdrawal, + description = "Matching Firefly Tx", + amount = "234.56", + sourceId = "1", + destinationName = "Unknown Transfer Recipient", + externalId = "plaid-plaidWithdrawalWithMatchingFfId", + ), ObjectLink() + ), + ), +// expectedResult: TransactionConverter.ConvertPollSyncResult, + TransactionConverter.ConvertPollSyncResult( + creates = listOf( + FireflyTransactionDto( + null, + FireflyFixtures.getTransaction( + type = TransactionTypeProperty.deposit, + description = "Plaid deposit", + amount = "1111.22", + sourceName = "Unknown Transfer Source", + destinationId = "1", + externalId = "plaid-plaidDepositId", + ).transactions.first() + ), + FireflyTransactionDto( + null, + FireflyFixtures.getTransaction( + type = TransactionTypeProperty.withdrawal, + description = "Plaid withdrawal", + amount = "123.45", + sourceId = "1", + destinationName = "Unknown Transfer Recipient", + externalId = "plaid-plaidWithdrawalId", + ).transactions.first() + ), + ), + updates = listOf(), + deletes = listOf(), + ), + ), ) } @@ -180,6 +465,34 @@ internal class TransactionConverterTest { ) ) } + + fun convertCreates( + converter: TransactionConverter, + poll: Boolean, + inputPlaidTxs: List, + accountMap: Map, + ): List { + return runBlocking { + if (poll) { + val result = converter.convertPollSync( + accountMap, + inputPlaidTxs, + listOf(), + listOf(), + listOf(), + ) + // Since this isn't passing-in any existing Plaid transactions, these should always be empty + assertThat(result.deletes).isEmpty() + assertThat(result.updates).isEmpty() + return@runBlocking result.creates + } else { + return@runBlocking converter.convertBatchSync( + inputPlaidTxs, + accountMap, + ) + } + } + } } @ParameterizedTest(name = "{index} => {0}") @@ -215,10 +528,6 @@ internal class TransactionConverterTest { } } - @Test - fun getSourceKey() { - } - @ParameterizedTest(name = "{index} => {0}") @MethodSource("provideConvertSingleSourceAndDestination") fun convertSingleSourceAndDestination( @@ -250,4 +559,99 @@ internal class TransactionConverterTest { assertEquals(expectedDestinationName, tx.destinationName) } } + + @ParameterizedTest(name = "poll mode = {0}") + @ValueSource(booleans = [true, false]) + fun convertWithCategorizationEnabledAddsExpectedTags(poll: Boolean) { + val converter = TransactionConverter( + false, + enablePrimaryCategorization = true, + primaryCategoryPrefix = "pcat-", + enableDetailedCategorization = true, + detailedCategoryPrefix = "dcat-", + timeZoneString = "America/New_York", + transferMatchWindowDays = 10L, + ) + + val plaidTxWithCategory = PlaidFixtures.getPaymentTransaction( + accountId = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + name = "Plaid transaction with categories", + transactionId = "plaidIdWithCats", + amount = 123.45, + personalFinanceCategory = PersonalFinanceCategoryEnum.TRAVEL_FLIGHTS.toPersonalFinanceCategory(), + ) + + val plaidTxWithoutCategory = PlaidFixtures.getPaymentTransaction( + accountId = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + name = "Plaid transaction without categories", + transactionId = "plaidIdWithoutCats", + amount = 234.56, + personalFinanceCategory = null, + ) + + val expectedFfTxWithTags = FireflyTransactionDto( + null, + FireflyFixtures.getTransaction( + type = TransactionTypeProperty.withdrawal, + description = "Plaid transaction with categories", + amount = "123.45", + sourceId = "1", + destinationName = "Unknown Payment Recipient", + tags = listOf("pcat-travel", "dcat-flights"), + externalId = "plaid-plaidIdWithCats", + ).transactions.first() + ) + + val expectedFfTxWithoutTags = FireflyTransactionDto( + null, + FireflyFixtures.getTransaction( + type = TransactionTypeProperty.withdrawal, + description = "Plaid transaction without categories", + amount = "234.56", + sourceId = "1", + destinationName = "Unknown", + tags = listOf(), + externalId = "plaid-plaidIdWithoutCats", + ).transactions.first() + ) + + val actual = convertCreates( + converter = converter, + poll = poll, + inputPlaidTxs = listOf(plaidTxWithCategory, plaidTxWithoutCategory), + accountMap = PlaidFixtures.getStandardAccountMapping() + ) + assertThat(actual).containsExactlyInAnyOrder(expectedFfTxWithTags, expectedFfTxWithoutTags) + } + + @ParameterizedTest(name = "poll mode = {0}") + @ValueSource(booleans = [true, false]) + fun convertPollSyncThrowsWhenFireflyAccountIdNotFound(poll: Boolean) { + val converter = TransactionConverter( + useNameForDestination = false, + enablePrimaryCategorization = false, + primaryCategoryPrefix = "a", + enableDetailedCategorization = false, + detailedCategoryPrefix = "b", + timeZoneString = "America/New_York", + transferMatchWindowDays = 10L, + ) + + val plaidTx = PlaidFixtures.getPaymentTransaction( + accountId = "unknownAccountId", + name = "Plaid transaction", + transactionId = "plaidId", + amount = 123.45, + ) + + assertThatThrownBy { + convertCreates( + converter = converter, + poll = poll, + inputPlaidTxs = listOf(plaidTx), + accountMap = PlaidFixtures.getStandardAccountMapping() + ) + } + .hasMessageContaining("Can not match Plaid transactions from accounts not mapped to a Firefly account id") + } } From 32687b8ddcdcebb59488fae5d79f082d2ea1975e Mon Sep 17 00:00:00 2001 From: Nathan Przybyszewski <172687074+nprzy@users.noreply.github.com> Date: Fri, 5 Jul 2024 01:25:52 -0600 Subject: [PATCH 16/16] Represent Transfers as subtype of PlaidFireflyTransaction --- .../transactions/PlaidFireflyTransaction.kt | 52 +++++++- .../transactions/TransactionConverter.kt | 120 +++++++----------- .../transactions/TransferMatcher.kt | 20 +-- .../transactions/TransferMatcherTest.kt | 38 ++---- 4 files changed, 118 insertions(+), 112 deletions(-) diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PlaidFireflyTransaction.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PlaidFireflyTransaction.kt index da1e6c2..c5afd0f 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PlaidFireflyTransaction.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PlaidFireflyTransaction.kt @@ -4,9 +4,11 @@ import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionTypePropert import net.djvk.fireflyPlaidConnector2.api.plaid.models.Transaction import java.time.OffsetDateTime import java.time.ZoneId +import java.util.function.Supplier +import kotlin.math.abs /** - * Represents a single non-transfer transaction, including both the Plaid and the Firefly representation of it. + * Represents a single transaction, including both the Plaid and the Firefly representation of it. * * See the descriptions of the individual subtypes for more details about the situations in which each is used. */ @@ -123,7 +125,9 @@ sealed interface PlaidFireflyTransaction { /** * This subtype is used when we've received a transaction from Plaid that already has a corresponding transaction - * in Firefly. Note that this is NOT related to + * in Firefly. Note that this is NOT related to the matching of two discrete transactions from two different + * accounts into a "transfer". Both the Firefly and Plaid transactions within this object will be related to the + * same account. */ data class MatchedTransaction( override val plaidTransaction: Transaction, @@ -136,4 +140,48 @@ sealed interface PlaidFireflyTransaction { return getPlaidTransactionDate(plaidTransaction, zoneId) } } + + /** + * This subtype represents a pair of two transactions that we believe to be a transfer from one account into a + * different account. + */ + data class Transfer private constructor( + val deposit: PlaidFireflyTransaction, + val withdrawal: PlaidFireflyTransaction, + ): PlaidFireflyTransaction { + companion object { + fun create(first: PlaidFireflyTransaction, second: PlaidFireflyTransaction): Transfer { + if ((first.amount > 0) == (second.amount > 0)) { + throw IllegalArgumentException("A transfer must have one withdrawal and one deposit") + } + if (abs(first.amount) != abs(second.amount)) { + throw IllegalArgumentException("A transfer must have the same withdrawal and deposit amounts") + } + if (first.fireflyAccountId == second.fireflyAccountId) { + throw IllegalArgumentException("A transfer must not have the same Firefly account IDs") + } + + return if (first.amount >= 0) { + Transfer( + deposit = first, + withdrawal = second, + ) + } else { + Transfer( + deposit = second, + withdrawal = first, + ) + } + } + } + + override val plaidTransaction = deposit.plaidTransaction ?: withdrawal.plaidTransaction + override val fireflyTransaction = deposit.fireflyTransaction ?: withdrawal.fireflyTransaction + override val amount = abs(deposit.amount) + override val transactionId = deposit.transactionId + override val fireflyAccountId get() = throw RuntimeException("Can not get Firefly account ID for a transfer") + override fun getTimestamp(zoneId: ZoneId): OffsetDateTime { + return deposit.getTimestamp(zoneId) + } + } } diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt index 4b5240e..19c54da 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt @@ -86,10 +86,6 @@ class TransactionConverter( private fun requirePlaidTransaction(tx: PlaidFireflyTransaction?): PlaidTransaction { return tx?.plaidTransaction ?: throw RuntimeException("Transaction missing required Plaid information") } - - private fun requireFireflyTransaction(tx: PlaidFireflyTransaction?): FireflyTransactionDto { - return tx?.fireflyTransaction ?: throw RuntimeException("Transaction missing required Firefly information") - } } fun getTxTimestamp(tx: PlaidTransaction): OffsetDateTime { @@ -148,24 +144,20 @@ class TransactionConverter( accountMap: Map, ): List { logger.debug("Batch sync converting Plaid transactions to Firefly transactions") - val (singles, pairs) = transferMatcher.match(PlaidFireflyTransaction.normalizeByTransactionId(txs, listOf(), accountMap)) - val out = mutableListOf() - - for (single in singles) { - out.add(convertSingle(requirePlaidTransaction(single), accountMap)) - } - - for (pair in pairs) { - out.add( - convertDoublePlaid( - requirePlaidTransaction(pair.first), - requirePlaidTransaction(pair.second), - accountMap - ) - ) + return transferMatcher.match(PlaidFireflyTransaction.normalizeByTransactionId(txs, listOf(), accountMap)).map { + when (it) { + is PlaidFireflyTransaction.Transfer -> { + convertDoublePlaid( + requirePlaidTransaction(it.withdrawal), + requirePlaidTransaction(it.deposit), + accountMap + ) + } + else -> { + convertSingle(requirePlaidTransaction(it), accountMap) + } + } } - - return out } data class ConvertPollSyncResult( @@ -190,8 +182,6 @@ class TransactionConverter( ): ConvertPollSyncResult { logger.trace("Starting ${::convertPollSync.name}") val transferCandidateExistingFireflyTxs = filterFireflyCandidateTransferTxs(existingFireflyTxs) - val createdSet = plaidCreatedTxs.toSet() - val updatedSet = plaidUpdatedTxs.toSet() val creates = mutableListOf() val updates = mutableListOf() @@ -201,17 +191,21 @@ class TransactionConverter( * Don't pass in [plaidUpdatedTxs] here because we're not going to try to update transfers for now * because it's more complexity than I want to deal with, and I haven't seen any Plaid updates in the wild yet */ - val (singles, pairs) = transferMatcher.match( + val wrappedCreates = transferMatcher.match( PlaidFireflyTransaction.normalizeByTransactionId(plaidCreatedTxs, transferCandidateExistingFireflyTxs, accountMap) ) - logger.debug("${::convertPollSync.name} call to transferMatcher returned ${singles.size} singles and ${pairs.size} pairs") + logger.debug( + "{} call to transferMatcher returned {} transactions", + ::convertPollSync.name, + wrappedCreates.size, + ) /** * Handle singles, which are transactions that didn't have any transfer pair matches */ - for (single in singles) { - val convertedSingle = when (single) { - is PlaidFireflyTransaction.PlaidTransaction -> convertSingle(single.plaidTransaction, accountMap) + for (create in wrappedCreates) { + val convertedSingle = when (create) { + is PlaidFireflyTransaction.PlaidTransaction -> convertSingle(create.plaidTransaction, accountMap) // In both of these cases a Firefly transaction already exists. We don't need to do anything to it. // If we have an associated Plaid transaction, log a message. Otherwise, silently ignore it. @@ -219,11 +213,35 @@ class TransactionConverter( is PlaidFireflyTransaction.MatchedTransaction -> { logger.debug( "Ignoring Plaid transaction id {} because it already has a corresponding Firefly transaction {}", - single.plaidTransaction.transactionId, - single.fireflyTransaction.id, + create.plaidTransaction.transactionId, + create.fireflyTransaction.id, ) continue } + + is PlaidFireflyTransaction.Transfer -> { + if (create.withdrawal.fireflyTransaction != null && create.deposit.fireflyTransaction != null) { + logger.debug("TransferMatcher found multiple existing Firefly transactions that appear to " + + "be a transfer. Converting multiple existing Firefly transactions to a transfer is " + + "not supported. Skipping: {}", create) + continue + } + + val fireflyComponent = create.fireflyTransaction + if (fireflyComponent != null) { + convertDoubleFirefly( + requirePlaidTransaction(create), + fireflyComponent, + accountMap, + ) + } else { + convertDoublePlaid( + requirePlaidTransaction(create.deposit), + requirePlaidTransaction(create.withdrawal), + accountMap, + ) + } + } } if (convertedSingle.id == null) { @@ -233,48 +251,6 @@ class TransactionConverter( } } - /** - * Handle pairs that we think should become Firefly transfers - */ - for (pair in pairs) { - val (hasFireflyTx, noFireflyTx) = pair.toList().partition { it.fireflyTransaction != null } - if (hasFireflyTx.size > 1) { - logger.debug("TransferMatcher found multiple existing Firefly transactions that appear to " - + "be a transfer. Converting multiple existing Firefly transactions to a transfer is not " - + "supported. Skipping: {}", pair) - continue - } - - val plaidComponent = requirePlaidTransaction(noFireflyTx.getOrNull(0)) - - val out = when { - hasFireflyTx.isNotEmpty() -> - convertDoubleFirefly( - plaidComponent, - requireFireflyTransaction(hasFireflyTx.getOrNull(0)), - accountMap, - ) - - else -> - convertDoublePlaid( - plaidComponent, - requirePlaidTransaction(noFireflyTx.getOrNull(1)), - accountMap, - ) - } - - if (out.id != null) { - updates.add(out) - } else if (createdSet.contains(plaidComponent)) { - // TODO: what happens if the pair is two Plaid transactions, one create and one update? - creates.add(out) - } else if (updatedSet.contains(plaidComponent)) { - updates.add(out) - } else { - throw IllegalArgumentException("Unable to determine create/update status of sorted pair: $pair") - } - } - val indexer = FireflyTransactionExternalIdIndexer(existingFireflyTxs) /** * Handle Plaid updates diff --git a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcher.kt b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcher.kt index 2f13a10..bb87c5e 100644 --- a/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcher.kt +++ b/src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcher.kt @@ -29,11 +29,6 @@ class TransferMatcher( PersonalFinanceCategoryEnum.Primary.BANK_FEES, ) - data class SortByPairsResult( - val singles: List, - val pairs: List>, - ) - /** * Identify matching transaction pairs that can be converted to a single "transfer" in Firefly. * @@ -41,7 +36,7 @@ class TransferMatcher( * that it would not make sense to act on, such as matching pairs of Firefly transactions that do not have * corresponding Plaid transactions. */ - fun match(txs: List): SortByPairsResult { + fun match(txs: List): List { logger.trace("Starting ${::match.name}") // Split-out the transactions that are unlikely to be transfers based on their category. If we're not sure, @@ -51,8 +46,7 @@ class TransferMatcher( category == null || transferTypes.contains(PersonalFinanceCategoryEnum.from(category).primary) } - val pairsOut = mutableListOf>() - val singlesOut = nonTransfers.toMutableList() + val results = nonTransfers.toMutableList() val amountIndexedTxs = possibleTransfers.groupBy { it.amount } // The loop below will process an amount value and its inverse, so we use this to mark the inverse @@ -71,7 +65,7 @@ class TransferMatcher( // If there are no matching txs, then this group has no soulmates and we should move on if (matchingGroupTxs == null) { - singlesOut.addAll(groupTxs) + results.addAll(groupTxs) continue } val txsSecondsDiff = mutableListOf() @@ -123,15 +117,15 @@ class TransferMatcher( }.forEach { (_, aTx, bTx) -> logger.trace("${::match.name} found valid pair with timestamps ${aTx.getTimestamp(zoneId)};" + "${bTx.getTimestamp(zoneId)} and amount $amount") - pairsOut.add(Pair(aTx, bTx)) + results.add(PlaidFireflyTransaction.Transfer.create(aTx, bTx)) } // Output all leftover transactions as singles - singlesOut.addAll(groupTxs.filter { !usedATxIds.contains(it.transactionId) }) - singlesOut.addAll(matchingGroupTxs.filter { !usedBTxIds.contains(it.transactionId) }) + results.addAll(groupTxs.filter { !usedATxIds.contains(it.transactionId) }) + results.addAll(matchingGroupTxs.filter { !usedBTxIds.contains(it.transactionId) }) } - return SortByPairsResult(singlesOut, pairsOut) + return results } private data class CandidatePair( diff --git a/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcherTest.kt b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcherTest.kt index 12ee150..2910d84 100644 --- a/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcherTest.kt +++ b/src/test/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcherTest.kt @@ -1,10 +1,9 @@ package net.djvk.fireflyPlaidConnector2.transactions import net.djvk.fireflyPlaidConnector2.api.firefly.models.TransactionTypeProperty -import net.djvk.fireflyPlaidConnector2.api.plaid.models.Transaction import net.djvk.fireflyPlaidConnector2.lib.FireflyFixtures import net.djvk.fireflyPlaidConnector2.lib.PlaidFixtures -import org.junit.jupiter.api.Assertions.assertIterableEquals +import org.assertj.core.api.Assertions.assertThat import org.junit.jupiter.params.ParameterizedTest import org.junit.jupiter.params.provider.Arguments import org.junit.jupiter.params.provider.MethodSource @@ -37,12 +36,6 @@ internal class TransferMatcherTest { return FireflyTransactionDto("tx-${transactionIndex++}", txSplit) } - private fun sortListOfPairs(input: List>): List> { - return input.map { pair -> - pair.toList().sortedBy { tx -> tx.transactionId } - }.sortedBy { it.first().transactionId } - } - @JvmStatic fun provideTransferPairTestCases(): List { val plaidAcctA = "aaa" @@ -50,7 +43,7 @@ internal class TransferMatcherTest { val plaidAcctB = "bbb" val fireflyAcctB = 2 - val pairPlaid = Pair( + val pairPlaid = PlaidFireflyTransaction.Transfer.create( PlaidFireflyTransaction.PlaidTransaction( PlaidFixtures.getTransferTestTransaction( name = "Plaid Transfer Source", @@ -73,7 +66,7 @@ internal class TransferMatcherTest { ), ) - val pairMixedA = Pair( + val pairMixedA = PlaidFireflyTransaction.Transfer.create( PlaidFireflyTransaction.FireflyTransaction( getFireflyDto( description = "Firefly Transfer Source", @@ -95,7 +88,7 @@ internal class TransferMatcherTest { ), ) - val pairMixedB = Pair( + val pairMixedB = PlaidFireflyTransaction.Transfer.create( PlaidFireflyTransaction.PlaidTransaction( PlaidFixtures.getTransferTestTransaction( name = "Plaid Transfer Source", @@ -186,21 +179,18 @@ internal class TransferMatcherTest { return listOf( Arguments.of( "Matching Plaid Source/Dest", - (singles + sequenceOf(pairPlaid.first, pairPlaid.second)).shuffled(), - singles, - listOf(pairPlaid), + (singles + sequenceOf(pairPlaid.deposit, pairPlaid.withdrawal)).shuffled(), + singles + listOf(pairPlaid), ), Arguments.of( "Matching Plaid Source and Firefly Destination", - (singles + sequenceOf(pairMixedA.first, pairMixedA.second)).shuffled(), - singles, - listOf(pairMixedA), + (singles + sequenceOf(pairMixedA.deposit, pairMixedA.withdrawal)).shuffled(), + singles + listOf(pairMixedA), ), Arguments.of( "Matching Firefly Source and Plaid Destination", - (singles + sequenceOf(pairMixedB.first, pairMixedB.second)).shuffled(), - singles, - listOf(pairMixedB), + (singles + sequenceOf(pairMixedB.deposit, pairMixedB.withdrawal)).shuffled(), + singles + listOf(pairMixedB), ), ) } @@ -211,16 +201,14 @@ internal class TransferMatcherTest { fun match( testName: String, input: List, - expectedSingles: List, - expectedPairs: List>, + expected: List, ) { val matcher = TransferMatcher( timeZoneString = "America/New_York", transferMatchWindowDays = matchWindowDays, ) - val (actualSingles, actualPairs) = matcher.match(input) - assertIterableEquals(sortListOfPairs(expectedPairs), sortListOfPairs(actualPairs)) - assertIterableEquals(expectedSingles.sortedBy { it.transactionId }, actualSingles.sortedBy { it.transactionId }) + val actual = matcher.match(input) + assertThat(actual).containsExactlyInAnyOrderElementsOf(expected) } } \ No newline at end of file