Bug 32166 - When adding to a basket from a staged file we may use the wrong inputs
Summary: When adding to a basket from a staged file we may use the wrong inputs
Status: RESOLVED 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: 31569
Blocks: 36053 32171
  Show dependency treegraph
 
Reported: 2022-11-10 15:00 UTC by Nick Clemens
Modified: 2024-02-08 20:32 UTC (History)
2 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


Attachments
Bug 32166: Use import record id for retrieving correct inputs (16.67 KB, patch)
2022-11-10 20:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Test records (6.46 KB, text/xml)
2022-11-10 20:20 UTC, Nick Clemens
Details
Bug 32166: Use import record id for retrieving correct inputs (16.73 KB, patch)
2022-11-15 15:59 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 32166: Use import record id for retrieving correct inputs (16.79 KB, patch)
2022-11-16 13:30 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 Nick Clemens 2022-11-10 15:00:18 UTC
On addorderiso2709.pl we loop over the records in the staged batch:
    for my $biblio (@$biblios){
        $biblio_count++;

skipping any not selected, which inlcudes any already imported:
        next if not grep { $_ eq $$biblio{import_record_id} } @import_record_id_selected;

Later we use that count to find the inputs:
        my @homebranches = $input->multi_param('homebranch_' . $biblio_count);

The problem being that if a record is already imported, our count increases, but the input on the page still counts from 1
Comment 1 Nick Clemens 2022-11-10 20:19:43 UTC
Created attachment 143705 [details] [review]
Bug 32166: Use import record id for retrieving correct inputs

When importing from a staged file we retrieve the records form the DB, skip any that are not selected,
and process the rest.

When we skip some, we still raise our record count, and use this to retrieve the inputs.

When building the page, we don't increment for skipped reocrds, so there can be a mismatch, i.e.
Record #1 on the page to add records may be the 3rd record in the import file

Rather than using a counting system, let us use the import record id directly

To test:
 0 - Set system preferences:

MarcFieldsToOrder:
price: 949$g
quantity: 949$k
budget_code: 949$l
discount: 949$m
sort1: 949$n
sort2: 949$q

MarcItemFieldsToOrder:
homebranch: 949$a
holdingbranch: 949$b
itype: 949$y
nonpublic_note: 949$x
public_note: 949$z
loc: 949$c
ccode: 949$8
notforloan: 949$7
uri: 949$u
copyno: 949$t
price: 949$g
replacementprice: 949$v
itemcallnumber: 949$o
quantity: 949$k
budget_code: 949$l

 1 - Stage attached sample file, Format:MARCXML, Record matching:Koha biblio 999$c
 2 - Add to a basket from the staged file
 3 - Select 1st record to basket and save
 4 - Record is added with the fields above as expected
 5 - Add to basket again, select 2nd record
 6 - Record is added with price from 020a, ignoring incoming fields
 7 - Repeat with 3rd, same problem
 8 - Apply patch
 9 - Stage file and repeat step 3
10 - Confirm added with correct values
11 - Add 2nd record and save, values correct
12 - Add 3rd record, values correct
Comment 2 Nick Clemens 2022-11-10 20:20:16 UTC
Created attachment 143706 [details]
Test records

Test records for importing
Comment 3 Andrew Fuerste-Henry 2022-11-14 21:58:07 UTC
I got an error in testing this and bug 32171 together. The error is due to a typo in 32171.
Comment 4 Katrin Fischer 2022-11-15 15:18:10 UTC
(In reply to Andrew Fuerste-Henry from comment #3)
> I got an error in testing this and bug 32171 together. The error is due to a
> typo in 32171.

Nick, can you please check and explain a bit more about 32171?
Comment 5 Nick Clemens 2022-11-15 15:28:44 UTC
(In reply to Katrin Fischer from comment #4)
> (In reply to Andrew Fuerste-Henry from comment #3)
> > I got an error in testing this and bug 32171 together. The error is due to a
> > typo in 32171.
> 
> Nick, can you please check and explain a bit more about 32171?

32171 is updated - it is a separate issue, but hard to fix one without the other  as you won't see the prices
Comment 6 ByWater Sandboxes 2022-11-15 15:59:18 UTC
Created attachment 143916 [details] [review]
Bug 32166: Use import record id for retrieving correct inputs

When importing from a staged file we retrieve the records form the DB, skip any that are not selected,
and process the rest.

When we skip some, we still raise our record count, and use this to retrieve the inputs.

When building the page, we don't increment for skipped reocrds, so there can be a mismatch, i.e.
Record #1 on the page to add records may be the 3rd record in the import file

Rather than using a counting system, let us use the import record id directly

To test:
 0 - Set system preferences:

MarcFieldsToOrder:
price: 949$g
quantity: 949$k
budget_code: 949$l
discount: 949$m
sort1: 949$n
sort2: 949$q

MarcItemFieldsToOrder:
homebranch: 949$a
holdingbranch: 949$b
itype: 949$y
nonpublic_note: 949$x
public_note: 949$z
loc: 949$c
ccode: 949$8
notforloan: 949$7
uri: 949$u
copyno: 949$t
price: 949$g
replacementprice: 949$v
itemcallnumber: 949$o
quantity: 949$k
budget_code: 949$l

 1 - Stage attached sample file, Format:MARCXML, Record matching:Koha biblio 999$c
 2 - Add to a basket from the staged file
 3 - Select 1st record to basket and save
 4 - Record is added with the fields above as expected
 5 - Add to basket again, select 2nd record
 6 - Record is added with price from 020a, ignoring incoming fields
 7 - Repeat with 3rd, same problem
 8 - Apply patch
 9 - Stage file and repeat step 3
10 - Confirm added with correct values
11 - Add 2nd record and save, values correct
12 - Add 3rd record, values correct

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 7 Katrin Fischer 2022-11-16 13:30:25 UTC
Created attachment 143953 [details] [review]
Bug 32166: Use import record id for retrieving correct inputs

When importing from a staged file we retrieve the records form the DB, skip any that are not selected,
and process the rest.

When we skip some, we still raise our record count, and use this to retrieve the inputs.

When building the page, we don't increment for skipped reocrds, so there can be a mismatch, i.e.
Record #1 on the page to add records may be the 3rd record in the import file

Rather than using a counting system, let us use the import record id directly

To test:
 0 - Set system preferences:

MarcFieldsToOrder:
price: 949$g
quantity: 949$k
budget_code: 949$l
discount: 949$m
sort1: 949$n
sort2: 949$q

MarcItemFieldsToOrder:
homebranch: 949$a
holdingbranch: 949$b
itype: 949$y
nonpublic_note: 949$x
public_note: 949$z
loc: 949$c
ccode: 949$8
notforloan: 949$7
uri: 949$u
copyno: 949$t
price: 949$g
replacementprice: 949$v
itemcallnumber: 949$o
quantity: 949$k
budget_code: 949$l

 1 - Stage attached sample file, Format:MARCXML, Record matching:Koha biblio 999$c
 2 - Add to a basket from the staged file
 3 - Select 1st record to basket and save
 4 - Record is added with the fields above as expected
 5 - Add to basket again, select 2nd record
 6 - Record is added with price from 020a, ignoring incoming fields
 7 - Repeat with 3rd, same problem
 8 - Apply patch
 9 - Stage file and repeat step 3
10 - Confirm added with correct values
11 - Add 2nd record and save, values correct
12 - Add 3rd record, values correct

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 8 Tomás Cohen Arazi 2022-11-16 18:56:37 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 9 Lucas Gass 2022-11-22 20:35:18 UTC
This patch won't apply cleanly to 22.05.x but I know it is needed for 22.05.07. Please rebase!
Comment 10 Nick Clemens 2022-11-22 20:47:18 UTC
(In reply to Lucas Gass from comment #9)
> This patch won't apply cleanly to 22.05.x but I know it is needed for
> 22.05.07. Please rebase!

I think the confusion is 29955 says it is in 22.05/x, but it isn't?
Comment 11 Nick Clemens 2022-11-23 12:26:53 UTC
(In reply to Nick Clemens from comment #10)
> (In reply to Lucas Gass from comment #9)
> > This patch won't apply cleanly to 22.05.x but I know it is needed for
> > 22.05.07. Please rebase!
> 
> I think the confusion is 29955 says it is in 22.05/x, but it isn't?

This depends on 31569

This was a tangle of confusion in fixing bugs, 32167 is needed for 22.05 - but this is not