Created attachment 9383 [details] Example of Updated Table FUND COLUMN adjustments Add a FUND column to the “Already Received” table, placing it between the “quantity” and “est cost” columns. The FUND column would contain the fund account number (e.g. 402000, GIFT, 20351, etc.) relating to the associated title on the same line in “Summary” column. The fund column would extend into the “subtotal” section on the “Already Received” table. In that section, each fund would have the subtotal of all titles received on that fund, under the “TOTAL” column. The “TOTAL” line on the “Already Received” table would be a total of all titles received on that receipt summary (which would be the total of all fund subtotals). See screen shot. HOLDS COLUMN or HOLDS INDICATOR Add a column to the “Already Received” table, placing it between the “order line” and “summary” columns. This column would contain a red X on the same line as a title that has holds. OR Make titles with holds, appear in red font, on the “Summary” line of the “Already Received” table.
Created attachment 9384 [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.
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt
Created attachment 11089 [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.
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)
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)
Resetting to needs signoff. If anyone has an opinion on comments 4/5, please let me know.
Created attachment 13152 [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.
This new patch solves the problem Chris brought up in comment 4.
On further view, I don't think it does solve the problem. I'll have to post a followup.
Created attachment 13153 [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.
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)
Created attachment 14892 [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.
This patch does not apply on master: error: patch failed: C4/Acquisition.pm:2336 error: C4/Acquisition.pm: patch does not apply
Created attachment 16336 [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.
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.
(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?
(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?
(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.
Patch tested with a sandbox, by Pierre Angot <tredok.pierre@gmail.com>
Created attachment 16635 [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>
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..)
Created attachment 16651 [details] screenshot problem with GST
Created attachment 16660 [details] screenshot problem with GST
I have not tested this problem. So failed qa
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
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
Created attachment 16749 [details] [review] Bug 8037 - Add holds and funds to items already received in parcel.pl - QA Followup
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.
Created attachment 18021 [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>
Created attachment 18022 [details] [review] Bug 8037 - Add holds and funds to items already received in parcel.pl - QA Followup
Created attachment 18023 [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.
(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?
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
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
(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
Created attachment 18693 [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>
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.
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.
Created attachment 19672 [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>
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.
Created attachment 19789 [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>
Created attachment 19790 [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>
Created attachment 19791 [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>
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.
Created attachment 22390 [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".
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>
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>
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>
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
Created attachment 22639 [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".
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>
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
Pushed to master, along with some follow-ups. Thanks, Kyle!