Bug 8037 - Add holds and funds to items already received in parcel.pl
Summary: Add holds and funds to items already received in parcel.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Jonathan Druart
URL:
Keywords: rel_3_14_candidate
Depends on:
Blocks:
 
Reported: 2012-05-01 18:05 UTC by Kyle M Hall
Modified: 2014-05-26 21:04 UTC (History)
11 users (show)

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


Attachments
Example of Updated Table (110.57 KB, image/jpeg)
2012-05-01 18:05 UTC, Kyle M Hall
Details
Bug 8037 - Add holds and funds to items already received in parcel.pl (10.67 KB, patch)
2012-05-01 18:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8037 - Add holds and funds to items already received in parcel.pl (10.59 KB, patch)
2012-07-23 20:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8037 - Add holds and funds to items already received in parcel.pl (8.05 KB, patch)
2012-10-31 18:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8037 - Add holds and funds to items already received in parcel.pl (9.43 KB, patch)
2012-10-31 18:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8037 - Add holds and funds to items already received in parcel.pl (10.04 KB, patch)
2013-01-28 14:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8037 - Add holds and funds to items already received in parcel.pl (10.04 KB, patch)
2013-03-19 09:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8037 - Add holds and funds to items already received in parcel.pl (10.10 KB, patch)
2013-03-21 13:57 UTC, Paul Poulain
Details | Diff | Splinter Review
screenshot problem with GST (56.59 KB, image/jpeg)
2013-03-21 14:44 UTC, Koha Team Lyon 3
Details
screenshot problem with GST (56.21 KB, image/jpeg)
2013-03-21 14:55 UTC, Koha Team Lyon 3
Details
Bug 8037 - Add holds and funds to items already received in parcel.pl - QA Followup (2.25 KB, patch)
2013-03-22 10:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8037 - Add holds and funds to items already received in parcel.pl (10.10 KB, patch)
2013-05-08 13:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8037 - Add holds and funds to items already received in parcel.pl - QA Followup (1.92 KB, patch)
2013-05-08 13:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8037 - Add holds and funds to items already received in parcel.pl - QA Followup 2 (1.82 KB, patch)
2013-05-08 13:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8037 - Add holds and funds to items already received in parcel.pl - QA Followup 2 (1.88 KB, patch)
2013-06-06 08:05 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 8037 - Add holds and funds to items already received in parcel.pl - QA Followup 2 (1.72 KB, patch)
2013-07-15 15:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8037 - Add holds and funds to items already received in parcel.pl (10.16 KB, patch)
2013-07-19 10:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8037 - Add holds and funds to items already received in parcel.pl - QA Followup (1.99 KB, patch)
2013-07-19 10:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8037 - Add holds and funds to items already received in parcel.pl - QA Followup 2 (1.79 KB, patch)
2013-07-19 10:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
duplication of baskets on the already-received table (127.35 KB, image/png)
2013-10-22 00:07 UTC, Galen Charlton
Details
Bug 8037: FIX bad sql query and variable renaming (6.24 KB, patch)
2013-10-25 12:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8037 - Add holds and funds to items already received in parcel.pl (10.32 KB, patch)
2013-10-31 13:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8037 - Add holds and funds to items already received in parcel.pl - QA Followup (2.02 KB, patch)
2013-10-31 13:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8037 - Add holds and funds to items already received in parcel.pl - QA Followup 2 (1.76 KB, patch)
2013-10-31 13:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8037: FIX bad sql query and variable renaming (6.26 KB, patch)
2013-10-31 13:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8037: FIX bad sql query and variable renaming (6.35 KB, patch)
2013-10-31 14:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2012-05-01 18:05:56 UTC Comment hidden (obsolete)
Comment 1 Kyle M Hall 2012-05-01 18:12:36 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2012-07-15 09:34:29 UTC
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt
Comment 3 Kyle M Hall 2012-07-23 20:31:47 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2012-07-24 02:04:53 UTC
This is mostly working well.

However we have one issue.

The patch is doing this
+    my ($count) = &GetReservesFromBiblionumber($line{biblionumber});
+    $line{holds} = $count;

So if I have 3 items attached to a biblio, one of them being one I received and can see on this screen, it will tell me I have a hold on it.
But I might have a hold on one specific item, not the one attached to this order.

Do we care about that? (IE this works fine for biblio level holds, false positives for item level ones)
Comment 5 Kyle M Hall 2012-07-25 13:25:28 UTC
GetParcels does not return any itemnumbers, so we would have to add an additional check to aqorders_items. Then we could modify GetReservesFromBiblionumber to add an optional itemnumber parameter such that the count returned is only for bib-level reserves + item level reserves for that item.

Should I file a separate bug for this behavior, or do you feel that it is important enough that I write a followup?


(In reply to comment #4)
> This is mostly working well.
> 
> However we have one issue.
> 
> The patch is doing this
> +    my ($count) = &GetReservesFromBiblionumber($line{biblionumber});
> +    $line{holds} = $count;
> 
> So if I have 3 items attached to a biblio, one of them being one I received
> and can see on this screen, it will tell me I have a hold on it.
> But I might have a hold on one specific item, not the one attached to this
> order.
> 
> Do we care about that? (IE this works fine for biblio level holds, false
> positives for item level ones)
Comment 6 Kyle M Hall 2012-09-26 11:44:32 UTC
Resetting to needs signoff. If anyone has an opinion on comments 4/5, please let me know.
Comment 7 Kyle M Hall 2012-10-31 18:09:07 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2012-10-31 18:10:00 UTC
This new patch solves the problem Chris brought up in comment 4.
Comment 9 Kyle M Hall 2012-10-31 18:11:35 UTC
On further view, I don't think it does solve the problem. I'll have to post a followup.
Comment 10 Kyle M Hall 2012-10-31 18:31:13 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2012-12-27 11:50:58 UTC
Hi Kyle,
2 problems:
- Your patch breaks the sort on the table (js error: "a.aoColumns[c] is undefined")
- The total per fund is wrong I think, you don't multiply by the quantity (in your example the total for "Test Fund 1" would be 180)
Comment 12 Kyle M Hall 2013-01-28 14:39:13 UTC Comment hidden (obsolete)
Comment 13 Alex Arnaud 2013-03-18 13:09:17 UTC
This patch does not apply on master:

error: patch failed: C4/Acquisition.pm:2336
error: C4/Acquisition.pm: patch does not apply
Comment 14 Kyle M Hall 2013-03-19 09:29:02 UTC Comment hidden (obsolete)
Comment 15 Koha Team Lyon 3 2013-03-20 10:06:07 UTC
The new columns are added. I try an hold on a specific item : column holds is filled only for the item on hold. 
But if a tax rate is added in the vendor and the invoice price don't include tax, the tax has to be added in the subtotal per fund. Mark "failed QA" for this point.
Comment 16 Kyle M Hall 2013-03-20 16:50:39 UTC
(In reply to comment #15)
> The new columns are added. I try an hold on a specific item : column holds
> is filled only for the item on hold. 
> But if a tax rate is added in the vendor and the invoice price don't include
> tax, the tax has to be added in the subtotal per fund. Mark "failed QA" for
> this point.

I'm having a hard time figuring out how this should be solved. Could you post a screenshot and describe which numbers should be changed?
Comment 17 Jonathan Druart 2013-03-21 08:47:22 UTC
(In reply to comment #16)
> (In reply to comment #15)
> > The new columns are added. I try an hold on a specific item : column holds
> > is filled only for the item on hold. 
> > But if a tax rate is added in the vendor and the invoice price don't include
> > tax, the tax has to be added in the subtotal per fund. Mark "failed QA" for
> > this point.
> 
> I'm having a hard time figuring out how this should be solved. Could you
> post a screenshot and describe which numbers should be changed?

Same here. Koha Team Lyon 3, could you describe more precisely the issue please?
Comment 18 Kyle M Hall 2013-03-21 09:13:56 UTC
(In reply to comment #15)
> The new columns are added. I try an hold on a specific item : column holds
> is filled only for the item on hold. 
> But if a tax rate is added in the vendor and the invoice price don't include
> tax, the tax has to be added in the subtotal per fund. Mark "failed QA" for
> this point.

I'm going to reset this to needs signoff and here is my reasoning. The subtotals should total estimated and actual costs as they are displayed. I don't think it would be a good idea to "magically" add the tax in which would make the numbers not add up correctly. I hope that makes sense.
Comment 19 Paul Poulain 2013-03-21 13:57:31 UTC
Patch tested with a sandbox, by Pierre Angot <tredok.pierre@gmail.com>
Comment 20 Paul Poulain 2013-03-21 13:57:52 UTC Comment hidden (obsolete)
Comment 21 Koha Team Lyon 3 2013-03-21 14:44:04 UTC
I complete my comment :
1. change gist in syspref
2. edit a vendor and change tax rate and choose invoice prices  don't include tax
3. make some orders
4. when you receive your books : you put a price HT in actual cost 
5. in the table of "already receive" you only see the HT price but your library have to pay TTC price, so you don't see the amount that really be deduct from your fund...
(I attach a screenshot..)
Comment 22 Koha Team Lyon 3 2013-03-21 14:44:43 UTC Comment hidden (obsolete)
Comment 23 Koha Team Lyon 3 2013-03-21 14:55:31 UTC Comment hidden (obsolete)
Comment 24 Pierre Angot 2013-03-21 15:30:21 UTC
I have not tested this problem. So failed qa
Comment 25 Brendan Gallagher 2013-03-21 16:45:13 UTC
A group here at the hackfest communicated and felt that they were asking for something that was outside of this scope of this patch.

Setting back to signed off
Comment 26 Paul Poulain 2013-03-22 08:17:06 UTC
QA comments:
 * the new sub GetBudgetByOrderNumber needs a unit test
 * there is an unconditionnal warn:
+    warn "ITEMNUMBER: $itemnumber";
 * instead of CURRENT_DATE() please use cast(now() AS date) 
http://wiki.koha-community.org/wiki/PostgreSQL

sorry, failed QA
Comment 27 Kyle M Hall 2013-03-22 10:35:54 UTC Comment hidden (obsolete)
Comment 28 Paul Poulain 2013-03-22 15:49:52 UTC
OK, after some more explanations in french from Sonia, I think I've got the point she wanted to explain: whatever is the GIST status of the prices you've setup, the final cost for you will be the "all taxes included" price, so it's the "all included" price that must be removed from the budget.
In other terms, it means that the budget split should not just sum all lines.

How to fix this problem ? The best option, imo, would be to completely relook this screen to have both GIST excluded&included prices & totals & sub-total
But that's a long term goal.
So, at the end, I feel what would be enough is to just display a message if the vendor is "prices exclude GIST" saying ("GIST not summed", or something like that)

In the meantime, failed QA.
Comment 29 Kyle M Hall 2013-05-08 13:08:37 UTC Comment hidden (obsolete)
Comment 30 Kyle M Hall 2013-05-08 13:08:41 UTC Comment hidden (obsolete)
Comment 31 Kyle M Hall 2013-05-08 13:08:44 UTC Comment hidden (obsolete)
Comment 32 Kyle M Hall 2013-05-08 13:09:37 UTC
(In reply to comment #28)
> OK, after some more explanations in french from Sonia, I think I've got the
> point she wanted to explain: whatever is the GIST status of the prices
> you've setup, the final cost for you will be the "all taxes included" price,
> so it's the "all included" price that must be removed from the budget.
> In other terms, it means that the budget split should not just sum all lines.
> 
> How to fix this problem ? The best option, imo, would be to completely
> relook this screen to have both GIST excluded&included prices & totals &
> sub-total
> But that's a long term goal.
> So, at the end, I feel what would be enough is to just display a message if
> the vendor is "prices exclude GIST" saying ("GIST not summed", or something
> like that)
> 
> In the meantime, failed QA.

Paul, is this second followup what you are looking for?
Comment 33 Melia Meggs 2013-06-05 20:41:24 UTC
Hi Paul and Sonia,

Just checking in - any chance you could retest this with Kyle's new follow up patch to see if it resolves the problem you described?  If so, that would be wonderful!

Thank you,

Melia
Comment 34 Pierre Angot 2013-06-06 07:10:00 UTC
Sandbox setup by tredok.pierre@gmail.com with database 2 and bug 8037 on Thu Jun 6 09:08:01 2013

Something went wrong !
Applying: Bug 8037 - Add holds and funds to items already received in parcel.pl
Applying: Bug 8037 - Add holds and funds to items already received in parcel.pl - QA Followup
Applying: Bug 8037 - Add holds and funds to items already received in parcel.pl - QA Followup 2
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 8037 - Add holds and funds to items already received in parcel.pl - QA Followup 2
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Bug 8037 - Add holds and funds to items already received in parcel.pl

Bug 8037 - Add holds and funds to items already received in parcel.pl
Apply? [yn]
Bug 8037 - Add holds and funds to items already received in parcel.pl - QA Followup
Apply? [yn]
Bug 8037 - Add holds and funds to items already received in parcel.pl - QA Followup 2
Apply? [yn]
Patch left in /tmp/Bug-8037---Add-holds-and-funds-to-items-already-re-9gNZuE.patch
Comment 35 Jonathan Druart 2013-06-06 07:24:31 UTC
(In reply to comment #34)
> Sandbox setup by tredok.pierre@gmail.com with database 2 and bug 8037 on Thu
> Jun 6 09:08:01 2013
> 
> Something went wrong !

Works for me
Comment 36 Paul Poulain 2013-06-06 08:05:24 UTC
Patch tested with a sandbox, by Pierre Angot <tredok.pierre@gmail.com>
Comment 37 Paul Poulain 2013-06-06 08:05:46 UTC Comment hidden (obsolete)
Comment 38 Pierre Angot 2013-06-06 08:09:44 UTC
I've created a basket with 2 order. I've placed a hold on one of them. The second order was associated with an other fund.

The total per fund is correct. 

It maybe a good idea to add the holds columns in Pending orders.
Comment 39 Marcel de Rooy 2013-07-11 12:57:35 UTC
Applying: Bug 8037 - Add holds and funds to items already received in parcel.pl - QA Followup 2
fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Comment 40 Kyle M Hall 2013-07-15 15:30:45 UTC Comment hidden (obsolete)
Comment 41 Jonathan Druart 2013-07-19 10:07:08 UTC
QA comment:

Looks good to me.

minor:
- a css class named font-weight-normal is not well chosen.
- unit test inserts manually an order instead of calling C4::Acq::AddOrder

Inconsistent things still exist on this page but this patch does not introduce regression neither add another inconsistency.

I validated this patch with Sonia (from the Koha Team Lyon 3), thanks to her!

Marked as Passed QA.
Comment 42 Jonathan Druart 2013-07-19 10:12:00 UTC Comment hidden (obsolete)
Comment 43 Jonathan Druart 2013-07-19 10:12:07 UTC Comment hidden (obsolete)
Comment 44 Jonathan Druart 2013-07-19 10:12:13 UTC Comment hidden (obsolete)
Comment 45 Galen Charlton 2013-10-22 00:07:24 UTC
Created attachment 22244 [details]
duplication of baskets on the already-received table

In testing I discovered that the change to GetInvoiceDetails() resulted in each order being returned multiple times if more than zero or one items was attached to it.  I've attached a screenshot demonstrating this, and am setting this to failed QA.

One suggestion -- leave GetInvoiceDetails alone, and use GetItemnumbersFromOrder() to grab the list of items.
Comment 46 Jonathan Druart 2013-10-25 12:32:18 UTC Comment hidden (obsolete)
Comment 47 Marcel de Rooy 2013-10-31 13:08:36 UTC
Created attachment 22636 [details] [review]
Bug 8037 - Add holds and funds to items already received in parcel.pl

Adds a column to indicate holds on recieved items, as well as adding
a new column for fund and showing the subtotals per fund above
the total subtotal.

Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 48 Marcel de Rooy 2013-10-31 13:08:44 UTC
Created attachment 22637 [details] [review]
Bug 8037 - Add holds and funds to items already received in parcel.pl - QA Followup

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 49 Marcel de Rooy 2013-10-31 13:08:51 UTC
Created attachment 22638 [details] [review]
Bug 8037 - Add holds and funds to items already received in parcel.pl - QA Followup 2

Adds the note "GST exc." for subtotals by fund.

Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 50 Marcel de Rooy 2013-10-31 13:11:11 UTC
Rebased the "first three" but this last followup 

Applying: Bug 8037: FIX bad sql query and variable renaming
fatal: sha1 information is lacking or useless (C4/Acquisition.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 8037: FIX bad sql query and variable renaming
Comment 51 Jonathan Druart 2013-10-31 13:18:05 UTC Comment hidden (obsolete)
Comment 52 Marcel de Rooy 2013-10-31 14:13:37 UTC
Created attachment 22647 [details] [review]
Bug 8037: FIX bad sql query and variable renaming

The first patch does a left join on aqorders_items which returns too
much order lines.

This patch follows the Galen's suggestion: it removes the join and calls
the GetItemnumbersFromOrder routine for retrieving itemnumbers.

Bonus: the "parcelitems" variable is badly named and obfuscates the code.
I changed it for "orders".

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 53 Marcel de Rooy 2013-10-31 14:18:11 UTC
QA Comment:
I signed off on the last patch and looked at that one specifically.

no koha-qa warnings on all four patches
there was no test plan but I tested with AcqCreateItem=receiving, two orders on one basket, received lines separately with second partially, placed hold on the first item
ran perl t/db_dependent/Acquisition/Invoices.t: pass
ran perl t/Acquisition/Invoice.t: pass

I checked the calls to GetInvoiceDetails; removing the itemnumber from aqorder_items seems to be no problem :) and solves the duplication problem.
Furthermore, the code looks good to me (code in last patch).

Passed QA
Comment 54 Galen Charlton 2013-10-31 15:19:50 UTC
Pushed to master, along with some follow-ups.

Thanks, Kyle!