If we call Koha::Libraries->delete but Koha::Library->delete exists (ie. Koha::Object->delete is overriden), then we Koha::Objects->delete will be called and the overriden will not be executed.
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
not backported to 19.11