Bug 29607

Summary:	addorderiso2709: The stored discount when importing an order from a file is invalid
Product:	Koha	Reporter:	Christophe TORIN <christophe.torin>
Component:	Acquisitions	Assignee:	Christophe TORIN <christophe.torin>
Status:	CLOSED FIXED	QA Contact:	Katrin Fischer <katrin.fischer>
Severity:	minor
Priority:	P5 - low	CC:	arthur.suzuki, david, jonathan.druart, lucas, wainuiwitikapark
Version:	unspecified
Hardware:	All
OS:	All
See Also:	https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14680
Change sponsored?:	---	Patch complexity:	Small patch
Documentation contact:		Documentation submission:
Text to go in the release notes:	This fixes how the discount amount for an order is stored and shown when an order is added to a basket using "From staged MARC records". The discount amount was incorrectly stored in the database and shown incorrectly when modifying the order (for example, a 25% discount shown as 0.2500 in the database and .25% on the form). This would result in the order amount changing when modifying an order.	Version(s) released in:	22.11.00, 22.05.03, 21.11.19
Circulation function:
Attachments:	Patch for the bug 29607 Bug 29607: Store the discount amount correctly when importing and order from a file Bug 29607: Store the discount amount correctly when importing and order from a file Bug 29607: Store the discount amount correctly when importing and order from a file

Description Christophe TORIN 2021-12-01 09:22:07 UTC

In the Acquisitions module :
- Create a new vendor with a discount (Or select one that has a discount). Here the discount will be 25%
- Create a new basket or select an existing one
- Select "Add to basket" => From a staged file
- Add an order from one of the file
	==> Here, you can use a specific discount for the order, to override the vendor discount. The result should be quite the same if you do, but with this discount
- The order is created ==> Modify the order
- The discount is 0.25% instead of 25%

Note that the prices are initially correctly calculated, but the discount value will appear as false. Plus, if you modify the order and change a price, the wrong discount will be used.
In database, the discount is indeed stored as a value between 0 and 1, but only in that case (for the vendor, it is the actual percentage, and when you create an order from anoher source, it is stored correctly)


This was reported on Koha 20.11, but is still relevant on master.


In the code, this happens because the discount value is first divided by 100 to calculate the price, but it is that divided value that is stored in database, instead of the whole percentage value.

On the master branch, in the file Koha/acqui/addorderiso2709.pl, the line where it's done are lines 166, 279 and 340
What I would suggest is : 
- Remove the division per 100 on those lines (166, 279 and 340)
- Divide per 100 only when calculating the price (lines 283, 285, 344, 346) ==> ( 1 - $c/100 ) instead of ( 1 - $c )
- In the template (for importing from file), add a "%" just to be clear that the discount is a percentage, and not a value between 0 and 1

I know there was a kind of security (divide by 100 if the input is > 1), but it could be the other way around (multiply by 100 if the input is < 1). But as it is based on the field that is in the form, with a simple "%" indication it should not be needed.

Comment 1 Jonathan Druart 2021-12-01 10:37:19 UTC

Hello Christophe, that looks correct, can you provide a patch please?

Comment 2 Christophe TORIN 2021-12-01 13:51:23 UTC

Created attachment 128127 [details] [review]
Patch for the bug 29607

Here is the patch I suggest for the resolution of the bug.

With this, the discount is properly stored.

I oversaw this but the '%' is already present in the template.

Comment 3 David Nind 2022-06-01 01:11:17 UTC

I get this message when trying to apply the patch (using koha-testing-docker) - I think that this means the patch isn't correctly formatted, but I could be wrong:

git bz apply 29607

Bug 29607 - addorderiso2709: The stored discount when importing an order from a file is invalid

128127 - Patch for the bug 29607

Apply? [(y)es, (n)o, (i)nteractive] y
Traceback (most recent call last):
  File "/usr/bin/git-bz", line 2716, in <module>
    applied = do_apply(bug_ref)
  File "/usr/bin/git-bz", line 1827, in do_apply
    users = bug.server.get_xmlrpc_proxy().User.get({ 'names': [patch.attacher] })['users']
  File "/usr/lib/python2.7/xmlrpclib.py", line 1243, in __call__
    return self.__send(self.__name, args)
  File "/usr/lib/python2.7/xmlrpclib.py", line 1602, in __request
    verbose=self.__verbose
  File "/usr/lib/python2.7/xmlrpclib.py", line 1283, in request
    return self.single_request(host, handler, request_body, verbose)
  File "/usr/lib/python2.7/xmlrpclib.py", line 1331, in single_request
    response.msg,
xmlrpclib.ProtocolError: <ProtocolError for bugs.koha-community.org/xmlrpc.cgi: 404 Not Found>

Testing notes
~~~~~~~~~~~~~

The QA Team will probably want the commit message formatted according to https://wiki.koha-community.org/wiki/Commit_messages I have drafted a test plan, based on the comments in the bug description.

Bug 29607: Store the discount amount correctly when importing and order from a file

This bug fixes how the discount amount for an order is stored, when an order is added to a basket using "From staged MARC records".

Test plan:
1. Export a record (without the items) (Tools > Catalog > Export data).
2. Delete the record from the catalog (including any items).
3. Stage the record for import (Tools > Catalog > Stage MARC records for import).
4. Create a new vendor with a discount of 25%. (Or select and existing vendor that has a discount). (Acquisitions > New vendor)
5. Create a new basket for the vendor. (Acquisitions > Manage orders > search for vendors > New > Basket).
6. Add to the basket using "From a staged file":
   . select "Add orders" next to the staged file
   . select the record to add to the order
   . enter a price
   . leave the discount field blank
   . select a fund
   . select "Save"
   ==> The order is created!
7. Modify the order - note that the discount is showing on the form as .25% instead of 25%, also note that the discount amount is calculated correctly.
8. Check what is recorded in the database:
   . koha-mysql kohadev
   . select * from aqorders;
   ==> discount field for the basket item shows as 0.2500
9. Change the discount to 25%. Run step 8 again - discount amount will be correctly shown as 25.0000
10. Apply the patch.
11. Repeat steps 1-9 - discount amount is shown and calculated correctly.
12. Test modifying the discount amount - should be calculated and shown correctly.
13. Sign off!

See additional comments in the bug description.

Comment 4 Jonathan Druart 2022-06-01 12:16:13 UTC

Created attachment 135538 [details] [review]
Bug 29607: Store the discount amount correctly when importing and order from a file

This bug fixes how the discount amount for an order is stored, when an order is added to a basket using "From staged MARC records".

Test plan:
1. Export a record (without the items) (Tools > Catalog > Export data).
2. Delete the record from the catalog (including any items).
3. Stage the record for import (Tools > Catalog > Stage MARC records for import).
4. Create a new vendor with a discount of 25%. (Or select and existing vendor that has a discount). (Acquisitions > New vendor)
5. Create a new basket for the vendor. (Acquisitions > Manage orders > search for vendors > New > Basket).
6. Add to the basket using "From a staged file":
   . select "Add orders" next to the staged file
   . select the record to add to the order
   . enter a price
   . leave the discount field blank
   . select a fund
   . select "Save"
   ==> The order is created!
7. Modify the order - note that the discount is showing on the form as .25% instead of 25%, also note that the discount amount is calculated correctly.
8. Check what is recorded in the database:
   . koha-mysql kohadev
   . select * from aqorders;
   ==> discount field for the basket item shows as 0.2500
9. Change the discount to 25%. Run step 8 again - discount amount will be correctly shown as 25.0000
10. Apply the patch.
11. Repeat steps 1-9 - discount amount is shown and calculated correctly.
12. Test modifying the discount amount - should be calculated and shown correctly.
13. Sign off!

See additional comments in the bug description.

Comment 5 David Nind 2022-06-01 21:29:46 UTC

Created attachment 135561 [details] [review]
Bug 29607: Store the discount amount correctly when importing and order from a file

This bug fixes how the discount amount for an order is stored, when an order is added to a basket using "From staged MARC records".

Test plan:
1. Export a record (without the items) (Tools > Catalog > Export data).
2. Delete the record from the catalog (including any items).
3. Stage the record for import (Tools > Catalog > Stage MARC records for import).
4. Create a new vendor with a discount of 25%. (Or select and existing vendor that has a discount). (Acquisitions > New vendor)
5. Create a new basket for the vendor. (Acquisitions > Manage orders > search for vendors > New > Basket).
6. Add to the basket using "From a staged file":
   . select "Add orders" next to the staged file
   . select the record to add to the order
   . enter a price
   . leave the discount field blank
   . select a fund
   . select "Save"
   ==> The order is created!
7. Modify the order - note that the discount is showing on the form as .25% instead of 25%, also note that the discount amount is calculated correctly.
8. Check what is recorded in the database:
   . koha-mysql kohadev
   . select * from aqorders;
   ==> discount field for the basket item shows as 0.2500
9. Change the discount to 25%. Run step 8 again - discount amount will be correctly shown as 25.0000
10. Apply the patch.
11. Repeat steps 1-9 - discount amount is shown and calculated correctly.
12. Test modifying the discount amount - should be calculated and shown correctly.
13. Sign off!

See additional comments in the bug description.

Signed-off-by: David Nind <david@davidnind.com>

Comment 6 David Nind 2022-06-01 21:30:29 UTC

Thanks for fixing and updating the patch Jonathan!

Comment 7 Katrin Fischer 2022-06-06 13:08:30 UTC

Hi Christophe, thank you for your patch! Please add yourself as Assignee when you start working on a bug. I've fixed it here.

Comment 8 Katrin Fischer 2022-06-06 13:20:34 UTC

Created attachment 135728 [details] [review]
Bug 29607: Store the discount amount correctly when importing and order from a file

This bug fixes how the discount amount for an order is stored, when an order is added to a basket using "From staged MARC records".

Test plan:
1. Export a record (without the items) (Tools > Catalog > Export data).
2. Delete the record from the catalog (including any items).
3. Stage the record for import (Tools > Catalog > Stage MARC records for import).
4. Create a new vendor with a discount of 25%. (Or select and existing vendor that has a discount). (Acquisitions > New vendor)
5. Create a new basket for the vendor. (Acquisitions > Manage orders > search for vendors > New > Basket).
6. Add to the basket using "From a staged file":
   . select "Add orders" next to the staged file
   . select the record to add to the order
   . enter a price
   . leave the discount field blank
   . select a fund
   . select "Save"
   ==> The order is created!
7. Modify the order - note that the discount is showing on the form as .25% instead of 25%, also note that the discount amount is calculated correctly.
8. Check what is recorded in the database:
   . koha-mysql kohadev
   . select * from aqorders;
   ==> discount field for the basket item shows as 0.2500
9. Change the discount to 25%. Run step 8 again - discount amount will be correctly shown as 25.0000
10. Apply the patch.
11. Repeat steps 1-9 - discount amount is shown and calculated correctly.
12. Test modifying the discount amount - should be calculated and shown correctly.
13. Sign off!

See additional comments in the bug description.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Comment 9 Tomás Cohen Arazi (tcohen) 2022-06-06 17:33:04 UTC

Pushed to master for 22.11.

Nice work everyone, thanks!

Comment 10 Lucas Gass (lukeg) 2022-07-12 16:04:09 UTC

Backported to 22.05.x for 22.05.03

Comment 11 Arthur Suzuki 2023-04-05 11:03:13 UTC

applied to 21.11 for 21.11.19

Comment 12 Wainui Witika-Park 2023-04-27 05:55:19 UTC

Not backported to 21.05.x