This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-7825-be8fc4f8fd76c3330b43defa131b2defbfa5e055 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 1529ae13cba2bd13d8c8ad6aa68cb04bddd7f545 Author: Xinyuan Lin <[email protected]> AuthorDate: Thu Sep 24 03:42:23 2026 +0000 fix(frontend): sort the admin user columns ascending under the ascend caret (#7825) ### What changes were proposed in this PR? Seven sortable columns in the admin user table rendered backwards relative to the caret they lit. `sortByID`, `sortByName`, `sortByEmail`, `sortByAffiliation`, `sortByJoiningReason`, `sortByComment` and `sortByRole` all compared with reversed operands. ng-zorro uses an `NzTableSortFn`'s result **as-is** for `ascend` and negates it for `descend` (`ng-zorro-antd-table.mjs:822`). Every one of these seven headers declares `nzSortDirections="['ascend','descend']"` — no `null` member — so the first click is always `ascend`, and reversed operands render Z→A under a lit up-caret. All seven were verified individually against their own `<th>` before being touched; none was already correct and none started at `descend`: | Comparator | `<th>` `nzSortDirections` | |---|---| | `sortByID` | line 41, `['ascend','descend']` | | `sortByName` | line 46, `['ascend','descend']` | | `sortByEmail` | line 60, `['ascend','descend']` | | `sortByAffiliation` | line 74, `['ascend','descend']` | | `sortByJoiningReason` | line 79, `['ascend','descend']` | | `sortByComment` | line 84, `['ascend','descend']` | | `sortByRole` | line 99 (`nzSortFn` at 105), `['ascend','descend']` | Same defect and same signature as `sortBySize` in #7806. Two corroborations inside this file: the tie-breaker in all six string comparators is the contract-correct ascending `a.uid - b.uid`, and `sortByAccountCreation` already uses ascending form under an identical header. ### Why `sortByID` is in scope after all An earlier revision of this PR left `sortByID` alone, on the theory that "highest uid first" might be a deliberate newest-accounts-first default rather than the same defect. It isn't, for two reasons: 1. **No column in this template sets `nzSortOrder`** (`grep -c nzSortOrder` over `admin-user.component.html` returns 0). The table has no default sort at all — it renders in the order rows arrive until someone clicks a header. So the reversal cannot express a "newest first" default, because the comparator does not run until a click. Its only observable effect was that clicking the **up** caret on ID rendered highest-uid-first. 2. **The column that actually encodes recency is already ascending.** `sortByAccountCreation` is `(a - b)`, i.e. oldest-first under the up caret. If newest-first were the product stance, that is the column that would be reversed. It isn't, so ID was the lone inversion in the table. `sortByActive` was examined for the same signature and deliberately **not** changed, only documented. Its key is a boolean, so there is no natural A→Z to reverse; ng-zorro's contract asks a comparator to declare its own ascending order, and "active before inactive" is that order — it is also what makes the first click useful. The existing unit test already pins both directions plus the uid tiebreak. ### The existing spec was cementing the defect Its "column sort comparators" block asserted the reversed behaviour directly — e.g. `sortByName(Alice, Bob) > 0`, and `sortByID` had a bare `it("orders by descending uid")`. Those assertions are corrected here, which is why they appear in the failing-before set below rather than being untouched. ### Failing before, passing after | | production reverted | with fix | |---|---|---| | `admin-user.component.spec.ts` | **14 failed, 41 passed** | **55 passed** | 14 is exactly 7 comparators × 2 tests each. The rendered-header failures show the #7806 signature — the caret assertions *passed* and only the order failed, e.g. `sorts User Role A-to-Z…` → `expected [3, 2, 1, 4] to deeply equal [4, 1, 2, 3]`, and for the ID column `expected [4, 3, 2, 1] to deeply equal [1, 2, 3, 4]`. ### Test design, which matters more here than the fix The fix is seven operand swaps; the risk is a test that cannot tell a correct comparator from one inverted the other way. Two layers, both covering all seven fields individually: 1. Corrected per-comparator unit assertions (direction only; the existing null-value uid-tiebreak assertions are unaffected and kept). 2. A `describe("sorted columns (rendered header)")`, table-driven over `ASCENDING_BY_COLUMN` and generating one `it()` per column so each gets a fresh fixture — these headers have no `null` state, so a shared fixture would leave earlier columns sorted as secondary keys. Each case clicks the **real rendered `<th>`**, waits a macrotask (nz-table republishes its sort operators on `delay(0)`, so a bare `detectChanges()` reads the previous ordering), and pins the uid order **and** which caret carries `.active`, in both directions. The four fixture rows carry a distinct value in every sorted field, so no comparator reaches its uid tiebreak and any row exchange is visible. The seven per-field orders were chosen so all fourteen sequences (7 ascending + 7 descending) are distinct across the table and none equals the supplied order — no column's expectation can be satisfied by another column's sort, or by the table not sorting at all. Adding the ID column tightened that last constraint, since uid is what the assertions are written in. The rows were supplied `1,2,3,4`, which is exactly ID's ascending expectation — that leg would have passed against a table that never sorted. They are now supplied `4,2,1,3`, checked against all 24 permutations to collide with none of the fourteen asserted sequences, and the invariant is recorded in a comment so a future reshuffle re-checks it. Three mutation checks confirm both halves are load-bearing: - Re-inverting **only** `sortByID`: `2 failed | 53 passed` — its unit test and its rendered test, nothing else. - Re-inverting **only** `sortByAffiliation`: `2 failed | 53 passed`. So no test passes while six of seven are still broken. - Keeping affiliation inverted but flipping its header to `['descend','ascend']` — the tempting wrong "fix" that yields the right order with the wrong caret — still fails, on `expected false to be true` from the caret assertion. ### Verification - `admin-user.component.spec.ts` **55/55**; sibling `user-quota.component.spec.ts` 25/25, unaffected. - No other file in `frontend/src` references these comparators (`user-project.component.ts`'s `sortByNameAsc`/`Desc` are explicit-direction buttons, not `NzTableSortFn`). - `yarn format:ci` exits 0; no `junit.xml` left behind. ### Any related issues, documentation, discussions? Closes #7824 ### How was this PR tested? ``` npx ng test --watch=false --include="**/admin-user.component.spec.ts" ``` ``` Test Files 1 passed (1) Tests 55 passed (55) ``` ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5) --- .../admin/user/admin-user.component.spec.ts | 132 ++++++++++++++++++--- .../component/admin/user/admin-user.component.ts | 21 ++-- 2 files changed, 130 insertions(+), 23 deletions(-) diff --git a/frontend/src/app/dashboard/component/admin/user/admin-user.component.spec.ts b/frontend/src/app/dashboard/component/admin/user/admin-user.component.spec.ts index f0e3980ac2..4ec582cc1b 100644 --- a/frontend/src/app/dashboard/component/admin/user/admin-user.component.spec.ts +++ b/frontend/src/app/dashboard/component/admin/user/admin-user.component.spec.ts @@ -512,39 +512,41 @@ describe("AdminUserComponent", () => { }); }); + /** + * Every comparator here has to order its column *ascending*, because nz-table applies the + * comparator's result as-is for 'ascend' and negates it for 'descend'. A comparator with + * reversed operands therefore lights the up caret while rendering the column backwards. The + * rendered-header tests further down pin the caret and the row order together per column. + */ describe("column sort comparators", () => { - it("sortByID orders by descending uid", () => { - expect(component.sortByID(mk({ uid: 1 }), mk({ uid: 2 }))).toBe(1); - expect(component.sortByID(mk({ uid: 5 }), mk({ uid: 2 }))).toBe(-3); + it("sortByID orders by ascending uid", () => { + expect(component.sortByID(mk({ uid: 1 }), mk({ uid: 2 }))).toBe(-1); + expect(component.sortByID(mk({ uid: 5 }), mk({ uid: 2 }))).toBe(3); }); it("sortByName compares names and falls back to uid on a tie", () => { - expect(component.sortByName(mk({ uid: 1, name: "Alice" }), mk({ uid: 2, name: "Bob" }))).toBeGreaterThan(0); + expect(component.sortByName(mk({ uid: 1, name: "Alice" }), mk({ uid: 2, name: "Bob" }))).toBeLessThan(0); // equal names (both empty via null coalescing) -> uid tiebreak expect(component.sortByName(mk({ uid: 1, name: null as any }), mk({ uid: 2, name: null as any }))).toBe(-1); }); it("sortByEmail compares emails and falls back to uid on a tie", () => { - expect(component.sortByEmail(mk({ uid: 1, email: "[email protected]" }), mk({ uid: 2, email: "[email protected]" }))).toBeGreaterThan( - 0 - ); + expect(component.sortByEmail(mk({ uid: 1, email: "[email protected]" }), mk({ uid: 2, email: "[email protected]" }))).toBeLessThan(0); expect(component.sortByEmail(mk({ uid: 1, email: null as any }), mk({ uid: 2, email: null as any }))).toBe(-1); }); it("sortByComment compares comments and falls back to uid on a tie", () => { - expect(component.sortByComment(mk({ uid: 1, comment: "aaa" }), mk({ uid: 2, comment: "bbb" }))).toBeGreaterThan( - 0 - ); + expect(component.sortByComment(mk({ uid: 1, comment: "aaa" }), mk({ uid: 2, comment: "bbb" }))).toBeLessThan(0); expect(component.sortByComment(mk({ uid: 1, comment: null as any }), mk({ uid: 2, comment: null as any }))).toBe( -1 ); }); it("sortByRole compares roles and falls back to uid on a tie", () => { - // "ADMIN".localeCompare("REGULAR") is negative - expect(component.sortByRole(mk({ uid: 1, role: Role.REGULAR }), mk({ uid: 2, role: Role.ADMIN }))).toBeLessThan( - 0 - ); + // "REGULAR".localeCompare("ADMIN") is positive, so REGULAR sorts after ADMIN + expect( + component.sortByRole(mk({ uid: 1, role: Role.REGULAR }), mk({ uid: 2, role: Role.ADMIN })) + ).toBeGreaterThan(0); expect(component.sortByRole(mk({ uid: 1, role: Role.ADMIN }), mk({ uid: 2, role: Role.ADMIN }))).toBe(-1); }); @@ -564,7 +566,7 @@ describe("AdminUserComponent", () => { it("sortByAffiliation compares affiliations and falls back to uid on a tie", () => { expect( component.sortByAffiliation(mk({ uid: 1, affiliation: "MIT" }), mk({ uid: 2, affiliation: "UCLA" })) - ).toBeGreaterThan(0); + ).toBeLessThan(0); expect( component.sortByAffiliation(mk({ uid: 1, affiliation: undefined }), mk({ uid: 2, affiliation: undefined })) ).toBe(-1); @@ -576,7 +578,7 @@ describe("AdminUserComponent", () => { mk({ uid: 1, joiningReason: "research" }), mk({ uid: 2, joiningReason: "teaching" }) ) - ).toBeGreaterThan(0); + ).toBeLessThan(0); expect( component.sortByJoiningReason( mk({ uid: 1, joiningReason: null as any }), @@ -765,4 +767,102 @@ describe("AdminUserComponent", () => { expect(addSpy).toHaveBeenCalled(); }); }); + + /** + * Drives the real sortable headers so that the caret nz-table lights and the order it renders are + * observed together. Asserting only the row order would still pass against a comparator inverted + * the other way — nz-table uses the comparator's result as-is for 'ascend' and negates it for + * 'descend', so reversed operands produce a Z-to-A up caret. Every column below therefore pins + * both halves in both directions. + * + * The four rows carry a distinct value in every sorted field, so no comparator ever reaches its + * uid tiebreak and swapping any two rows is visible. The per-field orders are chosen so that each + * column's ascending and descending sequence is unique across the whole table and differs from the + * order the rows are supplied in — no column's expectation can be satisfied by another column's + * sort, or by the table not sorting at all. + */ + describe("sorted columns (rendered header)", () => { + // The supplied order is deliberately none of the sequences asserted below - not uid order (which + // would make the ID column's ascending leg pass without sorting anything) and not any other + // column's ascending or descending order either. Reshuffling this needs the same check. + const SORT_USERS: ReadonlyArray<User> = [ + mk({ uid: 4, name: "Cyd", email: "[email protected]", affiliation: "Aff-C", joiningReason: "reason-b", comment: "note-c", role: Role.ADMIN }), // prettier-ignore + mk({ uid: 2, name: "Dov", email: "[email protected]", affiliation: "Aff-A", joiningReason: "reason-c", comment: "note-a", role: Role.REGULAR }), // prettier-ignore + mk({ uid: 1, name: "Bea", email: "[email protected]", affiliation: "Aff-B", joiningReason: "reason-a", comment: "note-d", role: Role.INACTIVE }), // prettier-ignore + mk({ uid: 3, name: "Ada", email: "[email protected]", affiliation: "Aff-D", joiningReason: "reason-d", comment: "note-b", role: Role.RESTRICTED }), // prettier-ignore + ]; + const SUPPLIED_ORDER = [4, 2, 1, 3]; + + /** Column header label -> the uids it must render top-to-bottom under the up (ascend) caret. */ + const ASCENDING_BY_COLUMN: ReadonlyArray<[label: string, ascendingUids: number[]]> = [ + ["ID", [1, 2, 3, 4]], // lowest uid (oldest account) first, like every other ascending column + ["Name", [3, 1, 4, 2]], // Ada, Bea, Cyd, Dov + ["Email", [1, 3, 2, 4]], // a@, b@, c@, d@ + ["Affiliation", [2, 1, 4, 3]], // Aff-A, Aff-B, Aff-C, Aff-D + ["Joining Reason", [1, 4, 2, 3]], // reason-a, reason-b, reason-c, reason-d + ["Comment", [2, 3, 4, 1]], // note-a, note-b, note-c, note-d + ["User Role", [4, 1, 2, 3]], // ADMIN, INACTIVE, REGULAR, RESTRICTED + ]; + + /** `fixture.nativeElement` is `any`, so narrow it once for the DOM queries below. */ + const host = (): HTMLElement => fixture.nativeElement as HTMLElement; + + /** The uid cell of each rendered row, top to bottom. */ + function renderedUids(): number[] { + return Array.from(host().querySelectorAll<HTMLElement>("tbody tr")).map(row => + Number((row.querySelectorAll("td")[1].textContent ?? "").trim()) + ); + } + + function sortableHeader(label: string): HTMLElement { + const header = Array.from(host().querySelectorAll<HTMLElement>("thead th")).find( + th => (th.querySelector(".ant-table-column-title")?.textContent ?? "").replace(/\s+/g, " ").trim() === label + ); + if (!header) { + throw new Error(`no sortable header titled "${label}"`); + } + return header; + } + + function caretActive(header: HTMLElement, direction: "up" | "down"): boolean { + const caret = header.querySelector(`.ant-table-column-sorter-${direction}`); + if (!caret) { + throw new Error(`header has no ${direction} caret`); + } + return caret.classList.contains("active"); + } + + async function clickSort(header: HTMLElement): Promise<void> { + header.click(); + // nz-table republishes its sort operators on a macrotask (`delay(0)`), so a bare + // detectChanges() would still read the previous ordering. + await new Promise(resolve => setTimeout(resolve, 0)); + fixture.detectChanges(); + } + + ASCENDING_BY_COLUMN.forEach(([label, ascendingUids]) => { + const descendingUids = [...ascendingUids].reverse(); + + it(`sorts ${label} A-to-Z under the up caret and Z-to-A under the down caret`, async () => { + component.userList = [...SORT_USERS]; + component.listOfDisplayUser = [...SORT_USERS]; + fixture.detectChanges(); + expect(renderedUids()).toEqual(SUPPLIED_ORDER); + + const header = sortableHeader(label); + + // nzSortDirections is ['ascend', 'descend'] on all of these headers, so the first click + // selects 'ascend' and lights the up caret: the column must read A-to-Z. + await clickSort(header); + expect(caretActive(header, "up")).toBe(true); + expect(caretActive(header, "down")).toBe(false); + expect(renderedUids()).toEqual(ascendingUids); + + await clickSort(header); + expect(caretActive(header, "down")).toBe(true); + expect(caretActive(header, "up")).toBe(false); + expect(renderedUids()).toEqual(descendingUids); + }); + }); + }); }); diff --git a/frontend/src/app/dashboard/component/admin/user/admin-user.component.ts b/frontend/src/app/dashboard/component/admin/user/admin-user.component.ts index 1a7934a6d3..359c91b299 100644 --- a/frontend/src/app/dashboard/component/admin/user/admin-user.component.ts +++ b/frontend/src/app/dashboard/component/admin/user/admin-user.component.ts @@ -244,24 +244,28 @@ export class AdminUserComponent implements OnInit { this.editAttribute = ""; } - public sortByID: NzTableSortFn<User> = (a: User, b: User) => b.uid - a.uid; + // NzTableSortFn must order ascending: nz-table applies the result as-is for the 'ascend' sort + // order and negates it for 'descend'. Reversing the operands here would render the column + // backwards under the up caret. + public sortByID: NzTableSortFn<User> = (a: User, b: User) => a.uid - b.uid; + public sortByName: NzTableSortFn<User> = (a: User, b: User) => { - const compare = (b.name || "").localeCompare(a.name || ""); + const compare = (a.name || "").localeCompare(b.name || ""); return compare === 0 ? a.uid - b.uid : compare; }; public sortByEmail: NzTableSortFn<User> = (a: User, b: User) => { - const compare = (b.email || "").localeCompare(a.email || ""); + const compare = (a.email || "").localeCompare(b.email || ""); return compare === 0 ? a.uid - b.uid : compare; }; public sortByComment: NzTableSortFn<User> = (a: User, b: User) => { - const compare = (b.comment || "").localeCompare(a.comment || ""); + const compare = (a.comment || "").localeCompare(b.comment || ""); return compare === 0 ? a.uid - b.uid : compare; }; public sortByRole: NzTableSortFn<User> = (a: User, b: User) => { - const compare = b.role.localeCompare(a.role); + const compare = a.role.localeCompare(b.role); return compare === 0 ? a.uid - b.uid : compare; }; @@ -271,12 +275,12 @@ export class AdminUserComponent implements OnInit { }; public sortByAffiliation: NzTableSortFn<User> = (a: User, b: User) => { - const compare = (b.affiliation || "").localeCompare(a.affiliation || ""); + const compare = (a.affiliation || "").localeCompare(b.affiliation || ""); return compare === 0 ? a.uid - b.uid : compare; }; public sortByJoiningReason: NzTableSortFn<User> = (a: User, b: User) => { - const compare = (b.joiningReason || "").localeCompare(a.joiningReason || ""); + const compare = (a.joiningReason || "").localeCompare(b.joiningReason || ""); return compare === 0 ? a.uid - b.uid : compare; }; @@ -352,6 +356,9 @@ export class AdminUserComponent implements OnInit { return user.accountCreation * 1000; } + // Active-before-inactive IS this column's ascending order, so the up caret surfaces the users + // who are online now. Unlike the columns above there is no natural A-to-Z to reverse here: the + // key is a boolean, and ranking active first is what makes the first click useful. sortByActive: NzTableSortFn<User> = (a: User, b: User) => { const aActive = this.isUserActive(a); const bActive = this.isUserActive(b);
