From 3b776091596a9ac6e0baf6dc5ceb01eee4c8e5cb Mon Sep 17 00:00:00 2001 From: Matiss Janis Aboltins Date: Mon, 4 Mar 2024 18:42:34 +0000 Subject: [PATCH] :sparkles: (bank-sync) quality of life improvements for sync (#2416) --- .../src/components/BankSyncStatus.tsx | 29 ++-- .../src/components/accounts/Account.jsx | 2 +- .../src/components/accounts/Header.jsx | 5 +- .../src/components/sidebar/Account.tsx | 10 +- .../src/components/sidebar/Accounts.tsx | 7 + .../desktop-client/src/style/themes/dark.ts | 1 + .../src/style/themes/development.ts | 1 + .../desktop-client/src/style/themes/light.ts | 1 + .../src/style/themes/midnight.ts | 1 + .../loot-core/src/client/actions/account.ts | 152 ++++++++---------- packages/loot-core/src/client/actions/sync.ts | 2 +- packages/loot-core/src/client/constants.ts | 1 - .../loot-core/src/client/reducers/account.ts | 15 +- .../src/client/state-types/account.d.ts | 18 +-- packages/loot-core/src/server/main.ts | 10 +- .../loot-core/src/types/server-handlers.d.ts | 2 +- upcoming-release-notes/2416.md | 6 + 17 files changed, 121 insertions(+), 142 deletions(-) create mode 100644 upcoming-release-notes/2416.md diff --git a/packages/desktop-client/src/components/BankSyncStatus.tsx b/packages/desktop-client/src/components/BankSyncStatus.tsx index f49b9200749..77ae6457d1f 100644 --- a/packages/desktop-client/src/components/BankSyncStatus.tsx +++ b/packages/desktop-client/src/components/BankSyncStatus.tsx @@ -14,19 +14,17 @@ export function BankSyncStatus() { const accountsSyncing = useSelector( (state: State) => state.account.accountsSyncing, ); + const accountsSyncingCount = accountsSyncing.length; - const name = accountsSyncing - ? accountsSyncing === '__all' - ? 'accounts' - : accountsSyncing - : null; - - const transitions = useTransition(name, { - from: { opacity: 0, transform: 'translateY(-100px)' }, - enter: { opacity: 1, transform: 'translateY(0)' }, - leave: { opacity: 0, transform: 'translateY(-100px)' }, - unique: true, - }); + const transitions = useTransition( + accountsSyncingCount > 0 ? 'syncing' : null, + { + from: { opacity: 0, transform: 'translateY(-100px)' }, + enter: { opacity: 1, transform: 'translateY(0)' }, + leave: { opacity: 0, transform: 'translateY(-100px)' }, + unique: true, + }, + ); return ( - Syncing {item} + + Syncing... {accountsSyncingCount} account + {accountsSyncingCount > 1 && 's'} remaining + ), diff --git a/packages/desktop-client/src/components/accounts/Account.jsx b/packages/desktop-client/src/components/accounts/Account.jsx index 4db7abb7c27..62f2a7f6f11 100644 --- a/packages/desktop-client/src/components/accounts/Account.jsx +++ b/packages/desktop-client/src/components/accounts/Account.jsx @@ -462,7 +462,7 @@ class AccountInternal extends PureComponent { const accountId = this.props.accountId; const account = this.props.accounts.find(acct => acct.id === accountId); - await this.props.syncAndDownload(account ? account.id : null); + await this.props.syncAndDownload(account ? account.id : undefined); }; onImport = async () => { diff --git a/packages/desktop-client/src/components/accounts/Header.jsx b/packages/desktop-client/src/components/accounts/Header.jsx index eaade96bfb2..93cd4511503 100644 --- a/packages/desktop-client/src/components/accounts/Header.jsx +++ b/packages/desktop-client/src/components/accounts/Header.jsx @@ -232,8 +232,9 @@ export function AccountHeader({ width={13} height={13} animating={ - (account && accountsSyncing === account.name) || - accountsSyncing === '__all' + account + ? accountsSyncing.includes(account.id) + : accountsSyncing.length > 0 } style={{ marginRight: 4 }} />{' '} diff --git a/packages/desktop-client/src/components/sidebar/Account.tsx b/packages/desktop-client/src/components/sidebar/Account.tsx index 3cd28ba2e61..2c232a73a14 100644 --- a/packages/desktop-client/src/components/sidebar/Account.tsx +++ b/packages/desktop-client/src/components/sidebar/Account.tsx @@ -38,6 +38,7 @@ type AccountProps = { query: Binding; account?: AccountEntity; connected?: boolean; + pending?: boolean; failed?: boolean; updated?: boolean; style?: CSSProperties; @@ -50,6 +51,7 @@ export function Account({ name, account, connected, + pending = false, failed, updated, to, @@ -125,9 +127,11 @@ export function Account({ width: 5, height: 5, borderRadius: 5, - backgroundColor: failed - ? theme.sidebarItemBackgroundFailed - : theme.sidebarItemBackgroundPositive, + backgroundColor: pending + ? theme.sidebarItemBackgroundPending + : failed + ? theme.sidebarItemBackgroundFailed + : theme.sidebarItemBackgroundPositive, marginLeft: 2, transition: 'transform .3s', opacity: connected ? 1 : 0, diff --git a/packages/desktop-client/src/components/sidebar/Accounts.tsx b/packages/desktop-client/src/components/sidebar/Accounts.tsx index e85e32e4a19..d85cb537bd7 100644 --- a/packages/desktop-client/src/components/sidebar/Accounts.tsx +++ b/packages/desktop-client/src/components/sidebar/Accounts.tsx @@ -1,7 +1,9 @@ // @ts-strict-ignore import React, { useState } from 'react'; +import { useSelector } from 'react-redux'; import * as queries from 'loot-core/src/client/queries'; +import { type State } from 'loot-core/src/client/state-types'; import { useBudgetedAccounts } from '../../hooks/useBudgetedAccounts'; import { useClosedAccounts } from '../../hooks/useClosedAccounts'; @@ -35,6 +37,9 @@ export function Accounts({ const offbudgetAccounts = useOffBudgetAccounts(); const budgetedAccounts = useBudgetedAccounts(); const closedAccounts = useClosedAccounts(); + const syncingAccountIds = useSelector( + (state: State) => state.account.accountsSyncing, + ); const getAccountPath = account => `/accounts/${account.id}`; @@ -78,6 +83,7 @@ export function Accounts({ name={account.name} account={account} connected={!!account.bank} + pending={syncingAccountIds.includes(account.id)} failed={failedAccounts && failedAccounts.has(account.id)} updated={updatedAccounts && updatedAccounts.includes(account.id)} to={getAccountPath(account)} @@ -103,6 +109,7 @@ export function Accounts({ name={account.name} account={account} connected={!!account.bank} + pending={syncingAccountIds.includes(account.id)} failed={failedAccounts && failedAccounts.has(account.id)} updated={updatedAccounts && updatedAccounts.includes(account.id)} to={getAccountPath(account)} diff --git a/packages/desktop-client/src/style/themes/dark.ts b/packages/desktop-client/src/style/themes/dark.ts index a2a2f3526c6..4d2800587ec 100644 --- a/packages/desktop-client/src/style/themes/dark.ts +++ b/packages/desktop-client/src/style/themes/dark.ts @@ -39,6 +39,7 @@ export const tableRowHeaderBackground = colorPalette.navy700; export const tableRowHeaderText = colorPalette.navy150; export const sidebarBackground = colorPalette.navy900; +export const sidebarItemBackgroundPending = colorPalette.orange200; export const sidebarItemBackgroundPositive = colorPalette.green500; export const sidebarItemBackgroundFailed = colorPalette.red300; export const sidebarItemAccentSelected = colorPalette.purple200; diff --git a/packages/desktop-client/src/style/themes/development.ts b/packages/desktop-client/src/style/themes/development.ts index 68e190b6b69..9ce9db8795d 100644 --- a/packages/desktop-client/src/style/themes/development.ts +++ b/packages/desktop-client/src/style/themes/development.ts @@ -39,6 +39,7 @@ export const tableRowHeaderBackground = colorPalette.navy50; export const tableRowHeaderText = colorPalette.navy800; export const sidebarBackground = colorPalette.navy900; +export const sidebarItemBackgroundPending = colorPalette.orange200; export const sidebarItemBackgroundPositive = colorPalette.green500; export const sidebarItemBackgroundFailed = colorPalette.red300; export const sidebarItemBackgroundHover = colorPalette.navy800; diff --git a/packages/desktop-client/src/style/themes/light.ts b/packages/desktop-client/src/style/themes/light.ts index a442f746a0f..124bb671e28 100644 --- a/packages/desktop-client/src/style/themes/light.ts +++ b/packages/desktop-client/src/style/themes/light.ts @@ -39,6 +39,7 @@ export const tableRowHeaderBackground = colorPalette.navy50; export const tableRowHeaderText = colorPalette.navy800; export const sidebarBackground = colorPalette.navy900; +export const sidebarItemBackgroundPending = colorPalette.orange200; export const sidebarItemBackgroundPositive = colorPalette.green500; export const sidebarItemBackgroundFailed = colorPalette.red300; export const sidebarItemBackgroundHover = colorPalette.navy800; diff --git a/packages/desktop-client/src/style/themes/midnight.ts b/packages/desktop-client/src/style/themes/midnight.ts index 6e6dea4bda1..1fc41c202cd 100644 --- a/packages/desktop-client/src/style/themes/midnight.ts +++ b/packages/desktop-client/src/style/themes/midnight.ts @@ -39,6 +39,7 @@ export const tableRowHeaderBackground = colorPalette.gray700; export const tableRowHeaderText = colorPalette.gray150; export const sidebarBackground = colorPalette.gray900; +export const sidebarItemBackgroundPending = colorPalette.orange200; export const sidebarItemBackgroundPositive = colorPalette.green400; export const sidebarItemBackgroundFailed = colorPalette.red300; export const sidebarItemAccentSelected = colorPalette.purple200; diff --git a/packages/loot-core/src/client/actions/account.ts b/packages/loot-core/src/client/actions/account.ts index e7f7f269b4b..f6448ce9e56 100644 --- a/packages/loot-core/src/client/actions/account.ts +++ b/packages/loot-core/src/client/actions/account.ts @@ -2,7 +2,6 @@ import { send } from '../../platform/client/fetch'; import * as constants from '../constants'; import type { - AccountSyncFailuresAction, AccountSyncStatusAction, SetAccountsSyncingAction, } from '../state-types/account'; @@ -17,11 +16,11 @@ import { getPayees, getAccounts } from './queries'; import type { Dispatch, GetState } from './types'; export function setAccountsSyncing( - name: SetAccountsSyncingAction['name'], + ids: SetAccountsSyncingAction['ids'], ): SetAccountsSyncingAction { return { type: constants.SET_ACCOUNTS_SYNCING, - name, + ids, }; } @@ -47,14 +46,6 @@ export function markAccountSuccess( failed: false, }; } -export function setFailedAccounts( - syncErrors: AccountSyncFailuresAction['syncErrors'], -): AccountSyncFailuresAction { - return { - type: constants.ACCOUNT_SYNC_FAILURES, - syncErrors, - }; -} export function unlinkAccount(id: string) { return async (dispatch: Dispatch) => { @@ -107,96 +98,89 @@ export function connectAccounts( }; } -// TODO: type correctly or remove (unused) -export function connectGoCardlessAccounts( - institution, - publicToken, - accountIds, - offbudgetIds, -) { - return async (dispatch: Dispatch) => { - const ids = await send('gocardless-accounts-connect', { - institution, - publicToken, - accountIds, - offbudgetIds, - }); - await dispatch(getPayees()); - await dispatch(getAccounts()); - return ids; - }; -} - -export function syncAccounts(id: string) { +export function syncAccounts(id?: string) { return async (dispatch: Dispatch, getState: GetState) => { - if (getState().account.accountsSyncing) { + // Disallow two parallel sync operations + if (getState().account.accountsSyncing.length > 0) { return false; } - if (id) { - const account = getState().queries.accounts.find(a => a.id === id); - dispatch(setAccountsSyncing(account.name)); - } else { - dispatch(setAccountsSyncing('__all')); - } + // Build an array of IDs for accounts to sync.. if no `id` provided + // then we assume that all accounts should be synced + const accountIdsToSync = id + ? [id] + : getState() + .queries.accounts.filter( + ({ bank, closed, tombstone }) => !!bank && !closed && !tombstone, + ) + .map(({ id }) => id); + + dispatch(setAccountsSyncing(accountIdsToSync)); - const { errors, newTransactions, matchedTransactions, updatedAccounts } = - await send('gocardless-accounts-sync', { id }); - dispatch(setAccountsSyncing(null)); + let isSyncSuccess = false; - if (id) { - const error = errors.find(error => error.accountId === id); + // Loop through the accounts and perform sync operation.. one by one + for (let idx = 0; idx < accountIdsToSync.length; idx++) { + const accountId = accountIdsToSync[idx]; + // Perform sync operation + const { errors, newTransactions, matchedTransactions, updatedAccounts } = + await send('gocardless-accounts-sync', { + id: accountId, + }); + + // Mark the account as failed or succeeded (depending on sync output) + const [error] = errors; if (error) { // We only want to mark the account as having problem if it // was a real syncing error. if (error.type === 'SyncError') { - dispatch(markAccountFailed(id, error.category, error.code)); + dispatch(markAccountFailed(accountId, error.category, error.code)); } } else { - dispatch(markAccountSuccess(id)); + dispatch(markAccountSuccess(accountId)); } - } else { - dispatch( - setFailedAccounts( - errors - .filter(error => error.type === 'SyncError') - .map(error => ({ - id: error.accountId, - type: error.category, - code: error.code, - })), - ), - ); - } - errors.forEach(error => { - if (error.type === 'SyncError') { - dispatch( - addNotification({ - type: 'error', - message: error.message, - }), - ); - } else { - dispatch( - addNotification({ - type: 'error', - message: error.message, - internal: error.internal, - }), - ); - } - }); + // Dispatch errors (if any) + errors.forEach(error => { + if (error.type === 'SyncError') { + dispatch( + addNotification({ + type: 'error', + message: error.message, + }), + ); + } else { + dispatch( + addNotification({ + type: 'error', + message: error.message, + internal: error.internal, + }), + ); + } + }); - dispatch({ - type: constants.SET_NEW_TRANSACTIONS, - newTransactions, - matchedTransactions, - updatedAccounts, - }); + // Set new transactions + dispatch({ + type: constants.SET_NEW_TRANSACTIONS, + newTransactions, + matchedTransactions, + updatedAccounts, + }); + + // Dispatch the ids for the accounts that are yet to be synced + dispatch(setAccountsSyncing(accountIdsToSync.slice(idx + 1))); + + if (newTransactions.length > 0 || matchedTransactions.length > 0) { + isSyncSuccess = true; + } + } - return newTransactions.length > 0 || matchedTransactions.length > 0; + // Rest the sync state back to empty (fallback in case something breaks + // in the logic above) + dispatch(setAccountsSyncing([])); + return isSyncSuccess; }; } diff --git a/packages/loot-core/src/client/actions/sync.ts b/packages/loot-core/src/client/actions/sync.ts index 94aa3f7db06..77838897fca 100644 --- a/packages/loot-core/src/client/actions/sync.ts +++ b/packages/loot-core/src/client/actions/sync.ts @@ -50,7 +50,7 @@ export function sync() { }; } -export function syncAndDownload(accountId) { +export function syncAndDownload(accountId?: string) { return async (dispatch: Dispatch) => { // It is *critical* that we sync first because of transaction // reconciliation. We want to get all transactions that other diff --git a/packages/loot-core/src/client/constants.ts b/packages/loot-core/src/client/constants.ts index c107dea1c82..03f0e578630 100644 --- a/packages/loot-core/src/client/constants.ts +++ b/packages/loot-core/src/client/constants.ts @@ -26,5 +26,4 @@ export const SET_LAST_UNDO_STATE = 'SET_LAST_UNDO_STATE'; export const SET_LAST_SPLIT_STATE = 'SET_LAST_SPLIT_STATE'; export const SET_ACCOUNTS_SYNCING = 'SET_ACCOUNTS_SYNCING'; export const ACCOUNT_SYNC_STATUS = 'ACCOUNT_SYNC_STATUS'; -export const ACCOUNT_SYNC_FAILURES = 'ACCOUNT_SYNC_FAILURES'; export const SIGN_OUT = 'SIGN_OUT'; diff --git a/packages/loot-core/src/client/reducers/account.ts b/packages/loot-core/src/client/reducers/account.ts index 7c36ee5f73c..37387c5e52b 100644 --- a/packages/loot-core/src/client/reducers/account.ts +++ b/packages/loot-core/src/client/reducers/account.ts @@ -4,7 +4,7 @@ import type { AccountState } from '../state-types/account'; const initialState: AccountState = { failedAccounts: new Map(), - accountsSyncing: null, + accountsSyncing: [], }; export function update(state = initialState, action: Action): AccountState { @@ -12,7 +12,7 @@ export function update(state = initialState, action: Action): AccountState { case constants.SET_ACCOUNTS_SYNCING: return { ...state, - accountsSyncing: action.name, + accountsSyncing: action.ids, }; case constants.ACCOUNT_SYNC_STATUS: { const failedAccounts = new Map(state.failedAccounts); @@ -27,17 +27,6 @@ export function update(state = initialState, action: Action): AccountState { return { ...state, failedAccounts }; } - case constants.ACCOUNT_SYNC_FAILURES: { - const failures = new Map(); - action.syncErrors.forEach(error => { - failures.set(error.id, { - type: error.type, - code: error.code, - }); - }); - - return { ...state, failedAccounts: failures }; - } default: } return state; diff --git a/packages/loot-core/src/client/state-types/account.d.ts b/packages/loot-core/src/client/state-types/account.d.ts index 2b119bc2714..5abad2a6df2 100644 --- a/packages/loot-core/src/client/state-types/account.d.ts +++ b/packages/loot-core/src/client/state-types/account.d.ts @@ -2,12 +2,12 @@ import type * as constants from '../constants'; export type AccountState = { failedAccounts: Map; - accountsSyncing: string | null; + accountsSyncing: string[]; }; export type SetAccountsSyncingAction = { type: typeof constants.SET_ACCOUNTS_SYNCING; - name: string | null; + ids: string[]; }; export type AccountSyncStatusAction = { @@ -24,16 +24,4 @@ export type AccountSyncStatusAction = { } ); -export type AccountSyncFailuresAction = { - type: typeof constants.ACCOUNT_SYNC_FAILURES; - syncErrors: Array<{ - id: string; - type: string; - code: string; - }>; -}; - -export type AccountActions = - | SetAccountsSyncingAction - | AccountSyncStatusAction - | AccountSyncFailuresAction; +export type AccountActions = SetAccountsSyncingAction | AccountSyncStatusAction; diff --git a/packages/loot-core/src/server/main.ts b/packages/loot-core/src/server/main.ts index 21c3b7420e8..64cd29938bb 100644 --- a/packages/loot-core/src/server/main.ts +++ b/packages/loot-core/src/server/main.ts @@ -1266,18 +1266,14 @@ handlers['gocardless-accounts-sync'] = async function ({ id }) { 'user-id', 'user-key', ]); - let accounts = await db.runQuery( + const accounts = await db.runQuery( `SELECT a.*, b.bank_id as bankId FROM accounts a LEFT JOIN banks b ON a.bank = b.id - WHERE a.tombstone = 0 AND a.closed = 0`, - [], + WHERE a.tombstone = 0 AND a.closed = 0 AND a.id = ?`, + [id], true, ); - if (id) { - accounts = accounts.filter(acct => acct.id === id); - } - const errors = []; let newTransactions = []; let matchedTransactions = []; diff --git a/packages/loot-core/src/types/server-handlers.d.ts b/packages/loot-core/src/types/server-handlers.d.ts index 1883a36ccac..eff46e2f601 100644 --- a/packages/loot-core/src/types/server-handlers.d.ts +++ b/packages/loot-core/src/types/server-handlers.d.ts @@ -246,7 +246,7 @@ export interface ServerHandlers { | { error: 'failed' } >; - 'gocardless-accounts-sync': (arg: { id }) => Promise<{ + 'gocardless-accounts-sync': (arg: { id: string }) => Promise<{ errors; newTransactions; matchedTransactions; diff --git a/upcoming-release-notes/2416.md b/upcoming-release-notes/2416.md new file mode 100644 index 00000000000..eb28989aa24 --- /dev/null +++ b/upcoming-release-notes/2416.md @@ -0,0 +1,6 @@ +--- +category: Enhancements +authors: [MatissJanis] +--- + +Bank sync quality of life improvements: show "pending" status on accounts, progressively import new transactions instead of waiting for all account sync to finish before adding them to the ledger.