Bug 33863 - On receive "change currency" is always checked
Summary: On receive "change currency" is always checked
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 25655 33864
Blocks:
  Show dependency treegraph
 
Reported: 2023-05-30 12:37 UTC by Jonathan Druart
Modified: 2023-07-18 14:22 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.02


Attachments
Bug 33863: (bug 25655 follow-up) Check "Change currency" only if checked before (1.58 KB, patch)
2023-05-30 12:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33863: Reset value if checkbox is unchecked (983 bytes, patch)
2023-07-05 09:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33863: Update values in data that will be saved (5.40 KB, patch)
2023-07-11 08:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33863: (bug 25655 follow-up) Check "Change currency" only if checked before (1.63 KB, patch)
2023-07-12 06:43 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 33863: Reset value if checkbox is unchecked (1.01 KB, patch)
2023-07-12 06:43 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 33863: Update values in data that will be saved (5.45 KB, patch)
2023-07-12 06:43 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 33863: (bug 25655 follow-up) Check "Change currency" only if checked before (1.68 KB, patch)
2023-07-13 15:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33863: Reset value if checkbox is unchecked (1.07 KB, patch)
2023-07-13 15:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33863: Update values in data that will be saved (5.51 KB, patch)
2023-07-13 15:37 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2023-05-30 12:37:29 UTC
On bug 25655 we stored the invoice currency and invoice price. The "change currency" checkbox should be checked only if the previous order (in case of partial receives) had it checked.
Comment 1 Jonathan Druart 2023-05-30 12:41:58 UTC
Created attachment 151796 [details] [review]
Bug 33863: (bug 25655 follow-up) Check "Change currency" only if checked before

We should not check "Change currency" if it has not modified in the
original order.

Test plan:
Create an order with 3+ items
Receive 1 item, don't check "change currency"
Receive an other item
=> "Change currency" is not checked
Check "Change currency", modify the currency and the price
Receive a third item
=> Change currency is checked, invoice currency and price are set
correctly to the previous values
Comment 2 Katrin Fischer 2023-05-31 05:41:59 UTC
I tried testing, but the 'Confirm' button always remains locked/inactive, so I can't receive the first item.

Could this have to do with one of the other bugs?
Comment 3 Katrin Fischer 2023-05-31 05:42:51 UTC
I set quantity received, checked the "change currency", entered price and changed currency. Order line had create items while cataloguing.
Comment 4 Katrin Fischer 2023-05-31 05:50:14 UTC
(In reply to Katrin Fischer from comment #2)
> I tried testing, but the 'Confirm' button always remains locked/inactive, so
> I can't receive the first item.
> 
> Could this have to do with one of the other bugs?

This appears to be fixed by bug 33864, making them dependent for now.
Comment 5 Katrin Fischer 2023-05-31 05:55:58 UTC
It almost works:

Test plan:
Create an order with 3+ items
Receive 1 item, don't check "change currency"
Receive an other item
=> "Change currency" is not checked
Check "Change currency", modify the currency and the price
Receive a third item
=> Change currency is checked, invoice currency and price are set
correctly to the previous values

This works as described, but in the last step, the "Actual cost" is 0 and it should have the calculated price from the foreign currency. You need to click into the field and add a space or so to trigger the JS.
Comment 6 Jonathan Druart 2023-05-31 07:25:34 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2023-05-31 07:29:45 UTC
I think this patch is correct and what Katrin describes is an other bug (unitprice retrieved from parent order but not adjusted from already received order, when invoice_unitprice is stored in the parent order).
Comment 8 Katrin Fischer 2023-05-31 07:37:47 UTC
Talked to Jonathan about this and trying to summarize:

* When we do a partial receive, the order lines are split into a parent and a child order line. The child is the one that is set to received, the parent remains.

* But we do store some values in the parent order from the first receive:

parent order has:
unitprice: 0.000000
invoice_unitprice: 42.000000
invoice_currency: EUR  

Received child order line: 
unitprice: 36.710000
invoice_unitprice: 42.000000
invoice_currency: EUR   

* So we can check the checkbox and set the foreign currency and original price, but the actual cost remains empty that would be pulled from unitprice, if it was set.

I think we could trigger the JS on opening the page if the actual cost was empty, filling in the actual cost with the calculated price in active currency.

I am only worried about one use case:

* Create a basket, create an order line, fill in "actual cost" on the order form
* Receive the first item and set original price etc.
* What happens to the actual cost in the parent? Does it remain the original value from ordering or is it updated? I assume it remains.
* Receive the next item: original price would show as entered previously, but the actual cost would not be calculated from it, but be the one entered at order time. Could this be confusing?

I can think of some ways to resolve:

a) Store not only the original price (invoice_unitprice) in the parent, but also the unitprice. 
BUT: exchange rates might change between receives. So we'd still want to recalculate the unitprice on receive with the current exchange rate. Storing it doesn't seem to give us an advantage.

b) When receiving, automatically calculate the actual cost (unitprice) from the invoice_unitprice with the current exchange rate (trigger JS). I'd do this independently if unitprice was set or not, so there is a relation between the original price (invoice_unitprice) we see and the actual cost (unitprice)

c) Don't store the invoice_unitprice in the parent. People will need to enter it, calculation is triggered. An existing actual cost value might show before the value is entered.

I think we can scratch a). b) or c)?
Comment 9 Katrin Fischer 2023-05-31 07:40:15 UTC
(In reply to Jonathan Druart from comment #7)
> I think this patch is correct and what Katrin describes is an other bug
> (unitprice retrieved from parent order but not adjusted from already
> received order, when invoice_unitprice is stored in the parent order).

I disagree. The problem appears because we are trying to pre-set values that might not match up (unitprice and invoice_unitprice should depend on each other, being calculated from current exchange rate) Without this change, we don't have the problem.
Comment 10 Jonathan Druart 2023-05-31 08:15:09 UTC
The problem here is that the checkbox is always checked. I think we should fix that for 23.05.00.
Comment 11 Katrin Fischer 2023-05-31 09:09:03 UTC
(In reply to Jonathan Druart from comment #10)
> The problem here is that the checkbox is always checked. I think we should
> fix that for 23.05.00.

Ok, yes, we could uncheck it.
Comment 12 Michaela Sieber 2023-07-04 14:57:25 UTC
(In reply to Jonathan Druart from comment #0)
> On bug 25655 we stored the invoice currency and invoice price. The "change
> currency" checkbox should be checked only if the previous order (in case of
> partial receives) had it checked.


I agree. And you fixed that with the existing patch (2023-05-30)

BUT : if you uncheck the 'change currency' this does not trigger a change of the invoice_currency back to the "normally" (=active) used currency in the system.

If you want to switch back to the active system currency you first have to select that currency from the drop down and THEN deactivate the checkbox 'Change currency'

This behavior requires very good documenting!



The use case could be a vendor change of a standing order (e.g. for serials)
Comment 13 Jonathan Druart 2023-07-05 09:55:47 UTC
Created attachment 153043 [details] [review]
Bug 33863: Reset value if checkbox is unchecked
Comment 14 Jonathan Druart 2023-07-05 09:56:20 UTC
Thanks for you comment, Michaela. What about this patch? Will it make the behaviour less confusing?
Comment 15 Michaela Sieber 2023-07-10 06:29:19 UTC
(In reply to Jonathan Druart from comment #14)
> Thanks for you comment, Michaela. What about this patch? Will it make the
> behaviour less confusing?


No sorry, there is no change. invoice_currency remains the old value in the database if you do not change the currency code before deactivating the checkbox  "change currency".
Comment 16 Jonathan Druart 2023-07-11 08:25:23 UTC
Created attachment 153306 [details] [review]
Bug 33863: Update values in data that will be saved

We are not saving the values of the input but a representation of the
form, contains in $("#order_edit").data('row');

To acchieve that it's preferable to reuse _update_unitprice and call it
when #change_currency is modified.

Some code cleaning slipped into the patch, for readability purpose.
Comment 17 Jonathan Druart 2023-07-11 08:28:08 UTC
Oops, sorry Michaela. It seems that I have not properly tested the second patch.

I've caught several inconsistencies in the code and tried to clean them in the last patch.

I think the problem is fixed now, can you retest please?
Comment 18 PTFS Europe Sandboxes 2023-07-12 06:43:30 UTC
Created attachment 153341 [details] [review]
Bug 33863: (bug 25655 follow-up) Check "Change currency" only if checked before

We should not check "Change currency" if it has not modified in the
original order.

Test plan:
Create an order with 3+ items
Receive 1 item, don't check "change currency"
Receive an other item
=> "Change currency" is not checked
Check "Change currency", modify the currency and the price
Receive a third item
=> Change currency is checked, invoice currency and price are set
correctly to the previous values

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 19 PTFS Europe Sandboxes 2023-07-12 06:43:32 UTC
Created attachment 153342 [details] [review]
Bug 33863: Reset value if checkbox is unchecked

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 20 PTFS Europe Sandboxes 2023-07-12 06:43:35 UTC
Created attachment 153343 [details] [review]
Bug 33863: Update values in data that will be saved

We are not saving the values of the input but a representation of the
form, contains in $("#order_edit").data('row');

To acchieve that it's preferable to reuse _update_unitprice and call it
when #change_currency is modified.

Some code cleaning slipped into the patch, for readability purpose.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 21 Michaela Sieber 2023-07-12 06:45:04 UTC
(In reply to Jonathan Druart from comment #17)
> 
> I think the problem is fixed now, can you retest please?

Thanks for fixing , Signed Off :-)
Comment 22 Nick Clemens 2023-07-13 15:37:55 UTC
Created attachment 153402 [details] [review]
Bug 33863: (bug 25655 follow-up) Check "Change currency" only if checked before

We should not check "Change currency" if it has not modified in the
original order.

Test plan:
Create an order with 3+ items
Receive 1 item, don't check "change currency"
Receive an other item
=> "Change currency" is not checked
Check "Change currency", modify the currency and the price
Receive a third item
=> Change currency is checked, invoice currency and price are set
correctly to the previous values

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 23 Nick Clemens 2023-07-13 15:37:57 UTC
Created attachment 153403 [details] [review]
Bug 33863: Reset value if checkbox is unchecked

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 24 Nick Clemens 2023-07-13 15:37:59 UTC
Created attachment 153404 [details] [review]
Bug 33863: Update values in data that will be saved

We are not saving the values of the input but a representation of the
form, contains in $("#order_edit").data('row');

To acchieve that it's preferable to reuse _update_unitprice and call it
when #change_currency is modified.

Some code cleaning slipped into the patch, for readability purpose.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 25 Tomás Cohen Arazi 2023-07-14 12:01:16 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 26 Martin Renvoize 2023-07-18 11:52:08 UTC
Thanks for all the hard work!

Pushed to 23.05.x for the next release
Comment 27 Matt Blenkinsop 2023-07-18 14:22:48 UTC
Missing dependencies - not backporting to 22.11.x

Nice work everyone!