Bug 26172 - Add a cashup summary view (with option to print)
Summary: Add a cashup summary view (with option to print)
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on: 24492
Blocks: 26274 26690 26938
  Show dependency treegraph
 
Reported: 2020-08-07 08:10 UTC by Martin Renvoize
Modified: 2020-11-16 21:59 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
20.11.00


Attachments
Bug 26172: Add cashup summary view modal (7.92 KB, patch)
2020-08-12 09:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26172: Add cashup summary view modal (8.70 KB, patch)
2020-08-12 11:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26172: Add cashup summary view modal (8.93 KB, patch)
2020-08-18 14:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26172: Add cashup summary view modal (8.93 KB, patch)
2020-08-21 13:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26172: (follow-up) Summary by payment_type (4.31 KB, patch)
2020-08-21 13:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26172: (follow-up) Add print option (3.63 KB, patch)
2020-08-21 13:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26172: Add cashup summary view modal (8.93 KB, patch)
2020-08-21 14:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26172: (follow-up) Summary by payment_type (4.31 KB, patch)
2020-08-21 14:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26172: (follow-up) Add print option (3.63 KB, patch)
2020-08-21 14:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26172: Add cashup summary view modal (9.01 KB, patch)
2020-10-02 15:52 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 26172: (follow-up) Summary by payment_type (4.39 KB, patch)
2020-10-02 15:52 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 26172: (follow-up) Add print option (3.71 KB, patch)
2020-10-02 15:52 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 26172: Add cashup summary view modal (9.01 KB, patch)
2020-10-14 11:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26172: (follow-up) Summary by payment_type (4.39 KB, patch)
2020-10-14 11:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26172: (follow-up) Add print option (3.71 KB, patch)
2020-10-14 11:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26172: (QA follow-up) Add missing filter (1.22 KB, patch)
2020-10-14 11:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26172: Add cashup summary view modal (8.99 KB, patch)
2020-10-14 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26172: (follow-up) Summary by payment_type (4.38 KB, patch)
2020-10-14 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26172: (follow-up) Add print option (3.69 KB, patch)
2020-10-14 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26172: (QA follow-up) Add missing filter (1.22 KB, patch)
2020-10-14 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26172: (QA follow-up) Fix order of summary (1.39 KB, patch)
2020-10-14 16:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26172: (QA follow-up) Add Unit Test (9.63 KB, patch)
2020-10-14 16:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26172: (QA follow-up) Add Unit Test (9.69 KB, patch)
2020-10-15 00:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26172: (QA follow-up) Add Unit Test (9.69 KB, patch)
2020-10-15 08:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26172: Fix return value for ->register (779 bytes, patch)
2020-10-15 09:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26172: Fix return value for ->register (779 bytes, patch)
2020-10-15 09:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26175: (QA follow-up) Add tests for previous cashups (3.30 KB, patch)
2020-10-15 13:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26172: (QA follow-up) Add tests for previous cashups (3.25 KB, patch)
2020-10-15 13:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26172: Replace calls to _result[set] (2.22 KB, patch)
2020-11-03 10:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26172: Replace _result[set] calls (2.22 KB, patch)
2020-11-03 10:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26172: Replace _result[set] calls (2.22 KB, patch)
2020-11-03 10:50 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2020-08-07 08:10:52 UTC
Bug 23355 adds a process for recording cash register cashups; however it does not expose the captured cashup data, leaving that to the end user via reports.

This bug serves as a request for enhancement to add a way to display a quick summary of the transactions that have make up a cashup set.
Comment 1 Martin Renvoize 2020-08-12 09:48:33 UTC
Created attachment 108093 [details] [review]
Bug 26172: Add cashup summary view modal

This patch adds a cashup summary modal to the register details page.

Test plan
1/ Enable Cash Registers and Point of Sale
2/ Enable some debit_types as 'Sale items'
3/ Perform a series of transactions
4/ Perform a 'Cashup' on the register
5/ Note the new '(Summary)' link next to the last cashup on the register
page.
6/ Click the link and confirm the modal contains the pertinent
information
Comment 2 Martin Renvoize 2020-08-12 09:49:42 UTC
Initial proof of concept patch attached.. it would be good to get some early feedback whilst I write tests for the existing code and refine the finer details.
Comment 3 Martin Renvoize 2020-08-12 11:09:55 UTC
Created attachment 108095 [details] [review]
Bug 26172: Add cashup summary view modal

This patch adds a cashup summary modal to the register details page.

Test plan
1/ Enable Cash Registers and Point of Sale
2/ Enable some debit_types as 'Sale items'
3/ Perform a series of transactions
4/ Perform a 'Cashup' on the register
5/ Note the new '(Summary)' link next to the last cashup on the register
page.
6/ Click the link and confirm the modal contains the pertinent
information
Comment 4 Sally 2020-08-14 15:19:44 UTC
Hi Martin, thanks for writing this!

When I confirmed the cashup:  cgi-bin/koha/pos/register.pl?op=cashup

I received the following error:  

Template process failed: undef error - DBIx::Class::Storage::DBI::_dbh_execute(): 'koha_testing1.debit_type_code.description' isn't in GROUP BY at /kohadevbox/koha/Koha/Objects.pm line 413
 at /kohadevbox/koha/C4/Templates.pm line 122
Comment 5 Martin Renvoize 2020-08-18 14:57:26 UTC
Created attachment 108523 [details] [review]
Bug 26172: Add cashup summary view modal

This patch adds a cashup summary modal to the register details page.

Test plan
1/ Enable Cash Registers and Point of Sale
2/ Enable some debit_types as 'Sale items'
3/ Perform a series of transactions
4/ Perform a 'Cashup' on the register
5/ Note the new '(Summary)' link next to the last cashup on the register
page.
6/ Click the link and confirm the modal contains the pertinent
information
Comment 6 Martin Renvoize 2020-08-18 14:58:04 UTC
I've fixed the issue you highlighted now Sally, thanks for testing.
Comment 7 Sally 2020-08-19 15:51:18 UTC
(In reply to Martin Renvoize from comment #2)
> Initial proof of concept patch attached.. it would be good to get some early
> feedback whilst I write tests for the existing code and refine the finer
> details.

This looks great, Martin - really really useful.  

It's in a really logical location to retrieve, and it's great that the summary contains both the register name and cashing up period.

The table is really clear, and the split between the 'Total' takings and the 'Bankable' physical cash is really useful.

Having had a test with it, there's two things that would be useful:

1)  Could a button be added to send the contents of the modal to the printer?  Highlighting it and then choosing Print Selection achieves this (obviously), but it would be great to make that easier for those librarians who are less confident with computers.

2)  From the Total and the Bankable, it's possible to calculate the payments which has already been reconciled (Banked?) - but could this total also be displayed to save any math errors by the librarians?  

I know Martin knows our system very well but to explain for others reading this, we currently have a custom cash module and all transactions processed through the till are listed under 'Total'.  This is then split into payment types - e.g. Cash, Card, Cheque.  This is because we submit these differently to our accounting department, so the cash up summary looks like this:  

Total:  57.00
Card:  37.00
Cash:  11.00
Cheque:  9.00

With the current modal, it would be useful if we could see the payment breakdown (like above), or an 'Banked' total listed, e.g.  

Total: 57.00 
Banked: 37.00
Bankable: 20.00

So Total is all takings, Banked is credit/debit card payments that have already been reconciled, Bankable is cash/cheque which needs to be taken from the till and deposited in the bank.

But yeah, it works brilliantly.
Comment 8 Martin Renvoize 2020-08-21 13:11:55 UTC
Created attachment 108817 [details] [review]
Bug 26172: Add cashup summary view modal

This patch adds a cashup summary modal to the register details page.

Test plan
1/ Enable Cash Registers and Point of Sale
2/ Enable some debit_types as 'Sale items'
3/ Perform a series of transactions
4/ Perform a 'Cashup' on the register
5/ Note the new '(Summary)' link next to the last cashup on the register
page.
6/ Click the link and confirm the modal contains the pertinent
information
Comment 9 Martin Renvoize 2020-08-21 13:11:58 UTC
Created attachment 108818 [details] [review]
Bug 26172: (follow-up) Summary by payment_type

This patch alters the modal so that the totals are broken down by
payment type's available on the system.
Comment 10 Martin Renvoize 2020-08-21 13:20:57 UTC
(In reply to Sally from comment #7)
> 1)  Could a button be added to send the contents of the modal to the
> printer?  Highlighting it and then choosing Print Selection achieves this
> (obviously), but it would be great to make that easier for those librarians
> who are less confident with computers.

I'm still working on this one.

> 
> 2)  From the Total and the Bankable, it's possible to calculate the payments
> which has already been reconciled (Banked?) - but could this total also be
> displayed to save any math errors by the librarians?  
> 
> I know Martin knows our system very well but to explain for others reading
> this, we currently have a custom cash module and all transactions processed
> through the till are listed under 'Total'.  This is then split into payment
> types - e.g. Cash, Card, Cheque.  This is because we submit these
> differently to our accounting department, so the cash up summary looks like
> this:  
> 
> Total:  57.00
> Card:  37.00
> Cash:  11.00
> Cheque:  9.00
> 
> With the current modal, it would be useful if we could see the payment
> breakdown (like above), or an 'Banked' total listed, e.g.  
> 
> Total: 57.00 
> Banked: 37.00
> Bankable: 20.00
> 
> So Total is all takings, Banked is credit/debit card payments that have
> already been reconciled, Bankable is cash/cheque which needs to be taken
> from the till and deposited in the bank.
> 
> But yeah, it works brilliantly.

Thanks for the feedback Sally :)

I can easily implement the former approach, summarising totals by their payment types, and have attached a followup patch to achieve this. The latter format of Banked/Bankable is proving more allusive as it would require some additional handling within the definition of payments types such that we could easily identify types that are 'automatically banked' vs those that require the librarian to collect from the till at the end of the day.  It seems obvious on first look, but I wonder for example in the cases where libraries take foodbank donations in lue of payments for example if that would be 'automatically banked' in so much as it immediately gets moved into a shared pile of foodbank goods vs being kept under the till to be checked at the end of the day.  As such I think the later approach lends itself to requiring the payment_types authorized value being merged with credit_types first.
Comment 11 Martin Renvoize 2020-08-21 13:46:00 UTC
Created attachment 108822 [details] [review]
Bug 26172: (follow-up) Add print option
Comment 12 Martin Renvoize 2020-08-21 14:23:26 UTC
Created attachment 108825 [details] [review]
Bug 26172: Add cashup summary view modal

This patch adds a cashup summary modal to the register details page.

Test plan
1/ Enable Cash Registers and Point of Sale
2/ Enable some debit_types as 'Sale items'
3/ Perform a series of transactions
4/ Perform a 'Cashup' on the register
5/ Note the new '(Summary)' link next to the last cashup on the register
page.
6/ Click the link and confirm the modal contains the pertinent
information
Comment 13 Martin Renvoize 2020-08-21 14:23:29 UTC
Created attachment 108826 [details] [review]
Bug 26172: (follow-up) Summary by payment_type

This patch alters the modal so that the totals are broken down by
payment type's available on the system.
Comment 14 Martin Renvoize 2020-08-21 14:23:32 UTC
Created attachment 108827 [details] [review]
Bug 26172: (follow-up) Add print option
Comment 15 Martin Renvoize 2020-09-16 10:31:59 UTC
Added Eric as a CC here as they expressed interest in the 'End of Day' functionality which this enhances.
Comment 16 PTFS Europe Sandboxes 2020-10-02 15:52:27 UTC
Created attachment 111162 [details] [review]
Bug 26172: Add cashup summary view modal

This patch adds a cashup summary modal to the register details page.

Test plan
1/ Enable Cash Registers and Point of Sale
2/ Enable some debit_types as 'Sale items'
3/ Perform a series of transactions
4/ Perform a 'Cashup' on the register
5/ Note the new '(Summary)' link next to the last cashup on the register
page.
6/ Click the link and confirm the modal contains the pertinent
information

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 17 PTFS Europe Sandboxes 2020-10-02 15:52:30 UTC
Created attachment 111163 [details] [review]
Bug 26172: (follow-up) Summary by payment_type

This patch alters the modal so that the totals are broken down by
payment type's available on the system.

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 18 PTFS Europe Sandboxes 2020-10-02 15:52:34 UTC
Created attachment 111164 [details] [review]
Bug 26172: (follow-up) Add print option

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 19 Sally 2020-10-02 15:56:31 UTC
(In reply to Martin Renvoize from comment #10)
> I can easily implement the former approach, summarising totals by their
> payment types, and have attached a followup patch to achieve this.

Fantastic, that definitely meets our requirements.  :)

I added a couple of payment types and had a test, and all worked well.  

Thanks for implementing the print button, that's really useful.  It'd be great if it could just send the contents of the modal to the printer (currently it sends the rest of the page as well) - but I don't feel it's a blocker, so I've signed off.
Comment 20 Martin Renvoize 2020-10-03 06:38:59 UTC
Thanks for testing Sally, I'm really happy it's working for you.

On the print front, can I ask what browser you tested with and whether you ran the CSS build option on the sandbox?  The code is intended to hide the background and only print the modal content so if you weren't seeing that I may still have a bug there.  I'll investigate and can work through it with whoever picks this up for QA if it's not working as expected.

Thanks again
Comment 21 Sally 2020-10-05 12:20:16 UTC
(In reply to Martin Renvoize from comment #20)
> Thanks for testing Sally, I'm really happy it's working for you.
> 
> On the print front, can I ask what browser you tested with and whether you
> ran the CSS build option on the sandbox?  The code is intended to hide the
> background and only print the modal content so if you weren't seeing that I
> may still have a bug there.  I'll investigate and can work through it with
> whoever picks this up for QA if it's not working as expected.
> 
> Thanks again

Argh, my mistake - I didn't run build CSS.  I shouldn't have been rushing to get it tested on Friday! 

I've re-tested, and can confirm that it works as expected.
Comment 22 Katrin Fischer 2020-10-14 11:28:14 UTC
Created attachment 111598 [details] [review]
Bug 26172: Add cashup summary view modal

This patch adds a cashup summary modal to the register details page.

Test plan
1/ Enable Cash Registers and Point of Sale
2/ Enable some debit_types as 'Sale items'
3/ Perform a series of transactions
4/ Perform a 'Cashup' on the register
5/ Note the new '(Summary)' link next to the last cashup on the register
page.
6/ Click the link and confirm the modal contains the pertinent
information

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 23 Katrin Fischer 2020-10-14 11:28:20 UTC
Created attachment 111599 [details] [review]
Bug 26172: (follow-up) Summary by payment_type

This patch alters the modal so that the totals are broken down by
payment type's available on the system.

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 24 Katrin Fischer 2020-10-14 11:28:26 UTC
Created attachment 111600 [details] [review]
Bug 26172: (follow-up) Add print option

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 25 Katrin Fischer 2020-10-14 11:28:32 UTC
Created attachment 111601 [details] [review]
Bug 26172: (QA follow-up) Add missing filter
Comment 26 Katrin Fischer 2020-10-14 11:32:59 UTC
Ok, failing this for the missing unit tests, but already adding my QA stamp on the other patches.

2 things:

- When doing the cash-up action from the tools page, after the cash-up the summary link for printing doesn't display. It only displays on the 'register details'. I will file a separate bug for this as a later improvement.

- I think it would be pretty great to have a way to see the 'recorded' cashups with summary links for each to allow libraries to get more of an overview of cashups, the amounts taken - this would be helpful when something has gone wrong but also to see if processes could be improved.

I'll link both to this one as see alsos.
Comment 27 Martin Renvoize 2020-10-14 16:16:26 UTC
Created attachment 111629 [details] [review]
Bug 26172: Add cashup summary view modal

This patch adds a cashup summary modal to the register details page.

Test plan
1/ Enable Cash Registers and Point of Sale
2/ Enable some debit_types as 'Sale items'
3/ Perform a series of transactions
4/ Perform a 'Cashup' on the register
5/ Note the new '(Summary)' link next to the last cashup on the register
page.
6/ Click the link and confirm the modal contains the pertinent
information

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 28 Martin Renvoize 2020-10-14 16:16:31 UTC
Created attachment 111630 [details] [review]
Bug 26172: (follow-up) Summary by payment_type

This patch alters the modal so that the totals are broken down by
payment type's available on the system.

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 29 Martin Renvoize 2020-10-14 16:16:41 UTC
Created attachment 111631 [details] [review]
Bug 26172: (follow-up) Add print option

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 30 Martin Renvoize 2020-10-14 16:16:51 UTC
Created attachment 111632 [details] [review]
Bug 26172: (QA follow-up) Add missing filter
Comment 31 Martin Renvoize 2020-10-14 16:17:02 UTC
Created attachment 111633 [details] [review]
Bug 26172: (QA follow-up) Fix order of summary
Comment 32 Martin Renvoize 2020-10-14 16:17:08 UTC
Created attachment 111634 [details] [review]
Bug 26172: (QA follow-up) Add Unit Test
Comment 33 Martin Renvoize 2020-10-14 16:17:59 UTC
Unit tests added, leaving to Katrin for the final QA Stamp :)
Comment 34 Katrin Fischer 2020-10-15 00:18:29 UTC
Created attachment 111668 [details] [review]
Bug 26172: (QA follow-up) Add Unit Test

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 35 Katrin Fischer 2020-10-15 00:20:32 UTC
The new tests pass, but the QA script fails:

 FAIL	t/db_dependent/Koha/Cash/Register/Action.t
   OK	  critic
   FAIL	  file permissions
		File must have the exec flag
   FAIL	  forbidden patterns
		forbidden pattern: Do not use compare float with precision (bug 24408). Ignore this if you know what you are doing. (line 197)
		forbidden pattern: Do not use compare float with precision (bug 24408). Ignore this if you know what you are doing. (line 207)


Can you assure me you know what you are doing or make it go away?
Comment 36 Martin Renvoize 2020-10-15 08:19:10 UTC
Created attachment 111683 [details] [review]
Bug 26172: (QA follow-up) Add Unit Test

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 37 Martin Renvoize 2020-10-15 08:20:30 UTC
Grr, How did I forget to run the QA script against this!

I've corrected the file permission issue.

As for the float comparison I will defer to Jonathan.. really I should be doing the +0 trick somewhere, but as this is within an 'is_deeply' comparison, I'm not actually sure where I can place it.
Comment 38 Jonathan Druart 2020-10-15 09:48:59 UTC
Created attachment 111691 [details] [review]
Bug 26172: Fix return value for ->register

There is a ON CASCADE DELETE clause on this FK
Comment 39 Jonathan Druart 2020-10-15 09:50:21 UTC
(In reply to Jonathan Druart from comment #38)
> Created attachment 111691 [details] [review] [review]
> Bug 26172: Fix return value for ->register
> 
> There is a ON DELETE CASCADE clause on this FK

Martin, there is the same problem for ->manager (not coming from this patchset). It must be fixed separately. Actually I think the code is code, but the ON DELETE CASCADE should be SET NULL instead for this one.
Comment 40 Jonathan Druart 2020-10-15 09:51:56 UTC
Created attachment 111692 [details] [review]
Bug 26172: Fix return value for ->register

There is a ON DELETE CASCADE clause on this FK
Comment 41 Jonathan Druart 2020-10-15 10:30:56 UTC
1. Tests must cover the "$previous" case (that is never set).
2. Then the code may call something like
  my ($timestamp) = ->search->get_column('timestamp');

to avoid the single and _result calls.
Comment 42 Martin Renvoize 2020-10-15 13:15:41 UTC
Created attachment 111731 [details] [review]
Bug 26175: (QA follow-up) Add tests for previous cashups
Comment 43 Martin Renvoize 2020-10-15 13:16:43 UTC
Created attachment 111732 [details] [review]
Bug 26172: (QA follow-up) Add tests for previous cashups
Comment 44 Jonathan Druart 2020-11-03 10:48:27 UTC
Created attachment 112907 [details] [review]
Bug 26172: Replace calls to _result[set]
Comment 45 Jonathan Druart 2020-11-03 10:48:51 UTC
Created attachment 112908 [details] [review]
Bug 26172: Replace _result[set] calls
Comment 46 Jonathan Druart 2020-11-03 10:50:07 UTC
Created attachment 112909 [details] [review]
Bug 26172: Replace _result[set] calls
Comment 47 Jonathan Druart 2020-11-03 10:50:21 UTC
(In reply to Jonathan Druart from comment #46)
> Created attachment 112909 [details] [review] [review]
> Bug 26172: Replace _result[set] calls

Martin, do you agree with this change?
Comment 48 Martin Renvoize 2020-11-03 14:26:12 UTC
Seems reasonable to me.. I'm going to run the test with dbic_trace enabled to double check the db calls we make.. it feels like we might have introduced a couple of extra one's this way.

Having said that.. I'm not sure how a correlate subquery performs compared to a pair of independent queries anyway.
Comment 49 Martin Renvoize 2020-11-03 17:17:45 UTC
So.. I've tested my hypothesis and your version does indeed result in 2 additional DB calls (the subselect being executed independently).. However, it does not change performance in any meaningful way (I benchamarked it)..

So.. for consistency and to be more Koha::Object like, not having to reach inside dbic guts to much I would take your approach.

Thanks for the followup :)
Comment 50 Jonathan Druart 2020-11-05 14:25:26 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 51 Lucas Gass 2020-11-16 21:59:59 UTC
enhancement will not be backported to 20.05.x