Bug 30520 - Command line stage and import broken
Summary: Command line stage and import broken
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical (vote)
Assignee: Andreas Jonsson
QA Contact: Marcel de Rooy
URL:
Keywords: rel_21_11_candidate
Depends on: 14957
Blocks:
  Show dependency treegraph
 
Reported: 2022-04-13 07:05 UTC by Andreas Jonsson
Modified: 2023-06-08 22:32 UTC (History)
12 users (show)

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


Attachments
Bug 30520: Allow BatchImport to be used by command line tools. (1.40 KB, patch)
2022-04-13 07:23 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 30520: Allow BatchImport to be used by command line tools. (1.46 KB, patch)
2022-05-11 16:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30520: Allow BatchImport to be used by command line tools. (1.56 KB, patch)
2022-05-13 06:40 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 Andreas Jonsson 2022-04-13 07:05:17 UTC
As part of the work done for bug 14957, a change was made where it is assumed the import is made by a logged in user, and crashes if there isn't one.  Specifically, this change causes the problem:

https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=126754&action=diff

This affects the command line tools stage_file.pl and commit_file.pl, when using the --match flag to match and replace existing records.
Comment 1 Andreas Jonsson 2022-04-13 07:23:55 UTC
Created attachment 133232 [details] [review]
Bug 30520: Allow BatchImport to be used by command line tools.
Comment 2 Andreas Jonsson 2022-04-13 07:32:07 UTC
Test plan:

* Stage and commit a marc file with a match rule such that a replacement will be triggered (either using the tool in the staff interface or the command line tools)
* Code review patch and observe:
  - If $logged_in_user is undefined the code will crash without patch and will not crash with patch
  - If $logged_in_user is defined the code will behave identically both with and without patch
Comment 3 David Nind 2022-05-06 20:11:22 UTC
Hi Andreas.

I would like to test (using koha-testing-docker), but I am not familiar enough with the importing records from the command line using misc/stage_file.pl and misc/commit_file.pl.

Are you able to provide a test plan for this that has some more detail?

I had a go, but there are some steps I'm not sure how to do in steps 1 and 2:

1. Stage and import a record using the staff interface (this should work successfully both before and after the patch is applied):
   . Have a sample file with records (for example, export some records from Koha with these settings, change the title in the file)
     . 
     . 
     => Record successfully imported and record updated with new title

2. Stage and import the records using the command line:
     . Stage the records for importing with these settings: perl misc/stage_file.pl --file koha-bz30520-test.mrc --add-items (notes: default --match rule is to match to ISBN or title; default action is to replace) 
     . Identify the batch number of the staged records: perl misc/commit_file.pl --list-batches (or lookup in the staff interface: Tools > Catalog > Staged MARC record management)
     . Commit the records for importing with these settings (where # = the batch numbere): perl misc/commit_file.pl --batch-number # --framework BK 
   . What the "crash" or "broken" looks like - error message, error in log files, silent fail, etc.

3. Apply the patch.

4. Repeat steps 1 and 2.

5. Expected result: records are successfully staged and imported - search for XYZ record, should find.

6. Sign off!

Also, presumably if you use the staff interface you are already logged in, so it won't produce an error? Or is this just to check that records can still be successfully staged and committed  using the staff interface after the patch is applied?

David
Comment 4 Martin Renvoize 2022-05-11 16:01:06 UTC
Created attachment 134902 [details] [review]
Bug 30520: Allow BatchImport to be used by command line tools.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2022-05-11 16:03:33 UTC
Good catch Andreas.. interestingly I couldn't easily spot where userid and categorycode are actually ever referred to in the overlay context.. but it doesn't appear to break anything.

Signing off as unit tests continue to pass and the fix does indeed work as proposed.

Might be night to add a unit test for this particular change though ;P
Comment 6 David Cook 2022-05-12 00:46:50 UTC
Oh interesting... I found the web based MARC stage and import broken in 21.11 as well. I'm starting to look into it...
Comment 7 Marcel de Rooy 2022-05-13 06:29:02 UTC
QAing
Comment 8 Marcel de Rooy 2022-05-13 06:40:25 UTC
Created attachment 134943 [details] [review]
Bug 30520: Allow BatchImport to be used by command line tools.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2022-05-13 06:41:13 UTC
I think that this is not critical but major btw.
Comment 10 Katrin Fischer 2022-05-13 07:39:19 UTC
(In reply to Marcel de Rooy from comment #9)
> I think that this is not critical but major btw.

For us this would be quite bad, but I realize it depends on workflows and if you use this daily or irregularly.
Comment 11 Fridolin Somers 2022-05-13 08:17:13 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 12 David Cook 2022-05-17 23:31:59 UTC
(In reply to David Cook from comment #6)
> Oh interesting... I found the web based MARC stage and import broken in
> 21.11 as well. I'm starting to look into it...

Turns out it wasn't broken after all. It was just silently hiding an item import error.

Bug 30738 helped clear that up...
Comment 13 Magnus Enger 2022-08-09 10:21:18 UTC
The status on this is "Pushed to stable". Is that correct? Should "Version(s) released in" be updated to reflect that?
Comment 14 Lucas Gass 2022-08-09 13:48:07 UTC
(In reply to Magnus Enger from comment #13)
> The status on this is "Pushed to stable". Is that correct? Should
> "Version(s) released in" be updated to reflect that?

Looking at the 21.11.x shortlog and I see no evidence that this bug was ever backported beyond 22.05.x:

https://git.koha-community.org/gitweb/?p=koha.git;a=shortlog;h=refs/heads/21.11.x
Comment 15 Martin Renvoize 2022-08-09 15:12:16 UTC
With bug 14957 in 21.11 it feels like this fix should have been backported to oldstable.
Comment 16 Martin Renvoize 2023-01-30 12:18:26 UTC
Restored status.. this really is needed for 21.11.x series.. and doesn't look too terrible to backport.
Comment 17 Marcel de Rooy 2023-01-30 13:09:44 UTC
(In reply to Martin Renvoize from comment #16)
> Restored status.. this really is needed for 21.11.x series.. and doesn't
> look too terrible to backport.

It applied immediately in my 21.11.
Nick added the 21_11 keyword months ago. So I do not really understand why this was not backported? Could you explain, Arthur? Are we forgetting something?