[Bug 25303] New: Koha::Objects->delete should not skip overriden object class ->delete
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Bug ID: 25303 Summary: Koha::Objects->delete should not skip overriden object class ->delete Change sponsored?: --- Product: Koha Version: unspecified Hardware: All OS: All Status: ASSIGNED Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: jonathan.druart@bugs.koha-community.org Reporter: jonathan.druart@bugs.koha-community.org QA Contact: testopia@bugs.koha-community.org 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Koha::Objects->delete |Koha::Objects->delete |should not skip overriden |should not skip overridden |object class ->delete |object class ->delete -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 103855 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103855&action=edit 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 { -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=21761 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com, tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=23185 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 --- Comment #2 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 --- Comment #3 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #103855|0 |1 is obsolete| | --- Comment #4 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 103859 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103859&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #5 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |23185 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 [Bug 23185] Koha::Objects supports passing through 'update' which means we can side step 'set' + 'store' -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|unspecified |master QA Contact|testopia@bugs.koha-communit |tomascohen@gmail.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #103859|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 --- Comment #6 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 103864 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103864&action=edit Bug 25303: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 --- Comment #7 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 103865 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103865&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |ASSIGNED --- Comment #8 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Don't push this, the tests never finish (infinite loop). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 --- Comment #9 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 103941 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103941&action=edit Bug 25303: Effectively delete the city to avoid endless loop -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 --- Comment #10 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |20.05.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 --- Comment #11 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work everyone! Pushed to master for 20.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 103998 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=103998&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #13 from Tomás Cohen Arazi <tomascohen@gmail.com> --- @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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 --- Comment #14 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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++; -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 --- Comment #15 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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++;
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #103998|0 |1 is obsolete| | --- Comment #16 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 104018 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104018&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 --- Comment #17 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #16)
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
+10 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 --- Comment #18 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Tomás Cohen Arazi from comment #17)
(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
Please add tests on Objects.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 104027 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104027&action=edit Bug 25303: Add a test for ->reset -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 --- Comment #20 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work everyone! Pushed to master for 20.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|23185 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23185 [Bug 23185] Koha::Objects supports passing through 'update' which means we can side step 'set' + 'store' -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25303 Joy Nelson <joy@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joy@bywatersolutions.com --- Comment #21 from Joy Nelson <joy@bywatersolutions.com> --- not backported to 19.11 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org