After the bug 17855 is signed off then writing a patch for this bug can start. The aim of this bug is to remove all sql code from onboarding.pl, smart-rules.pl and itemtypes.pl scripts. Additionally all unnecessary use pragmas in the onboarding.pl script will be removed. This will make all three of the aforementioned perl scripts meet the Koha coding guidelines.
Start with onboarding in this bug. Put smart rules and itemtypes in separate bugs. Smaller bugs get QA'd faster. Link them in the see alsos.
Created attachment 59391 [details] [review] Bug 17944 - Moved the sql code from itemtypes.pl into Koha/ItemTypes.pm
Test plan: 1. View the admin/itemtypes.pl script and confirm that sql code is written in this file 2. Apply this patch 3. View admin/itemtypes.pl script notice the sql code has been removed and replaced with the perl module call (on line 142): my $total = Koha::ItemTypes->check_items_and_biblioitems($itemtype_code); 4. View Koha/ItemTypes.pm and observe the subroutine check_items_and_biblioitems (This subroutine contains the sql code that was in itemtypes.pl) 5. Now to check that the deletion of an item type still works with the sql code moved out of the itemtypes.pl script, log into the Koha staff interface 6. Visit Administration->Item type. Select the 'Delete' button next to an item type 7. Select the 'Yes, delete this item type' button 8. Observe the message informing you the item type has been deleted. Thus the patch works as intended. Note: as I understand it placing the sql code into a perl module in the Koha directory is acceptable, because sql code exists in the perl modules in the C4 directory. Please advise me if this isn't the case and I will move this code again.
Comment on attachment 59391 [details] [review] Bug 17944 - Moved the sql code from itemtypes.pl into Koha/ItemTypes.pm Review of attachment 59391 [details] [review]: ----------------------------------------------------------------- ::: Koha/ItemTypes.pm @@ +53,5 @@ > + ) AS tmp > + WHERE tmp.t=? > + ', {}, $itemtype_code ); > + return $total; > +} While not an easy thing, I don't even know how to do it off the top of my head, consider reworking in DBIx. Also, wouldn't this partially double count? Don't worry, this is a pre-existing issue.
Created attachment 59564 [details] [review] Bug 17944 - Removed the sql code from Itemtypes.pm and replaced it with DBIx database query in the itemtypes.pl administrative script Test plan: 1. In the staff interface, stage and manage MARC records for import 2. Try to delete an itemtype. If there are items of that itemtype in the database then a message telling you the number of items of that itemtype there are will be displayed. 3. Record that number 4. View the admin/itemtpes.pl script and confirm that there is sql code written in this file. 5. Apply this patch 6. View the admin/itemtypes.pl script and observe that there is no sql in this file. There is however DBIx code, for example $schema->resultset('Item')->search({ 'itype' => $itemtype_code} ); which is searching for items with the itype value matching $itemtype_code value. 7. In the staff interface try to delete the same itemtype 8. Record the number of items there are with that itemtype in the resulting message 9. The numbers recorded in steps 3 and 8 should match showing that the DBIx code is working as intended
(in reply to comment 4 by M.tompsett) I have tried to implement DBIx in the itemtypes.pl script. To get it working successfully I had to implement two DBIx queries, one to search the items and another to search the biblioitems for the itemtype being attempted to be deleted. Then I add the number of retrieved items together. I tried to implement the entire original sql query into the equivalent singular DBIx query however I could not get a successful search of both items and biblioitems.
Comment on attachment 59564 [details] [review] Bug 17944 - Removed the sql code from Itemtypes.pm and replaced it with DBIx database query in the itemtypes.pl administrative script Review of attachment 59564 [details] [review]: ----------------------------------------------------------------- ::: admin/itemtypes.pl @@ +145,5 @@ > + my $itemtotal = $schema->resultset('Item')->search({ 'itype' => $itemtype_code} ); > + my $bibliototal = $schema->resultset('Biblioitem')->search({ 'itemtype' => $itemtype_code} ); > + > + $itemtotal->count; > + $bibliototal->count; Why? @@ +146,5 @@ > + my $bibliototal = $schema->resultset('Biblioitem')->search({ 'itemtype' => $itemtype_code} ); > + > + $itemtotal->count; > + $bibliototal->count; > + my $overalltotal = $itemtotal + $bibliototal; I like the idea, but this segment of code is not quite right. Why the context line? why not my $foo = $schema->resulset('Bar')->search({ 'foobar' => $itemtypecode })->count // 0; with the appropriate variables and tables changed?
I'll try implementing that now.
So before I attach the new commit just to check would you be happy with this? my $schema = Koha::Database->new()->schema(); my $itemtotal = $schema->resultset('Item')->search({ 'itype' => $itemtype_code})->count; my $bibliototal = $schema->resultset('Biblioitem')->search({ 'itemtype' => $itemtype_code})->count; my $overalltotal = $itemtotal + $bibliototal; if ($overalltotal) { push @messages, { type => 'error', code => 'cannot_be_deleted', total => $overalltotal }; $op = 'list'; } else { my $itemtype = Koha::ItemTypes->find($itemtype_code); $template->param( itemtype => $itemtype, ); } I have tested it and it outputs the correct value
(In reply to Alex Buckley from comment #9) > So before I attach the new commit just to check would you be happy with this? Yes, and it is prettier. Thank you. :)
Created attachment 59565 [details] [review] Bug 17944 - Tidied the DBIx code in itemtypes.pl
Created attachment 59655 [details] [review] [SIGNED-OFF] Bug 17944 - Moved the sql code from itemtypes.pl into Koha/ItemTypes.pm Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 59656 [details] [review] [SIGNED-OFF] Bug 17944 - Removed the sql code from Itemtypes.pm Removed the sql code from Itemtypes.pm and replaced it with DBIx database query in the itemtypes.pl administrative script Test plan: 1. In the staff interface, stage and manage MARC records for import 2. Try to delete an itemtype. If there are items of that itemtype in the database then a message telling you the number of items of that itemtype there are will be displayed. 3. Record that number 4. View the admin/itemtpes.pl script and confirm that there is sql code written in this file. 5. Apply this patch 6. View the admin/itemtypes.pl script and observe that there is no sql in this file. There is however DBIx code, for example $schema->resultset('Item')->search({ 'itype' => $itemtype_code} ); which is searching for items with the itype value matching $itemtype_code value. 7. In the staff interface try to delete the same itemtype 8. Record the number of items there are with that itemtype in the resulting message 9. The numbers recorded in steps 3 and 8 should match showing that the DBIx code is working as intended Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 59657 [details] [review] [SIGNED-OFF] Bug 17944 - Tidied the DBIx code in itemtypes.pl Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Can I suggest to create a method in Koha::ItemType (something like ->is_used), which would return Koha::Items->search({ itype => $itemtype_code })->count + Koha::Biblioitems->search({ itemtype => $itemtype_code })->count That way we could provide tests for this change.
Created attachment 59715 [details] [review] Bug 17944 - Moved the DBIx code into the ItemType.pm file
Comment on attachment 59715 [details] [review] Bug 17944 - Moved the DBIx code into the ItemType.pm file Review of attachment 59715 [details] [review]: ----------------------------------------------------------------- ::: admin/itemtypes.pl @@ +140,4 @@ > > } elsif ( $op eq 'delete_confirm' ) { > > + my $overalltotal = Koha::ItemType->get_items_and_biblioitems($itemtype_code); This is what was asked for in comment #15. @@ +151,4 @@ > > } elsif ( $op eq 'delete_confirmed' ) { > > + my $itemtype = Koha::ItemType->get_items_and_biblioitems($itemtype_code); This change confuses me.
Created attachment 59716 [details] [review] Bug 17955 - removed incorrect function call
Created attachment 59717 [details] [review] Bug 17944 - Added additional USE pragma
Created attachment 59718 [details] [review] Bug 17944 - Moved the DBIx code into the ItemType.pm file Created get_items_and_biblioitems in Koha::ItemType using DBIx coding style. Changed itemtypes.pl to use that method This addresses comment #15. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Alex, There are some wrong things here: 1/ No tests are provided 2/ You do not instanciate a Koha::ItemType object but using get_items_and_biblioitems as a class method. 3/ get_items_and_biblioitems is badly named, it does not return the items and the biblioitems, but the number of item and biblioitems. That's why I suggested ->is_used, which would return true or false. 4/ There is no POD for this new method 5/ Why do not you want to use Koha::Items and Koha::Biblioitems?
(in reply to Jonathan Druart in comment 21) 1/ What tests specifically do you want? 3/ I tried '->is_used' but it threw a strict refs error (as this is my first time writing perl subroutines I tried a different subroutine name (get_items_and_biblioitems) which worked successfully.
(in reply to Jonathan Druart in comment 21) 1/ Presumably you mean test plan rather than the output of a qa test? If so the test plan is slightly modified from comment 5) Test plan: 1. In the staff interface, stage and manage MARC records for import 2. Try to delete an itemtype. If there are items of that itemtype in the database then a message telling you the number of items of that itemtype there are will be displayed. 3. Record that number 4. View the admin/itemtpes.pl script and confirm that there is sql code written in this file. 5. Apply this patch 6. View the admin/itemtypes.pl script and observe that there is no sql in this file. Confirm there is a module call my $overalltotal = Koha::ItemType->get_items_and_biblioitems($itemtype_code) 7. Go to Koha/ItemType.pm and confirm that there is a function named get_items_and_biblioitems 8. In the staff interface try to delete the same itemtype 9. Record the number of items there are with that itemtype in the resulting message 10. The numbers recorded in steps 3 and 9 should match showing that the DBIx code in the get_items_and_biblioitems subroutine is working as intended
(in reply to Jonathan Druart in comment 21) Please disregard comment 22, I have fixed the strict refs error issue. In the commit I am currently working on I have addressed your following points 2, and 3. Could you please explain what you mean in your point 5? Are you asking why I havent put the is_used subroutine in the Koha::Items and Koha::Biblioitems files?
(In reply to Alex Buckley from comment #22) > (in reply to Jonathan Druart in comment 21) > > 1/ What tests specifically do you want? When a subroutine or a method is added or updated, we need to provide tests ("unit tests") in the t directory. For a method like "Koha::ItemType->is_used" you will want to add tests to t/db_dependent/Koha/ItemTypes.t. Create a new subtest per method is a good idea (`git grep subtest t` to see examples) In your case you want to test that this new method returns true if at least 1 item and/or 1 biblioitem is using it. Otherwise it returns false. (In reply to Alex Buckley from comment #24) > (in reply to Jonathan Druart in comment 21) > Could you please explain what you mean in your point 5? Are you asking why I > havent put the is_used subroutine in the Koha::Items and Koha::Biblioitems > files? Instead of using $schema->resultset('Item')->search you can use Koha::Items->search We have Koha objects to represent our data model, so when they exist (Koha::Items and Koha::Biblioitems do exist) it's better to use them.
Created attachment 59953 [details] [review] Bug 17944 - Updates following second tester feedback Changed function name from get_items_and_biblioitems to is_used Added POD for is_used function Initial unit test for is_used implemented (not yet working) Implemented use of Koha::Items and Koha::Biblioitems instead of using Koha resultsets
Created attachment 59954 [details] [review] Bug 17944 - Implemented a subtest in the ItemTypes.t file for is_used function in ItemType.pm Test plan: 1. Go to t/db_dependent/Koha/ 2. Start koha-shell: sudo koha-shell <instancename> 3. Run the ItemTypes.t unit test: prove -v ItemTypes.t 4. Notice all tests are successful, and notice that the last two outputs show the is_used subtest gave successful results
Looks like it doesn't work on my koha : On the master : All tests successful. Files=1, Tests=18, 1 wallclock secs ( 0.02 usr 0.01 sys + 0.84 cusr 0.04 csys = 0.91 CPU) Result: PASS On branch bug/17944 : DBD::mysql::st execute failed: Duplicate entry '1' for key 'PRIMARY' [for Statement "INSERT INTO `biblio` ( `biblionumber`) VALUES ( ? )" with ParamValues: 0=1] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1834. DBIx::Class::Storage::DBI::_dbh_execute(): Duplicate entry '1' for key 'PRIMARY' at /home/koha/src/Koha/Object.pm line 120 # Looks like you planned 20 tests but ran 2. # Looks like your test exited with 255 just after 2.
(In reply to Baptiste in comment 28) Ah sorry I see what the issue is I accidentally missed some steps in the test plan for this patch. The ItemTypes.t unit test needs to be run after running the web installer (installing libraries, categories and item types) but before adding/importing any bibliographic records. You were getting a duplicate entry error (which I have just replicated) by having biblio record(s) in your Koha database before running this unit test and so when a record with the primary key of 1 is inserted into the biblio table by this unit test (a change I added to the ItemTypes.t unit test as part of this patch) it throws an error because a biblio record with the primary key of 1 already exists. Therefore a corrected test plan for attachment 59954 [details] [review] (Bug 17944 - Implemented a subtest in the ItemTypes.t file for is_used function in ItemType.pm) is: 1. Apply patches of this bug 2. Restart memcached: sudo service memcached restart 3. Drop and recreate your Koha database 4. Run through the Koha web installer making sure to select all data values for installation 5. When the web installer has finished, in your terminal navigate to t/db_dependent/Koha/ 6. Start koha-shell: sudo koha-shell <instancename> 7. Run the ItemTypes.t unit test: prove -v ItemTypes.t 8. Notice all tests are successful, and notice that the last two outputs show the is_used subtest gave successful results
Wanted to test, but can not apply: Wende an: Bug 17944 - Moved the sql code from itemtypes.pl into Koha/ItemTypes.pm Wende an: Bug 17944 - Removed the sql code from Itemtypes.pm Wende an: Bug 17944 - Tidied the DBIx code in itemtypes.pl Wende an: Bug 17944 - Moved the DBIx code into the ItemType.pm file Wende an: Bug 17944 - Updates following second tester feedback /home/marc/koha/.git/rebase-apply/patch:16: trailing whitespace. =head3 is_used /home/marc/koha/.git/rebase-apply/patch:91: trailing whitespace. itemlost => 0, fatal: sha1 information is lacking or useless (Koha/ItemType.pm). Dem Repository fehlen notwendige Blobs um auf einen 3-Wege-Merge zurückzufallen. Kann nicht zu 3-Wege-Merge zurückfallen. (Can not fall back to 3-way-merge because of missing blobs)
Created attachment 62499 [details] [review] Bug 17944 - Moved the sql code from itemtypes.pl into Koha/ItemTypes.pm Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 62500 [details] [review] Bug 17944 - Removed the sql code from Itemtypes.pm Removed the sql code from Itemtypes.pm and replaced it with DBIx database query in the itemtypes.pl administrative script Test plan: 1. In the staff interface, stage and manage MARC records for import 2. Try to delete an itemtype. If there are items of that itemtype in the database then a message telling you the number of items of that itemtype there are will be displayed. 3. Record that number 4. View the admin/itemtpes.pl script and confirm that there is sql code written in this file. 5. Apply this patch 6. View the admin/itemtypes.pl script and observe that there is no sql in this file. There is however DBIx code, for example $schema->resultset('Item')->search({ 'itype' => $itemtype_code} ); which is searching for items with the itype value matching $itemtype_code value. 7. In the staff interface try to delete the same itemtype 8. Record the number of items there are with that itemtype in the resulting message 9. The numbers recorded in steps 3 and 8 should match showing that the DBIx code is working as intended Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 62501 [details] [review] Bug 17944 - Tidied the DBIx code in itemtypes.pl Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 62502 [details] [review] Bug 17944 - Tidied the DBIx code in itemtypes.pl Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 62503 [details] [review] Bug 17944 - Moved the DBIx code into the ItemType.pm file Created get_items_and_biblioitems in Koha::ItemType using DBIx coding style. Changed itemtypes.pl to use that method This addresses comment #15. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 62504 [details] [review] Bug 17944 - Updates following second tester feedback Changed function name from get_items_and_biblioitems to is_used Added POD for is_used function Initial unit test for is_used implemented (not yet working) Implemented use of Koha::Items and Koha::Biblioitems instead of using Koha resultsets
Created attachment 62505 [details] [review] Bug 17944 -Integrated the removal of SQL from itemtypes.pl admin script with the onboarding tool following tester feedback of merge conflict
I have merged bug 17855 and the patches of this bug and reattached so now the patches on this bug report apply successfully after you have applied bug 17855
Alex, Please squash all the patches and resubmit, for readability ;) For the check, I think the best would be to override Koha::Object->delete from Koha::ItemType->delete (`git grep 'sub delete' Koha` for examples), and raise an exception if the item type cannot be deleted. You will need to surround the ->delete call with an eval in the pl script. Let me known if you have any questions about this suggestion.
(In reply to Jonathan Druart in comment 39) Hi Jonathan I have squashed all the bugs together locally and I have implemented a delete method in ItemType.pm to override Koha::Object->delete. I have implemented a try catch so if the delete doesn't work then catch is run and Koha::Exceptions::CannotDeleteDefault->throw is run (as shown below) >116 sub delete{ >117 my ( $self ) = @_; >118 try { >119 my $deleted = $self->SUPER::delete($self); >120 logaction( 'ITEMTYPE', 'DELETE', $self->reserve_id, Dumper( >$self->unblessed) ) >121 if C4::Context->preference('ItemTypeLog'); >122 return $deleted; >123 } catch { >124 Koha::Exceptions::CannotDeleteDefault->throw('itemtype >could not be deleted'); >125 } >126 } However I am a bit confused where you would like the Koha::ItemType->delete call to be in itemtypes.pl. Correct me if I am wrong but I think it cant go in elsif ( $op eq 'delete_confirm') block because putting a caller to delete into this block means if the delete runs successfully the user has no choice to confirm (or not) the deletion.
Created attachment 63045 [details] [review] Bug 17944 - Overridden Koha::Object->delete with Koha::ItemType->delete and implemented exception to be shown if the itemtype cannot be deleted.
Alex, Some additional remarks (we are aiming the perfect patch, right? :)) 1. The POD of the new method is incorrect, please adapt it. Note that a line should not be longer than 80 chars 2. The method can_be_deleted should be something like: sub can_be_deleted { my ($self) = @_; my $nb_items = Koha::Items->search( { itype => $self->itemtype } )->count; my $nb_biblioitems = Koha::Biblioitems->search( { itemtype => $self->itemtype } )->count; return $nb_items + $nb_biblioitems == 0; } $self is the current object, from this object you know the itemtype (code), so you so not need to pass it as an argument. 3. The tests are wrong: I have a biblio with biblionumber=1, so they fail (Duplicate entry '1' for key 'PRIMARY'). You need to use our TestBuilder module to generate random data. What you want is: a) Generate the $builder object: my $builder = t::lib::TestBuilder->new; b) create an item type: my $item_type = $builder->build_object({ class => 'Koha::ItemTypes' }); c) you want to test if an item type, not used, can be removed. It's time! is( $item_type->can_be_deleted, 1, 'An item type that is not used can be deleted' ); d) create an item using this item type: my $item = $builder->build_object({ class => 'Koha::Items', value => { itype => $item_type->itemtype }); e) Then you want to know if the item type can be removed is( $item_type->can_be_deleted, 0, 'An item type that is used by an item cannot be deleted ); Then you can continue with a biblioitem object. Please resubmit a patch without the delete method, just can_be_deleted step-by-step :)
Created attachment 63152 [details] [review] Bug 17944 - Removed SQl query from itemtypes.pl and replaced it with can_be_deleted method in Koha/ItemType.pm
Hi Jonathan I have implemented the changes you suggested in a single patch for bug 17944. Because I have implemented the following return line (which you suggested) from can_be_deleted() subroutine: return $nb_items + $nb_biblioitems == 0; I had to slightly change the unit test from how you suggested it, this is because whenever there were items and/or biblioitems with the itemtype it meant that ' ' was returned. So the unit test callers are now: is( $item_type->can_be_deleted, '', 'An item type that is used by an item cannot be deleted' ); is ( $item_type->can_be_deleted, '', 'An item type that is used by a biblioitem cannot be deleted' ); Additionally as '' is returned if the itemtype is in use, rather than the number of items and biblioitems using the itemtype I have changed itemtypes.tt to saying: Cannot delete this item type. This record is in use. Deletion is not possible. If you would still like the number of items and biblioitems using the itemtype to be displayed to the user in itemtypes.tt then I will need to change the return for can_be_deleted to a longer if else statement. Could you please have a look at this patch?
Alex, great! The patch is much better :) Indeed the return value of can_be_deleted should be 1 or 0. You can achieve that with: return $nb_items + $nb_biblioitems == 0 ? 1 : 0; You will avoid the empty string return value. In the tests: The last test says 'An item type that is used by a biblioitem cannot be deleted'. But actually you are testing that "an item that is used by AN item AND A biblioitem cannot be deleted'. You should delete the item to make it test what you want to test. Finally you will need to update the admin script.
I am switching the status to 'in discussion' until it's ready to be tested.
Created attachment 63220 [details] [review] Bug 17944 - Removed SQl query from itemtypes.pl and replaced it with can_be_deleted method in Koha/ItemType.pm In Koha/ItemTypes.t I have reworded unit test output and added test for item deletion. In addition to updating admin script by removing warn and changing if statement check
Hi Jonathan I have implemented: > return $nb_items + $nb_biblioitems == 0 ? 1 : 0; I have changed the wording of the last test and have implemented a test to remove the item. All tests run successfully. As for updating the admin script I have removed the warn and changed the if statement check so it is now: > if ($overalltotal == 0){ Was there any other updates you wanted for the itemtypes.pl admin script?
Comment on attachment 63220 [details] [review] Bug 17944 - Removed SQl query from itemtypes.pl and replaced it with can_be_deleted method in Koha/ItemType.pm Review of attachment 63220 [details] [review]: ----------------------------------------------------------------- ::: admin/itemtypes.pl @@ +139,5 @@ > $op = 'list'; > + > + } elsif ( $op eq 'delete_confirm' ) { > + my $ItemType = Koha::ItemType->new(); > + my $overalltotal = $ItemType->can_be_deleted(); You are not retrieving any item type here, ->can_be_deleted will only return 0. You need to call Koha::ItemType->find($id); to retrieve the item type object you want from the DB. @@ +154,2 @@ > my $itemtype = Koha::ItemTypes->find($itemtype_code); > + my $deleted = eval{ $itemtype->delete };; Unnecessary change. ::: t/db_dependent/Koha/ItemTypes.t @@ +139,5 @@ > +is( $item_type->can_be_deleted, 0, 'An item type that is used by an item cannot be deleted' ); > + > +my $biblio = $builder->build_object({ class => 'Koha::Biblioitems', value => { itemtype => $item_type->itemtype }}); > + > +is ( $item_type->can_be_deleted, 0, 'An item that is used by an item and a biblioitem cannot be deleted' ); This second try does not test what you want, see my previous comment: """ The last test says 'An item type that is used by a biblioitem cannot be deleted'. But actually you are testing that "an item that is used by AN item AND A biblioitem cannot be deleted'. You should delete the item to make it test what you want to test. """
Created attachment 63259 [details] [review] Bug 17944 - Removed SQl query from itemtypes.pl and replaced it with can_be_deleted method in Koha/ItemType.pm In Koha/ItemTypes.t I have reworded unit test output and added test for item deletion. In addition to updating admin script by removing warn and changing if statement check
Hi Jonathan In the patch I just attached I have addressed all of your points: 1) > You are not retrieving any item type here, ->can_be_deleted will only return 0. > You need to call Koha::ItemType->find($id); to retrieve the item type object > you want from the DB. I have changed the itemtypes.pl admin script (see below) so it now performs the can_be_deleted() function on the result of the find() function, therefore it is actually retrieving a item type here: > 142 my $ItemType = Koha::ItemTypes->find($itemtype_code); > 143 my $overalltotal = $ItemType->can_be_deleted(); > 144 if ($overalltotal == 0) { > 145 push @messages, { type => 'error', code => 'cannot_be_delet ed'}; 2) > my $itemtype = Koha::ItemTypes->find($itemtype_code); > + my $deleted = eval{ $itemtype->delete };; > Unnecessary change. I have changed the above code removing the changes to the second line and removing the second semi colon in the second line (see below): > 153 my $itemtype = Koha::ItemTypes->find($itemtype_code); > 154 my $deleted = eval { $itemtype->delete }; 3) > This second try does not test what you want, see my previous comment: > """ > The last test says 'An item type that is used by a biblioitem cannot be > deleted'. But actually you are testing that "an item that is used by AN item > AND A biblioitem cannot be deleted'. > You should delete the item to make it test what you want to test. > """ I have changed the test output from 'an item that is used by AN item AND A biblioitem cannot be deleted' to 'An item type that is used by an item and a biblioitem cannot be deleted' because we are actually running the can_be_deleted() function on itemtype not an item. Additionally I have implemented a test for deleting the item and biblioitem that use the itemtype in question. Then I have implemented another further test to show that now the item and biblioitem have been removed the can_be_deleted() function correctly shows that the itemtype can be deleted: > 145 is ( $item->delete, 1, 'An item has been deleted' ); > 146 > 147 is ( $biblio->delete, 1, 'A biblioitem has been deleted' ); > 148 > 149 is ( $item_type->can_be_deleted, 1, 'The item type that was being u sed > by the removed item and biblioitem can now be deleted' ); I have run the ItemTypes.t unit test and all tests are successful: > kohadev-koha@kohadevbox:/home/vagrant/kohaclone/t/db_dependent/Koha$ prove -v > ItemTypes.t > ItemTypes.t .. > 1..26 > ok 1 - use Koha::ItemType; > ok 2 - use Koha::ItemTypes; > ok 3 - first result > ok 4 - itemtype/code > ok 5 - description > ok 6 - rentalcharge > ok 7 - imageurl > ok 8 - summary > ok 9 - checkinmsg > ok 10 - checkinmsgtype > ok 11 - second result > ok 12 - itemtype/code > ok 13 - description > ok 14 - rentalcharge > ok 15 - imageurl > ok 16 - summary > ok 17 - checkinmsg > ok 18 - checkinmsgtype > ok 19 - There are 3 item types > ok 20 - item types should be sorted by translated description > ok 21 - An item type that is not used can be deleted > ok 22 - An item type that is used by an item cannot be deleted > ok 23 - An item that is used by an item and a biblioitem cannot be deleted > ok 24 - An item has been deleted > ok 25 - A biblioitem has been deleted > ok 26 - The item type that was being used by the removed item and biblioitem > can be deleted > ok > All tests successful. Cheers Alex
Created attachment 63307 [details] [review] Bug 17944 - Removed SQl query from itemtypes.pl and replaced it with can_be_deleted method in Koha/ItemType.pm In Koha/ItemTypes.t I have reworded unit test output and added test for item deletion. In addition to updating admin script by removing warn and changing if statement check Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 63308 [details] [review] Bug 17944: QA follow-up - Remove an unused use statement - Fix pod - Use snake_case - Fix test "An itemtype cannot be deleted if and only if there is biblioitem linked with it" Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 63309 [details] [review] Bug 17944 - Add Koha::ItemType->can_be_deleted and use it from admin/itemtypes.pl Removed the sql code from Itemtypes.pm and replaced it with DBIx database query in the itemtypes.pl administrative script Test plan: 1. In the staff interface, stage and manage MARC records for import 2. Try to delete an itemtype. If there are items of that itemtype in the database then a message telling you the number of items of that itemtype there are will be displayed. 3. Record that number 4. View the admin/itemtpes.pl script and confirm that there is sql code written in this file. 5. Apply this patch 6. View the admin/itemtypes.pl script and observe that there is no sql in this file. There is however DBIx code, for example $schema->resultset('Item')->search({ 'itype' => $itemtype_code} ); which is searching for items with the itype value matching $itemtype_code value. 7. In the staff interface try to delete the same itemtype 8. Record the number of items there are with that itemtype in the resulting message 9. The numbers recorded in steps 3 and 8 should match showing that the DBIx code is working as intended Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 63310 [details] [review] Bug 17944: QA follow-up - Remove an unused use statement - Fix pod - Use snake_case - Fix test "An itemtype cannot be deleted if and only if there is biblioitem linked with it" Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
String changes. Return after freeze.
Created attachment 63924 [details] [review] Bug 17944 - Add Koha::ItemType->can_be_deleted and use it from admin/itemtypes.pl Removed the sql code from Itemtypes.pm and replaced it with DBIx database query in the itemtypes.pl administrative script Test plan: 1. In the staff interface, stage and manage MARC records for import 2. Try to delete an itemtype. If there are items of that itemtype in the database then a message telling you the number of items of that itemtype there are will be displayed. 3. Record that number 4. View the admin/itemtpes.pl script and confirm that there is sql code written in this file. 5. Apply this patch 6. View the admin/itemtypes.pl script and observe that there is no sql in this file. There is however DBIx code, for example $schema->resultset('Item')->search({ 'itype' => $itemtype_code} ); which is searching for items with the itype value matching $itemtype_code value. 7. In the staff interface try to delete the same itemtype 8. Record the number of items there are with that itemtype in the resulting message 9. The numbers recorded in steps 3 and 8 should match showing that the DBIx code is working as intended Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 63925 [details] [review] Bug 17944: QA follow-up - Remove an unused use statement - Fix pod - Use snake_case - Fix test "An itemtype cannot be deleted if and only if there is biblioitem linked with it" Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Pushed to master for 17.11, thanks to everybody involved!
Pushed to 17.05.x, will be in 17.05.01
Onboarding tool is a new feature of 17.05 - so this is not needed in 16.11.x