Bug 15336 - Script for merging vendors
Summary: Script for merging vendors
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Alex Arnaud
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-09 09:38 UTC by Alex Arnaud
Modified: 2018-04-06 14:30 UTC (History)
8 users (show)

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


Attachments
Bug 15336 - New command-line script: merge_bookseller.pl (2.86 KB, patch)
2015-12-09 10:16 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 15336 - New command-line script: merge_bookseller.pl (2.94 KB, patch)
2015-12-09 13:53 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15336 - Fix QA comments (4.67 KB, patch)
2015-12-11 12:59 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 15336 - Move code related to merging into Koha::Acquisition::Bookseller.pm (8.19 KB, patch)
2015-12-17 15:00 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 15336 - Fix QA comments (7.63 KB, patch)
2016-03-18 13:31 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 15336 - New command-line script: merge_bookseller.pl (13.73 KB, patch)
2017-03-14 15:18 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 15336 - New command-line script: merge_bookseller.pl (13.73 KB, patch)
2017-03-14 15:26 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 15336 - Fix unit tests (t/Bookseller.t) (1.95 KB, patch)
2017-03-15 16:04 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 15336 - Fix unit tests (t/Bookseller.t) (1.84 KB, patch)
2017-03-15 16:09 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 15336 - New command-line script: merge_bookseller.pl (13.78 KB, patch)
2017-03-15 16:30 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15336 - Fix unit tests (t/Bookseller.t) (1.97 KB, patch)
2017-03-15 16:30 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15336 - Fix a QA issue (699 bytes, patch)
2017-06-23 13:01 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 15336 - New command-line script: merge_bookseller.pl (14.03 KB, patch)
2017-09-01 16:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15336 - Fix unit tests (t/Bookseller.t) (2.02 KB, patch)
2017-09-01 16:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15336 - Fix a QA issue (755 bytes, patch)
2017-09-01 16:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15336 - New command-line script: merge_bookseller.pl (14.09 KB, patch)
2017-10-27 10:39 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 15336 - New command-line script: misc/migration_tools/merge_vendors.pl (14.79 KB, patch)
2018-02-21 13:50 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 15336 - New command-line script: misc/migration_tools/merge_vendors.pl (14.79 KB, patch)
2018-03-13 08:15 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 15336: Add new command-line script: misc/migration_tools/merge_vendors.pl (14.87 KB, patch)
2018-03-18 21:52 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Arnaud 2015-12-09 09:38:36 UTC
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.
Comment 1 Alex Arnaud 2015-12-09 10:16:04 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2015-12-09 13:53:05 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2015-12-10 10:14:10 UTC
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.
Comment 4 Alex Arnaud 2015-12-10 10:42:13 UTC
> @@ +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 ?
Comment 5 Jonathan Druart 2015-12-10 10:49:48 UTC
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.
Comment 6 Alex Arnaud 2015-12-11 12:59:55 UTC Comment hidden (obsolete)
Comment 7 Katrin Fischer 2015-12-13 12:43:42 UTC
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 :)
Comment 8 Marc Véron 2015-12-14 06:37:02 UTC
(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!
Comment 9 Paul Poulain 2015-12-16 15:45:53 UTC
(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 !
Comment 10 Katrin Fischer 2015-12-16 16:24:12 UTC
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?
Comment 11 Alex Arnaud 2015-12-16 17:43:23 UTC
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.
Comment 12 Alex Arnaud 2015-12-17 15:00:50 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2015-12-17 21:41:18 UTC
Thank you Alex!
Comment 14 Koha Team AMU 2016-03-11 09:05:34 UTC
Test ok. Thanks!
Comment 15 Jonathan Druart 2016-03-14 18:15:19 UTC
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.
Comment 16 Alex Arnaud 2016-03-18 13:31:19 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2016-04-05 09:38:56 UTC
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)
Comment 18 Jonathan Druart 2016-04-06 13:02:55 UTC
Moreover it would make sense to execute all the moves in single transaction.
Comment 19 Alex Arnaud 2017-03-14 15:18:32 UTC Comment hidden (obsolete)
Comment 20 Alex Arnaud 2017-03-14 15:26:10 UTC
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.
Comment 21 Marc Véron 2017-03-15 11:13:15 UTC
- 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.
Comment 22 Alex Arnaud 2017-03-15 16:04:20 UTC Comment hidden (obsolete)
Comment 23 Alex Arnaud 2017-03-15 16:09:23 UTC
Created attachment 61105 [details] [review]
Bug 15336 - Fix unit tests (t/Bookseller.t)
Comment 24 Marc Véron 2017-03-15 16:30:01 UTC
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>
Comment 25 Marc Véron 2017-03-15 16:30:54 UTC
Created attachment 61109 [details] [review]
Bug 15336 - Fix unit tests (t/Bookseller.t)

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 26 Marcel de Rooy 2017-06-16 09:52:06 UTC
 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
Comment 27 Alex Arnaud 2017-06-23 13:01:06 UTC
Created attachment 64595 [details] [review]
Bug 15336 - Fix a QA issue
Comment 28 Kyle M Hall 2017-09-01 16:38:51 UTC
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>
Comment 29 Kyle M Hall 2017-09-01 16:38:58 UTC
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>
Comment 30 Kyle M Hall 2017-09-01 16:39:02 UTC
Created attachment 66767 [details] [review]
Bug 15336 - Fix a QA issue

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 31 Jonathan Druart 2017-09-01 20:18:12 UTC
I do not understand why you put the transaction in the pl and not in the method, are there any good reasons?
Comment 32 Jonathan Druart 2017-09-04 15:25:09 UTC
+ Add more POD to explain what the script actually does.
Comment 33 Alex Arnaud 2017-10-27 09:43:19 UTC
(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 ?
Comment 34 Alex Arnaud 2017-10-27 10:39:21 UTC
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>
Comment 35 Alex Arnaud 2017-10-27 10:40:44 UTC
(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 36 Katrin Fischer 2018-02-18 15:01:21 UTC
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.
Comment 37 Alex Arnaud 2018-02-21 13:50:10 UTC
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>
Comment 38 Katrin Fischer 2018-03-12 13:58:27 UTC
The file doesn't have the right permissions (misisng +x). Can you pleas fix? Tests look good, will earn it a PQA most likely!
Comment 39 Alex Arnaud 2018-03-13 08:15:59 UTC
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>
Comment 40 Katrin Fischer 2018-03-18 21:52:04 UTC
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>
Comment 41 Jonathan Druart 2018-03-27 17:22:54 UTC
(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.
Comment 42 Jonathan Druart 2018-03-27 17:43:24 UTC Comment hidden (obsolete)
Comment 43 Jonathan Druart 2018-03-27 17:43:52 UTC
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
Comment 44 Tomás Cohen Arazi 2018-04-06 14:30:36 UTC
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.