Bug 38838 - optgroup construct needs cleaning in the reports module
Summary: optgroup construct needs cleaning in the reports module
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Jonathan Druart
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords: roadmap_25_05
Depends on:
Blocks: 38714
  Show dependency treegraph
 
Reported: 2025-01-07 09:22 UTC by Jonathan Druart
Modified: 2025-03-24 18:49 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement updates what is shown when selecting the columns when creating a new dictionary definition in reports. It now shows "Field description / tablename.fieldname", instead of just the "Field description" - the same as for creating reports. Example, 'Publication date / biblioitems.publicationyear' (previously it just showed 'Publication date').
Version(s) released in:
25.05.00,24.11.03
Circulation function:


Attachments
Bug 38838: Clean C4::Reports::Guided->get_columns (11.23 KB, patch)
2025-01-07 09:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38838: Clean C4::Reports::Guided->get_columns (11.27 KB, patch)
2025-01-25 21:00 UTC, David Nind
Details | Diff | Splinter Review
Bug 38838: Clean C4::Reports::Guided->get_columns (11.34 KB, patch)
2025-02-10 13:12 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2025-01-07 09:22:31 UTC
reports/dictionary.tt and reports/guided_reports_start.tt display a list of the columns available using optgroup to group the columns per table.

The template code is quite ugly because of the perl structure choose in C4::Reports::Guided->get_columns
Comment 1 Jonathan Druart 2025-01-07 09:45:04 UTC
Created attachment 176170 [details] [review]
Bug 38838: Clean C4::Reports::Guided->get_columns

reports/dictionary.tt and reports/guided_reports_start.tt display a list
of the columns available using optgroup to group the columns per table.

The template code is quite ugly because of the perl structure choose in
C4::Reports::Guided->get_columns

This patch removes $cgi and the 'first' variable that were not used.
Now we have an usual structure with the table names as keys and column
info in an arrayref.

Test plan:
Create a report and a new dictionary definition.
At step 3 you should see the list of the columns.
Note that they are now identical. The list when you create a dictionary
now show the description of the columns.
Comment 2 David Nind 2025-01-25 21:00:09 UTC
Created attachment 177146 [details] [review]
Bug 38838: Clean C4::Reports::Guided->get_columns

reports/dictionary.tt and reports/guided_reports_start.tt display a list
of the columns available using optgroup to group the columns per table.

The template code is quite ugly because of the perl structure choose in
C4::Reports::Guided->get_columns

This patch removes $cgi and the 'first' variable that were not used.
Now we have an usual structure with the table names as keys and column
info in an arrayref.

Test plan:
Create a report and a new dictionary definition.
At step 3 you should see the list of the columns.
Note that they are now identical. The list when you create a dictionary
now show the description of the columns.

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 David Nind 2025-01-25 21:13:31 UTC
Signed off and release note added.

I'm not sure about changing the formatting of the table group title from bold to bold and italic/emphasis. I think I prefer just the bold.
Comment 4 Victor Grousset/tuxayo 2025-02-09 17:11:28 UTC
(In reply to David Nind from comment #3)
> Signed off and release note added.

Thanks :D

> I'm not sure about changing the formatting of the table group title from
> bold to bold and italic/emphasis. I think I prefer just the bold.

What do you mean? I don't see a difference on that.
main report creation: https://lutim.lagout.org/YxS2Ztt7/EziG1QO4.png
vs
with patch report creation: https://lutim.lagout.org/6nlRmlJu/eqTMYXF1.png


main Dictionary new definition: https://lutim.lagout.org/0cb7U9uw/CSwG8gwH.png
vs
with patch Dictionary new definition: https://lutim.lagout.org/2dBXIb2J/gWG8zI6O.png
Comment 5 Martin Renvoize (ashimema) 2025-02-10 13:12:49 UTC
Created attachment 177691 [details] [review]
Bug 38838: Clean C4::Reports::Guided->get_columns

reports/dictionary.tt and reports/guided_reports_start.tt display a list
of the columns available using optgroup to group the columns per table.

The template code is quite ugly because of the perl structure choose in
C4::Reports::Guided->get_columns

This patch removes $cgi and the 'first' variable that were not used.
Now we have an usual structure with the table names as keys and column
info in an arrayref.

Test plan:
Create a report and a new dictionary definition.
At step 3 you should see the list of the columns.
Note that they are now identical. The list when you create a dictionary
now show the description of the columns.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize (ashimema) 2025-02-10 13:13:32 UTC
Also looks OK to me.. in the name of progress and I'm going to PQA.. we can revisit later if there does prove to be resistance.. but the code is much cleaner now.

Passing
Comment 7 Katrin Fischer 2025-02-11 14:03:18 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 8 Victor Grousset/tuxayo 2025-02-11 16:49:34 UTC
(In reply to Martin Renvoize (ashimema) from comment #6)
> Also looks OK to me.. in the name of progress and I'm going to PQA.. we can
> revisit later if there does prove to be resistance.. but the code is much
> cleaner now.

Thanks for review. As far as I looked at the code changes in the previous days, I hadn't find anything suspicious yet. So let's go with this :)
Comment 9 David Nind 2025-02-13 09:32:34 UTC
(In reply to Victor Grousset/tuxayo from comment #4)
> (In reply to David Nind from comment #3)
> > Signed off and release note added.
> 
> Thanks :D
> 
> > I'm not sure about changing the formatting of the table group title from
> > bold to bold and italic/emphasis. I think I prefer just the bold.
> 
> What do you mean? I don't see a difference on that.
> main report creation: https://lutim.lagout.org/YxS2Ztt7/EziG1QO4.png
> vs
> with patch report creation: https://lutim.lagout.org/6nlRmlJu/eqTMYXF1.png
> 
> 
> main Dictionary new definition:
> https://lutim.lagout.org/0cb7U9uw/CSwG8gwH.png
> vs
> with patch Dictionary new definition:
> https://lutim.lagout.org/2dBXIb2J/gWG8zI6O.png

Not sure anymore. In an updated main, the field group heading is just bold. Example using circulation:

  biblioitems <-- This is now just bold, when I tested it was bold and italics
    Biblio item number (internal) /  biblioitems.biblioitemnumber
    .....
Comment 10 Paul Derscheid 2025-03-24 18:49:42 UTC
Nice work everyone!

Pushed to 24.11.x for 24.11.03