Bug 29788 - Make Koha::Item->safe_to_delete return a Koha::Result::Boolean object
Summary: Make Koha::Item->safe_to_delete return a Koha::Result::Boolean object
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 29746 29789 29790 29794
Blocks: 30644 32656
  Show dependency treegraph
 
Reported: 2022-01-04 14:37 UTC by Jonathan Druart
Modified: 2023-01-17 17:17 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00


Attachments
Bug 29788: Make Koha::Item->safe_to_delete use Koha::Result::Boolean (16.92 KB, patch)
2022-01-04 15:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29788: Make Koha::Item->safe_to_delete use Koha::Result::Boolean (16.59 KB, patch)
2022-01-04 19:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29788: Make Koha::Item->safe_to_delete use Koha::Result::Boolean (16.60 KB, patch)
2022-01-04 20:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29788: Fix batch delete item (5.24 KB, patch)
2022-01-05 09:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29788: Fix batch delete item (5.12 KB, patch)
2022-01-06 13:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29788: Make Koha::Item->safe_to_delete use Koha::Result::Boolean (16.57 KB, patch)
2022-01-06 13:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29788: Fix batch delete item (5.12 KB, patch)
2022-01-06 13:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29788: Make Koha::Item->safe_to_delete use Koha::Result::Boolean (16.60 KB, patch)
2022-01-06 14:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29788: Fix batch delete item (5.16 KB, patch)
2022-01-06 14:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29788: Make Koha::Item->safe_to_delete use Koha::Result::Boolean (16.67 KB, patch)
2022-01-07 14:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29788: Fix batch delete item (5.23 KB, patch)
2022-01-07 14:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29788: Fix t/db_dependent/Reserves.t (922 bytes, patch)
2022-01-12 06:33 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 29788: (follow-up) Make Koha::Item->safe_to_delete use Koha::Result::Boolean (2.48 KB, patch)
2022-03-23 11:31 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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