Sometimes, librarian create and use duplicates booksellers. Here is a command-line tool that can merge 2 booksellers. In fact, it moves baskets, basketgroups, contracts etc ... in the target bookseller.
Created attachment 45521 [details] [review] Bug 15336 - New command-line script: merge_bookseller.pl Test plan (i.e. merge bookseller n° 3 into the n° 6): run perl misc/migration_tools/merge_booksellers.pl -f 3 -t 6 -v -c Check that basketgroups, baskets, contacts, contracts and invoices had moved into bookseller 6. Check that bookseller n° 3 has been deleted. Remove -c (confirm) parameter to run in test mode only.
Created attachment 45530 [details] [review] Bug 15336 - New command-line script: merge_bookseller.pl Test plan (i.e. merge bookseller n° 3 into the n° 6): run perl misc/migration_tools/merge_booksellers.pl -f 3 -t 6 -v -c Check that basketgroups, baskets, contacts, contracts and invoices had moved into bookseller 6. Check that bookseller n° 3 has been deleted. Remove -c (confirm) parameter to run in test mode only. Followed test plan, works as expected. Signed-off-by: Marc Véron <veron@veron.ch>
Comment on attachment 45530 [details] [review] Bug 15336 - New command-line script: merge_bookseller.pl Review of attachment 45530 [details] [review]: ----------------------------------------------------------------- ::: misc/migration_tools/merge_booksellers.pl @@ +1,1 @@ > +#!/usr/bin/perl The copyright is missing. @@ +1,4 @@ > +#!/usr/bin/perl > +# Script that merge source bookseller (-f) into target bookseller (-t). > + > +use strict; use Modern::Perl; @@ +18,5 @@ > + 't:s' => \$mergeto, > + 'c' => \$confirm, > +); > + > +if ($help || !$mergefrom || !$mergeto) { you could also test mergefrom == mergeto @@ +30,5 @@ > +my $to_bookseller = Koha::Acquisition::Bookseller->fetch({id => $mergeto}); > +die "Unknown bookseller id ($mergeto)" unless $to_bookseller; > + > +my $report; > +foreach (qw(Aqbasketgroup Aqbasket Aqcontract Aqcontact Aqinvoice)) { What about the tables items, deleteditems, subscription? @@ +31,5 @@ > +die "Unknown bookseller id ($mergeto)" unless $to_bookseller; > + > +my $report; > +foreach (qw(Aqbasketgroup Aqbasket Aqcontract Aqcontact Aqinvoice)) { > + my $rs = Koha::Database->new()->schema->resultset($_); Please don't use $_ @@ +47,5 @@ > +print "\n" if $verbose; > +print_report($report); > +print "Nothing done (test only). Use confirm option to commit changes in database\n" unless $confirm; > + > +sub print_usage { use Pod::Usage; see misc/export_records.pl for an example.
> @@ +30,5 @@ > > +my $to_bookseller = Koha::Acquisition::Bookseller->fetch({id => $mergeto}); > > +die "Unknown bookseller id ($mergeto)" unless $to_bookseller; > > + > > +my $report; > > +foreach (qw(Aqbasketgroup Aqbasket Aqcontract Aqcontact Aqinvoice)) { > > What about the tables items, deleteditems, subscription? I checked aqbooksellers relationship on this page: http://schema.koha-community.org/tables/aqbooksellers.html that doesn't mention items, deleteditems or subscription tables. Looks that the schema is not up to date. Is there any other tables i forgot to handle ?
grep booksellerid installer/data/mysql/kohastructure.sql And look at the FK, there is none on these 3 tables, that's why you haven't found them.
Created attachment 45607 [details] [review] Bug 15336 - Fix QA comments
I wonder if it wouldn't be nice to offer this option from the GUI instead of it only being available via command line. Having the logic in one of the modules would also allow for unit testing and make me feel better about this :)
(In reply to Katrin Fischer from comment #7) > I wonder if it wouldn't be nice to offer this option from the GUI instead of > it only being available via command line. Having the logic in one of the > modules would also allow for unit testing and make me feel better about this > :) Good idea!
(In reply to Katrin Fischer from comment #7) > I wonder if it wouldn't be nice to offer this option from the GUI instead of > it only being available via command line. Having the logic in one of the > modules would also allow for unit testing and make me feel better about this > :) The sponsored dev was related to something in command line : that's considered a back-office stuff, that has to be done "once", to fix some inconsistencies in the database. so let's sign/qa/push this one, and if someone else feel it's useful to have it as command line, that's for another ticket !
I think even as a command line script it's worth while to have this as a function in the proper place and testable - don't you think?
I started to move code related to "merging" into Koha::Acquisition::Bookseller" (and use it in the command line script) so that i can test it and if we want to have this option from GUI it could be done in another BZ. Patch is coming tomorrow.
Created attachment 45777 [details] [review] Bug 15336 - Move code related to merging into Koha::Acquisition::Bookseller.pm
Thank you Alex!
Test ok. Thanks!
1/ I think merge_with is more appropriate than merge_to 2/ + unless (my $to_bookseller = Koha::Acquisition::Bookseller->fetch({id => $mergeto})) { $to_bookseller is never used 3/ merge_to method should not return the error, an error should be raised and caught by the caller 4/ t/Bookseller.t is DB dependent, it should be moved to the appropriate dir (and perltidied) Note that I have not tested the patch set yet.
Created attachment 49313 [details] [review] Bug 15336 - Fix QA comments
Everything looks good, I have only 2 concerns: 1/ It would be nice to add a big warning in the help page to warn users that they must ony use this script if they know what they are doing 2/ Could you rename the script and if possible occurrences of bookseller with vendor + perltidy the script (certainly easily by squashing the patchset)
Moreover it would make sense to execute all the moves in single transaction.
Created attachment 61070 [details] [review] Bug 15336 - New command-line script: merge_bookseller.pl Test plan (i.e. merge bookseller n° 3 into the n° 6): run perl misc/migration_tools/merge_booksellers.pl -f 3 -t 6 -v -c Check that basketgroups, baskets, contacts, contracts and invoices had moved into bookseller 6. Check that bookseller n° 3 has been deleted. Remove -c (confirm) parameter to run in test mode only.
Created attachment 61071 [details] [review] Bug 15336 - New command-line script: merge_bookseller.pl Test plan (i.e. merge bookseller n° 3 into the n° 6): run perl misc/migration_tools/merge_booksellers.pl -f 3 -t 6 -v -c Check that basketgroups, baskets, contacts, contracts and invoices had moved into bookseller 6. Check that bookseller n° 3 has been deleted. Remove -c (confirm) parameter to run in test mode only.
- Script works fine - t/db_dependent/Bookseller.t passes OK - t/Bookseller.t fails with: t/Bookseller.t .. 1/14 Can't use string ("Koha::Acquisition::Bookseller") as a HASH ref while "strict refs" in use at (...)/Koha/Object.pm line 275.
Created attachment 61104 [details] [review] Bug 15336 - Fix unit tests (t/Bookseller.t)
Created attachment 61105 [details] [review] Bug 15336 - Fix unit tests (t/Bookseller.t)
Created attachment 61108 [details] [review] Bug 15336 - New command-line script: merge_bookseller.pl Test plan (i.e. merge bookseller n° 3 into the n° 6): run perl misc/migration_tools/merge_booksellers.pl -f 3 -t 6 -v -c Check that basketgroups, baskets, contacts, contracts and invoices had moved into bookseller 6. Check that bookseller n° 3 has been deleted. Remove -c (confirm) parameter to run in test mode only. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 61109 [details] [review] Bug 15336 - Fix unit tests (t/Bookseller.t) Signed-off-by: Marc Véron <veron@veron.ch>
FAIL t/db_dependent/Bookseller.t OK critic OK forbidden patterns OK git manipulation OK pod SKIP spelling FAIL valid "our" variable $builder masks earlier declaration in same scope
Created attachment 64595 [details] [review] Bug 15336 - Fix a QA issue
Created attachment 66765 [details] [review] Bug 15336 - New command-line script: merge_bookseller.pl Test plan (i.e. merge bookseller n° 3 into the n° 6): run perl misc/migration_tools/merge_booksellers.pl -f 3 -t 6 -v -c Check that basketgroups, baskets, contacts, contracts and invoices had moved into bookseller 6. Check that bookseller n° 3 has been deleted. Remove -c (confirm) parameter to run in test mode only. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 66766 [details] [review] Bug 15336 - Fix unit tests (t/Bookseller.t) Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 66767 [details] [review] Bug 15336 - Fix a QA issue Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
I do not understand why you put the transaction in the pl and not in the method, are there any good reasons?
+ Add more POD to explain what the script actually does.
(In reply to Jonathan Druart from comment #31) > I do not understand why you put the transaction in the pl and not in the > method, are there any good reasons? My opinion is that commit or rollback should not take place in the API. As Katrin asked, we could use merge_with to add this feature on the GUI. In this case, what is point to have the transaction in there ? Will we offer the ability to "test" before confirm on the GUI ?
Created attachment 68676 [details] [review] Bug 15336 - New command-line script: merge_bookseller.pl Test plan (i.e. merge bookseller n° 3 into the n° 6): run perl misc/migration_tools/merge_booksellers.pl -f 3 -t 6 -v -c Check that basketgroups, baskets, contacts, contracts and invoices had moved into bookseller 6. Check that bookseller n° 3 has been deleted. Remove -c (confirm) parameter to run in test mode only. Signed-off-by: Marc Véron <veron@veron.ch>
(In reply to Jonathan Druart from comment #32) > + Add more POD to explain what the script actually does. I've added a little description for the script. Are you ok with that? Also squashed patches
Comment on attachment 68676 [details] [review] Bug 15336 - New command-line script: merge_bookseller.pl Review of attachment 68676 [details] [review]: ----------------------------------------------------------------- Hi Alex, started QA on this, but found some issues, please fix: 1) QA tools fail: FAIL Koha/Acquisition/Bookseller.pm FAIL pod empty =head3 in file Koha/Acquisition/Bookseller.pm *** ERROR: FAIL pod coverage POD is missing for 'merge_with' 2) Please fix your commit message to include the correct script name! I think you also change subscriptions and items? Please document in your test plan. ::: Koha/Acquisition/Bookseller.pm @@ +77,5 @@ > + > + my $from_vendor = Koha::Acquisition::Booksellers->find( $mergefrom ); > + my $result = $from_vendor->merge_with($mergeto); > + > +merge source vendor (-f) into target vendor (-t) 3) ...merge with :) ::: misc/migration_tools/merge_vendors.pl @@ +102,5 @@ > + > +=item B<--confirm> > + > + --confirm Commit the changes in database. Running without this > + parameter is like testing. It is recommanded to run without 4) ... recommended @@ +107,5 @@ > + --confirm first to be sure of the changes. > + > +=item B<--from> > + > + --from=<vendorid> The source vendor identifier that will be merged in 5) Suggestion: The identifier of the source vendor that will be merged with the target vendor. @@ +112,5 @@ > + target vendor. > + > +=item B<--to> > + > + --to=<vendorid> The target vendor identifier in which merge 6) Suggestion: The identifier of the target vendor that the source vendor is merged with. ::: t/Bookseller.t @@ +1,3 @@ > #!/usr/bin/perl > # > +# This Koha test module is a stub! 7) Could possibly remove this now :) All tests pass for me.
Created attachment 72023 [details] [review] Bug 15336 - New command-line script: misc/migration_tools/merge_vendors.pl Test plan (i.e. merge bookseller n° 3with the n° 6): run perl misc/migration_tools/merge_vendors.pl -f 3 -t 6 -v -c Check that basketgroups, baskets, contacts, contracts, invoices, deleteditems, items and subscriptions had moved into bookseller 6. Check that bookseller n° 3 has been deleted. Remove -c (confirm) parameter to run in test mode only. Signed-off-by: Marc Véron <veron@veron.ch>
The file doesn't have the right permissions (misisng +x). Can you pleas fix? Tests look good, will earn it a PQA most likely!
Created attachment 72734 [details] [review] Bug 15336 - New command-line script: misc/migration_tools/merge_vendors.pl Test plan (i.e. merge bookseller n° 3with the n° 6): run perl misc/migration_tools/merge_vendors.pl -f 3 -t 6 -v -c Check that basketgroups, baskets, contacts, contracts, invoices, deleteditems, items and subscriptions had moved into bookseller 6. Check that bookseller n° 3 has been deleted. Remove -c (confirm) parameter to run in test mode only. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 73078 [details] [review] Bug 15336: Add new command-line script: misc/migration_tools/merge_vendors.pl Test plan (i.e. merge bookseller n° 3with the n° 6): run perl misc/migration_tools/merge_vendors.pl -f 3 -t 6 -v -c Check that basketgroups, baskets, contacts, contracts, invoices, deleteditems, items and subscriptions had moved into bookseller 6. Check that bookseller n° 3 has been deleted. Remove -c (confirm) parameter to run in test mode only. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
(In reply to Alex Arnaud from comment #33) > (In reply to Jonathan Druart from comment #31) > > I do not understand why you put the transaction in the pl and not in the > > method, are there any good reasons? > > My opinion is that commit or rollback should not take place in the API. As > Katrin asked, we could use merge_with to add this feature on the GUI. In > this case, what is point to have the transaction in there ? Will we offer > the ability to "test" before confirm on the GUI ? Tomas, Can I get your opinion on this please? I am expecting the whole move to be done in a transaction (handled by the method) and an exception raised if something failed. The dry-run mode could be done using nested transactions.
14:27 < tcohen> I agree with you on 20446 14:27 < tcohen> and I don't like code 'dying' instead of raising exceptions 14:29 < tcohen> I think rollback+exception should take place in the lib, because the controller should consider the API call an atomic action, that can fail
14:27 < tcohen> and I don't like code 'dying' instead of raising exceptions 14:29 < tcohen> I think rollback+exception should take place in the lib, because the controller should consider the API call an atomic action, that can fail
Alex, can you move the transaction into ->merge_with (similar to the code in 'Consistency follow-up' in bug 9302)? And also make sure those tests added to t/Bookseller.t are actually in db_dependent? Regarding the transaction, you can use both $self->_result->result_source->schema->txn_do( sub { <code> } ); or your txn_begin / txn_commit|txn_rollback construct.