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.
Created attachment 133232 [details] [review] Bug 30520: Allow BatchImport to be used by command line tools.
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
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
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>
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
Oh interesting... I found the web based MARC stage and import broken in 21.11 as well. I'm starting to look into it...
QAing
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>
I think that this is not critical but major btw.
(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.
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
(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...
The status on this is "Pushed to stable". Is that correct? Should "Version(s) released in" be updated to reflect that?
(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
With bug 14957 in 21.11 it feels like this fix should have been backported to oldstable.
Restored status.. this really is needed for 21.11.x series.. and doesn't look too terrible to backport.
(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?