Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

useTransactions hook to simplify loading of transactions #3685

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,12 @@ module.exports = {
],
'no-with': 'warn',
'no-whitespace-before-property': 'warn',
'react-hooks/exhaustive-deps': 'warn',
'react-hooks/exhaustive-deps': [
'warn',
{
additionalHooks: '(useQuery)',
},
],
joel-jeremy marked this conversation as resolved.
Show resolved Hide resolved
'require-yield': 'warn',
'rest-spread-spacing': ['warn', 'never'],
strict: ['warn', 'never'],
Expand Down
7 changes: 7 additions & 0 deletions packages/desktop-client/e2e/accounts.mobile.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ test.describe('Mobile Accounts', () => {

test('opens the accounts page and asserts on balances', async () => {
const accountsPage = await navigation.goToAccountsPage();
await accountsPage.waitFor();

const account = await accountsPage.getNthAccount(1);

Expand All @@ -37,7 +38,10 @@ test.describe('Mobile Accounts', () => {

test('opens individual account page and checks that filtering is working', async () => {
const accountsPage = await navigation.goToAccountsPage();
await accountsPage.waitFor();

const accountPage = await accountsPage.openNthAccount(0);
await accountPage.waitFor();

await expect(accountPage.heading).toHaveText('Bank of America');
await expect(accountPage.transactionList).toBeVisible();
Expand All @@ -50,6 +54,9 @@ test.describe('Mobile Accounts', () => {
await expect(accountPage.transactions).toHaveCount(0);
await expect(page).toMatchThemeScreenshots();

await accountPage.clearSearch();
await expect(accountPage.transactions).not.toHaveCount(0);

await accountPage.searchByText('Kroger');
await expect(accountPage.transactions).not.toHaveCount(0);
await expect(page).toMatchThemeScreenshots();
Expand Down
3 changes: 3 additions & 0 deletions packages/desktop-client/e2e/accounts.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ test.describe('Accounts', () => {

test('creates a transfer from two existing transactions', async () => {
accountPage = await navigation.goToAccountPage('For budget');
await accountPage.waitFor();

joel-jeremy marked this conversation as resolved.
Show resolved Hide resolved
await expect(accountPage.accountName).toHaveText('Budgeted Accounts');

await accountPage.filterByNote('Test Acc Transfer');
Expand Down Expand Up @@ -109,6 +111,7 @@ test.describe('Accounts', () => {
offBudget: false,
balance: 0,
});
await accountPage.waitFor();
});

async function importCsv(screenshot = false) {
Expand Down
4 changes: 4 additions & 0 deletions packages/desktop-client/e2e/page-models/account-page.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ export class AccountPage {
this.selectTooltip = this.page.getByTestId('transactions-select-tooltip');
}

async waitFor() {
await this.transactionTable.waitFor();
}
joel-jeremy marked this conversation as resolved.
Show resolved Hide resolved

/**
* Enter details of a transaction
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ export class MobileAccountPage {
});
}

async waitFor() {
await this.transactionList.waitFor();
}

/**
* Retrieve the balance of the account as a number
*/
Expand All @@ -29,6 +33,10 @@ export class MobileAccountPage {
await this.searchBox.fill(term);
}

async clearSearch() {
await this.searchBox.clear();
}

/**
* Go to transaction creation page
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,14 @@ export class MobileAccountsPage {
constructor(page) {
this.page = page;

this.accountList = this.page.getByLabel('Account list');
this.accounts = this.page.getByTestId('account');
}

async waitFor() {
await this.accountList.waitFor();
}

/**
* Get the name and balance of the nth account
*/
Expand Down
7 changes: 5 additions & 2 deletions packages/desktop-client/src/components/ManageRules.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import React, {
} from 'react';
import { useDispatch } from 'react-redux';

import { useSchedules } from 'loot-core/client/data-hooks/schedules';
import { q } from 'loot-core/shared/query';
import { pushModal } from 'loot-core/src/client/actions/modals';
import { initiallyLoadPayees } from 'loot-core/src/client/actions/queries';
import { send } from 'loot-core/src/platform/client/fetch';
Expand All @@ -21,7 +23,6 @@ import { type NewRuleEntity } from 'loot-core/src/types/models';
import { useAccounts } from '../hooks/useAccounts';
import { useCategories } from '../hooks/useCategories';
import { usePayees } from '../hooks/usePayees';
import { useSchedules } from '../hooks/useSchedules';
import { useSelected, SelectedProvider } from '../hooks/useSelected';
import { theme } from '../style';

Expand Down Expand Up @@ -113,7 +114,9 @@ export function ManageRules({
const [filter, setFilter] = useState('');
const dispatch = useDispatch();

const { data: schedules = [] } = useSchedules();
const { schedules = [] } = useSchedules({
query: useMemo(() => q('schedules').select('*'), []),
});
const { list: categories } = useCategories();
const payees = usePayees();
const accounts = useAccounts();
Expand Down
84 changes: 39 additions & 45 deletions packages/desktop-client/src/components/accounts/Account.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,14 @@ import { type UndoState } from 'loot-core/server/undo';
import { useFilters } from 'loot-core/src/client/data-hooks/filters';
import {
SchedulesProvider,
useDefaultSchedulesQueryTransform,
accountSchedulesQuery,
} from 'loot-core/src/client/data-hooks/schedules';
import * as queries from 'loot-core/src/client/queries';
import { runQuery, pagedQuery } from 'loot-core/src/client/query-helpers';
import {
runQuery,
pagedQuery,
type PagedQuery,
} from 'loot-core/src/client/query-helpers';
import { send, listen } from 'loot-core/src/platform/client/fetch';
import { currentDay } from 'loot-core/src/shared/months';
import { q, type Query } from 'loot-core/src/shared/query';
Expand Down Expand Up @@ -212,7 +216,7 @@ function AllTransactions({
return balances;
}, [filtered, prependBalances, balances]);

if (!prependTransactions) {
if (!prependTransactions?.length || filtered) {
return children(transactions, balances);
}
return children(allTransactions, allBalances);
Expand Down Expand Up @@ -240,7 +244,7 @@ function getField(field?: string) {
}

type AccountInternalProps = {
accountId?: string;
accountId?: AccountEntity['id'] | 'budgeted' | 'offbudget' | 'uncategorized';
filterConditions: RuleConditionEntity[];
showBalances?: boolean;
setShowBalances: (newValue: boolean) => void;
Expand Down Expand Up @@ -322,7 +326,7 @@ class AccountInternal extends PureComponent<
AccountInternalProps,
AccountInternalState
> {
paged: ReturnType<typeof pagedQuery> | null;
paged: PagedQuery<TransactionEntity> | null;
rootQuery: Query;
currentQuery: Query;
table: TableRef;
Expand Down Expand Up @@ -457,7 +461,7 @@ class AccountInternal extends PureComponent<
}

fetchAllIds = async () => {
const { data } = await runQuery(this.paged?.getQuery().select('id'));
const { data } = await runQuery(this.paged?.query.select('id'));
joel-jeremy marked this conversation as resolved.
Show resolved Hide resolved
// Remember, this is the `grouped` split type so we need to deal
// with the `subtransactions` property
return data.reduce((arr: string[], t: TransactionEntity) => {
Expand All @@ -472,7 +476,7 @@ class AccountInternal extends PureComponent<
};

fetchTransactions = (filterConditions?: ConditionEntity[]) => {
const query = this.makeRootQuery();
const query = this.makeRootTransactionsQuery();
this.rootQuery = this.currentQuery = query;
if (filterConditions) this.applyFilters(filterConditions);
else this.updateQuery(query);
Expand All @@ -482,10 +486,10 @@ class AccountInternal extends PureComponent<
}
};

makeRootQuery = () => {
makeRootTransactionsQuery = () => {
const accountId = this.props.accountId;

return queries.makeTransactionsQuery(accountId);
return queries.transactions(accountId);
joel-jeremy marked this conversation as resolved.
Show resolved Hide resolved
};

updateQuery(query: Query, isFiltered: boolean = false) {
Expand All @@ -502,12 +506,9 @@ class AccountInternal extends PureComponent<
query = query.filter({ reconciled: { $eq: false } });
}

this.paged = pagedQuery(
query.select('*'),
async (
data: TransactionEntity[],
prevData: TransactionEntity[] | null,
) => {
this.paged = pagedQuery(query.select('*'), {
onData: async (groupedData, prevData) => {
const data = ungroupTransactions([...groupedData]);
const firstLoad = prevData == null;

if (firstLoad) {
Expand All @@ -529,7 +530,7 @@ class AccountInternal extends PureComponent<
this.setState(
{
transactions: data,
transactionCount: this.paged?.getTotalCount(),
transactionCount: this.paged?.totalCount,
joel-jeremy marked this conversation as resolved.
Show resolved Hide resolved
transactionsFiltered: isFiltered,
loading: false,
workingHard: false,
Expand All @@ -549,12 +550,11 @@ class AccountInternal extends PureComponent<
},
);
},
{
options: {
pageCount: 150,
onlySync: true,
mapper: ungroupTransactions,
},
);
});
}

UNSAFE_componentWillReceiveProps(nextProps: AccountInternalProps) {
Expand Down Expand Up @@ -590,7 +590,7 @@ class AccountInternal extends PureComponent<
);
} else {
this.updateQuery(
queries.makeTransactionSearchQuery(
queries.transactionsSearch(
this.currentQuery,
this.state.search,
this.props.dateFormat,
Expand Down Expand Up @@ -652,27 +652,19 @@ class AccountInternal extends PureComponent<
);
};

onTransactionsChange = (
newTransaction: TransactionEntity,
data: TransactionEntity[],
) => {
onTransactionsChange = (updatedTransaction: TransactionEntity) => {
// Apply changes to pagedQuery data
this.paged?.optimisticUpdate(
(data: TransactionEntity[]) => {
if (newTransaction._deleted) {
return data.filter(t => t.id !== newTransaction.id);
} else {
return data.map(t => {
return t.id === newTransaction.id ? newTransaction : t;
});
}
},
() => {
return data;
},
);
this.paged?.optimisticUpdate(data => {
if (updatedTransaction._deleted) {
return data.filter(t => t.id !== updatedTransaction.id);
} else {
return data.map(t => {
return t.id === updatedTransaction.id ? updatedTransaction : t;
});
}
});
joel-jeremy marked this conversation as resolved.
Show resolved Hide resolved

this.props.updateNewTransactions(newTransaction.id);
this.props.updateNewTransactions(updatedTransaction.id);
};

canCalculateBalance = () => {
Expand All @@ -696,8 +688,7 @@ class AccountInternal extends PureComponent<
}

const { data } = await runQuery(
this.paged
?.getQuery()
this.paged?.query
joel-jeremy marked this conversation as resolved.
Show resolved Hide resolved
.options({ splits: 'none' })
.select([{ balance: { $sumOver: '$amount' } }]),
);
Expand Down Expand Up @@ -862,13 +853,13 @@ class AccountInternal extends PureComponent<
getBalanceQuery(id?: string) {
return {
name: `balance-query-${id}`,
query: this.makeRootQuery().calculate({ $sum: '$amount' }),
query: this.makeRootTransactionsQuery().calculate({ $sum: '$amount' }),
} as const;
}

getFilteredAmount = async () => {
const { data: amount } = await runQuery(
this.paged?.getQuery().calculate({ $sum: '$amount' }),
this.paged?.query.calculate({ $sum: '$amount' }),
joel-jeremy marked this conversation as resolved.
Show resolved Hide resolved
);
return amount;
};
Expand Down Expand Up @@ -1887,10 +1878,13 @@ export function Account() {
const savedFiters = useFilters();
const actionCreators = useActions();

const transform = useDefaultSchedulesQueryTransform(params.id);
const schedulesQuery = useMemo(
() => accountSchedulesQuery(params.id),
[params.id],
);
Comment on lines +1881 to +1884
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle potential undefined params.id

The accountSchedulesQuery is called with params.id which could be undefined, potentially causing runtime errors.

Apply this diff to handle the undefined case:

  const schedulesQuery = useMemo(
-   () => accountSchedulesQuery(params.id),
+   () => accountSchedulesQuery(params.id || ''),
    [params.id],
  );

Committable suggestion skipped: line range outside the PR's diff.


return (
<SchedulesProvider transform={transform}>
<SchedulesProvider query={schedulesQuery}>
<SplitsExpandedProvider
initialMode={expandSplits ? 'collapse' : 'expand'}
>
Expand Down
9 changes: 7 additions & 2 deletions packages/desktop-client/src/components/accounts/Balance.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,13 @@ function SelectedBalance({ selectedItems, account }) {
});

let scheduleBalance = null;
const scheduleData = useCachedSchedules();
const schedules = scheduleData ? scheduleData.schedules : [];

const { isLoading, schedules = [] } = useCachedSchedules();

if (isLoading) {
return null;
}

const previewIds = [...selectedItems]
.filter(id => isPreviewId(id))
.map(id => id.slice(8));
Expand Down
Loading