https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21684 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21684 --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 81317 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=81317&action=edit Bug 21684: (QA follow-up) On the road to a perfect delete Adjusting Koha::Patrons->delete: We should not throw an exception if we get a -1 from Patron->delete. A zero should trigger an exception. And an undef too since it is probably a DBIx exception.. Test plan: Run t/db_dependent/Koha/Patrons.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21684 --- Comment #3 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 81319 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=81319&action=edit Bug 21684: Add tests to show some of the discrepancies -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21684 --- Comment #5 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #2)

Created attachment 81318 [details] [review] Bug 21684: Add auto_savepoint to DBIC connection

https://metacpan.org/pod/distribution/DBIx-Class/lib/DBIx/Class/Manual/ Cookbook.pod#Nested-transactions-and-auto-savepoints

Otherwise: DBIx::Class::Storage::txn_rollback(): A txn_rollback in nested transaction is ineffective! (depth 1) at t/db_dependent/Koha/Objects.t line 274

Possible side-effects? Slowness?

Would be nice to have, and has support in InnoDB. We should be sure about performance though. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21684 --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jonathan Druart from comment #4)

I am in favour of such move but I would like people to give their opinion.

Is silence acquiescence? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21684 --- Comment #9 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I think we should follow this path. If you all agree I will provide a follow-up to fix the different calls. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21684 --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Not sure if this works: + ok( ref($r_us) && ref($r_them), + 'Successful delete should return the object ' ); Note that it depends on the storage backend: "The return value is a pass through of what the underlying storage backend returned, and may vary." -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21684 --- Comment #13 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (In reply to Marcel de Rooy from comment #11)

Not sure if this works: + ok( ref($r_us) && ref($r_them), + 'Successful delete should return the object ' );

Note that it depends on the storage backend: "The return value is a pass through of what the underlying storage backend returned, and may vary."

Hmm, well at the present time we only support MySQL and MariaDB as our backends and I believe those are consistent with one another in this regard. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21684 --- Comment #14 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 96254 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=96254&action=edit Bug 21684: Adjust some tests -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21684 Nick Clemens <nick@bywatersolutions.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=21684 --- Comment #17 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 96271 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=96271&action=edit Bug 21684: Fix delete methods and add more tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21684 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #96270|0 |1 is obsolete| | --- Comment #19 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 96429 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=96429&action=edit Bug 21684: Add tests to show some of the discrepancies Signed-off-by: Nick Clemens <nick@bywatersolutions.com> 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=21684 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #96272|0 |1 is obsolete| | --- Comment #21 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 96431 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=96431&action=edit Bug 21684: Adjust some tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> 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=21684 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|major |enhancement -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21684 --- Comment #23 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=21684 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |minor -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21684 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |additional_work_needed Depends on| |17669 --- Comment #25 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- # Failed test 'Test delete via UploadedFile as well as UploadedFiles' # at t/db_dependent/Upload.t line 193. DBIx::Class::Row::delete(): Not in database at /kohadevbox/koha/Koha/Object.pm line 219 Failing test, the previous test was expecting 0E0 if the delete failed, now it raises an exception. I think we can simply adjust the test accordingly. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17669 [Bug 17669] Add purging temporary uploads to cleanup_database -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21684 --- Comment #27 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Marcel, could I get your opinion on this follow-up? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21684 --- Comment #29 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #25)

# Failed test 'Test delete via UploadedFile as well as UploadedFiles' # at t/db_dependent/Upload.t line 193. DBIx::Class::Row::delete(): Not in database at /kohadevbox/koha/Koha/Object.pm line 219

Failing test, the previous test was expecting 0E0 if the delete failed, now it raises an exception. I think we can simply adjust the test accordingly.

Yes, we should bring that in line with the changed delete subs. The subroutines in UploadedFile[s] also need attention.. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21684 --- Comment #31 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- How do we view this line now in both modules? return $rv==0 ? "0E0" : $rv; Also in delete_missing. The Upload.t test also refers to -1 values. Are they still possible? At first glance the test should still be adjusted a bit more, although it passes now. Second patch + my $deleted = $self->_result()->delete; + if ( ref $deleted ) { + my $object_class = Koha::Object::_get_object_class( $self->_result->result_class ); + $deleted = $object_class->_new_from_dbic($deleted); + } + return $deleted; Isnt this a bit expensive to do since we will generally not even look at what we get back? Most often $obj->delete is not even tested. Parameterize the return?? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21684 --- Comment #33 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Marcel de Rooy from comment #31)

How do we view this line now in both modules? return $rv==0 ? "0E0" : $rv; Also in delete_missing.

I do not understand exactly what you mean.

The Upload.t test also refers to -1 values. Are they still possible? At first glance the test should still be adjusted a bit more, although it passes now.

Indeed, done.

Second patch + my $deleted = $self->_result()->delete; + if ( ref $deleted ) { + my $object_class = Koha::Object::_get_object_class( $self->_result->result_class ); + $deleted = $object_class->_new_from_dbic($deleted); + } + return $deleted; Isnt this a bit expensive to do since we will generally not even look at what we get back?

I do not think it's expensive, the _new_from_dbic will do nothing if the object is not reused after ->delete. So only _get_object_class will be effective.

Most often $obj->delete is not even tested.

It is widely tested in Objects.t

Parameterize the return??

Not a good idea IMO. We must behave like DBIx::Class, it's the whole point of this patchset. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21684 --- Comment #35 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Marcel de Rooy from comment #34)

(In reply to Jonathan Druart from comment #33)

...

I do not understand exactly what you mean. Short version: Get rid of 0E0 ?

How? It's the expected behavior (DBI return value). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21684 --- Comment #37 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #35)

(In reply to Marcel de Rooy from comment #34)

...

(In reply to Jonathan Druart from comment #33)

...

I do not understand exactly what you mean. Short version: Get rid of 0E0 ?

How? It's the expected behavior (DBI return value).

Theoretically it might not be DBI. And DBIx depends on the backend. But we can leave it for now; much more of this kind. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21684 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed | --- Comment #39 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 4 followups pushed.. code all makes sense to me and resolves the failures. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21684 Bug 21684 depends on bug 24150, which changed state. Bug 24150 Summary: Add missing Koha::Old::*[s] classes https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24150 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21684 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=26639 --- Comment #42 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jonathan Druart from comment #2)

Created attachment 81318 [details] [review] Bug 21684: Add auto_savepoint to DBIC connection

https://metacpan.org/pod/distribution/DBIx-Class/lib/DBIx/Class/Manual/ Cookbook.pod#Nested-transactions-and-auto-savepoints

Otherwise: DBIx::Class::Storage::txn_rollback(): A txn_rollback in nested transaction is ineffective! (depth 1) at t/db_dependent/Koha/Objects.t line 274

Possible side-effects? Slowness?

This patch has been moved to bug 26639. -- You are receiving this mail because: You are watching all bug changes.