fix(reports): persist layout settings and de-duplicate CSV group column - #57
Merged
Conversation
…e CSV group column Report save silently dropped part of the layout. Laravel's validate() only returns keys that carry a rule, and store/update declared rules for layout.fields and layout.group_order only, so view_id, show_headers_only and per_page never reached the database. The consequences reported by manual QA: the selected view never persisted, and a saved report's PDF fell back to the default table because its layout carried no view_id. Declare the three keys explicitly. The CSV export also prepended the group-by field and then looped over every selected column. Since the grouped field is normally displayed too, its header and value appeared twice on every row. Emit it once. Both are covered by regression tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two of the reports defects from manual QA, both with regression tests. A third one (PDF pagination) turned out not to be what I first thought — see below.
(c) + (e) — the selected view never persisted
These two symptoms are one backend bug.
ReportController::storeandupdatedeclared validation rules forlayout.fieldsandlayout.group_orderonly. Laravel'svalidate()returns just the keys that carry a rule, soview_id,show_headers_onlyandper_pagewere stripped before the report was written:So the chosen view never came back (e), and a saved report's PDF fell back to the default table because its stored layout had no
view_id(c).show_headers_onlyandper_pagewere silently lost the same way — not reported yet, same cause.The three keys are now declared explicitly.
(a) — group column duplicated in CSV
generateCsvResponseprepended the group-by field and then looped over every selected column. Since the grouped field is normally displayed as well, its header and its value appeared twice on every row. It is emitted once now.(d) — PDF pagination: diagnosed, not fixed here
My first reading blamed
page-break-inside: avoidon.group-section. Measured against the running stack, that is wrong — Dompdf ignores the rule on block containers; page counts were identical with and without it (7 → 7 on 20 small groups, 7 → 6 on 3 large ones). I reverted that speculative change rather than ship it.What the numbers actually show, using the real dev report (247 records):
The card in that view stacks 12 of its 16 fields in one column, making it ~370–420pt tall; two cards do not fit in an A4 text block, so each takes a page and wastes the rest. Removing
page-break-insidefrom.report-card, dropping the 80px column min-height and tightening field margins each left it at one card per page — the height is arithmetic, not a break-rule artefact.This matters more now, not less: fixing (c)/(e) above means saved reports will actually start rendering in card mode, so both the density problem and the OOM become reachable in normal use. Worth deciding on its own — most naturally alongside the print/PDF unification, where this layout gets consolidated anyway.
Verification
ReportQueryService+ Dompdf in the API container