Bug 4126 - bulkmarcimport.pl allows -b and -a to be specified simultaneously
Summary: bulkmarcimport.pl allows -b and -a to be specified simultaneously
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Bibliographic data support (show other bugs)
Version: master
Hardware: PC All
: P5 - low normal (vote)
Assignee: Magnus Enger
QA Contact: Bugs List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-06 01:20 UTC by Galen Charlton
Modified: 2017-02-20 09:32 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 4126 - Exit bulkmarcimport if -a and -b given (1.46 KB, patch)
2016-11-10 14:40 UTC, Magnus Enger
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 4126 - Exit bulkmarcimport if -a and -b given (1.52 KB, patch)
2017-01-11 16:24 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 4126 - Exit bulkmarcimport if -a and -b given (1.58 KB, patch)
2017-01-27 13:46 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 01:23:26 UTC


---- Reported by gmcharlt@gmail.com 2010-02-06 13:20:25 ----

When both -a and -b are set, bulkmarcimport.pl loads the records as authorities, but tries to do the ISBN cleanup as they were bibs.  -a and -b should be mutually exclusive.



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:23 UTC  ---

This bug was previously known as _bug_ 4126 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4126

Actual time not defined. Setting to 0.0
Setting qa contact to the default for this product.
   This bug either had no qa contact or an invalid one.

Comment 1 Tomás Cohen Arazi 2013-03-26 18:20:03 UTC
I haven't checked the exact situation reported, but it looks like the somultaneous authorities + biblios switches are not handled correctly, by code like this:

if ($delete) {
        if ($biblios){
        print "deleting biblios\n";
        $dbh->do("truncate biblio");
        $dbh->do("truncate biblioitems");
        $dbh->do("truncate items");
        }
        else {
        print "deleting authorities\n";
        $dbh->do("truncate auth_header");
        }
    $dbh->do("truncate zebraqueue");
}

Which should truncate both if present, not just biblios.
Regards
To+
Comment 2 Magnus Enger 2016-11-10 14:40:45 UTC
Created attachment 57428 [details] [review]
Bug 4126 - Exit bulkmarcimport if -a and -b given

Currently it is possible to spceify both --biblios and --authorities
as command line switches to bulkmarcimport.pl. This does not make sense
so we should exit early and explain that these switches are mutually
exclusive.

To test:
- Run one of these and check that there is no complaint about missing
  options:
  perl misc/migration_tools/bulkmarcimport.pl -a -b
  sudo koha-shell -c "perl misc/migration_tools/bulkmarcimport.pl -a -b"
  kohadev
- Observe that this displays the perldoc, but does not complain about
  mutually exclusive switches.
- Apply the patch
- Rerun the command(s) from earlier.
- Verify that the script is now halted and a small explanation given.
Comment 3 M. Tompsett 2017-01-11 16:24:29 UTC
Created attachment 58833 [details] [review]
[SIGNED-OFF] Bug 4126 - Exit bulkmarcimport if -a and -b given

Currently it is possible to spceify both --biblios and --authorities
as command line switches to bulkmarcimport.pl. This does not make sense
so we should exit early and explain that these switches are mutually
exclusive.

To test:
- Run one of these and check that there is no complaint about missing
  options:
  perl misc/migration_tools/bulkmarcimport.pl -a -b
  sudo koha-shell -c "perl misc/migration_tools/bulkmarcimport.pl -a -b"
  kohadev
- Observe that this displays the perldoc, but does not complain about
  mutually exclusive switches.
- Apply the patch
- Rerun the command(s) from earlier.
- Verify that the script is now halted and a small explanation given.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 4 Jonathan Druart 2017-01-11 18:17:21 UTC
Should not we also add the same exit if none are given instead of assuming it's biblio?

I can provide a patch if you agree with that.
Comment 5 M. Tompsett 2017-01-11 22:20:54 UTC
(In reply to Jonathan Druart from comment #4)
> Should not we also add the same exit if none are given instead of assuming
> it's biblio?
> 
> I can provide a patch if you agree with that.

While I agree that for clarity sake forcing people to specify is good, that seems to be a problem beyond the scope of this bug. Also, one has to consider: do you really want to have to type an extra 3 characters to run the command when, for the most part, it will likely be biblios?

I think the default behaviour is adequate and see no reason to change it, unless there is going to be a sweeping "we must be explicit about everything" in all the parameters for all scripts. I vote for keeping it moving on to Pass QA. :)
Comment 6 Jonathan Druart 2017-01-27 13:46:14 UTC
Created attachment 59618 [details] [review]
Bug 4126 - Exit bulkmarcimport if -a and -b given

Currently it is possible to spceify both --biblios and --authorities
as command line switches to bulkmarcimport.pl. This does not make sense
so we should exit early and explain that these switches are mutually
exclusive.

To test:
- Run one of these and check that there is no complaint about missing
  options:
  perl misc/migration_tools/bulkmarcimport.pl -a -b
  sudo koha-shell -c "perl misc/migration_tools/bulkmarcimport.pl -a -b"
  kohadev
- Observe that this displays the perldoc, but does not complain about
  mutually exclusive switches.
- Apply the patch
- Rerun the command(s) from earlier.
- Verify that the script is now halted and a small explanation given.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 7 Kyle M Hall 2017-02-14 14:00:31 UTC
Pushed to master for 17.05, thanks Magnus!
Comment 8 Katrin Fischer 2017-02-14 20:17:37 UTC
This patch has been pushed to 16.11.x and will be in 16.11.04.
Comment 9 Julian Maurice 2017-02-20 09:32:40 UTC
Pushed to 3.22.x for 3.22.17