[Bug 17944] New: Remove the sql code from onboarding and administrative perl scripts, and remove unnecessary use pragmas from onboarding.pl
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Bug ID: 17944 Summary: Remove the sql code from onboarding and administrative perl scripts, and remove unnecessary use pragmas from onboarding.pl Change sponsored?: --- Product: Koha Version: 17.05 Hardware: All OS: All Status: NEW Severity: minor Priority: P5 - low Component: Installation and upgrade (web-based installer) Assignee: koha-bugs@lists.koha-community.org Reporter: alexbuckley@catalyst.net.nz QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com Depends on: 17855 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. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17855 [Bug 17855] Updated Koha web installer and new onboarding tool feature to guide users through setting up Koha -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |alexbuckley@catalyst.net.nz |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtompset@hotmail.com --- Comment #1 from M. Tompsett <mtompset@hotmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Remove the sql code from |Remove the sql code from |onboarding and |itemtypes.pl administrative |administrative perl |perl script |scripts, and remove | |unnecessary use pragmas | |from onboarding.pl | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #2 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 59391 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59391&action=edit Bug 17944 - Moved the sql code from itemtypes.pl into Koha/ItemTypes.pm -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff --- Comment #3 from Alex Buckley <alexbuckley@catalyst.net.nz> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=17954 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=17955 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #4 from M. Tompsett <mtompset@hotmail.com> --- Comment on attachment 59391 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59391 Bug 17944 - Moved the sql code from itemtypes.pl into Koha/ItemTypes.pm Review of attachment 59391: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=17944&attachment=59391) ----------------------------------------------------------------- ::: 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #5 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 59564 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59564&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #6 from Alex Buckley <alexbuckley@catalyst.net.nz> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #7 from M. Tompsett <mtompset@hotmail.com> --- Comment on attachment 59564 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59564 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: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=17944&attachment=59564) ----------------------------------------------------------------- ::: 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |In Discussion -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #8 from Alex Buckley <alexbuckley@catalyst.net.nz> --- I'll try implementing that now. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #9 from Alex Buckley <alexbuckley@catalyst.net.nz> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #10 from M. Tompsett <mtompset@hotmail.com> --- (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. :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #11 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 59565 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59565&action=edit Bug 17944 - Tidied the DBIx code in itemtypes.pl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59391|0 |1 is obsolete| | --- Comment #12 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 59655 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59655&action=edit [SIGNED-OFF] Bug 17944 - Moved the sql code from itemtypes.pl into Koha/ItemTypes.pm Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59564|0 |1 is obsolete| | --- Comment #13 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 59656 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59656&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59565|0 |1 is obsolete| | --- Comment #14 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 59657 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59657&action=edit [SIGNED-OFF] Bug 17944 - Tidied the DBIx code in itemtypes.pl Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #15 from Jonathan Druart <jonathan.druart@bugs.koha-community.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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #16 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 59715 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59715&action=edit Bug 17944 - Moved the DBIx code into the ItemType.pm file -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #17 from M. Tompsett <mtompset@hotmail.com> --- Comment on attachment 59715 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59715 Bug 17944 - Moved the DBIx code into the ItemType.pm file Review of attachment 59715: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=17944&attachment=59715) ----------------------------------------------------------------- ::: 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #18 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 59716 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59716&action=edit Bug 17955 - removed incorrect function call -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #19 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 59717 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59717&action=edit Bug 17944 - Added additional USE pragma -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59715|0 |1 is obsolete| | Attachment #59716|0 |1 is obsolete| | Attachment #59717|0 |1 is obsolete| | --- Comment #20 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 59718 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59718&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |In Discussion --- Comment #22 from Alex Buckley <alexbuckley@catalyst.net.nz> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #23 from Alex Buckley <alexbuckley@catalyst.net.nz> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #24 from Alex Buckley <alexbuckley@catalyst.net.nz> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #25 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|17.05 |unspecified -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #26 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 59953 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59953&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #27 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 59954 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59954&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Baptiste <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |baptiste.wojtkowski@biblibr | |e.com --- Comment #28 from Baptiste <baptiste.wojtkowski@biblibre.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #29 from Alex Buckley <alexbuckley@catalyst.net.nz> --- (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 (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |veron@veron.ch --- Comment #30 from Marc Véron <veron@veron.ch> --- 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) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59954|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59953|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59718|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59657|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59656|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59655|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #31 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 62499 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62499&action=edit Bug 17944 - Moved the sql code from itemtypes.pl into Koha/ItemTypes.pm Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #32 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 62500 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62500&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #33 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 62501 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62501&action=edit Bug 17944 - Tidied the DBIx code in itemtypes.pl Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #62501|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #34 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 62502 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62502&action=edit Bug 17944 - Tidied the DBIx code in itemtypes.pl Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #35 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 62503 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62503&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #36 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 62504 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62504&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #37 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 62505 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=62505&action=edit Bug 17944 -Integrated the removal of SQL from itemtypes.pl admin script with the onboarding tool following tester feedback of merge conflict -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff --- Comment #38 from Alex Buckley <alexbuckley@catalyst.net.nz> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |BLOCKED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #39 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #40 from Alex Buckley <alexbuckley@catalyst.net.nz> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #62499|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #62500|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #62502|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #62503|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #62504|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #62505|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #41 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 63045 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63045&action=edit Bug 17944 - Overridden Koha::Object->delete with Koha::ItemType->delete and implemented exception to be shown if the itemtype cannot be deleted. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #42 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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 :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #63045|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #43 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 63152 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63152&action=edit Bug 17944 - Removed SQl query from itemtypes.pl and replaced it with can_be_deleted method in Koha/ItemType.pm -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #44 from Alex Buckley <alexbuckley@catalyst.net.nz> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #45 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |In Discussion --- Comment #46 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I am switching the status to 'in discussion' until it's ready to be tested. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #63152|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #47 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 63220 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63220&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff --- Comment #48 from Alex Buckley <alexbuckley@catalyst.net.nz> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #49 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Comment on attachment 63220 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63220 Bug 17944 - Removed SQl query from itemtypes.pl and replaced it with can_be_deleted method in Koha/ItemType.pm Review of attachment 63220: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=17944&attachment=63220) ----------------------------------------------------------------- ::: 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. """ -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #63220|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #50 from Alex Buckley <alexbuckley@catalyst.net.nz> --- Created attachment 63259 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63259&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #51 from Alex Buckley <alexbuckley@catalyst.net.nz> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #63259|0 |1 is obsolete| | --- Comment #52 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 63307 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63307&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #53 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 63308 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63308&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #63307|0 |1 is obsolete| | Attachment #63308|0 |1 is obsolete| | --- Comment #54 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 63309 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63309&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 --- Comment #55 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 63310 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63310&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Bug 17944 depends on bug 17855, which changed state. Bug 17855 Summary: New onboarding tool feature to guide users through setting up Koha, and minor web installer UI improvements https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17855 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #56 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- String changes. Return after freeze. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #63309|0 |1 is obsolete| | --- Comment #57 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 63924 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63924&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #63310|0 |1 is obsolete| | --- Comment #58 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 63925 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63925&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #59 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 17.11, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable CC| |fridolin.somers@biblibre.co | |m --- Comment #60 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- Pushed to 17.05.x, will be in 17.05.01 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17944 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Stable |RESOLVED Resolution|--- |FIXED CC| |katrin.fischer@bsz-bw.de --- Comment #61 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Onboarding tool is a new feature of 17.05 - so this is not needed in 16.11.x -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org