Bug 32166

Summary: When adding to a basket from a staged file we may use the wrong inputs
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: AcquisitionsAssignee: Nick Clemens (kidclamp) <nick>
Status: RESOLVED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: major    
Priority: P5 - low CC: andrew, lucas
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00
Circulation function:
Bug Depends on: 31569    
Bug Blocks: 36053, 32171    
Attachments: Bug 32166: Use import record id for retrieving correct inputs
Test records
Bug 32166: Use import record id for retrieving correct inputs
Bug 32166: Use import record id for retrieving correct inputs

Description Nick Clemens (kidclamp) 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 (kidclamp) 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 (kidclamp) 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 (kidclamp) 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 (tcohen) 2022-11-16 18:56:37 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 9 Lucas Gass (lukeg) 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 (kidclamp) 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 (kidclamp) 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