From 0f41e9595245ad0a1e4cc22e2157a25f69d85a75 Mon Sep 17 00:00:00 2001
From: Ryan Bianchi <1435081+qedi-r@users.noreply.github.com>
Date: Wed, 25 Sep 2024 18:40:07 -0400
Subject: [PATCH] Fix Issue 3331, sort favorite payees before other frequently
used payees (#3412)
* add tests for payee dropdown bug on new transactions on all accounts page
* add tests for bugfixes
* sort favorite payees first, add tests for PayeeAutocomplete.getPayeeSuggestions
* lint and release notes
* fix release note number
* lint fixes
* add missing file in previous lint fix
* fix typecheck and linting errors
---------
Co-authored-by: youngcw <calebyoung94@gmail.com>
---
.../autocomplete/PayeeAutocomplete.test.tsx | 239 ++++++++++++++++++
.../autocomplete/PayeeAutocomplete.tsx | 33 ++-
.../transactions/TransactionsTable.test.jsx | 113 ++++++++-
packages/loot-core/src/mocks/index.ts | 30 ++-
upcoming-release-notes/3412.md | 6 +
5 files changed, 400 insertions(+), 21 deletions(-)
create mode 100644 packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.test.tsx
create mode 100644 upcoming-release-notes/3412.md
diff --git a/packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.test.tsx b/packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.test.tsx
new file mode 100644
index 000000000..0bc3f317b
--- /dev/null
+++ b/packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.test.tsx
@@ -0,0 +1,239 @@
+import { render, type Screen, screen } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+import { vi } from 'vitest';
+
+import { generateAccount } from 'loot-core/src/mocks';
+import { TestProvider } from 'loot-core/src/mocks/redux';
+import type { AccountEntity, PayeeEntity } from 'loot-core/types/models';
+
+import { useCommonPayees } from '../../hooks/usePayees';
+import { ResponsiveProvider } from '../../ResponsiveProvider';
+
+import {
+ PayeeAutocomplete,
+ type PayeeAutocompleteItem,
+ type PayeeAutocompleteProps,
+} from './PayeeAutocomplete';
+
+const PAYEE_SELECTOR = '[data-testid][role=option]';
+const PAYEE_SECTION_SELECTOR = '[data-testid$="-item-group"]';
+
+const payees = [
+ makePayee('Bob', { favorite: true }),
+ makePayee('Alice', { favorite: true }),
+ makePayee('This guy on the side of the road'),
+];
+
+const accounts: AccountEntity[] = [
+ generateAccount('Bank of Montreal', false, false),
+];
+const defaultProps = {
+ value: null,
+ embedded: true,
+ payees,
+ accounts,
+};
+
+function makePayee(name: string, options?: { favorite: boolean }): PayeeEntity {
+ return {
+ id: name.toLowerCase() + '-id',
+ name,
+ favorite: options?.favorite ?? false,
+ transfer_acct: undefined,
+ };
+}
+
+function extractPayeesAndHeaderNames(screen: Screen) {
+ return [
+ ...screen
+ .getByTestId('autocomplete')
+ .querySelectorAll(`${PAYEE_SELECTOR}, ${PAYEE_SECTION_SELECTOR}`),
+ ]
+ .map(e => e.getAttribute('data-testid'))
+ .map(firstOrIncorrect);
+}
+
+function renderPayeeAutocomplete(
+ props?: Partial<PayeeAutocompleteProps>,
+): HTMLElement {
+ const autocompleteProps = {
+ ...defaultProps,
+ ...props,
+ };
+
+ render(
+ <TestProvider>
+ <ResponsiveProvider>
+ <div data-testid="autocomplete-test">
+ <PayeeAutocomplete
+ {...autocompleteProps}
+ onSelect={vi.fn()}
+ type="single"
+ value={null}
+ embedded={false}
+ />
+ </div>
+ </ResponsiveProvider>
+ </TestProvider>,
+ );
+ return screen.getByTestId('autocomplete-test');
+}
+
+// Not good, see `Autocomplete.js` for details
+function waitForAutocomplete() {
+ return new Promise(resolve => setTimeout(resolve, 0));
+}
+
+async function clickAutocomplete(autocomplete: HTMLElement) {
+ const input = autocomplete.querySelector(`input`);
+ if (input != null) {
+ await userEvent.click(input);
+ }
+ await waitForAutocomplete();
+}
+
+vi.mock('../../hooks/usePayees', () => ({
+ useCommonPayees: vi.fn(),
+ usePayees: vi.fn().mockReturnValue([]),
+}));
+
+function firstOrIncorrect(id: string | null): string {
+ return id?.split('-', 1)[0] || 'incorrect';
+}
+
+describe('PayeeAutocomplete.getPayeeSuggestions', () => {
+ beforeEach(() => {
+ vi.mocked(useCommonPayees).mockReturnValue([]);
+ });
+
+ test('favorites get sorted alphabetically', async () => {
+ const autocomplete = renderPayeeAutocomplete();
+ await clickAutocomplete(autocomplete);
+
+ expect(
+ [
+ ...screen.getByTestId('autocomplete').querySelectorAll(PAYEE_SELECTOR),
+ ].map(e => e.getAttribute('data-testid')),
+ ).toStrictEqual([
+ 'Alice-payee-item',
+ 'Bob-payee-item',
+ 'This guy on the side of the road-payee-item',
+ ]);
+ });
+
+ test('list with less than the maximum favorites adds common payees', async () => {
+ //Note that the payees list assumes the payees are already sorted
+ const payees: PayeeAutocompleteItem[] = [
+ makePayee('Alice'),
+ makePayee('Bob'),
+ makePayee('Eve', { favorite: true }),
+ makePayee('Bruce'),
+ makePayee('Carol'),
+ makePayee('Natasha'),
+ makePayee('Steve'),
+ makePayee('Tony'),
+ ];
+ vi.mocked(useCommonPayees).mockReturnValue([
+ makePayee('Bruce'),
+ makePayee('Natasha'),
+ makePayee('Steve'),
+ makePayee('Tony'),
+ makePayee('Carol'),
+ ]);
+ const expectedPayeeOrder = [
+ 'Suggested Payees',
+ 'Eve',
+ 'Bruce',
+ 'Natasha',
+ 'Steve',
+ 'Tony',
+ 'Payees',
+ 'Alice',
+ 'Bob',
+ 'Carol',
+ ];
+ await clickAutocomplete(renderPayeeAutocomplete({ payees }));
+
+ expect(
+ [
+ ...screen
+ .getByTestId('autocomplete')
+ .querySelectorAll(`${PAYEE_SELECTOR}, ${PAYEE_SECTION_SELECTOR}`),
+ ]
+ .map(e => e.getAttribute('data-testid'))
+ .map(firstOrIncorrect),
+ ).toStrictEqual(expectedPayeeOrder);
+ });
+
+ test('list with more than the maximum favorites only lists favorites', async () => {
+ //Note that the payees list assumes the payees are already sorted
+ const payees = [
+ makePayee('Alice', { favorite: true }),
+ makePayee('Bob', { favorite: true }),
+ makePayee('Eve', { favorite: true }),
+ makePayee('Bruce', { favorite: true }),
+ makePayee('Carol', { favorite: true }),
+ makePayee('Natasha'),
+ makePayee('Steve'),
+ makePayee('Tony', { favorite: true }),
+ ];
+ vi.mocked(useCommonPayees).mockReturnValue([
+ makePayee('Bruce'),
+ makePayee('Natasha'),
+ makePayee('Steve'),
+ makePayee('Tony'),
+ makePayee('Carol'),
+ ]);
+ const expectedPayeeOrder = [
+ 'Suggested Payees',
+ 'Alice',
+ 'Bob',
+ 'Bruce',
+ 'Carol',
+ 'Eve',
+ 'Tony',
+ 'Payees',
+ 'Natasha',
+ 'Steve',
+ ];
+ const autocomplete = renderPayeeAutocomplete({ payees });
+ await clickAutocomplete(autocomplete);
+
+ expect(extractPayeesAndHeaderNames(screen)).toStrictEqual(
+ expectedPayeeOrder,
+ );
+ });
+
+ test('list with no favorites shows just the payees list', async () => {
+ //Note that the payees list assumes the payees are already sorted
+ const payees = [
+ makePayee('Alice'),
+ makePayee('Bob'),
+ makePayee('Eve'),
+ makePayee('Natasha'),
+ makePayee('Steve'),
+ ];
+ const expectedPayeeOrder = ['Alice', 'Bob', 'Eve', 'Natasha', 'Steve'];
+ const autocomplete = renderPayeeAutocomplete({ payees });
+ await clickAutocomplete(autocomplete);
+
+ expect(
+ [
+ ...screen
+ .getByTestId('autocomplete')
+ .querySelectorAll('[data-testid][role=option]'),
+ ]
+ .map(e => e.getAttribute('data-testid'))
+ .flatMap(firstOrIncorrect),
+ ).toStrictEqual(expectedPayeeOrder);
+ expect(
+ [
+ ...screen
+ .getByTestId('autocomplete')
+ .querySelectorAll('[data-testid$="-item-group"]'),
+ ]
+ .map(e => e.getAttribute('data-testid'))
+ .flatMap(firstOrIncorrect),
+ ).toStrictEqual(['Payees']);
+ });
+});
diff --git a/packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx b/packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx
index 55b452ad5..ea4d47913 100644
--- a/packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx
+++ b/packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx
@@ -39,7 +39,7 @@ import {
} from './Autocomplete';
import { ItemHeader } from './ItemHeader';
-type PayeeAutocompleteItem = PayeeEntity;
+export type PayeeAutocompleteItem = PayeeEntity;
const MAX_AUTO_SUGGESTIONS = 5;
@@ -47,30 +47,37 @@ function getPayeeSuggestions(
commonPayees: PayeeAutocompleteItem[],
payees: PayeeAutocompleteItem[],
): (PayeeAutocompleteItem & PayeeItemType)[] {
+ const favoritePayees = payees
+ .filter(p => p.favorite)
+ .map(p => {
+ return { ...p, itemType: determineItemType(p, true) };
+ })
+ .sort((a, b) => a.name.localeCompare(b.name));
+
+ let additionalCommonPayees: (PayeeAutocompleteItem & PayeeItemType)[] = [];
if (commonPayees?.length > 0) {
- const favoritePayees = payees.filter(p => p.favorite);
- let additionalCommonPayees: PayeeAutocompleteItem[] = [];
if (favoritePayees.length < MAX_AUTO_SUGGESTIONS) {
additionalCommonPayees = commonPayees
.filter(
p => !(p.favorite || favoritePayees.map(fp => fp.id).includes(p.id)),
)
- .slice(0, MAX_AUTO_SUGGESTIONS - favoritePayees.length);
+ .slice(0, MAX_AUTO_SUGGESTIONS - favoritePayees.length)
+ .map(p => {
+ return { ...p, itemType: determineItemType(p, true) };
+ })
+ .sort((a, b) => a.name.localeCompare(b.name));
}
- const frequentPayees: (PayeeAutocompleteItem & PayeeItemType)[] =
- favoritePayees.concat(additionalCommonPayees).map(p => {
- return { ...p, itemType: 'common_payee' };
- });
+ }
+ if (favoritePayees.length + additionalCommonPayees.length) {
const filteredPayees: (PayeeAutocompleteItem & PayeeItemType)[] = payees
- .filter(p => !frequentPayees.find(fp => fp.id === p.id))
+ .filter(p => !favoritePayees.find(fp => fp.id === p.id))
+ .filter(p => !additionalCommonPayees.find(fp => fp.id === p.id))
.map<PayeeAutocompleteItem & PayeeItemType>(p => {
return { ...p, itemType: determineItemType(p, false) };
});
- return frequentPayees
- .sort((a, b) => a.name.localeCompare(b.name))
- .concat(filteredPayees);
+ return favoritePayees.concat(additionalCommonPayees).concat(filteredPayees);
}
return payees.map(p => {
@@ -245,7 +252,7 @@ function PayeeList({
);
}
-type PayeeAutocompleteProps = ComponentProps<
+export type PayeeAutocompleteProps = ComponentProps<
typeof Autocomplete<PayeeAutocompleteItem>
> & {
showMakeTransfer?: boolean;
diff --git a/packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx b/packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx
index c9abf7956..a9644f977 100644
--- a/packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx
+++ b/packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx
@@ -37,8 +37,27 @@ vi.mock('../../hooks/useSyncedPref', () => ({
const accounts = [generateAccount('Bank of America')];
const payees = [
- { id: 'payed-to', favorite: true, name: 'Payed To' },
- { id: 'guy', favorite: false, name: 'This guy on the side of the road' },
+ {
+ id: 'bob-id',
+ name: 'Bob',
+ favorite: true,
+ transfer_acct: null,
+ category: null,
+ },
+ {
+ id: 'alice-id',
+ name: 'Alice',
+ favorite: true,
+ transfer_acct: null,
+ category: null,
+ },
+ {
+ id: 'guy',
+ favorite: false,
+ transfer_acct: null,
+ category: null,
+ name: 'This guy on the side of the road',
+ },
];
const categoryGroups = generateCategoryGroups([
{
@@ -67,6 +86,7 @@ function generateTransactions(count, splitAtIndexes = [], showError = false) {
generateTransaction(
{
account: accounts[0].id,
+ payee: 'alice-id',
category:
i === 0
? null
@@ -284,6 +304,41 @@ function editField(container, name, rowIndex) {
return _editField(field, container);
}
+expect.extend({
+ payeesToHaveFavoriteStars(container, validPayeeListWithFavorite) {
+ const incorrectStarList = [];
+ const foundStarList = [];
+ validPayeeListWithFavorite.forEach(payeeItem => {
+ const shouldHaveFavorite = payeeItem != null;
+ let found = false;
+ if (container[0].querySelectorAll('svg').length === 1) {
+ found = true;
+ foundStarList.push(payeeItem);
+ }
+ if (shouldHaveFavorite !== found) {
+ incorrectStarList.push(payeeItem);
+ }
+ });
+ if (
+ foundStarList.length !== validPayeeListWithFavorite.length ||
+ incorrectStarList.length > 0
+ ) {
+ return {
+ message: () =>
+ `Expected ${validPayeeListWithFavorite.join(', ')} to have favorite stars.` +
+ `Received ${foundStarList.length} items with favorite stars. Incorrect: ${incorrectStarList.join(', ')}`,
+ pass: false,
+ };
+ } else {
+ return {
+ message: () =>
+ `Expected ${validPayeeListWithFavorite} to have favorite stars`,
+ pass: true,
+ };
+ }
+ },
+});
+
function expectToBeEditingField(container, name, rowIndex, isNew) {
let field;
if (isNew) {
@@ -590,6 +645,54 @@ describe('Transactions', () => {
);
});
+ test('dropdown payee displays on new transaction with account list column', async () => {
+ const { container, updateProps, queryByTestId } = renderTransactions({
+ currentAccountId: null,
+ });
+ updateProps({ isAdding: true });
+ expect(queryByTestId('new-transaction')).toBeTruthy();
+
+ await editNewField(container, 'payee');
+
+ const renderedPayees = screen
+ .getByTestId('autocomplete')
+ .querySelectorAll('[data-testid$="payee-item"]');
+
+ expect(
+ Array.from(renderedPayees.values()).map(p =>
+ p.getAttribute('data-testid'),
+ ),
+ ).toStrictEqual([
+ 'Alice-payee-item',
+ 'Bob-payee-item',
+ 'This guy on the side of the road-payee-item',
+ ]);
+ expect(renderedPayees).payeesToHaveFavoriteStars([
+ 'Alice-payee-item',
+ 'Bob-payee-item',
+ ]);
+ });
+
+ test('dropdown payee displays on existing non-transfer transaction', async () => {
+ const { container } = renderTransactions();
+
+ await editField(container, 'payee', 2);
+
+ const renderedPayees = screen
+ .getByTestId('autocomplete')
+ .querySelectorAll('[data-testid$="payee-item"]');
+
+ expect(
+ Array.from(renderedPayees.values()).map(p =>
+ p.getAttribute('data-testid'),
+ ),
+ ).toStrictEqual([
+ 'Alice-payee-item',
+ 'Bob-payee-item',
+ 'This guy on the side of the road-payee-item',
+ ]);
+ });
+
// TODO: fix this test
test.skip('dropdown invalid value resets correctly', async () => {
const { container, getTransactions } = renderTransactions();
@@ -866,7 +969,7 @@ describe('Transactions', () => {
id: expect.any(String),
is_parent: true,
notes: 'Notes',
- payee: 'payed-to',
+ payee: 'alice-id',
sort_order: 0,
},
{
@@ -879,7 +982,7 @@ describe('Transactions', () => {
id: expect.any(String),
is_child: true,
parent_id: parentId,
- payee: 'payed-to',
+ payee: 'alice-id',
sort_order: -1,
starting_balance_flag: null,
},
@@ -893,7 +996,7 @@ describe('Transactions', () => {
id: expect.any(String),
is_child: true,
parent_id: parentId,
- payee: 'payed-to',
+ payee: 'alice-id',
sort_order: -2,
starting_balance_flag: null,
},
diff --git a/packages/loot-core/src/mocks/index.ts b/packages/loot-core/src/mocks/index.ts
index e42823ed7..e04cbb7db 100644
--- a/packages/loot-core/src/mocks/index.ts
+++ b/packages/loot-core/src/mocks/index.ts
@@ -2,20 +2,44 @@
import { v4 as uuidv4 } from 'uuid';
import * as monthUtils from '../shared/months';
-import type { TransactionEntity } from '../types/models';
+import type {
+ _SyncFields,
+ AccountEntity,
+ TransactionEntity,
+} from '../types/models';
import { random } from './random';
-export function generateAccount(name, isConnected, offbudget) {
+export function generateAccount(
+ name,
+ isConnected,
+ offbudget,
+): AccountEntity & { bankId: number; bankName: string } {
return {
id: uuidv4(),
name,
balance_current: isConnected ? Math.floor(random() * 100000) : null,
- bank: isConnected ? Math.floor(random() * 10000) : null,
bankId: isConnected ? Math.floor(random() * 10000) : null,
bankName: isConnected ? 'boa' : null,
+ bank: isConnected ? Math.floor(random() * 10000).toString() : null,
offbudget: offbudget ? 1 : 0,
+ sort_order: 0,
+ tombstone: 0,
closed: 0,
+ ...emptySyncFields(),
+ };
+}
+
+function emptySyncFields(): _SyncFields<false> {
+ return {
+ account_id: null,
+ bank: null,
+ mask: null,
+ official_name: null,
+ balance_current: null,
+ balance_available: null,
+ balance_limit: null,
+ account_sync_source: null,
};
}
diff --git a/upcoming-release-notes/3412.md b/upcoming-release-notes/3412.md
new file mode 100644
index 000000000..7647c9a52
--- /dev/null
+++ b/upcoming-release-notes/3412.md
@@ -0,0 +1,6 @@
+---
+category: Bugfix
+authors: [qedi-r]
+---
+
+Sort suggested payee popup section by favorite status first, then alphabetically.
--
GitLab