Bug 29788

Summary: Make Koha::Item->safe_to_delete return a Koha::Result::Boolean object
Product: Koha Reporter: Jonathan Druart <jonathan.druart+koha>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart+koha>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: fridolin.somers, kyle, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29765
Change sponsored?: --- Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
22.05.00
Bug Depends on: 29790, 29746, 29789, 29794    
Bug Blocks: 30644    
Attachments: Bug 29788: Make Koha::Item->safe_to_delete use Koha::Result::Boolean
Bug 29788: Make Koha::Item->safe_to_delete use Koha::Result::Boolean
Bug 29788: Make Koha::Item->safe_to_delete use Koha::Result::Boolean
Bug 29788: Fix batch delete item
Bug 29788: Fix batch delete item
Bug 29788: Make Koha::Item->safe_to_delete use Koha::Result::Boolean
Bug 29788: Fix batch delete item
Bug 29788: Make Koha::Item->safe_to_delete use Koha::Result::Boolean
Bug 29788: Fix batch delete item
Bug 29788: Make Koha::Item->safe_to_delete use Koha::Result::Boolean
Bug 29788: Fix batch delete item
Bug 29788: Fix t/db_dependent/Reserves.t
Bug 29788: (follow-up) Make Koha::Item->safe_to_delete use Koha::Result::Boolean

Description Jonathan Druart 2022-01-04 14:37:51 UTC
Same as bug 29765 for the existing Koha::Item->safe_to_delete
Comment 1 Jonathan Druart 2022-01-04 15:16:47 UTC
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.
Comment 2 Tomás Cohen Arazi 2022-01-04 19:41:40 UTC
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>
Comment 3 Tomás Cohen Arazi 2022-01-04 20:05:59 UTC
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.
Comment 4 Tomás Cohen Arazi 2022-01-04 20:07:36 UTC
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>
Comment 5 Tomás Cohen Arazi 2022-01-04 20:08:42 UTC
(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).
Comment 6 Jonathan Druart 2022-01-05 09:09:31 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2022-01-05 09:10:01 UTC
(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.
Comment 8 Jonathan Druart 2022-01-05 09:35:03 UTC
Created attachment 129018 [details] [review]
Bug 29788: Fix batch delete item
Comment 9 Jonathan Druart 2022-01-05 09:35:28 UTC
Lot of errors in my first patch, sorry about that!
Comment 10 Jonathan Druart 2022-01-06 13:30:23 UTC
Created attachment 129075 [details] [review]
Bug 29788: Fix batch delete item
Comment 11 Jonathan Druart 2022-01-06 13:32:18 UTC
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.
Comment 12 Jonathan Druart 2022-01-06 13:32:22 UTC
Created attachment 129077 [details] [review]
Bug 29788: Fix batch delete item
Comment 13 Tomás Cohen Arazi 2022-01-06 14:46:55 UTC
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>
Comment 14 Tomás Cohen Arazi 2022-01-06 14:47:00 UTC
Created attachment 129098 [details] [review]
Bug 29788: Fix batch delete item

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2022-01-06 15:02:14 UTC
Tested the different places, including Order.t tests.
Things I found were fixed by Jonathan.
QA script is happy, signing off :-D
Comment 16 Kyle M Hall 2022-01-07 14:12:36 UTC
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>
Comment 17 Kyle M Hall 2022-01-07 14:12:45 UTC
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>
Comment 18 Fridolin Somers 2022-01-11 22:51:48 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 19 Fridolin Somers 2022-01-12 06:32:28 UTC
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
Comment 20 Fridolin Somers 2022-01-12 06:33:05 UTC
Created attachment 129326 [details] [review]
Bug 29788: Fix t/db_dependent/Reserves.t

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 21 Jonathan Druart 2022-03-23 11:31:33 UTC
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!
Comment 22 Fridolin Somers 2022-03-23 20:11:29 UTC
> Bug 29788: (follow-up) Make Koha::Item->safe_to_delete use Koha::Result::Boolean
Pushed to master