aglinxinyuan opened a new pull request, #7825:
URL: https://github.com/apache/texera/pull/7825
### What changes were proposed in this PR?
Six sortable columns in the admin user table rendered backwards relative to
the caret they lit. `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 six
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 six were verified individually against their own `<th>` before being
touched; none was already correct and none started at `descend`:
| Comparator | `<th>` `nzSortDirections` |
|---|---|
| `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 is the contract-correct ascending
`a.uid - b.uid`, and `sortByAccountCreation` already uses ascending form under
an identical header.
### The existing spec was cementing the defect
Its "column sort comparators" block asserted the reversed behaviour directly
— e.g. `sortByName(Alice, Bob) > 0`. Those six 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` | **12 failed, 42 passed** | **54 passed** |
12 is exactly 6 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]`.
### Test design, which matters more here than the fix
The fix is six operand swaps; the risk is a test that cannot tell a correct
comparator from one inverted the other way. Two layers, both covering all six
fields individually:
1. Corrected per-comparator unit assertions (direction only; the existing
null-value uid-tiebreak assertions are unaffected and kept).
2. A new `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 six
per-field orders were chosen so all twelve sequences (6 ascending + 6
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.
Two mutation checks confirm both halves are load-bearing:
- Re-inverting **only** `sortByAffiliation`: `2 failed | 52 passed` —
precisely its unit test and its rendered test. So no test passes while five of
six are still broken.
- Keeping it 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.
### One adjacent case deliberately left alone
`sortByID` (line 247) is `b.uid - a.uid` under an `['ascend','descend']`
header, so it has the same signature. Unlike the string columns, "highest uid
first" is plausibly a deliberate newest-accounts-first default, so it wants a
product decision rather than a mechanical flip.
### Verification
- `admin-user.component.spec.ts` **54/54**; 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 54 passed (54)
```
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
