Bug 14999 - Adding to basket orders from staged files mixes up the prices between different orders
Summary: Adding to basket orders from staged files mixes up the prices between differe...
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Joonas Kylmälä
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-10 12:38 UTC by Joonas Kylmälä
Modified: 2021-11-23 00:17 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.05,20.11.12,20.05.18


Attachments
Bug 14999: Fetch the corresponding values for the specific biblio item (3.39 KB, patch)
2015-10-10 12:44 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 14999: Fetch the corresponding values for the specific biblio item (3.09 KB, patch)
2015-10-13 05:42 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 14999: Fetch the corresponding values for the specific biblio item (3.68 KB, patch)
2015-10-13 05:47 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 14999: Make sure order prices are not mixed-up (3.83 KB, patch)
2021-10-02 12:39 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 14999: Make sure order prices are not mixed-up (3.89 KB, patch)
2021-10-04 10:27 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 14999: Make sure order prices are not mixed-up (3.95 KB, patch)
2021-11-01 15:19 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joonas Kylmälä 2015-10-10 12:38:18 UTC
If one adds multiple item orders from a staged file to an basket the orders might change their values like price and item quantity between each other. This happens because in addorderiso2709.pl under the part "3rd step = import the records" there is a call to hasGetImportRecordsRange($import_batch_id) which gives some biblio info in some random order. And so this biblio info is used then like it would be in order and then the values mix up because of this. I have done a patch for this, and I'll put it here soon, so it will be more easier for you to understand!
Comment 1 Joonas Kylmälä 2015-10-10 12:44:14 UTC Comment hidden (obsolete)
Comment 2 Joonas Kylmälä 2015-10-13 05:42:59 UTC Comment hidden (obsolete)
Comment 3 Joonas Kylmälä 2015-10-13 05:47:39 UTC
Created attachment 43364 [details] [review]
Bug 14999: Fetch the corresponding values for the specific biblio item

Test plan:

1. In acquisitions add orders to basket from a staged file. You need to
   add orders which when added doesn't show the right quantity/price in the
   basket (after it has been added). I succeeded in this by selecting order
   items from under different headings (in the page "Add orders from iso2709
   file"). So this means that you would need to have a staged file with
   multiple sections in it.
2. Notice the problem with the wrong prices and quantities showing in the
   basket.
3. Apply this patch.
4. Remove those items you added to the basket and add them again and notice
   that they have now the corresponding values in them in the basket.
5. Look for anomalies.

Sponsored-by: Vaara-kirjastot
Comment 4 Lucio Moraes 2015-11-17 22:40:41 UTC
Hi Joonas,
if you have a staged file to test, could you share with me?
Thank you
Comment 5 Joonas Kylmälä 2015-11-18 09:15:37 UTC
(In reply to Lucio Moraes from comment #4)
> Hi Joonas,
> if you have a staged file to test, could you share with me?
> Thank you

Unfortunately I don't have one. And now I'm thinking if the patch I supplied here is obsolete because this might only apply for the https://github.com/KohaSuomi/kohasuomi version of Koha. I'll change the status to "In Discussion" for now.
Comment 6 Jonathan Druart 2019-12-12 10:20:59 UTC
(In reply to j.kylmala from comment #5)
> (In reply to Lucio Moraes from comment #4)
> > Hi Joonas,
> > if you have a staged file to test, could you share with me?
> > Thank you
> 
> Unfortunately I don't have one. And now I'm thinking if the patch I supplied
> here is obsolete because this might only apply for the
> https://github.com/KohaSuomi/kohasuomi version of Koha. I'll change the
> status to "In Discussion" for now.

Does it mean the original issue you reported is not valid for the community version?
Comment 7 Joonas Kylmälä 2021-10-02 10:06:07 UTC
I was checking up what old bug reports I have done and came across this. Definitely a bug in the community version as well and a bug that still exists to this day! Seems pretty major as these are the prices you might send via EDI to the vendor.

The bug is probably hard to reproduce but if you read the code you see the problem clearly. The GetImportRecordsRange function might return the imported records in whatever order and we are not matching them in anyway with the parameter data we are getting here.
Comment 8 Joonas Kylmälä 2021-10-02 12:11:58 UTC
So this turned out to be tricky to reproduce but I finally found at least one bug where this happens! So if there is a duplicate matched then we have not shifted away for the previous biblio to be added the replacement and price parameters:

>                 replacementprice   => shift( @orderreplacementprices ),

>             my $price= shift( @prices ) || GetMarcPrice($marcrecord, C4::Context->preference('marcflavour'));


So when the line:

>                 $duplinbatch = $import_batch_id and next if $duplifound;

is called we are leaving the shifts undone. I wonder if we should go with the originally proposed patch here or just move the shifts to the beginning. If we just move the shifts to the beginning then I feel like it could be a bit more prone to someone re-introducing this bug later if a new field is added.

To reproduce this bug:
 1) Download two records from koha to marcxml file, then cat those
   cat bib1.marcxml bib2.marcxml > bibs.marcxml
 2) Delete bib2 from koha
 3) Stage bibs.marcxml for import
 4) Add new basket -> import from a staged file
 5) Select both bib1 and bib2 and set price for bib1 to be 99.00 and for bib2 to be 88.00
 6) Click save and notice bib2 was imported with the wrong price, 99.00!!
Comment 9 Joonas Kylmälä 2021-10-02 12:39:12 UTC
Created attachment 125648 [details] [review]
Bug 14999: Make sure order prices are not mixed-up

We were shifting the price and replacement price for imported orders
only after the line:

> $duplinbatch = $import_batch_id and next if $duplifound;

This lead to the "replacementprice" and "price" query parameters not
being shifted/removed from the list if a duplicate record came across
and caused the prices be applied to the next record being imported.

To reproduce:
 1) Download two records from koha to marcxml file, then cat those:
   cat bib1.marcxml bib2.marcxml > bibs.marcxml
 2) Delete bib2 from koha
 3) Stage bibs.marcxml for import
 4) Create a new order basket, then "Add to basket" using "From a
    staged file" option
 5) Select both bib1 and bib2 and set price & replacement price for
    bib1 to be 99.00 and for bib2 to be 88.00
 6) Click save and notice bib2 was imported with the wrong prices, 99.00!
 7) Apply patch and notice the prices are now correctly set to 88.00.
Comment 10 Joonas Kylmälä 2021-10-02 12:44:03 UTC
Adding Emmi to CC here since you are using the original patch submitted here 6 years ago. I think this should fix now all problems and if this is merged you should be able to drop the original patch so if you could sign-off that would be nice :)
Comment 11 Emmi Takkinen 2021-10-04 08:45:51 UTC
(In reply to Joonas Kylmälä from comment #10)
> Adding Emmi to CC here since you are using the original patch submitted here
> 6 years ago. I think this should fix now all problems and if this is merged
> you should be able to drop the original patch so if you could sign-off that
> would be nice :)

We no longer use this feature (most libraries are now using EDitX etc). But I'm happy sign-off nonetheless as soon as I have time :)
Comment 12 Emmi Takkinen 2021-10-04 10:27:15 UTC
Created attachment 125689 [details] [review]
Bug 14999: Make sure order prices are not mixed-up

We were shifting the price and replacement price for imported orders
only after the line:

> $duplinbatch = $import_batch_id and next if $duplifound;

This lead to the "replacementprice" and "price" query parameters not
being shifted/removed from the list if a duplicate record came across
and caused the prices be applied to the next record being imported.

To reproduce:
 1) Download two records from koha to marcxml file, then cat those:
   cat bib1.marcxml bib2.marcxml > bibs.marcxml
 2) Delete bib2 from koha
 3) Stage bibs.marcxml for import
 4) Create a new order basket, then "Add to basket" using "From a
    staged file" option
 5) Select both bib1 and bib2 and set price & replacement price for
    bib1 to be 99.00 and for bib2 to be 88.00
 6) Click save and notice bib2 was imported with the wrong prices, 99.00!
 7) Apply patch and notice the prices are now correctly set to 88.00.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 13 Martin Renvoize 2021-11-01 15:19:23 UTC
Created attachment 127176 [details] [review]
Bug 14999: Make sure order prices are not mixed-up

We were shifting the price and replacement price for imported orders
only after the line:

> $duplinbatch = $import_batch_id and next if $duplifound;

This lead to the "replacementprice" and "price" query parameters not
being shifted/removed from the list if a duplicate record came across
and caused the prices be applied to the next record being imported.

To reproduce:
 1) Download two records from koha to marcxml file, then cat those:
   cat bib1.marcxml bib2.marcxml > bibs.marcxml
 2) Delete bib2 from koha
 3) Stage bibs.marcxml for import
 4) Create a new order basket, then "Add to basket" using "From a
    staged file" option
 5) Select both bib1 and bib2 and set price & replacement price for
    bib1 to be 99.00 and for bib2 to be 88.00
 6) Click save and notice bib2 was imported with the wrong prices, 99.00!
 7) Apply patch and notice the prices are now correctly set to 88.00.

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2021-11-01 15:20:13 UTC
That took a bit of wrapping my head around.. all appears good and no regressions found.

Passing QA
Comment 15 Jonathan Druart 2021-11-02 15:52:34 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 16 Kyle M Hall 2021-11-05 12:34:09 UTC
Pushed to 21.05.x for 21.05.05
Comment 17 Fridolin Somers 2021-11-11 00:36:56 UTC
Pushed to 20.11.x for 20.11.12
Comment 18 Victor Grousset/tuxayo 2021-11-15 22:55:24 UTC
Backported: Pushed to 20.05.x branch for 20.05.18
Comment 19 wainuiwitikapark 2021-11-23 00:17:08 UTC
Not backporting to 19.11.x - LTS version will just backport security patches