From 0e9f652d84e0dfe543091acba07579ad8e6b79a0 Mon Sep 17 00:00:00 2001 From: Gregor Vostrak Date: Wed, 26 Aug 2026 14:46:00 +0200 Subject: [PATCH] refactor table sorting to composables and shared SortableTableHeaderCell --- e2e/clients.spec.ts | 20 +-- e2e/members.spec.ts | 26 ++-- e2e/projects.spec.ts | 123 +++++++++++++++--- e2e/tags.spec.ts | 12 +- e2e/tasks.spec.ts | 12 +- e2e/utils/table.ts | 11 +- .../Components/Common/Client/ClientTable.vue | 66 +++------- .../Common/Client/ClientTableHeading.vue | 57 +++----- .../Components/Common/Member/MemberTable.vue | 61 +++------ .../Common/Member/MemberTableHeading.vue | 75 ++++------- .../Common/Project/ProjectTable.vue | 70 +++------- .../Common/Project/ProjectTableHeading.vue | 98 +++++--------- .../Common/SortableTableHeaderCell.vue | 62 +++++++++ .../js/Components/Common/Tag/TagTable.vue | 60 +++------ .../Components/Common/Tag/TagTableHeading.vue | 39 +++--- .../js/Components/Common/Task/TaskTable.vue | 70 ++++------ .../Common/Task/TaskTableHeading.vue | 59 +++------ resources/js/Pages/Clients.vue | 27 +--- resources/js/Pages/Members.vue | 27 +--- resources/js/Pages/ProjectShow.vue | 27 +--- resources/js/Pages/Projects.vue | 23 ++-- resources/js/Pages/Tags.vue | 27 +--- resources/js/utils/useSortableTable.ts | 123 ++++++++++++++++++ resources/js/utils/useTableSortState.ts | 32 +++++ 24 files changed, 596 insertions(+), 611 deletions(-) create mode 100644 resources/js/Components/Common/SortableTableHeaderCell.vue create mode 100644 resources/js/utils/useSortableTable.ts create mode 100644 resources/js/utils/useTableSortState.ts diff --git a/e2e/clients.spec.ts b/e2e/clients.spec.ts index 6d8e3d05..c4626589 100644 --- a/e2e/clients.spec.ts +++ b/e2e/clients.spec.ts @@ -8,7 +8,7 @@ import { createProjectViaApi, createPublicProjectViaApi, } from './utils/api'; -import { getTableRowNames } from './utils/table'; +import { clearTableState, getTableRowNames } from './utils/table'; async function goToClientsOverview(page: Page) { await page.goto(PLAYWRIGHT_BASE_URL + '/clients'); @@ -210,18 +210,12 @@ test('test that client context menu delete deletes the client', async ({ page, c // Sorting Tests // ============================================= -async function clearClientTableState(page: Page) { - await page.evaluate(() => { - localStorage.removeItem('client-table-state'); - }); -} - test('test that sorting clients by name and status works', async ({ page, ctx }) => { await createClientViaApi(ctx, { name: 'AAA SortClient' }); await createClientViaApi(ctx, { name: 'ZZZ SortClient' }); await goToClientsOverview(page); - await clearClientTableState(page); + await clearTableState(page, 'client-table-state'); await page.reload(); const table = page.getByTestId('client_table'); @@ -253,7 +247,7 @@ test('test that sorting clients by project count works', async ({ page, ctx }) = await createProjectViaApi(ctx, { name: 'Proj2', client_id: clientWithMany.id }); await goToClientsOverview(page); - await clearClientTableState(page); + await clearTableState(page, 'client-table-state'); await page.reload(); const table = page.getByTestId('client_table'); @@ -274,7 +268,7 @@ test('test that sorting clients by project count works', async ({ page, ctx }) = test('test that client sort state persists after page reload', async ({ page }) => { await goToClientsOverview(page); - await clearClientTableState(page); + await clearTableState(page, 'client-table-state'); await page.reload(); const table = page.getByTestId('client_table'); @@ -397,7 +391,7 @@ test.describe('Clients Pagination', () => { ); await goToClientsOverview(page); - await clearClientTableState(page); + await clearTableState(page, 'client-table-state'); await page.reload(); // Default sort is name asc; first 15 clients (00–14) on page 1. @@ -450,7 +444,7 @@ test.describe('Clients Pagination', () => { ); await goToClientsOverview(page); - await clearClientTableState(page); + await clearTableState(page, 'client-table-state'); await page.reload(); await expect(page.getByTestId('client_table')).toBeVisible(); @@ -470,7 +464,7 @@ test.describe('Clients Pagination', () => { ); await goToClientsOverview(page); - await clearClientTableState(page); + await clearTableState(page, 'client-table-state'); await page.reload(); await expect(page.getByText(prefix + '00')).toBeVisible({ timeout: 10000 }); diff --git a/e2e/members.spec.ts b/e2e/members.spec.ts index 7fc21a9f..7fa13766 100644 --- a/e2e/members.spec.ts +++ b/e2e/members.spec.ts @@ -11,7 +11,7 @@ import { updateMemberBillableRateViaApi, updateOrganizationSettingViaApi, } from './utils/api'; -import { getTableRowNames } from './utils/table'; +import { clearTableState, getTableRowNames } from './utils/table'; // Tests that invite + accept members need more time test.describe.configure({ timeout: 45000 }); @@ -779,20 +779,18 @@ test('test that accepted invitation disappears from invitations tab', async ({ p // Sorting Tests // ============================================= -// Helper to clear localStorage before tests that check sorting -async function clearMemberTableState(page: Page) { - await page.evaluate(() => { - localStorage.removeItem('member-table-state'); - }); -} - test('test that sorting members by name, role, and status works', async ({ page, ctx }) => { - // Create two placeholder members with names that sort predictably around "John Doe" + // Create two placeholder members with names that sort predictably around "John Doe". + // Seeded alphabetically a second apart: created_at only has second precision and + // same-second rows fall back to a random UUID order. The spacing is what makes the + // API order (created_at desc: ZZZ, AAA, John) deterministic, so the tie-break + // assertions below are testing the tie-break rather than a coin flip. await createPlaceholderMemberViaImportApi(ctx, 'AAA SortFirst'); + await page.waitForTimeout(1100); await createPlaceholderMemberViaImportApi(ctx, 'ZZZ SortLast'); await goToMembersPage(page); - await clearMemberTableState(page); + await clearTableState(page, 'member-table-state'); await page.reload(); const table = page.getByTestId('member_table'); @@ -814,20 +812,24 @@ test('test that sorting members by name, role, and status works', async ({ page, const ownerIdx = names.indexOf('John Doe'); const placeholderIdx = names.indexOf('AAA SortFirst'); expect(ownerIdx).toBeLessThan(placeholderIdx); + expect(names.indexOf('AAA SortFirst')).toBeLessThan(names.indexOf('ZZZ SortLast')); await roleHeader.click(); // desc: Placeholder first names = await getTableRowNames(table); expect(names.indexOf('AAA SortFirst')).toBeLessThan(names.indexOf('John Doe')); + expect(names.indexOf('AAA SortFirst')).toBeLessThan(names.indexOf('ZZZ SortLast')); // -- Status sorting -- const statusHeader = table.getByText('Status').first(); await statusHeader.click(); // asc: Active(0) < Inactive(1) names = await getTableRowNames(table); expect(names.indexOf('John Doe')).toBeLessThan(names.indexOf('AAA SortFirst')); + expect(names.indexOf('AAA SortFirst')).toBeLessThan(names.indexOf('ZZZ SortLast')); await statusHeader.click(); // desc: Inactive first names = await getTableRowNames(table); expect(names.indexOf('AAA SortFirst')).toBeLessThan(names.indexOf('John Doe')); + expect(names.indexOf('AAA SortFirst')).toBeLessThan(names.indexOf('ZZZ SortLast')); // -- Email: just verify sort indicator appears -- const emailHeader = table.getByText('Email').first(); @@ -837,7 +839,7 @@ test('test that sorting members by name, role, and status works', async ({ page, test('test that member sort state persists after page reload', async ({ page }) => { await goToMembersPage(page); - await clearMemberTableState(page); + await clearTableState(page, 'member-table-state'); await page.reload(); const table = page.getByTestId('member_table'); @@ -875,7 +877,7 @@ test('test that sorting members by billable rate works', async ({ page, ctx }) = await updateMemberBillableRateViaApi(ctx, lowRateMember!.id, 5000); await goToMembersPage(page); - await clearMemberTableState(page); + await clearTableState(page, 'member-table-state'); await page.reload(); const table = page.getByTestId('member_table'); diff --git a/e2e/projects.spec.ts b/e2e/projects.spec.ts index 0d6a1442..20d41314 100644 --- a/e2e/projects.spec.ts +++ b/e2e/projects.spec.ts @@ -13,18 +13,12 @@ import { archiveProjectViaApi, updateOrganizationSettingViaApi, } from './utils/api'; +import { clearTableState, getTableRowNames } from './utils/table'; async function goToProjectsOverview(page: Page) { await page.goto(PLAYWRIGHT_BASE_URL + '/projects'); } -// Helper to clear localStorage before tests that check persistence -async function clearProjectTableState(page: Page) { - await page.evaluate(() => { - localStorage.removeItem('project-table-state'); - }); -} - // Create new project via modal test('test that creating and deleting a new project via the modal works', async ({ page }) => { const newProjectName = 'New Project ' + Math.floor(1 + Math.random() * 10000); @@ -84,7 +78,7 @@ test('test that archiving and unarchiving projects works', async ({ page, ctx }) await createProjectViaApi(ctx, { name: newProjectName }); await goToProjectsOverview(page); - await clearProjectTableState(page); + await clearTableState(page, 'project-table-state'); await page.reload(); await expect(page.getByText(newProjectName)).toBeVisible({ timeout: 10000 }); @@ -480,7 +474,7 @@ test('test that sorting projects by all columns works', async ({ page, ctx }) => }); await goToProjectsOverview(page); - await clearProjectTableState(page); + await clearTableState(page, 'project-table-state'); await page.reload(); await expect(page.getByTestId('project_table')).toBeVisible(); await expect(page.getByText('AAA Project')).toBeVisible(); @@ -609,7 +603,7 @@ test('test that filtering projects by status works', async ({ page, ctx }) => { await createProjectViaApi(ctx, { name: newProjectName }); await goToProjectsOverview(page); - await clearProjectTableState(page); + await clearTableState(page, 'project-table-state'); await page.reload(); await expect(page.getByText(newProjectName)).toBeVisible({ timeout: 10000 }); @@ -640,7 +634,7 @@ test('test that filtering projects by status works', async ({ page, ctx }) => { test('test that filter state persists after page reload', async ({ page }) => { await goToProjectsOverview(page); - await clearProjectTableState(page); + await clearTableState(page, 'project-table-state'); await page.reload(); // Apply Active status filter @@ -656,9 +650,108 @@ test('test that filter state persists after page reload', async ({ page }) => { await expect(page.getByTestId('status-filter-badge')).toBeVisible(); }); +test('test that projects without a client or estimate are ordered by name at the bottom', async ({ + page, + ctx, +}) => { + // The seeding below has to cross two second boundaries, which eats into the default + // per-test budget. + test.slow(); + + await createProjectViaApi(ctx, { name: 'AAA Tiebreak Project' }); + await page.waitForTimeout(1100); + await createProjectViaApi(ctx, { name: 'BBB Tiebreak Project' }); + await page.waitForTimeout(1100); + await createProjectViaApi(ctx, { name: 'ZZZ Tiebreak Project' }); + + const clientAardvark = await createClientViaApi(ctx, { name: 'Aardvark Co' }); + const clientZulu = await createClientViaApi(ctx, { name: 'Zulu Co' }); + const projectM = await createProjectViaApi(ctx, { + name: 'MMM Tiebreak Project', + client_id: clientAardvark.id, + estimated_time: 36000, // 10h, 1h tracked below = 10% + }); + await createTimeEntryViaApi(ctx, { duration: '1h', projectId: projectM.id }); + const projectN = await createProjectViaApi(ctx, { + name: 'NNN Tiebreak Project', + client_id: clientZulu.id, + estimated_time: 14400, // 4h, 2h tracked below = 50% + }); + await createTimeEntryViaApi(ctx, { duration: '2h', projectId: projectN.id }); + + await goToProjectsOverview(page); + await clearTableState(page, 'project-table-state'); + await page.reload(); + + const table = page.getByTestId('project_table'); + await expect(table).toBeVisible(); + + const seeded = [ + 'AAA Tiebreak Project', + 'BBB Tiebreak Project', + 'MMM Tiebreak Project', + 'NNN Tiebreak Project', + 'ZZZ Tiebreak Project', + ]; + const getOrder = async () => { + const rowNames = await getTableRowNames(table); + return rowNames + .map((rowName) => seeded.find((name) => rowName.includes(name))) + .filter((name): name is string => Boolean(name)); + }; + + // -- Client: empty rows last in both directions, alphabetical among themselves -- + const clientHeader = table.locator('.select-none', { hasText: 'Client' }).first(); + await clientHeader.click(); + await expect + .poll(getOrder) + .toEqual([ + 'MMM Tiebreak Project', + 'NNN Tiebreak Project', + 'AAA Tiebreak Project', + 'BBB Tiebreak Project', + 'ZZZ Tiebreak Project', + ]); + + await clientHeader.click(); + await expect + .poll(getOrder) + .toEqual([ + 'NNN Tiebreak Project', + 'MMM Tiebreak Project', + 'AAA Tiebreak Project', + 'BBB Tiebreak Project', + 'ZZZ Tiebreak Project', + ]); + + // -- Progress: same, and the first click sorts highest first -- + const progressHeader = table.locator('.select-none', { hasText: 'Progress' }).first(); + await progressHeader.click(); + await expect + .poll(getOrder) + .toEqual([ + 'NNN Tiebreak Project', + 'MMM Tiebreak Project', + 'AAA Tiebreak Project', + 'BBB Tiebreak Project', + 'ZZZ Tiebreak Project', + ]); + + await progressHeader.click(); + await expect + .poll(getOrder) + .toEqual([ + 'MMM Tiebreak Project', + 'NNN Tiebreak Project', + 'AAA Tiebreak Project', + 'BBB Tiebreak Project', + 'ZZZ Tiebreak Project', + ]); +}); + test('test that sort state persists after page reload', async ({ page }) => { await goToProjectsOverview(page); - await clearProjectTableState(page); + await clearTableState(page, 'project-table-state'); await page.reload(); // Click on Name header twice to sort descending @@ -1114,7 +1207,7 @@ test.describe('Projects Pagination', () => { ); await goToProjectsOverview(page); - await clearProjectTableState(page); + await clearTableState(page, 'project-table-state'); await page.reload(); // Default sort is name asc; first 15 projects (00–14) should be on page 1. @@ -1168,7 +1261,7 @@ test.describe('Projects Pagination', () => { ); await goToProjectsOverview(page); - await clearProjectTableState(page); + await clearTableState(page, 'project-table-state'); await page.reload(); await expect(page.getByTestId('project_table')).toBeVisible(); @@ -1185,7 +1278,7 @@ test.describe('Projects Pagination', () => { ); await goToProjectsOverview(page); - await clearProjectTableState(page); + await clearTableState(page, 'project-table-state'); await page.reload(); await expect(page.getByText(prefix + '00')).toBeVisible({ timeout: 10000 }); diff --git a/e2e/tags.spec.ts b/e2e/tags.spec.ts index 85c015f6..8748bc05 100644 --- a/e2e/tags.spec.ts +++ b/e2e/tags.spec.ts @@ -3,7 +3,7 @@ import type { Page } from '@playwright/test'; import { PLAYWRIGHT_BASE_URL } from '../playwright/config'; import { test } from '../playwright/fixtures'; import { createTagViaApi } from './utils/api'; -import { getTableRowNames } from './utils/table'; +import { clearTableState, getTableRowNames } from './utils/table'; async function goToTagsOverview(page: Page) { await page.goto(PLAYWRIGHT_BASE_URL + '/tags'); @@ -147,18 +147,12 @@ test('test that tag context menu delete deletes the tag', async ({ page, ctx }) // Sorting Tests // ============================================= -async function clearTagTableState(page: Page) { - await page.evaluate(() => { - localStorage.removeItem('tag-table-state'); - }); -} - test('test that sorting tags by name works', async ({ page, ctx }) => { await createTagViaApi(ctx, { name: 'AAA SortTag' }); await createTagViaApi(ctx, { name: 'ZZZ SortTag' }); await goToTagsOverview(page); - await clearTagTableState(page); + await clearTableState(page, 'tag-table-state'); await page.reload(); const table = page.getByTestId('tag_table'); @@ -176,7 +170,7 @@ test('test that sorting tags by name works', async ({ page, ctx }) => { test('test that tag sort state persists after page reload', async ({ page }) => { await goToTagsOverview(page); - await clearTagTableState(page); + await clearTableState(page, 'tag-table-state'); await page.reload(); const table = page.getByTestId('tag_table'); diff --git a/e2e/tasks.spec.ts b/e2e/tasks.spec.ts index 86855a15..8e92c164 100644 --- a/e2e/tasks.spec.ts +++ b/e2e/tasks.spec.ts @@ -11,18 +11,12 @@ import { updateOrganizationSettingViaApi, type TestContext, } from './utils/api'; -import { getTableRowNames } from './utils/table'; +import { clearTableState, getTableRowNames } from './utils/table'; async function goToProjectsOverview(page: Page) { await page.goto(PLAYWRIGHT_BASE_URL + '/projects'); } -async function clearTaskTableState(page: Page) { - await page.evaluate(() => { - localStorage.removeItem('task-table-state'); - }); -} - async function createSortableTasks(ctx: TestContext) { const project = await createProjectViaApi(ctx, { name: 'Task Sorting Project' }); const taskA = await createTaskViaApi(ctx, { @@ -357,7 +351,7 @@ test('test that creating a new project from the task create modal project dropdo test('test that sorting tasks by name, total time and progress works', async ({ page, ctx }) => { const { project, taskA, taskB, taskC } = await createSortableTasks(ctx); await goToProjectsOverview(page); - await clearTaskTableState(page); + await clearTableState(page, 'task-table-state'); await page.goto(PLAYWRIGHT_BASE_URL + '/projects/' + project.id); const table = page.getByTestId('task_table'); await expect(table).toBeVisible(); @@ -390,7 +384,7 @@ test('test that sorting tasks by name, total time and progress works', async ({ test('test that task sort state persists after page reload', async ({ page, ctx }) => { const { project, taskA, taskB, taskC } = await createSortableTasks(ctx); await goToProjectsOverview(page); - await clearTaskTableState(page); + await clearTableState(page, 'task-table-state'); await page.goto(PLAYWRIGHT_BASE_URL + '/projects/' + project.id); const table = page.getByTestId('task_table'); await expect(table).toBeVisible(); diff --git a/e2e/utils/table.ts b/e2e/utils/table.ts index e35626bc..6af1f8a9 100644 --- a/e2e/utils/table.ts +++ b/e2e/utils/table.ts @@ -1,4 +1,4 @@ -import type { Locator } from '@playwright/test'; +import type { Locator, Page } from '@playwright/test'; /** * Extract the first cell's text content from each row in a table. @@ -14,3 +14,12 @@ export async function getTableRowNames(table: Locator): Promise { } return names; } + +/** + * Drop a table's persisted sort/filter state so a test starts from the defaults. + */ +export async function clearTableState(page: Page, key: string) { + await page.evaluate((storageKey) => { + localStorage.removeItem(storageKey); + }, key); +} diff --git a/resources/js/Components/Common/Client/ClientTable.vue b/resources/js/Components/Common/Client/ClientTable.vue index fe0cf50c..f7153491 100644 --- a/resources/js/Components/Common/Client/ClientTable.vue +++ b/resources/js/Components/Common/Client/ClientTable.vue @@ -11,14 +11,13 @@ import Pagination from '@/Components/Common/Pagination.vue'; import { canCreateClients } from '@/utils/permissions'; import { useProjectsQuery } from '@/utils/useProjectsQuery'; import { - useVueTable, - getCoreRowModel, - getSortedRowModel, - type SortingState, -} from '@tanstack/vue-table'; + useSortableTable, + type SortableColumnDef, + type SortDirection, +} from '@/utils/useSortableTable'; export type SortColumn = 'name' | 'projects_count' | 'status'; -export type SortDirection = 'asc' | 'desc'; +export type { SortDirection } from '@/utils/useSortableTable'; const props = defineProps<{ clients: Client[]; @@ -44,17 +43,7 @@ const projectCountMap = computed(() => { return map; }); -// Name is always the secondary sort so rows with equal values render -// alphabetically instead of in API (created_at) order. -const sorting = computed(() => [ - { - id: props.sortColumn, - desc: props.sortDirection === 'desc', - }, - ...(props.sortColumn !== 'name' ? [{ id: 'name', desc: false }] : []), -]); - -const columns = computed(() => [ +const columns = computed[]>(() => [ { id: 'name', accessorFn: (row: Client) => row.name.toLowerCase(), @@ -70,41 +59,22 @@ const columns = computed(() => [ }, ]); -const descFirstColumns = new Set( - columns.value - .filter((c) => 'sortDescFirst' in c && c.sortDescFirst) - .map((c) => c.id as SortColumn) -); +const { + sortedRows: sortedClients, + descFirstColumns, + nextDirection, +} = useSortableTable({ + data: () => props.clients, + columns: () => columns.value, + sortColumn: () => props.sortColumn, + sortDirection: () => props.sortDirection, + tieBreakColumn: 'name', +}); function handleSort(column: SortColumn) { - if (props.sortColumn === column) { - emit('sort', column, props.sortDirection === 'asc' ? 'desc' : 'asc'); - } else { - emit('sort', column, descFirstColumns.has(column) ? 'desc' : 'asc'); - } + emit('sort', column, nextDirection(column)); } -const table = useVueTable({ - get data() { - return props.clients; - }, - get columns() { - return columns.value; - }, - getCoreRowModel: getCoreRowModel(), - getSortedRowModel: getSortedRowModel(), - state: { - get sorting() { - return sorting.value; - }, - }, - manualSorting: false, -}); - -const sortedClients = computed(() => { - return table.getRowModel().rows.map((row) => row.original); -}); - // Client-side pagination: the full list is in memory, only one page is mounted at a time. const PAGE_SIZE = 15; const currentPage = ref(1); diff --git a/resources/js/Components/Common/Client/ClientTableHeading.vue b/resources/js/Components/Common/Client/ClientTableHeading.vue index a3b9452b..21eb7295 100644 --- a/resources/js/Components/Common/Client/ClientTableHeading.vue +++ b/resources/js/Components/Common/Client/ClientTableHeading.vue @@ -1,6 +1,7 @@