Bug 36873 - Koha::Objects->delete should accept parameters and pass them through
Summary: Koha::Objects->delete should accept parameters and pass them through
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-05-15 19:01 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2024-12-02 10:42 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00,24.05.06,23.11.11
Circulation function:


Attachments
Bug 36873: Unit tests (2.22 KB, patch)
2024-05-15 19:19 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 36873: Make Koha::Objects->delete pass parameters to the underlying method (1.69 KB, patch)
2024-05-15 19:19 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 36873: Unit tests (2.27 KB, patch)
2024-05-16 00:31 UTC, David Nind
Details | Diff | Splinter Review
Bug 36873: Make Koha::Objects->delete pass parameters to the underlying method (1.74 KB, patch)
2024-05-16 00:31 UTC, David Nind
Details | Diff | Splinter Review
Bug 36873: Unit tests (2.33 KB, patch)
2024-06-28 14:32 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 36873: Make Koha::Objects->delete pass parameters to the underlying method (1.80 KB, patch)
2024-06-28 14:32 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi (tcohen) 2024-05-15 19:01:05 UTC
Taking `Koha::Item->delete` as an example, it accepts many parameters.
Calling `Koha::Items->delete` will find out there's a `delete()` method defined in `Koha::Item` and loop through all items, calling that particular method.

It would be handy if we could pass on some parameters. As an example, the following sequence:

```perl
$biblio->items->delete;
DelBiblio( $biblio->id);
```

would trigger reindexing the record for each item, and then when actually deleting the biblio. With the current codebase, to avoid it, we could instead do:

```perl
foreach my $item ($biblio->items->as_list) {
    $item->delete( { skip_record_index } );
}
DelBiblio( $biblio->id);
```

But it would be great if we could just write:

```perl
$biblio->items->delete( { skip_record_index => 1 } );
DelBiblio( $biblio->id );
```
Comment 1 Tomás Cohen Arazi (tcohen) 2024-05-15 19:19:18 UTC
Created attachment 166790 [details] [review]
Bug 36873: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi (tcohen) 2024-05-15 19:19:21 UTC
Created attachment 166791 [details] [review]
Bug 36873: Make Koha::Objects->delete pass parameters to the underlying method

This patch makes the generic method pass any parameters it got passed to
the underlying `delete()` method.

To test:
1. Apply the unit tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Objects.t
=> FAIL: Tests fail, parameters are not passed
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 David Nind 2024-05-16 00:31:29 UTC
Created attachment 166799 [details] [review]
Bug 36873: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Comment 4 David Nind 2024-05-16 00:31:32 UTC
Created attachment 166800 [details] [review]
Bug 36873: Make Koha::Objects->delete pass parameters to the underlying method

This patch makes the generic method pass any parameters it got passed to
the underlying `delete()` method.

To test:
1. Apply the unit tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Objects.t
=> FAIL: Tests fail, parameters are not passed
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Comment 5 Martin Renvoize (ashimema) 2024-06-28 14:32:26 UTC
Created attachment 168246 [details] [review]
Bug 36873: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize (ashimema) 2024-06-28 14:32:28 UTC
Created attachment 168247 [details] [review]
Bug 36873: Make Koha::Objects->delete pass parameters to the underlying method

This patch makes the generic method pass any parameters it got passed to
the underlying `delete()` method.

To test:
1. Apply the unit tests patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Objects.t
=> FAIL: Tests fail, parameters are not passed
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize (ashimema) 2024-06-28 14:33:45 UTC
Trivial bugfix, all working.

Passing QA
Comment 8 Katrin Fischer 2024-07-01 16:58:30 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 9 Katrin Fischer 2024-07-02 07:21:47 UTC
I thought this might be a good candidate for the failing test:

https://jenkins.koha-community.org/job/Koha_Main/2954/testReport/junit/(root)/t_db_dependent_Koha_Plugins_KitchenSink_t/Fun_with_KitchenSink__Handler__delete__2283_/

not ok 1 - Fun with KitchenSink, Handler->delete

... but again it's passing for me locally.
Comment 10 Tomás Cohen Arazi (tcohen) 2024-07-02 13:00:40 UTC
(In reply to Katrin Fischer from comment #9)
> I thought this might be a good candidate for the failing test:
> 
> https://jenkins.koha-community.org/job/Koha_Main/2954/testReport/junit/
> (root)/t_db_dependent_Koha_Plugins_KitchenSink_t/
> Fun_with_KitchenSink__Handler__delete__2283_/
> 
> not ok 1 - Fun with KitchenSink, Handler->delete
> 
> ... but again it's passing for me locally.

There are other factors there. Looking shortly.
Comment 11 Katrin Fischer 2024-07-02 13:01:44 UTC
Thanks!
Comment 12 Tomás Cohen Arazi (tcohen) 2024-07-03 11:51:17 UTC
(In reply to Katrin Fischer from comment #9)
> I thought this might be a good candidate for the failing test:
> 
> https://jenkins.koha-community.org/job/Koha_Main/2954/testReport/junit/
> (root)/t_db_dependent_Koha_Plugins_KitchenSink_t/
> Fun_with_KitchenSink__Handler__delete__2283_/
> 
> not ok 1 - Fun with KitchenSink, Handler->delete
> 
> ... but again it's passing for me locally.

I haven't found any relationship with this bug, unfortunately. What I was looking for were cases of ->delete being called with some parameter, and there aren't for plugin code.
Comment 13 Katrin Fischer 2024-07-03 11:55:21 UTC
(In reply to Tomás Cohen Arazi from comment #12)
> (In reply to Katrin Fischer from comment #9)
> > I thought this might be a good candidate for the failing test:
> > 
> > https://jenkins.koha-community.org/job/Koha_Main/2954/testReport/junit/
> > (root)/t_db_dependent_Koha_Plugins_KitchenSink_t/
> > Fun_with_KitchenSink__Handler__delete__2283_/
> > 
> > not ok 1 - Fun with KitchenSink, Handler->delete
> > 
> > ... but again it's passing for me locally.
> 
> I haven't found any relationship with this bug, unfortunately. What I was
> looking for were cases of ->delete being called with some parameter, and
> there aren't for plugin code.

It looks like it might have been a random one too. Let's see if ti appears again.
Comment 14 Jonathan Druart 2024-07-11 08:39:59 UTC
(In reply to Tomás Cohen Arazi from comment #12)
> (In reply to Katrin Fischer from comment #9)
> > I thought this might be a good candidate for the failing test:
> > 
> > https://jenkins.koha-community.org/job/Koha_Main/2954/testReport/junit/
> > (root)/t_db_dependent_Koha_Plugins_KitchenSink_t/
> > Fun_with_KitchenSink__Handler__delete__2283_/
> > 
> > not ok 1 - Fun with KitchenSink, Handler->delete
> > 
> > ... but again it's passing for me locally.
> 
> I haven't found any relationship with this bug, unfortunately. What I was
> looking for were cases of ->delete being called with some parameter, and
> there aren't for plugin code.

The error was:
koha_1       | Could not unlink /tmp/LSL387s/Koha/Plugin/Com/ByWaterSolutions/KitchenSink.pm.pm: No such file or directory at /kohadevbox/koha/Koha/Plugins/Handler.pm line 110.
koha_1       | 
koha_1       |     #   Failed test 'Module file no longer found'
koha_1       |     #   at t/db_dependent/Koha/Plugins/KitchenSink.t line 68.
koha_1       |     # Looks like you failed 1 test of 7.
koha_1       | 
koha_1       | #   Failed test 'Fun with KitchenSink, Handler->delete'
koha_1       | #   at t/db_dependent/Koha/Plugins/KitchenSink.t line 74.
koha_1       | # Looks like you failed 1 test of 1.
koha_1       | [17:50:24] t/db_dependent/Koha/Plugins/KitchenSink.t ................................... 
koha_1       | Dubious, test returned 1 (wstat 256, 0x100)


Which is bug 21670.
Comment 15 Lucas Gass (lukeg) 2024-11-18 17:18:37 UTC
Backported to 24.05.x for upcoming 24.05.06
Comment 16 Fridolin Somers 2024-12-02 10:42:42 UTC
Pushed to 23.11.x for 23.11.11