Bug 8854 - invoice filters don't all work
Summary: invoice filters don't all work
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low major (vote)
Assignee: Bugs List
QA Contact:
URL: cgi-bin/koha/acqui/invoices.pl
Keywords:
: 9186 11114 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-10-01 14:13 UTC by Nicole C. Engard
Modified: 2014-05-26 21:04 UTC (History)
7 users (show)

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


Attachments
bug 8854: fix some invoice search filters (4.65 KB, patch)
2013-01-29 17:53 UTC, Galen Charlton
Details | Diff | Splinter Review
title search still not working (35.29 KB, image/png)
2013-02-05 13:53 UTC, Nicole C. Engard
Details
title is on an invoice that shows with a vendor search (46.26 KB, image/png)
2013-02-05 13:54 UTC, Nicole C. Engard
Details
bug 8854: fix some invoice search filters (9.73 KB, patch)
2013-10-23 19:35 UTC, Galen Charlton
Details | Diff | Splinter Review
[SIGNED OFF] bug 8854: fix some invoice search filters (10.11 KB, patch)
2013-10-27 06:44 UTC, Katrin Fischer
Details | Diff | Splinter Review
bug 8854: fix some invoice search filters (10.20 KB, patch)
2013-10-28 22:01 UTC, Galen Charlton
Details | Diff | Splinter Review
[PASSED-QA] bug 8854: fix some invoice search filters (10.63 KB, patch)
2013-11-16 20:03 UTC, Brendan Gallagher
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2012-10-01 14:13:29 UTC
I am trying to do invoice searches and when I filter by branch or vendor or shipment date range I get no results, but if I search by number I see that those libraries and vendors do have invoices. It's possible that none of the other filters work, I didn't test them all.

Nicole
Comment 1 Nicole C. Engard 2012-10-18 18:36:20 UTC
This is a big issue that needs to be looked at before 3.10 release.
Comment 2 Nicole C. Engard 2012-12-10 21:36:14 UTC
*** Bug 9186 has been marked as a duplicate of this bug. ***
Comment 3 Galen Charlton 2013-01-29 17:53:31 UTC Comment hidden (obsolete)
Comment 4 Galen Charlton 2013-01-29 17:56:35 UTC
Note that the publication year search filter will work in UNIMARC but not MARC21.  This is because the default UNIMARC frameworks map a tag to the biblioitems.publicationyear column (which the date filter uses) but the default MARC21 frameworks do not.
Comment 5 Nicole C. Engard 2013-02-05 13:53:38 UTC
Created attachment 15055 [details]
title search still not working

Alot of the searches work - that didn't before.  I can now search by dates and vendor and library. But a search for title, author, isbn all fail even though there are results that match.  Is it possible that not only publication year is UNIMARC only? and if it's UNIMARC only is there a way to have it show only if you're using UNIMARC?

Nicole
Comment 6 Nicole C. Engard 2013-02-05 13:54:02 UTC
Created attachment 15056 [details]
title is on an invoice that shows with a vendor search
Comment 7 Joseph Alway 2013-05-29 17:45:12 UTC
We are using MARC21 and the only Search Filters working for the invoices are the invoice number, Vendor, and Billing date. The following searches are broken: Shipment date, ISBN / EAN / ISSN, Title, Author, Publisher, Publisher Year, and Library.
Comment 8 Jonathan Druart 2013-06-07 08:12:55 UTC
Galen, in fact the aqorders.branchcode field is added by bug 5336 (needs QA). So with this patch, the 2 join will become useless.
It seems there has been a mistake in bug's relations :-/
Comment 9 Galen Charlton 2013-10-23 18:56:28 UTC
(In reply to Jonathan Druart from comment #8)
> Galen, in fact the aqorders.branchcode field is added by bug 5336 (needs
> QA). So with this patch, the 2 join will become useless.
> It seems there has been a mistake in bug's relations :-/

Hmm, the patches for bug 5336, which is now pushed, do not add an aqorders.branchcode column.  Maybe something else does?
Comment 10 Galen Charlton 2013-10-23 19:35:45 UTC Comment hidden (obsolete)
Comment 11 Galen Charlton 2013-10-23 19:36:40 UTC
I have attached a replacement patch that should get all the filters working.  The question of aqorders.branchcode is still not settled, but I think the joins I've supplied are an acceptable compromise until such time as aqorders.branchcode is actually added.
Comment 12 Galen Charlton 2013-10-23 19:36:54 UTC
*** Bug 11114 has been marked as a duplicate of this bug. ***
Comment 13 Jonathan Druart 2013-10-24 09:19:20 UTC
(In reply to Galen Charlton from comment #9)
> Hmm, the patches for bug 5336, which is now pushed, do not add an
> aqorders.branchcode column.  Maybe something else does?

Galen,
Finally I decided to abandon this field (see bug 11001).
I am not sure searching by branch for invoices is relevant since orders from different branches can exist for a same invoice.
Moreover I don't know if filtering on the patron branchcode is the better choice: if the aqbasketgroups.billingplace is different, results could be a source of misunderstanding. Given that libraries have different work flow for acquisitions, results could be good for some and wrong for others.
Maybe should we simply remove the search by library for invoices?

I'm adding Katrin and Mathieu to this discussion.
Comment 14 Mathieu Saby 2013-10-24 09:53:56 UTC
(In reply to Jonathan Druart from comment #13)
> (In reply to Galen Charlton from comment #9)
> > Hmm, the patches for bug 5336, which is now pushed, do not add an
> > aqorders.branchcode column.  Maybe something else does?
> 
> Galen,
> Finally I decided to abandon this field (see bug 11001).
> I am not sure searching by branch for invoices is relevant since orders from
> different branches can exist for a same invoice.
> Moreover I don't know if filtering on the patron branchcode is the better
> choice: if the aqbasketgroups.billingplace is different, results could be a
> source of misunderstanding. Given that libraries have different work flow
> for acquisitions, results could be good for some and wrong for others.
> Maybe should we simply remove the search by library for invoices?
> 
> I'm adding Katrin and Mathieu to this discussion.

Hi
Not sure of what could be the best solution.

But I can tell you how we deal with that in Rennes 2 : 

But the only time whe had to search invoices in Koha, it was to compare the number and amount of invoices of Koha with the financial tool of the University (Sifac), to fix discrepancies (invoices received in one tool and not in the other...)
Koha invoices display was not good for that, so I had to make a SQL report.
The result look like that
Poste budgetaire	Fournisseur	N° facture	Nom facture	Frais de port (montant)	Frais de ports (poste)	Qté reçue	Cout reel	Prix public TTC	Prix remisé TTC	Cout reel * Qté	Cout reel * Qté + Frais de ports

The first column "Poste budgétaire" is the name of the fund, which in my library includes the name of the branch. For ex : "BMUS" for Library of MUSic.

But if the name of the fund had not included the name of the branch, I probably would have added 2 columns with basketgroup.billingplace and basketgroup.deliveryplace.
Because, as we use basketgroups are our main "grouping entity" for our relations with our vendors (they receive no info at all about our Koha baksets), the relevant information for us are basketgroup fields.
But in some libraries it could be baskets fields I presume.


By the way, the filling of basketgroup.{delivery/billing}place should be syspref dependant. Filling it with the connection site of the librarian is not good at all for us, and I suppose we are not an isolated case.

Mathieu
Comment 15 Galen Charlton 2013-10-24 17:01:21 UTC
(In reply to mathieu saby from comment #14)
> By the way, the filling of basketgroup.{delivery/billing}place should be
> syspref dependant. Filling it with the connection site of the librarian is
> not good at all for us, and I suppose we are not an isolated case.

I'm not sure it warrants a syspref, and I'd prefer that deliveryplace and billingplace be FKs to the branches table.  Would adding a freebillingplace column in addition to the exsting freedeliveryplace column meet your needs?
Comment 16 Katrin Fischer 2013-10-27 06:44:44 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2013-10-28 10:35:45 UTC
Katrin,

could you confirm the unit tests pass for you?
I got:
% prove t/Acquisition/Invoice.t
t/Acquisition/Invoice.t .. 1/49 
#   Failed test at t/Acquisition/Invoice.t line 47.
#          got: '16'
#     expected: '15'

#   Failed test at t/Acquisition/Invoice.t line 62.
#          got: 'publicationyear'
#     expected: 'branchcode'
# Looks like you failed 2 tests of 49.
Comment 18 Katrin Fischer 2013-10-28 21:43:00 UTC
Hm, Jonathan, you are right, the tests are failing for me too. I must have missed it.
Comment 19 Galen Charlton 2013-10-28 22:01:09 UTC Comment hidden (obsolete)
Comment 20 Brendan Gallagher 2013-11-16 20:03:41 UTC
Created attachment 22976 [details] [review]
[PASSED-QA] bug 8854: fix some invoice search filters

Fix the supplier, shipment date, and library filters
on the invoice search.  An invoice's library is
(in parallel with order search) defined as the library
of the staff member that approved the basket.  Before this
patch, the code was referring to an aqorders.branchcode
column that doesn't exist.

This patch also improves the author, title, ISBN/EAN/ISSN,
publisher, and publication year filters to no longer require
exact matches; substring matches now suffice.

Finally, this patch considers biblio.copyrightdate in addition
to biblioitems.publicationyear for publication date searches, as
the MARC21 frameworks use the former column but not the latter.

This patch also fixes the current test cases for invoices
so that they pass and adds regression tests.

Test plan:

[1] Create two invoices for different vendors.
[2] Do an invoice search and filter on shipment
    date.  Verify that the expected invoice(s)
    are returned.
[3] Do an invoice search and filter on branch
    (of the staff member that approved the basket).
    Verify that the expected invoice(s) are returned.
[4] Do an invoice search and filter on supplier.
    Verify that the expected invoice(s) are returned.
[5] Do invoice searches on author, title, ISBN/EAN/ISSN,
    publisher, and publication year and verify that the
    results are as expected.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Patch passes all tests, test plan and QA script.

(Adding from Katrin notes early) I agree with
Possible improvements:
- Document the behaviour of the library search as there are
  lots of branches all over acquisitions with different meaning.
- Add the shipment date to the results list table
- Change label ISBN/EAN/ISSN to not include EAN for MARC21
  installations
Comment 21 Mathieu Saby 2013-11-16 20:12:03 UTC
> - Change label ISBN/EAN/ISSN to not include EAN for MARC21
>   installations

Hi
Just to know : does not UPC in America plays the same role as EAN in Europe?
(for us, EAN = ISBN13 (even if the book is an old book with only an isbn10). 
And EAN is also used for non-books documents which does not have isbn)

Mathieu
Comment 22 Galen Charlton 2013-11-19 16:13:18 UTC
Pushed to master and 3.14.x.