for instance: Koha::Cities->find(1)->delete and $schema->resultset('City')->delete must returns the same value, as well as throw an exception if needed. Same for Koha::Objects->delete This is the result of a discussion started on bug 21337.
Created attachment 81317 [details] [review] 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>
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?
Created attachment 81319 [details] [review] Bug 21684: Add tests to show some of the discrepancies
I think we should remove our specific code from Koha::Object->delete, but that will imply we will need to adjust callers code. I am in favour of such move but I would like people to give their opinion. It seems to be too big (side-effects) to make it part of 18.11 anyway.
(In reply to Jonathan Druart from comment #2) > Created attachment 81318 [details] [review] [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.
Created attachment 90937 [details] [review] Bug 21684: Add tests to show some of the discrepancies
(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?
Created attachment 96218 [details] [review] Bug 21684: Fix delete methods and add more tests
I think we should follow this path. If you all agree I will provide a follow-up to fix the different calls.
Distracted by: warn "Deleted $count patrons\n" if $params->{verbose}; in Koha/Patrons.pm ;)
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."
I'm on board with this one.. we should be consistent in our handling of object return values. For consistency with DBIx::Class (which underpins our objects) we should stick to exceptions for failure cases. If you are able to continue this work Jonathan, that would be great :)
(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.
Created attachment 96254 [details] [review] Bug 21684: Adjust some tests
Actually it seems that the only occurrence is:members/deletemem.pl but actually it is an improvement, so could be done on a separate bug report I did not find any others delete statement where the return value where useful, but I may be wrong here!
Created attachment 96270 [details] [review] Bug 21684: Add tests to show some of the discrepancies Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 96271 [details] [review] Bug 21684: Fix delete methods and add more tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 96272 [details] [review] Bug 21684: Adjust some tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 96429 [details] [review] 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>
Created attachment 96430 [details] [review] Bug 21684: Fix delete methods and add more tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 96431 [details] [review] Bug 21684: Adjust some tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Thanks Jonathan, Passing QA
Nice work everyone! Pushed to master for 20.05
# 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.
Created attachment 96872 [details] [review] Bug 21684: Fix UploadedFile[s]->delete Tests were failing with: # 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 I am not sure this patch is perfect, a set of uploaded files should be deleted in a transaction, which would be rollback if something is wrong. But it will be tricky to restore the files after they have been deleted. It seems that we should deal with that with a more complicated process and should be part of a separate bug.
Marcel, could I get your opinion on this follow-up?
Created attachment 96888 [details] [review] Bug 21684: Koha::Object->delete must return a Koha::Object Not a DBIx::Class object. Tests have been added, add Bookseller.t fixed
(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..
(In reply to Jonathan Druart from comment #27) > Marcel, could I get your opinion on this follow-up? Looking now
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??
Created attachment 96895 [details] [review] Bug 21684: (follow-up) Fix UploadedFile[s]->delete This can be squashed
(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.
(In reply to Jonathan Druart from comment #33) > I do not understand exactly what you mean. Short version: Get rid of 0E0 ? > 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. Sounds good to me.
(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).
As I wrote in the commit message, IMO the whole thing needs a rewrite, to deal with deleting a set in a transaction.
(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.
Created attachment 96898 [details] [review] Bug 21684: Fix Hold.t
4 followups pushed.. code all makes sense to me and resolves the failures.
Not backported to 19.11.x because 24150 cannot be backported.
(In reply to Jonathan Druart from comment #2) > Created attachment 81318 [details] [review] [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.