Bug 17944 - Remove the sql code from itemtypes.pl administrative perl script
Summary: Remove the sql code from itemtypes.pl administrative perl script
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (web-based installer) (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low minor (vote)
Assignee: Alex Buckley
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 17855
Blocks:
  Show dependency treegraph
 
Reported: 2017-01-19 19:26 UTC by Alex Buckley
Modified: 2017-06-11 22:52 UTC (History)
8 users (show)

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


Attachments
Bug 17944 - Moved the sql code from itemtypes.pl into Koha/ItemTypes.pm (1.79 KB, patch)
2017-01-22 02:05 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 17944 - Removed the sql code from Itemtypes.pm and replaced it with DBIx database query in the itemtypes.pl administrative script (3.34 KB, patch)
2017-01-26 01:56 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 17944 - Tidied the DBIx code in itemtypes.pl (1.12 KB, patch)
2017-01-26 02:44 UTC, Alex Buckley
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17944 - Moved the sql code from itemtypes.pl into Koha/ItemTypes.pm (1.85 KB, patch)
2017-01-30 16:07 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17944 - Removed the sql code from Itemtypes.pm (3.47 KB, patch)
2017-01-30 16:08 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17944 - Tidied the DBIx code in itemtypes.pl (1.19 KB, patch)
2017-01-30 16:08 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 17944 - Moved the DBIx code into the ItemType.pm file (2.31 KB, patch)
2017-02-01 04:05 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 17955 - removed incorrect function call (877 bytes, patch)
2017-02-01 04:29 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 17944 - Added additional USE pragma (591 bytes, patch)
2017-02-01 04:47 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 17944 - Moved the DBIx code into the ItemType.pm file (2.15 KB, patch)
2017-02-01 05:02 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 17944 - Updates following second tester feedback (4.45 KB, patch)
2017-02-06 21:37 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 17944 - Implemented a subtest in the ItemTypes.t file for is_used function in ItemType.pm (2.09 KB, patch)
2017-02-06 21:37 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 17944 - Moved the sql code from itemtypes.pl into Koha/ItemTypes.pm (1.82 KB, patch)
2017-04-20 21:23 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 17944 - Removed the sql code from Itemtypes.pm (3.44 KB, patch)
2017-04-20 21:24 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 17944 - Tidied the DBIx code in itemtypes.pl (1.17 KB, patch)
2017-04-20 21:24 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 17944 - Tidied the DBIx code in itemtypes.pl (1.17 KB, patch)
2017-04-20 21:25 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 17944 - Moved the DBIx code into the ItemType.pm file (2.15 KB, patch)
2017-04-20 21:26 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 17944 - Updates following second tester feedback (4.34 KB, patch)
2017-04-20 21:26 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 17944 -Integrated the removal of SQL from itemtypes.pl admin script with the onboarding tool following tester feedback of merge conflict (1.80 KB, patch)
2017-04-20 21:26 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 17944 - Overridden Koha::Object->delete with Koha::ItemType->delete and implemented exception to be shown if the itemtype cannot be deleted. (5.82 KB, patch)
2017-05-03 21:19 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 17944 - Removed SQl query from itemtypes.pl and replaced it with can_be_deleted method in Koha/ItemType.pm (5.02 KB, patch)
2017-05-05 01:20 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 17944 - Removed SQl query from itemtypes.pl and replaced it with can_be_deleted method in Koha/ItemType.pm (5.25 KB, patch)
2017-05-07 10:07 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 17944 - Removed SQl query from itemtypes.pl and replaced it with can_be_deleted method in Koha/ItemType.pm (5.38 KB, patch)
2017-05-08 21:44 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 17944 - Removed SQl query from itemtypes.pl and replaced it with can_be_deleted method in Koha/ItemType.pm (5.45 KB, patch)
2017-05-09 18:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17944: QA follow-up (3.93 KB, patch)
2017-05-09 18:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17944 - Add Koha::ItemType->can_be_deleted and use it from admin/itemtypes.pl (6.32 KB, patch)
2017-05-09 19:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17944: QA follow-up (3.95 KB, patch)
2017-05-09 19:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17944 - Add Koha::ItemType->can_be_deleted and use it from admin/itemtypes.pl (6.39 KB, patch)
2017-06-02 08:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17944: QA follow-up (4.02 KB, patch)
2017-06-02 08:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Buckley 2017-01-19 19:26:36 UTC
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.
Comment 1 M. Tompsett 2017-01-20 01:55:01 UTC
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.
Comment 2 Alex Buckley 2017-01-22 02:05:39 UTC Comment hidden (obsolete)
Comment 3 Alex Buckley 2017-01-22 02:14:58 UTC
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 4 M. Tompsett 2017-01-22 17:01:33 UTC Comment hidden (obsolete)
Comment 5 Alex Buckley 2017-01-26 01:56:39 UTC Comment hidden (obsolete)
Comment 6 Alex Buckley 2017-01-26 02:05:09 UTC
(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 7 M. Tompsett 2017-01-26 02:30:19 UTC Comment hidden (obsolete)
Comment 8 Alex Buckley 2017-01-26 02:32:45 UTC
I'll try implementing that now.
Comment 9 Alex Buckley 2017-01-26 02:37:52 UTC
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
Comment 10 M. Tompsett 2017-01-26 02:42:33 UTC
(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. :)
Comment 11 Alex Buckley 2017-01-26 02:44:21 UTC Comment hidden (obsolete)
Comment 12 Owen Leonard 2017-01-30 16:07:55 UTC
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>
Comment 13 Owen Leonard 2017-01-30 16:08:06 UTC
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>
Comment 14 Owen Leonard 2017-01-30 16:08:16 UTC
Created attachment 59657 [details] [review]
[SIGNED-OFF] Bug 17944 - Tidied the DBIx code in itemtypes.pl

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 15 Jonathan Druart 2017-01-31 10:21:14 UTC
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.
Comment 16 Alex Buckley 2017-02-01 04:05:54 UTC
Created attachment 59715 [details] [review]
Bug 17944 - Moved the DBIx code into the ItemType.pm file
Comment 17 M. Tompsett 2017-02-01 04:20:35 UTC
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.
Comment 18 Alex Buckley 2017-02-01 04:29:21 UTC
Created attachment 59716 [details] [review]
Bug 17955 - removed incorrect function call
Comment 19 Alex Buckley 2017-02-01 04:47:47 UTC
Created attachment 59717 [details] [review]
Bug 17944 - Added additional USE pragma
Comment 20 M. Tompsett 2017-02-01 05:02:42 UTC
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>
Comment 21 Jonathan Druart 2017-02-01 07:53:30 UTC
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?
Comment 22 Alex Buckley 2017-02-01 20:08:44 UTC
(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.
Comment 23 Alex Buckley 2017-02-01 20:19:50 UTC
(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
Comment 24 Alex Buckley 2017-02-01 21:07:22 UTC
(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?
Comment 25 Jonathan Druart 2017-02-02 07:54:56 UTC
(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.
Comment 26 Alex Buckley 2017-02-06 21:37:24 UTC
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
Comment 27 Alex Buckley 2017-02-06 21:37:49 UTC
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
Comment 28 Baptiste 2017-02-17 10:23:21 UTC
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.
Comment 29 Alex Buckley 2017-02-17 21:35:43 UTC
(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
Comment 30 Marc Véron 2017-04-20 08:41:06 UTC
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)
Comment 31 Alex Buckley 2017-04-20 21:23:50 UTC
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>
Comment 32 Alex Buckley 2017-04-20 21:24:19 UTC
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>
Comment 33 Alex Buckley 2017-04-20 21:24:47 UTC
Created attachment 62501 [details] [review]
Bug 17944 - Tidied the DBIx code in itemtypes.pl

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 34 Alex Buckley 2017-04-20 21:25:32 UTC
Created attachment 62502 [details] [review]
Bug 17944 - Tidied the DBIx code in itemtypes.pl

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 35 Alex Buckley 2017-04-20 21:26:00 UTC
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>
Comment 36 Alex Buckley 2017-04-20 21:26:23 UTC
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
Comment 37 Alex Buckley 2017-04-20 21:26:40 UTC
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
Comment 38 Alex Buckley 2017-04-20 21:29:57 UTC
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
Comment 39 Jonathan Druart 2017-05-02 21:58:27 UTC
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.
Comment 40 Alex Buckley 2017-05-03 13:06:43 UTC
(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.
Comment 41 Alex Buckley 2017-05-03 21:19:47 UTC
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.
Comment 42 Jonathan Druart 2017-05-04 15:29:10 UTC
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 :)
Comment 43 Alex Buckley 2017-05-05 01:20:01 UTC
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
Comment 44 Alex Buckley 2017-05-05 01:55:16 UTC
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?
Comment 45 Jonathan Druart 2017-05-05 13:51:15 UTC
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.
Comment 46 Jonathan Druart 2017-05-05 13:51:42 UTC
I am switching the status to 'in discussion' until it's ready to be tested.
Comment 47 Alex Buckley 2017-05-07 10:07:41 UTC
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
Comment 48 Alex Buckley 2017-05-07 10:12:11 UTC
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 49 Jonathan Druart 2017-05-08 14:46:23 UTC
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.
"""
Comment 50 Alex Buckley 2017-05-08 21:44:06 UTC
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
Comment 51 Alex Buckley 2017-05-08 21:59:36 UTC
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
Comment 52 Jonathan Druart 2017-05-09 18:58:58 UTC
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>
Comment 53 Jonathan Druart 2017-05-09 18:59:05 UTC
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>
Comment 54 Jonathan Druart 2017-05-09 19:00:51 UTC
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>
Comment 55 Jonathan Druart 2017-05-09 19:00:56 UTC
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>
Comment 56 Marcel de Rooy 2017-05-19 08:58:54 UTC
String changes. Return after freeze.
Comment 57 Marcel de Rooy 2017-06-02 08:58:52 UTC
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>
Comment 58 Marcel de Rooy 2017-06-02 08:58:58 UTC
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>
Comment 59 Jonathan Druart 2017-06-05 15:00:47 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 60 Fridolin SOMERS 2017-06-09 14:30:41 UTC
Pushed to 17.05.x, will be in 17.05.01
Comment 61 Katrin Fischer 2017-06-11 22:52:53 UTC
Onboarding tool is a new feature of 17.05 - so this is not needed in 16.11.x