Bug 27653

Summary: Do not include 'caption' row in print/copy export of datatables
Product: Koha Reporter: Nick Clemens <nick>
Component: Staff ClientAssignee: Nick Clemens <nick>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: andrew, barbara.johnson, fridolin.somers, gmcharlt, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.03,20.05.09
Bug Depends on: 27646    
Bug Blocks:    
Attachments: Bug 27653: Set messageTop to null for KohaTables
Bug 27653: Set messageTop to null for KohaTables
Bug 27653: Set messageTop to null for KohaTables
Bug 27653: [Alternate] Use toolbar elements instead of captions
Bug 27653: [Alternate] Use toolbar elements instead of captions
Bug 27653: [Alternate] Use toolbar elements instead of captions
Bug 27653: [Alternate][Alternate][Alternate] Use div instead of caption for controls
Display problem
Bug 27653: [Alternate][Alternate][Alternate] Use div instead of caption for controls
Bug 27653: [Alternate][Alternate][Alternate] Use div instead of caption for controls

Description Nick Clemens 2021-02-08 12:48:41 UTC
The 'caption' field is used a a few dataTables as a place to store filters etc.

We do not wish this to be included when copying/printing the tables
Comment 1 Nick Clemens 2021-02-08 12:52:03 UTC
Created attachment 116495 [details] [review]
Bug 27653: Set messageTop to null for KohaTables

This patch sets messageTop to null for KohaTables. This prevents
the filter row from being included in an export.

To test:
1 - Browse to acquisitions home
2 - Click Export->Copy
3 - Paste the data to a text editor
4 - Click Export->Print
5 - Note the 'Expand all...' row is included in both of the above
6 - Apply patch
7 - Repeat
8 - The row is gone from both copy and print
Comment 2 Katrin Fischer 2021-02-08 22:05:55 UTC
Created attachment 116530 [details] [review]
Bug 27653: Set messageTop to null for KohaTables

This patch sets messageTop to null for KohaTables. This prevents
the filter row from being included in an export.

To test:
1 - Browse to acquisitions home
2 - Click Export->Copy
3 - Paste the data to a text editor
4 - Click Export->Print
5 - Note the 'Expand all...' row is included in both of the above
6 - Apply patch
7 - Repeat
8 - The row is gone from both copy and print

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 3 PTFS Europe Sandboxes 2021-02-08 23:44:16 UTC
Created attachment 116531 [details] [review]
Bug 27653: Set messageTop to null for KohaTables

This patch sets messageTop to null for KohaTables. This prevents
the filter row from being included in an export.

To test:
1 - Browse to acquisitions home
2 - Click Export->Copy
3 - Paste the data to a text editor
4 - Click Export->Print
5 - Note the 'Expand all...' row is included in both of the above
6 - Apply patch
7 - Repeat
8 - The row is gone from both copy and print

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Comment 4 Owen Leonard 2021-02-08 23:58:17 UTC
I'm a little concerned about this change because the example in the test plan isn't really good HTML. We shouldn't put controls like "expand all" links into a <caption> tag because it's not semantically correct.

Ideally the caption is designed to provide information about the contents of the table, so a good caption wouldn't be unwanted in a printout. We don't use <caption> very consistently, so I don't really know how often we have a situation where this patch would affect the output. But I am concerned about making this a global change.
Comment 5 Katrin Fischer 2021-02-09 10:58:03 UTC
Owen, do you have an idea, how we could set this individuallyfor those 2 tables (acq home page and funds table) or handle otherwise?
Comment 6 Nick Clemens 2021-02-09 14:41:46 UTC
Created attachment 116576 [details] [review]
Bug 27653: [Alternate] Use toolbar elements instead of captions

The aqbudgets and aqui-home tables use captions for additional table controls - these are
more appropriate as a toolbar

To test:
1 - View the tables on acquisitions home and budgets views
2 - Note the 'Expand all...' control row
3 - Apply patch
4 - The row remains, but is above table rows
5 - The row is not included when printing or copying the table
Comment 7 Nick Clemens 2021-02-09 18:40:55 UTC
Created attachment 116611 [details] [review]
Bug 27653: [Alternate] Use toolbar elements instead of captions

The aqbudgets and aqui-home tables use captions for additional table controls - these are
more appropriate as a DataTables custom toolbar

To test:
1 - View the tables on acquisitions home and budgets views
2 - Note the 'Expand all...' control row
3 - Apply patch
4 - The row remains, but is below the table controls
5 - The row is not included when printing or copying the table
Comment 8 Nick Clemens 2021-02-09 19:12:26 UTC
Created attachment 116614 [details] [review]
Bug 27653: [Alternate] Use toolbar elements instead of captions

The aqbudgets and aqui-home tables use captions for additional table controls - these are
more appropriate as a DataTables custom toolbar

To test:
1 - View the tables on acquisitions home and budgets views
2 - Note the 'Expand all...' control row
3 - Apply patch
4 - The row remains, but is below the table controls
5 - The row is not included when printing or copying the table
Comment 9 Owen Leonard 2021-02-09 19:42:47 UTC
Created attachment 116615 [details] [review]
Bug 27653: [Alternate][Alternate][Alternate] Use div instead of caption for controls

The funds and acquisitions home page tables use captions for additional
table controls. These are more appropriate as "toolbar" divs.

This patch converts the <caption> to <div class="toolbar btn-toolbar">
and moves it just above <table> in the markup to keep it valid.

To test:
1 - View the tables on acquisitions home and budgets views
2 - Note the 'Expand all...' control row
3 - Apply patch
4 - The row remains, but is below the table controls
5 - The row is not included when printing or copying the table
Comment 10 Barbara Johnson 2021-02-10 23:32:49 UTC
The row is not included when printing or copying the table but there's a display problem.  The 'Expand all' control row is duplicated.  Also there is extraneous data such as <<<<<<< HEAD ======= and >>>>>>> Bug 27653: [Alternate][Alternate][Alternate] Use div instead of caption for controls.  I attached a screenshot of the display I got.
Comment 11 Barbara Johnson 2021-02-10 23:33:29 UTC
Created attachment 116690 [details]
Display problem
Comment 12 Katrin Fischer 2021-02-10 23:39:00 UTC
(In reply to Barbara Johnson from comment #11)
> Created attachment 116690 [details]
> Display problem

Hi Barbara, these are called "merge markers" - they sometimes are visible when a patch hasn't applied correctly. I will obsolete the other alternates, maybe that will help.
Comment 13 ByWater Sandboxes 2021-02-12 17:56:29 UTC
Created attachment 116841 [details] [review]
Bug 27653: [Alternate][Alternate][Alternate] Use div instead of caption for controls

The funds and acquisitions home page tables use captions for additional
table controls. These are more appropriate as "toolbar" divs.

This patch converts the <caption> to <div class="toolbar btn-toolbar">
and moves it just above <table> in the markup to keep it valid.

To test:
1 - View the tables on acquisitions home and budgets views
2 - Note the 'Expand all...' control row
3 - Apply patch
4 - The row remains, but is below the table controls
5 - The row is not included when printing or copying the table

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Comment 14 Barbara Johnson 2021-02-12 17:57:29 UTC
Thanks Katrin.  I re-retested and everything worked great.
Comment 15 Katrin Fischer 2021-02-14 15:06:18 UTC
Created attachment 116864 [details] [review]
Bug 27653: [Alternate][Alternate][Alternate] Use div instead of caption for controls

The funds and acquisitions home page tables use captions for additional
table controls. These are more appropriate as "toolbar" divs.

This patch converts the <caption> to <div class="toolbar btn-toolbar">
and moves it just above <table> in the markup to keep it valid.

To test:
1 - View the tables on acquisitions home and budgets views
2 - Note the 'Expand all...' control row
3 - Apply patch
4 - The row remains, but is below the table controls
5 - The row is not included when printing or copying the table

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Jonathan Druart 2021-02-15 11:08:43 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 17 Fridolin Somers 2021-02-19 15:27:01 UTC
Pushed to 20.11.x for 20.11.03
Comment 18 Andrew Fuerste-Henry 2021-02-23 12:56:42 UTC
Missing dependency for 20.05, not backporting. If this is needed, please rebase bug 26602, thereby allowing backport of bug 27646.
Comment 19 Andrew Fuerste-Henry 2021-02-23 17:08:17 UTC
Pushed to 20.05.x for 20.05.09
Comment 20 Victor Grousset/tuxayo 2021-02-23 17:27:09 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.