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>

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']);
+  });
+});

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;

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,
       },

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,
   };
 }
 

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.