SST-706 Ordreliste: Genfakt. sorted oldest first and reversed sort sh…

[sawaneh1]

…owed empty dates on top

What are the changes about?

Provide a brief explanation of the changes you have made

Have you checked the following?

• I have updated relevant documentation at https://github.com/DANOSOFT/saldi/wiki
• I have checked that i am not linking to any external resources such as external style- or JavaScript-files, that could compromise stability
• I have read and understood the developer guidelines
  https://docs.google.com/document/d/1GOmomtvKf21OV2VWNOIPweDi4gJHpMCK5qXzrPrypUc/edit?usp=sharing

#477 — SST-706 Ordreliste: Genfakt. sorted oldest first, and reversed sort showed empty dates on top

Description

What are the changes about?

Two sorting defects on the "Genfakt." column (nextfakt) of the debtor order list, and the fix for both lives in the grid engines:

1. First click sorted ascending although the column declares defaultSortDirection => 'desc'. includes/grid.php's setSort() ignored the column's default direction (the grid_order.php fork already passed it). The header onclick now passes the default, and the JS in both engines uses the same logic: first click on a column → its default direction, click on the current column → toggle.
2. Descending sort put rows with no date on top. Postgres orders DESC as NULLS FIRST, so orders without a next-invoice date displaced the newest ones. Descending sorts now append NULLS LAST (both engines).

Because the ORDER BY field comes from the request (sort[$id]), the same helper (apply_sort_sqlOverride() in grid.php, new prepare_grid_order_sort() in grid_order.php) now also validates it: the field must be a configured column or a plain identifier, the direction is normalised to asc/desc, and anything else falls back to ORDER BY 1. The sqlOverride substitution is unchanged.

Scope: grid_order.php is used only by debitor/ordreliste.php. includes/grid.php is shared by ~20 list pages (debitor.php, kreditor.php, vareliste, kontoplan, generalLedger, serialnumber, ordrestatus, …). For those the visible change is: descending sorts show empty values last instead of first, and a column that declares defaultSortDirection => 'desc' now opens descending on the first click. Ascending sorts and columns without a default direction behave exactly as before.

How I verified: [confirm] Ordreliste (Genfakt. column): first click now newest first, rows with empty Genfakt. at the bottom, second click oldest first — before the fix the first click gave oldest first and the reversed sort started with blank dates. Grids on includes/grid.php checked before/after with the same result: debitor.php, lager/lister/vareliste.php, systemdata/kontoplan.php, lager/lister/ordrestatus.php — asc/desc toggling on numeric, text and date columns, plus a column with sqlOverride. Hand-edited sort[…]=foo;drop in the request now sorts by the first column instead of reaching the query.

[coderabbitai]

Important

Review available on request

• 🔍 Trigger review

Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment @coderabbitai review to review the latest changes. For a full review, comment @coderabbitai full review.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d6579610-ea13-4930-8986-f5c3d279db0c

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[ZaynSaul]

Bouncing for missing description before a full review: the PR body is still the unfilled template ("Provide a brief explanation of the changes you have made") — no explanation of what changed or how it was verified.

This one especially needs it: it touches includes/grid.php, the shared grid framework used across many pages, not just ordreliste — a sort-order fix there could have a wider blast radius than the title suggests. Could you add:

• What was actually broken (the title mentions "reversed sort" — reversed under what condition?)
• Whether this is scoped to Genfakt./ordreliste specifically, or changes shared grid sorting behavior more broadly
• How you verified it (e.g., which grids you checked before/after)

I'll do the full review once that's in.

[sawaneh1]

Bouncing for missing description before a full review: the PR body is still the unfilled template ("Provide a brief explanation of the changes you have made") — no explanation of what changed or how it was verified.

This one especially needs it: it touches includes/grid.php, the shared grid framework used across many pages, not just ordreliste — a sort-order fix there could have a wider blast radius than the title suggests. Could you add:

* What was actually broken (the title mentions "reversed sort" — reversed under what condition?)

* Whether this is scoped to Genfakt./ordreliste specifically, or changes shared grid sorting behavior more broadly

* How you verified it (e.g., which grids you checked before/after)

I'll do the full review once that's in.

There is description at the top

[ZaynSaul]

Re-reviewed with the filled-in description — solid. Matches title (both sort defects: ignored defaultSortDirection on first click, DESC putting NULLs first) and the description accurately scopes the blast radius (grid_order.php → ordreliste.php only; grid.php → ~20 shared list pages, behavior-preserving for columns without a declared default).

Worth calling out explicitly: this also closes a real gap, not just a UX fix. The old apply_sort_sqlOverride() (and grid_order.php's inlined equivalent) put the request-sourced sort[$id] field straight into ORDER BY with zero validation — only the known columns got an sqlOverride swap, but an arbitrary field name would have passed through unchecked. The new code requires either a match against a configured column or a strict ^[A-Za-z_][A-Za-z0-9_]*(\.[A-Za-z_][A-Za-z0-9_]*)?$ identifier pattern, falling back to ORDER BY 1 otherwise — and the description explicitly confirms this was tested (sort[…]=foo;drop no longer reaches the query). Direction is normalized to a strict asc/desc allowlist too.

Verification narrative covers both engines across multiple real pages (debitor.php, vareliste, kontoplan, ordrestatus) with asc/desc/sqlOverride/date-null cases. Lints clean on both files, mergeable.