Bug 14680 - When creating orders from a staged file discounts supplied in the form are added
Summary: When creating orders from a staged file discounts supplied in the form are added
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks: 32167
  Show dependency treegraph
 
Reported: 2015-08-14 08:31 UTC by Olli-Antti Kivilahti
Modified: 2023-12-28 20:43 UTC (History)
10 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:
22.11.00, 22.05.05, 21.11.12, 21.05.19


Attachments
Bug 14680 - when doing acquisitions from a staged file, MarcFieldsToOrder-syspref discounts are instead added. (4.35 KB, patch)
2015-08-14 08:34 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 14680 - when doing acquisitions from a staged file, MarcFieldsToOrder-syspref discounts are instead added. (4.35 KB, patch)
2015-08-17 12:18 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 14680: Always subtract discoutn form price to calculate estimated cost (2.54 KB, patch)
2022-07-12 18:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14680: Always subtract discoutn form price to calculate estimated cost (2.61 KB, patch)
2022-07-12 18:39 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 14680: Always subtract discount from price to calculate estimated cost (2.66 KB, patch)
2022-07-15 13:05 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Olli-Antti Kivilahti 2015-08-14 08:31:32 UTC
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.
Comment 1 Olli-Antti Kivilahti 2015-08-14 08:34:56 UTC Comment hidden (obsolete)
Comment 2 Olli-Antti Kivilahti 2015-08-14 08:36:38 UTC
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.
Comment 3 Olli-Antti Kivilahti 2015-08-17 12:18:53 UTC
Created attachment 41551 [details] [review]
Bug 14680 - when doing acquisitions from a staged file, MarcFieldsToOrder-syspref discounts are instead added.
Comment 4 Marc Véron 2015-12-24 13:25:02 UTC
Still a critical bug?
Comment 5 Mark Tompsett 2019-06-25 14:39:49 UTC
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"?
Comment 6 Nick Clemens 2019-11-25 13:45:47 UTC
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
Comment 7 Nick Clemens 2022-07-12 18:19:18 UTC
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
Comment 8 ByWater Sandboxes 2022-07-12 18:39:28 UTC
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>
Comment 9 Katrin Fischer 2022-07-15 11:24:47 UTC
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?
Comment 10 Nick Clemens 2022-07-15 12:15:50 UTC
(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
Comment 11 Katrin Fischer 2022-07-15 13:04:10 UTC
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;
Comment 12 Katrin Fischer 2022-07-15 13:05:33 UTC
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>
Comment 13 Tomás Cohen Arazi 2022-07-18 14:53:28 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 14 Lucas Gass 2022-08-23 17:25:37 UTC
Backported to 22.05.x for 22.05.05
Comment 15 Arthur Suzuki 2022-08-30 09:20:54 UTC
thx pushed to 21.11.x for 21.11.12
Comment 16 Victor Grousset/tuxayo 2022-09-10 00:04:11 UTC
Backported: Pushed to 21.05.x branch for 21.05.19

Maybe if the documentation is detailed enough there could be something to change?
Comment 17 Caroline Cyr La Rose 2023-02-27 17:09:50 UTC
This is a bug fix, nothing to add to the manual as far as I can see.