|
Description
Jonathan Druart
2020-04-28 12:53:17 UTC
Created attachment 103855 [details] [review] Bug 25303: Make Koha::Objects->delete loop on the object set If we call Koha::Libraries->delete but Koha::Library->delete exists (ie. Koha::Object->delete is overridden), then we Koha::Objects->delete will be called and the overridden will not be executed. This patch suggests to test if the method is overridden (using Class::Inspector->function_exists). If so we loop on the different objects of the set in a transaction and call the overridden ->delete method. Existing tests widely cover this change. t/db_dependent/Koha/Objects.t subtest 'Return same values as DBIx::Class' => sub { This is more similar to bug 23185 than bug 21761. For performance reasons I'm not so sure about always forcing a loop rather than allowing a fast path.. I think I would rather see checks for the corresponding 'delete' method in the Koha::Things class whenever we detect that a Koha::Thing has had a delete method added or updated. That way, we only require the loop to take place for Koha::Objects where an in-code delete trigger has indeed been added. It all comes down to the DB triggers discussion... I agree we could enforce people overriding Koha::Thing->delete to take care of implementing and overriden Koha::Things->delete that is conformant to the expected behaviour. Created attachment 103859 [details] [review] Bug 25303: Make Koha::Objects->delete loop on the object set If we call Koha::Libraries->delete but Koha::Library->delete exists (ie. Koha::Object->delete is overridden), then we Koha::Objects->delete will be called and the overridden will not be executed. This patch suggests to test if the method is overridden (using Class::Inspector->function_exists). If so we loop on the different objects of the set in a transaction and call the overridden ->delete method. Existing tests widely cover this change. t/db_dependent/Koha/Objects.t subtest 'Return same values as DBIx::Class' => sub { Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Having discussed this I see Tomas and I both missed the Class::Inspector use.. happy to signoff now I've confirmed this works as I wanted. Created attachment 103864 [details] [review] Bug 25303: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 103865 [details] [review] Bug 25303: Make Koha::Objects->delete loop on the object set If we call Koha::Libraries->delete but Koha::Library->delete exists (ie. Koha::Object->delete is overridden), then we Koha::Objects->delete will be called and the overridden will not be executed. This patch suggests to test if the method is overridden (using Class::Inspector->function_exists). If so we loop on the different objects of the set in a transaction and call the overridden ->delete method. Existing tests widely cover this change. t/db_dependent/Koha/Objects.t subtest 'Return same values as DBIx::Class' => sub { Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Don't push this, the tests never finish (infinite loop). Created attachment 103941 [details] [review] Bug 25303: Effectively delete the city to avoid endless loop (In reply to Jonathan Druart from comment #8) > Don't push this, the tests never finish (infinite loop). Good catch, I wonder why it passed for me. Nice work everyone! Pushed to master for 20.05 Created attachment 103998 [details] [review] Bug 25303: (QA follow-up) Fix test construction The test called ->delete on the resultset directly, which is not the same when we already called ->next and the Koha::Hold->delete won't be called as there's no ->next hold. I took the oportunity to wrap the subtest in its own transaction and build its own data to avoid interference between tests... Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> @RM, the follow-up fixes the failing tests in master. It might be relevant to document the behaviour change, whlst I think it was just a bad construction of the test in this case. This follow-up is doing this:
- $holds->delete;
+ $biblio->holds->delete;
I was not expected such behaviour changes, it's not as easy as fixing the tests. Did you check the occurrences in the code?
Maybe we should call ->reset before the loop instead?
@ Koha/Objects.pm:185 @ sub delete {
if ( Class::Inspector->function_exists( $self->object_class, 'delete' ) ) {
my $objects_deleted;
$self->_resultset->result_source->schema->txn_do( sub {
+ $self->reset;
while ( my $o = $self->next ) {
$o->delete;
$objects_deleted++;
(In reply to Jonathan Druart from comment #14) > This follow-up is doing this: > > - $holds->delete; > + $biblio->holds->delete; > > I was not expected such behaviour changes, it's not as easy as fixing the > tests. Did you check the occurrences in the code? I haven't. But that's why I highlighted the behavior change. I'm sure there wouldn't be placed in which we do the same we did on those tests. > Maybe we should call ->reset before the loop instead? I think we should. We should be consistent with DBIC behaviour and the code change you propose looks correct. The good thing is that you have a good schema for the regression tests that are needed. > > @ Koha/Objects.pm:185 @ sub delete { > if ( Class::Inspector->function_exists( $self->object_class, 'delete' ) > ) { > my $objects_deleted; > $self->_resultset->result_source->schema->txn_do( sub { > + $self->reset; > while ( my $o = $self->next ) { > $o->delete; > $objects_deleted++; Created attachment 104018 [details] [review] Bug 25303: Call ->reset before iterate on the set In case we already started to iterate. This fixes a failing tests in t/db_dependent/Koha/Biblios.t (In reply to Jonathan Druart from comment #16) > Created attachment 104018 [details] [review] [review] > Bug 25303: Call ->reset before iterate on the set > > In case we already started to iterate. > > This fixes a failing tests in t/db_dependent/Koha/Biblios.t +10 (In reply to Tomás Cohen Arazi from comment #17) > (In reply to Jonathan Druart from comment #16) > > Created attachment 104018 [details] [review] [review] [review] > > Bug 25303: Call ->reset before iterate on the set > > > > In case we already started to iterate. > > > > This fixes a failing tests in t/db_dependent/Koha/Biblios.t > > +10 Please add tests on Objects.t Created attachment 104027 [details] [review] Bug 25303: Add a test for ->reset Nice work everyone! Pushed to master for 20.05 not backported to 19.11 |