Using the syspref 'MarcFieldsToOrder', getting the discount directly from the MARC causes the price to actually increase by the discount percentage, not decrease, if the Vendor/Bookseller is configured to include the VAT/GST in the list price. REPLICATE ISSUE: -1. Define 'MarcFieldsToOrder'-syspref to match the discount and price columns of your staged MARC Records. 0. Stage a MARC batch. 1. Make and acquisition from the staged file using /acqui/addorderiso2709.pl (Save the big staging list, don't click "Add order" on an individual Biblio.) 2. Observe that the Replacement price (rrp) is actually the (price + discount) instead of (price - discount). And the estimated cost is just the price in the MARC Record without any discount. AFTER THIS PATCH: Replay step 1. 2. rrp is now the price in the MARC Record incl. VAT/GST. ecost is now the VAT/GST included discounted price. NOTE! Because the subroutine I modified doesn't make much sense, it is presumed that there can be regression in other parts for other kinds of vendor configurations. Because the acquisitions module is in urgent need of complete back-end refactoring + PageObject/Cucumber regression test coverage, no further investigation is made to resolve those.
Created attachment 41489 [details] [review] Bug 14680 - when doing acquisitions from a staged file, MarcFieldsToOrder-syspref discounts are instead added. Using the syspref 'MarcFieldsToOrder', getting the discount directly from the MARC causes the price to actually increase by the discount percentage, not decrease, if the Vendor/Bookseller is configured to include the VAT/GST in the list price. REPLICATE ISSUE: -1. Define 'MarcFieldsToOrder'-syspref to match the discount and price columns of your staged MARC Records. 0. Stage a MARC batch. 1. Make and acquisition from the staged file using /acqui/addorderiso2709.pl (Save the big staging list, don't click "Add order" on an individual Biblio.) 2. Observe that the Replacement price (rrp) is actually the (price + discount) instead of (price - discount). And the estimated cost is just the price in the MARC Record without any discount. AFTER THIS PATCH: Replay step 1. 2. rrp is now the price in the MARC Record incl. VAT/GST. ecost is now the VAT/GST included discounted price. NOTE! Because the subroutine I modified doesn't make much sense, it is presumed that there can be regression in other parts for other kinds of vendor configurations. Because the acquisitions module is in urgent need of complete back-end refactoring + PageObject/Cucumber regression test coverage, no further investigation is made to resolve those.
Failed QA because missing test coverage. Because the modification is in the controller, no unit testing is possible. Only way to test is with PageObjects. This needs a lot of scaffolding to be made to enable running tests on Acquisitions module.
Created attachment 41551 [details] [review] Bug 14680 - when doing acquisitions from a staged file, MarcFieldsToOrder-syspref discounts are instead added.
Still a critical bug?
Comment on attachment 41551 [details] [review] Bug 14680 - when doing acquisitions from a staged file, MarcFieldsToOrder-syspref discounts are instead added. Review of attachment 41551 [details] [review]: ----------------------------------------------------------------- ::: acqui/addorderiso2709.pl @@ +178,5 @@ > my $c_quantity = shift( @quantities ) || GetMarcQuantity($marcrecord, C4::Context->preference('marcflavour') ) || 1; > my $c_budget_id = shift( @budgets_id ) || $input->param('all_budget_id') || $budget_id; > my $c_discount = shift ( @discount); > + if ($c_discount) { > + $c_discount =~ s/%//g; #This screws up number conversion badly If there is a % sign, then we know it is a percent. Is it possible to have a non-percent discount? If not, then this okay. If so, then this whole section of code is messed up. @@ +179,5 @@ > my $c_budget_id = shift( @budgets_id ) || $input->param('all_budget_id') || $budget_id; > my $c_discount = shift ( @discount); > + if ($c_discount) { > + $c_discount =~ s/%//g; #This screws up number conversion badly > + $c_discount =~ s/,/./g; #Make this an actual digit for Perl And what if they do something stupid like: 1,000,00? What if they include 1000's? 1,000,000,000.00? We need a nice function to guess at an unlocalized number. @@ +180,5 @@ > my $c_discount = shift ( @discount); > + if ($c_discount) { > + $c_discount =~ s/%//g; #This screws up number conversion badly > + $c_discount =~ s/,/./g; #Make this an actual digit for Perl > + $c_discount = $c_discount / 100 if $c_discount >= 1; This seems like duplicate and wrong code. @@ +186,1 @@ > $c_discount = $c_discount / 100 if $c_discount > 1; But as noted in the duplicate code above, I think the condition might need correcting ">= 1" or perhaps "> 0"?
This is still valid, but I don't know if these code changes are the ones we need You can recreate this without MarcItemFieldsToOrder in fact. 1 - Stage a file 2 - Add to basket form a staged file 3 - For any of the titles check the box to add 4 - In the details set a price, replacement price, and discount 5 - Add the order 6 - ecost is set to price and rrp is the ecost plus dicsount 7 - Repeat but don't set a discount this time (and ensure bookseller has a discount set) 8 - rrp is set to price, ecost is the rrp minus the discount Why? It is from bug 7180, so ha been in place since 2014 There is much conversation on the bug, ending with: "(In reply to mathieu saby from comment #96) > To be clear : my main concern is the fact that Koha calculates a RRP price > > 20 EUR from a record with "20.00 EUR" in 010$d field. Forget the rrp value, it is not used by this patch." RRP is not really used for anything, it just says the price the vendor set, but we do have strange behaviour of sometimes the price field being the 'rrp' and sometimes being the 'ecost' I think ideally we should use the same display as when adding an order manually, so the user can choose the fields to enter, and we discount the rrp to generate ecost unless they specify. Adding some CC's here
Created attachment 137649 [details] [review] Bug 14680: Always subtract discoutn form price to calculate estimated cost This patch simply removes a discrepancty where when a discount is provided on the order form we add the discount to the price, and when it comes form the vendor we subtract the discount. Discounts should always descrease the estimated cost To test: 0 - Have a vendor with a discount of 10% specified 1 - Export a record from your Koha 2 - Stage the record for import and match on biblionumber 3 - Add to a basket in acq from the staged file 4 - Select the title, and set order price to $10 and do not fill the discount field 5 - Add the order - note $10 RRP and $9 actual/budgeted cost 6 - Repeat 2&3 7 - Select the tile, set price $10 and discount 50% 8 - Add the order - note $20 RRP and $10 actual/budgeted cost! FAIL 9 - Apply patch, restart all 10 - Repeat 2-7 11 - Add the order, note $10 RRP and $5 actual.budgeted cost
Created attachment 137650 [details] [review] Bug 14680: Always subtract discoutn form price to calculate estimated cost This patch simply removes a discrepancty where when a discount is provided on the order form we add the discount to the price, and when it comes form the vendor we subtract the discount. Discounts should always descrease the estimated cost To test: 0 - Have a vendor with a discount of 10% specified 1 - Export a record from your Koha 2 - Stage the record for import and match on biblionumber 3 - Add to a basket in acq from the staged file 4 - Select the title, and set order price to $10 and do not fill the discount field 5 - Add the order - note $10 RRP and $9 actual/budgeted cost 6 - Repeat 2&3 7 - Select the tile, set price $10 and discount 50% 8 - Add the order - note $20 RRP and $10 actual/budgeted cost! FAIL 9 - Apply patch, restart all 10 - Repeat 2-7 11 - Add the order, note $10 RRP and $5 actual.budgeted cost Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>
Hi Nick, the original bug description and the patch test plan don't match up. The original bug is about when the discount is pulled from the record, not the vendor. Should the patch be moved or the bug description be adjusted?
(In reply to Katrin Fischer from comment #9) > Hi Nick, > > the original bug description and the patch test plan don't match up. > > The original bug is about when the discount is pulled from the record, not > the vendor. > > Should the patch be moved or the bug description be adjusted? It's the same - when discount populated from file or entered manually it gets subtracted, will adjust title
I'd like to add that $c is really not a nice name for a variable, but that was there 'before patch': my $c = $c_discount ? $c_discount : $bookseller->discount;
Created attachment 137753 [details] [review] Bug 14680: Always subtract discount from price to calculate estimated cost This patch simply removes a discrepancty where when a discount is provided on the order form we add the discount to the price, and when it comes form the vendor we subtract the discount. Discounts should always descrease the estimated cost To test: 0 - Have a vendor with a discount of 10% specified 1 - Export a record from your Koha 2 - Stage the record for import and match on biblionumber 3 - Add to a basket in acq from the staged file 4 - Select the title, and set order price to $10 and do not fill the discount field 5 - Add the order - note $10 RRP and $9 actual/budgeted cost 6 - Repeat 2&3 7 - Select the tile, set price $10 and discount 50% 8 - Add the order - note $20 RRP and $10 actual/budgeted cost! FAIL 9 - Apply patch, restart all 10 - Repeat 2-7 11 - Add the order, note $10 RRP and $5 actual.budgeted cost Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Pushed to master for 22.11. Nice work everyone, thanks!
Backported to 22.05.x for 22.05.05
thx pushed to 21.11.x for 21.11.12
Backported: Pushed to 21.05.x branch for 21.05.19 Maybe if the documentation is detailed enough there could be something to change?
This is a bug fix, nothing to add to the manual as far as I can see.