Bug 7288

Summary: Invoice improvements - Show only subscriptions
Product: Koha Reporter: claire.hernandez <claire.hernandez>
Component: AcquisitionsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact:
Severity: enhancement    
Priority: P5 - low CC: fridolin.somers, gitbot, gmcharlt, jonathan.druart, julian.maurice, katrin.fischer, paola.rossi, ztajoli
Version: master   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 5339, 5343    
Bug Blocks: 7163    
Attachments: Bug 7288: Allow to display only subscriptions in the invoices table
Bug 7288: Allow to display only subscriptions in the invoices table
Bug 7288: Allow to display only subscriptions in the invoices table
Bug 7288: fix typo th/td
Bug 7288: Allow to display only subscriptions in the invoices table
Invoice improvements - Show only subscriptions
[PASSED QA] Bug 7288: Allow to display only subscriptions in the invoices table
Bug 7288: Set a boolean if the invoice if linked to subscriptions

Description claire.hernandez@biblibre.com 2011-12-02 09:17:19 UTC
adds ISSN to ISBN/EAN search field and a checkbox "show only subscriptions" in invoices.pl page so librarian can retrieve subscriptions invoices. (BibLibre MT6534)
Comment 1 Jonathan Druart 2012-11-05 09:40:59 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2012-11-05 09:42:46 UTC
The ISSN search field is already in master.
This patch depends on Bug 5343 (DB field aqorders.subscriptionid).
Comment 3 Galen Charlton 2013-05-31 03:34:25 UTC
Patch does not apply.  Please rebase and resubmit.
Comment 4 Jonathan Druart 2013-05-31 07:48:58 UTC Comment hidden (obsolete)
Comment 5 Paola Rossi 2013-10-14 13:02:12 UTC
The patch doesn't apply cleanly against master.
There is a merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoices.tt.
So I don't sign off the match, and I pass it to "failed QA".
Comment 6 Jonathan Druart 2013-10-15 10:03:09 UTC Comment hidden (obsolete)
Comment 7 Paola Rossi 2013-10-16 12:03:29 UTC
The applying (against master 031) of the patch is OK.
I've created some invoices for subscriptions' orders and some invoices not.
When the checkbox "Show only subscriptions" is not checked, the patch works well and all the invoices are shown in the invoices table.
On the contrary, when the checkbox "Show only subscriptions" is checked, no invoice is shown in the invoices table.
Is it OK?
This is the part of the patch about selecting only subscription' invoices:
resultst.fnFilter( "[0-9]+", 0, true )
Can I create the situation with "[0-9+]" and see the list of the subscriptions' invoices shown? Thanks.
When I see an invoice (from column "Invoice no." of the invoices table) and the invoice is not a subscription' one, I see "Invoice details" with a following table full of dates ("Summary" "Publisher" etc).
When I see an invoice (from column "Invoice no." of the invoices table) and the invoice is a subscription' one, I see "Invoice details" followed by "No orders yet". On the contrary the order is present in bd: it is listed by Orders-search' results table of Acquisitions, and is linked to the subscripted serial.
Comment 8 Jonathan Druart 2013-10-18 14:07:10 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2013-10-18 14:10:32 UTC
(In reply to Paola Rossi from comment #7)
Paola, I cannot reproduce your problem but I found a typo in the patch. Could your retest please?
The regex should match the subscription id in the first column of the table. Datatables uses it for filtering on subscriptions.
Comment 10 Paola Rossi 2013-10-28 14:42:29 UTC
Rebasing invoices.tt is neeeded.
Against master 3.13.00.032:
Applying: Bug 7288: Allow to display only subscriptions in the invoices table
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoices.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoices.tt
Auto-merging C4/Acquisition.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 7288: Allow to display only subscriptions in the invoices table

So I pass the patch to failed QA status.
Comment 11 Jonathan Druart 2013-10-28 15:43:40 UTC Comment hidden (obsolete)
Comment 12 Paola Rossi 2013-10-29 11:24:01 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2013-12-26 09:55:32 UTC
Created attachment 23815 [details] [review]
[PASSED QA] Bug 7288: Allow to display only subscriptions in the invoices table

This patch adds a checkbox "Show only subscriptions" in the invoices
table.
If this checkbox is checked, a filter is added on the table in order to
show only subscription invoices

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
To test:
- Test in a database with multiple existing invoices
- Create an order from a subscription, close basket, receive
- Test that the result table of the invoice search shows
  the new checkbox and that it works correctly

Passes all tests and QA script.
Also passes t/db_dependent/Acquisition.t and t/db_dependent/Acquisition/*.
Comment 14 Galen Charlton 2014-03-10 20:20:56 UTC
Comment on attachment 23815 [details] [review]
[PASSED QA] Bug 7288: Allow to display only subscriptions in the invoices table

Review of attachment 23815 [details] [review]:
-----------------------------------------------------------------

::: C4/Acquisition.pm
@@ +2504,4 @@
>      my $dbh = C4::Context->dbh;
>      my $query = qq{
>          SELECT aqinvoices.*, aqbooksellers.name AS suppliername,
> +          aqorders.subscriptionid,

This does not handle the case where a single invoice has more than one order with a subscription very well.  In particular, only one subscription would (arbitrarily) be displayed.

If the goal is to be able to link to all subscriptions on a given invoice, it's not sufficient to display just one.  If the goal is more simply to just be able to filter the invoice search for invoices that have one or more subscriptions, then it would be better calculate a Boolean flag.

Either way, as the patch stands now it is likely to cause confusion for libraries that have invoices that cover multiple subscriptions -- which is very, very common in the US, at least.

Failing QA.  When you resubmit, I would also appreciate if you wrote a unit test for the change in the core routine.
Comment 15 Jonathan Druart 2014-03-11 10:18:45 UTC
Created attachment 26065 [details] [review]
Bug 7288: Set a boolean if the invoice if linked to subscriptions

If an invoice is linked to subscription, we need to set a boolean to
true in order to filter them in the interface.
Comment 16 Jonathan Druart 2014-03-11 10:20:18 UTC
(In reply to Galen Charlton from comment #14)
> Failing QA.  When you resubmit, I would also appreciate if you wrote a unit
> test for the change in the core routine.

It is quite difficult to provide good unit tests here. To test if an invoice is linked to subscription, we should create a subscription (so a budget, a frequency, etc.).
Comment 17 Zeno Tajoli 2014-03-13 09:00:18 UTC
Sorry Galen,

do we need to write an extra unit test or the code is OK to be pushed in master ?

Zeno Tajoli
Comment 18 Galen Charlton 2014-04-21 05:20:11 UTC
Pushed to master, along with a follow-up that adds a direct unit test.

Thanks, Jonathan!
Comment 19 Fridolin Somers 2014-09-10 14:55:53 UTC
I choose to not apply to 3.14.x so I set as resolved.