Bug 24469 - Record biblionumber in import_biblio when adding to basket via file
Summary: Record biblionumber in import_biblio when adding to basket via file
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 27828
  Show dependency treegraph
 
Reported: 2020-01-21 15:31 UTC by Andrew Fuerste-Henry
Modified: 2021-03-02 10:36 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.03,20.05.09


Attachments
Bug 24469: Record biblionumber in import_biblio when adding to basket via file (2.19 KB, patch)
2020-02-12 15:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24469: Record biblionumber in import_biblio when adding to basket via file (2.18 KB, patch)
2020-02-17 10:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24469: Record biblionumber in import_biblio when adding to basket via file (2.15 KB, patch)
2020-04-13 10:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24469: Record biblionumber in import_biblio when adding to basket via file (2.22 KB, patch)
2021-02-03 13:19 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 24469: Record biblionumber in import_biblio when adding to basket via file (2.27 KB, patch)
2021-02-11 19:54 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 24469: Move the new queries to a dedicated ImportBatch subroutine (6.58 KB, patch)
2021-02-12 09:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24469: Move the new queries to a dedicated ImportBatch subroutine (6.78 KB, patch)
2021-02-12 10:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24469: Record biblionumber in import_biblio when adding to basket via file (2.37 KB, patch)
2021-02-18 10:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24469: Move the new queries to a dedicated ImportBatch subroutine (6.92 KB, patch)
2021-02-18 10:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2020-01-21 15:31:03 UTC
When we stage and then import a file through tools, the biblionumber of the created record gets recorded in import_biblios.matched_biblionumber.

When we stage a file and then use it to populate a basket in acquisitions, import_biblio.matched_biblionumber does not get populated. We see the same behavior whether the basket is populated using "From a staged file" or "From a new file."

To Test:
- Create a basket
- Add to basket From a New File
- Select a marc file and stage without checking for duplicates
- Add titles to your basket
- Confirm from acquisitions that new bibs were created
- Go to manage-marc-import.pl and find your file
- Confirm that the Record column is empty
Comment 1 Jonathan Druart 2020-02-12 15:31:02 UTC
Created attachment 98760 [details] [review]
Bug 24469: Record biblionumber in import_biblio when adding to basket via file

The column import_biblios.matched_biblionumber was not populated when an
order is created from a staged file.

Test plan:
A/ Create a new order from a stage file.
Use the "Save" button at the bottom of the "Add orders from" page
Then note the matched_biblionumber value. It should be populated correctly

B/ Import again the same record, this time you will have to use the "Add
order" link in the list of order (ie. not the "Save" button)
Note the matched_biblionumber value. It should be populated correctly
Comment 2 Andrew Fuerste-Henry 2020-02-12 16:32:09 UTC
In my testing, matched_biblionumber remains null in test A using the Save button. Test B using the Add order link successfully populated matched_biblionumber.
Comment 3 Jonathan Druart 2020-02-17 10:45:39 UTC
Created attachment 99073 [details] [review]
Bug 24469: Record biblionumber in import_biblio when adding to basket via file

The column import_biblios.matched_biblionumber was not populated when an
order is created from a staged file.

Test plan:
A/ Create a new order from a stage file.
Use the "Save" button at the bottom of the "Add orders from" page
Then note the matched_biblionumber value. It should be populated correctly

B/ Import again the same record, this time you will have to use the "Add
order" link in the list of order (ie. not the "Save" button)
Note the matched_biblionumber value. It should be populated correctly
Comment 4 Jonathan Druart 2020-02-17 10:46:12 UTC
(In reply to Andrew Fuerste-Henry from comment #2)
> In my testing, matched_biblionumber remains null in test A using the Save
> button. Test B using the Add order link successfully populated
> matched_biblionumber.

Indeed, can you try again with this new patch please?
Comment 5 Sally 2020-03-26 16:53:08 UTC
Patch no longer applies.
Comment 6 Jonathan Druart 2020-04-13 10:39:13 UTC
Created attachment 102805 [details] [review]
Bug 24469: Record biblionumber in import_biblio when adding to basket via file

The column import_biblios.matched_biblionumber was not populated when an
order is created from a staged file.

Test plan:
A/ Create a new order from a stage file.
Use the "Save" button at the bottom of the "Add orders from" page
Then note the matched_biblionumber value. It should be populated correctly

B/ Import again the same record, this time you will have to use the "Add
order" link in the list of order (ie. not the "Save" button)
Note the matched_biblionumber value. It should be populated correctly
Comment 7 Victor Grousset/tuxayo 2020-05-13 01:09:25 UTC
I tried to reproduce the issue in two ways:

1. With an imported record not already in my catalog
After the order was created the following sql query
select * from import_biblios;
shows that matched_biblionumber is null.

2. With an imported record that I just exported from my catalog
When doing "Add orders from MARC file" after I hit save, I get the following message:

> No records imported
> No record have been imported because they all match an existing record in your  catalog.
> You'll have to treat them individually.

Even though "Record matching rule = Do not look for matching records" in Home › Tools › Stage MARC records for import


Did I miss something?
Comment 8 Jonathan Druart 2020-05-13 13:33:29 UTC
Don't import it.
You should stage it, then create an order "From a staged file"
Comment 9 Victor Grousset/tuxayo 2020-05-13 14:55:14 UTC
> You should stage it, then create an order "From a staged file"

Whoops, I meant staged, terminology error.
"Home › Acquisitions › My Vendor › Basket 1 › Add orders from MARC file"

Which is the right way to try to reproduce?
1. With a staged record not already in my catalog
2. With a staged record that I just exported from my catalog
Comment 10 Victor Grousset/tuxayo 2020-05-14 02:17:49 UTC
Message from Jonathan on IRC

> about 24469 - the important bit is to use the "Add" link, not the "Save" button

I'm not even a that part yet. It's A/ on first phase (bug reproduction) I'm not even hitting the actual bug.

> A/ Create a new order from a stage file.
> Use the "Save" button at the bottom of the "Add orders from" page
> Then note the matched_biblionumber value. It should be populated correctly

No matched_biblionumber.

Tried with
- Debian 10 & MariaDB
- Debian 9 & MySQL 5.5
in case that was some compat issue. But no :(
Comment 11 Séverine Queune 2020-10-20 09:52:26 UTC
No problem to reproduce bug on master.
The patch works fine for me, except one point : I first thought status of staged files was very long to update from "Staged" to "Import", but after some tests on several files, it seems to me that status is updated only after I add an order using a new staged file.
Did you experiment the same ?
Comment 12 Andrew Fuerste-Henry 2021-02-03 13:10:31 UTC
(In reply to Séverine Queune from comment #11)
> No problem to reproduce bug on master.
> The patch works fine for me, except one point : I first thought status of
> staged files was very long to update from "Staged" to "Import", but after
> some tests on several files, it seems to me that status is updated only
> after I add an order using a new staged file.
> Did you experiment the same ?

I thought we fixed that status change issue in bug 24470.
Comment 13 Andrew Fuerste-Henry 2021-02-03 13:17:30 UTC
Fleshed out test plan based on general confusion.

Test plan:
A/ Create a new order from a stage file.
Use the "Save" button at the bottom of the "Add orders from" page
Then note the matched_biblionumber value. It should be populated correctly
1 - In acq, open a new basket, click Add To Basket, select From a New File
2 - select and stage your marc file without checking for matches
3 - from the staging results page, click Add staged files to basket
4 - on addorderiso2709.pl, select your title, add price, select fund
5 - make sure the Matching dropdown at the top of the page is set to Do not look for matching records
6 - click Save
7 - select * from import_biblios, confirm matched_biblionumber is not null

B/ Import again the same record, this time you will have to use the "Add
order" link in the list of order (ie. not the "Save" button)
Note the matched_biblionumber value. It should be populated correctly
1 - in Tools, stage your file without checking for matches, do not complete import
2 - in acq, click Add to Basket, select From a Staged File
3 - on the "Choose the file to add to the basket" page, click Add orders
4 - on addorderiso2709.pl, select your title, add price, select fund
5 - make sure the Matching dropdown at the top of the page is set to Do not look for matching records
6 - click Save
7 - select * from import_biblios, confirm matched_biblionumber is not null
Comment 14 Andrew Fuerste-Henry 2021-02-03 13:19:43 UTC
Created attachment 116255 [details] [review]
Bug 24469: Record biblionumber in import_biblio when adding to basket via file

The column import_biblios.matched_biblionumber was not populated when an
order is created from a staged file.

Test plan:
A/ Create a new order from a stage file.
Use the "Save" button at the bottom of the "Add orders from" page
Then note the matched_biblionumber value. It should be populated correctly

B/ Import again the same record, this time you will have to use the "Add
order" link in the list of order (ie. not the "Save" button)
Note the matched_biblionumber value. It should be populated correctly

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 15 Andrew Fuerste-Henry 2021-02-11 19:54:24 UTC
Created attachment 116779 [details] [review]
Bug 24469: Record biblionumber in import_biblio when adding to basket via file

The column import_biblios.matched_biblionumber was not populated when an
order is created from a staged file.

Test plan:
A/ Create a new order from a stage file.
Use the "Save" button at the bottom of the "Add orders from" page
Then note the matched_biblionumber value. It should be populated correctly

B/ Import again the same record, this time you will have to use the "Add
order" link in the list of order (ie. not the "Save" button)
Note the matched_biblionumber value. It should be populated correctly

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Sarah Daviau <sdaviau@arlingtonva.us>
Comment 16 Marcel de Rooy 2021-02-12 08:57:25 UTC
Please explain why we are adding $dbh to a script without SQL. We should have a very good reason to do so instead of adding a sub to ImportBatch. Which we could also test etc.
This looks like a quick production fix.
Comment 17 Jonathan Druart 2021-02-12 09:13:22 UTC
Yes Marcel, it's a valid question.
I decided to put those 2 lines in the controller to avoid C4::ImportBatch.
Having a new subroutine there to update matched_biblionumber doesn't really make sense, however adjusting SetImportBatchStatus to update it sounds better (it's basically what's done in C4::ImportBatch when SetImportRecordStatus is called).
Doing that is not trivial and will certainly introduce bugs, which I'd like to avoid. A simple batch seemed better to backport.

(I am just a developer, I am not dealing with production instances)
Comment 18 Jonathan Druart 2021-02-12 09:28:02 UTC
Created attachment 116792 [details] [review]
Bug 24469: Move the new queries to a dedicated ImportBatch subroutine
Comment 19 Jonathan Druart 2021-02-12 09:34:55 UTC
(In reply to Jonathan Druart from comment #17)
> A simple batch seemed better to backport.

Typo! A simple *patch* :D
Comment 20 Marcel de Rooy 2021-02-12 10:34:36 UTC
+        SetMatchedBiblionumber( $biblio->{import_record_id}, $biblionumber );

No compiler warning, but not exported in module?
Comment 21 Jonathan Druart 2021-02-12 10:40:39 UTC
Created attachment 116813 [details] [review]
Bug 24469: Move the new queries to a dedicated ImportBatch subroutine
Comment 22 Jonathan Druart 2021-02-12 10:41:00 UTC
Amanded the patch with:
@ C4/ImportBatch.pm:81 @ BEGIN {                                   
     SetImportRecordOverlayStatus
     GetImportRecordStatus               
     SetImportRecordStatus
+    SetMatchedBiblionumber
     GetImportRecordMatches                       
     SetImportRecordMatches                                      
    );
Comment 23 Marcel de Rooy 2021-02-18 10:30:43 UTC
Created attachment 116975 [details] [review]
Bug 24469: Record biblionumber in import_biblio when adding to basket via file

The column import_biblios.matched_biblionumber was not populated when an
order is created from a staged file.

Test plan:
A/ Create a new order from a stage file.
Use the "Save" button at the bottom of the "Add orders from" page
Then note the matched_biblionumber value. It should be populated correctly

B/ Import again the same record, this time you will have to use the "Add
order" link in the list of order (ie. not the "Save" button)
Note the matched_biblionumber value. It should be populated correctly

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Sarah Daviau <sdaviau@arlingtonva.us>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Marcel de Rooy 2021-02-18 10:30:47 UTC
Created attachment 116976 [details] [review]
Bug 24469: Move the new queries to a dedicated ImportBatch subroutine

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: POD line for $import_record_id.
Comment 25 Marcel de Rooy 2021-02-18 10:31:15 UTC
SO+QA in one pass
Comment 26 Jonathan Druart 2021-02-18 11:05:52 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 27 Fridolin Somers 2021-02-22 10:12:11 UTC
Pushed to 20.11.x for 20.11.03
Comment 28 Stefan Norinder 2021-03-02 10:10:05 UTC
After we upgraded Koha to version 20.05.09 we get Internal Server Error when importing a post to a basket. Here's output from the log file: 

```
Undefined subroutine &CGI::Compile::ROOT::usr_share_koha_intranet_cgi_2dbin_acqui_neworderempty_2epl::SetMatchedBiblionumber called at /usr/share/koha/intranet/cgi-bin/acqui/neworderempty.pl line 183.
```

Could it be related to the fix for this bug report?
Comment 29 Jonathan Druart 2021-03-02 10:30:51 UTC
The patches are indeed part of 20.05.09.
I don't recreate the problem on master however.
Comment 30 Jonathan Druart 2021-03-02 10:33:24 UTC
I do recreate on master!
Comment 31 Jonathan Druart 2021-03-02 10:36:26 UTC
(In reply to Jonathan Druart from comment #30)
> I do recreate on master!

See bug 27828.


(In reply to Jonathan Druart from comment #17)
> Doing that is not trivial and will certainly introduce bugs, which I'd like
> to avoid. A simple batch seemed better to backport.

Indeed...