Same as bug 29765 for the existing Koha::Item->safe_to_delete
Created attachment 128993 [details] [review] Bug 29788: Make Koha::Item->safe_to_delete use Koha::Result::Boolean This patch reuse the (awesome) Koha::Result::Boolean module to retrieve the return of Koha::Item->safe_to_delete. Test plan: Try to delete an item that has previously been checked out and confirm that you are still blocked. Try using the cronjobs, the item and biblio detail pages, as well as the batch delete item tool.
Created attachment 129010 [details] [review] Bug 29788: Make Koha::Item->safe_to_delete use Koha::Result::Boolean This patch reuse the (awesome) Koha::Result::Boolean module to retrieve the return of Koha::Item->safe_to_delete. Test plan: Try to delete an item that has previously been checked out and confirm that you are still blocked. Try using the cronjobs, the item and biblio detail pages, as well as the batch delete item tool. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
I added 29794, which I found while testing this. The results are: - biblio/item views works as expected => SUCCESS - delete_items.pl => SUCCESS/FAIL: The check works, deletion is prevented, but the error message prints the Koha::Object::Message reference. It should be calling ->message in line 86.- batch delete. It also felt like we could do a join on multiple messages too? This last one is not a blocker of course. => FAIL: There's a problem with Koha/UI/Table/Builder/Items.pm line 111 calling 'ne' on the 'safe_to_delete' output (Koha::Result::Boolean). I tried a fast fix (checking the attribute was 'safe_to_delete' and evaluating in boolean context) but it turned out it broke blocking_errors.inc and I got out of fuel.
Created attachment 129013 [details] [review] Bug 29788: Make Koha::Item->safe_to_delete use Koha::Result::Boolean This patch reuse the (awesome) Koha::Result::Boolean module to retrieve the return of Koha::Item->safe_to_delete. Test plan: Try to delete an item that has previously been checked out and confirm that you are still blocked. Try using the cronjobs, the item and biblio detail pages, as well as the batch delete item tool. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Tomás Cohen Arazi from comment #3) > => SUCCESS/FAIL: The check works, deletion is prevented, but the error > message prints the Koha::Object::Message reference. It should be calling > ->message in line 86.- batch delete. It also felt like we could do a join on > multiple messages too? This last one is not a blocker of course. Fixed inline on the rebased patch (on top of the bug 29746 follow-up).
(In reply to Tomás Cohen Arazi from comment #3) > It also felt like we could do a join on > multiple messages too? This last one is not a blocker of course. I don't know. Either we should restrict "messages" to only 1 message (do we use more so far?), or we should have a new method in the module to ease the retrieval of a single message. I don't think we should restrict to 1 message only, even if we are not using more than 1 now, we could later. Maybe we could have something like: sub get_unique_message_str { my ( $self ) = @_; croak "There are more than one message" if @{$self->{_messages}} > 1; return @{$deleted->messages}[0]->messages } But that's for a separate bug report anyway.
(In reply to Tomás Cohen Arazi from comment #3) > It also felt like we could do a join on > multiple messages too? This last one is not a blocker of course. I don't know. Either we should restrict "messages" to only 1 message (do we use more so far?), or we should have a new method in the module to ease the retrieval of a single message. I don't think we should restrict to 1 message only, even if we are not using more than 1 now, we could later. Maybe we could have something like: sub get_unique_message_str { my ( $self ) = @_; croak "There are more than one message" if @{$self->{_messages}} > 1; return @{$self->messages}[0]->messages } But that's for a separate bug report anyway.
Created attachment 129018 [details] [review] Bug 29788: Fix batch delete item
Lot of errors in my first patch, sorry about that!
Created attachment 129075 [details] [review] Bug 29788: Fix batch delete item
Created attachment 129076 [details] [review] Bug 29788: Make Koha::Item->safe_to_delete use Koha::Result::Boolean This patch reuse the (awesome) Koha::Result::Boolean module to retrieve the return of Koha::Item->safe_to_delete. Test plan: Try to delete an item that has previously been checked out and confirm that you are still blocked. Try using the cronjobs, the item and biblio detail pages, as well as the batch delete item tool.
Created attachment 129077 [details] [review] Bug 29788: Fix batch delete item
Created attachment 129097 [details] [review] Bug 29788: Make Koha::Item->safe_to_delete use Koha::Result::Boolean This patch reuse the (awesome) Koha::Result::Boolean module to retrieve the return of Koha::Item->safe_to_delete. Test plan: Try to delete an item that has previously been checked out and confirm that you are still blocked. Try using the cronjobs, the item and biblio detail pages, as well as the batch delete item tool. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 129098 [details] [review] Bug 29788: Fix batch delete item Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Tested the different places, including Order.t tests. Things I found were fixed by Jonathan. QA script is happy, signing off :-D
Created attachment 129169 [details] [review] Bug 29788: Make Koha::Item->safe_to_delete use Koha::Result::Boolean This patch reuse the (awesome) Koha::Result::Boolean module to retrieve the return of Koha::Item->safe_to_delete. Test plan: Try to delete an item that has previously been checked out and confirm that you are still blocked. Try using the cronjobs, the item and biblio detail pages, as well as the batch delete item tool. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 129170 [details] [review] Bug 29788: Fix batch delete item Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Ah Test suite fails on t/db_dependent/Reserves.t : https://jenkins.koha-community.org/view/master/job/Koha_Master_D10/505/testReport/ I see it needs to be fixed like t/db_dependent/Circulation/IsItemIssued.t
Created attachment 129326 [details] [review] Bug 29788: Fix t/db_dependent/Reserves.t Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Created attachment 132052 [details] [review] Bug 29788: (follow-up) Make Koha::Item->safe_to_delete use Koha::Result::Boolean There were several wrong things in the previous patch!
> Bug 29788: (follow-up) Make Koha::Item->safe_to_delete use Koha::Result::Boolean Pushed to master