Instead of going to DBIx rightaway, we need the additional housekeeping stuff in Koha::Patron first. Using this report as a building block in bug 21336
Created attachment 78586 [details] [review] Bug 21337: Add Koha::Patrons->delete (trivial wrapper) Adds Koha::Patrons->delete as wrapper around Koha::Patron->delete. We do not want to bypass Koha::Patron and let DBIx do the job without further housekeeping. A call to move_to_deleted is included now, but could be made optional with a parameter if needed. Test plan: Run t/db_dependent/Koha/Patrons.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 78619 [details] [review] Bug 21337: Add Koha::Patrons->delete (trivial wrapper) Adds Koha::Patrons->delete as wrapper around Koha::Patron->delete. We do not want to bypass Koha::Patron and let DBIx do the job without further housekeeping. A call to move_to_deleted is included now, but could be made optional with a parameter if needed. Test plan: Run t/db_dependent/Koha/Patrons.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 78621 [details] [review] Bug 21337: (Follow-up) Add parameter for move_to_deleted action For consistency, it would be better to make the move optional. Test plan: Run again t/db_dependent/Koha/Patrons.t. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 79461 [details] [review] Bug 21337: Add Koha::Patrons->delete (trivial wrapper) Adds Koha::Patrons->delete as wrapper around Koha::Patron->delete. We do not want to bypass Koha::Patron and let DBIx do the job without further housekeeping. A call to move_to_deleted is included now, but could be made optional with a parameter if needed. Test plan: Run t/db_dependent/Koha/Patrons.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 79462 [details] [review] Bug 21337: (Follow-up) Add parameter for move_to_deleted action For consistency, it would be better to make the move optional. Test plan: Run again t/db_dependent/Koha/Patrons.t. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com>
(In reply to Michal Denar from comment #5) > Signed-off-by: Michal Denar <black23@gmail.com> Thanks !
I do not think the implementation is correct. DBIC would execute only 1 query on Set->delete, which means no entries will be removed if at least one cannot be removed. I would say we should do the same, i.e. execute in a transaction and rollback if something went wrong.
Created attachment 79708 [details] [review] Bug 21337: (QA follow-up) Rollback for partial delete Puts delete loop in a txn_do. Raises Koha::Exceptions::Patron::Delete when Patron->delete does not return true (like 0 or -1). Unit test adjusted accordingly. Note: A follow-up report for raising exceptions in Object->delete could well be considered. Not here please. Test plan: Run t/db_dependent/Koha/Patrons.t
(In reply to Jonathan Druart from comment #7) > I do not think the implementation is correct. DBIC would execute only 1 > query on Set->delete, which means no entries will be removed if at least one > cannot be removed. > I would say we should do the same, i.e. execute in a transaction and > rollback if something went wrong. See follow-up.
Please ignore false message of qa tools about POD coverage. This exception class is just like all the others.
Created attachment 79709 [details] [review] Bug 21337: (QA follow-up) Rollback for partial delete Puts delete loop in a txn_do. Raises Koha::Exceptions::Patron::Delete when Patron->delete does not return true (like 0 or -1). Unit test adjusted accordingly. Note: A follow-up report for raising exceptions in Object->delete could well be considered. Not here please. Test plan: Run t/db_dependent/Koha/Patrons.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 79915 [details] [review] Bug 21337: Add Koha::Patrons->delete (trivial wrapper) Adds Koha::Patrons->delete as wrapper around Koha::Patron->delete. We do not want to bypass Koha::Patron and let DBIx do the job without further housekeeping. A call to move_to_deleted is included now, but could be made optional with a parameter if needed. Test plan: Run t/db_dependent/Koha/Patrons.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 79916 [details] [review] Bug 21337: (follow-up) Add parameter for move_to_deleted action For consistency, it would be better to make the move optional. Test plan: Run again t/db_dependent/Koha/Patrons.t. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 79917 [details] [review] Bug 21337: (QA follow-up) Rollback for partial delete Puts delete loop in a txn_do. Raises Koha::Exceptions::Patron::Delete when Patron->delete does not return true (like 0 or -1). Unit test adjusted accordingly. Note: A follow-up report for raising exceptions in Object->delete could well be considered. Not here please. Test plan: Run t/db_dependent/Koha/Patrons.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Awesome work all! Pushed to master for 18.11
Created attachment 80624 [details] [review] Bug 21337: Koha::Objects->delete must return the number of deleted rows To follow DBIC behaviour Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
I have fixed a failing test in Objects.t IMO the following line is wrong: $patron->delete == 1 || Koha::Exceptions::Patron::Delete->throw; We must follow dbic behaviours and so we must not throw an exception if something went wrong (what DBIC does) Then I would not name an exception "Koha::Exceptions::Patron::Delete" but more something like Koha::Exceptions::Patron::NotDeleted, or CannotDelete, CannotBeDeleted, etc.
(In reply to Jonathan Druart from comment #16) > Created attachment 80624 [details] [review] [review] > Bug 21337: Koha::Objects->delete must return the number of deleted rows > > To follow DBIC behaviour > > Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> follow-up pushed to master, thanks Jonathan
Enhancement.. currently not backported to 18.05.x though will reconsider when the dependant GDPR bug is pushed.
(In reply to Jonathan Druart from comment #17) > I have fixed a failing test in Objects.t Fine, thanks. Note that we must think about returning 0 or 0E0. But note that DBIx here also depends on the specific SQL backend. > IMO the following line is wrong: > $patron->delete == 1 || Koha::Exceptions::Patron::Delete->throw; > > We must follow dbic behaviours and so we must not throw an exception if > something went wrong (what DBIC does) Not sure about that. Will come back later. > Then I would not name an exception "Koha::Exceptions::Patron::Delete" but > more something like Koha::Exceptions::Patron::NotDeleted, or CannotDelete, > CannotBeDeleted, etc. Always in for a good discussion about names :)
(In reply to Jonathan Druart from comment #17) > IMO the following line is wrong: > $patron->delete == 1 || Koha::Exceptions::Patron::Delete->throw; > > We must follow dbic behaviours and so we must not throw an exception if > something went wrong (what DBIC does) See also comment 7 (what you wrote before): > I do not think the implementation is correct. DBIC would execute only 1 > query on Set->delete, which means no entries will be removed if at least one > cannot be removed. > I would say we should do the same, i.e. execute in a transaction and > rollback if something went wrong. The third patch implemented your suggestion from comment7, but it sounds like your comment17 wants to revert that again? Probably misunderstanding what you mean, so please clarify. In the meantime noting that the txn_do in Patrons::delete has the disadvantage (like in a few other places too) that the following statements behave the same with AUTOCOMMIT==1 but behave differently in a test (nested transaction): eval { Koha::Patrons->search({ borrowernumber => [ $id1, $id2 ] })->delete }; eval { $schema->resultset('Borrower')->search({ borrowernumber => [ $id1, $id2 ] })->delete }; In the nested transaction (so in the test script) the txn_do does not rollback the delete of $id1 when $id2 failed. But it does when there is no nested transaction.
I was not precise enough. I use "something went wrong" for both cases: the row cannot be deleted because of an error (FK for instance), or the row does not exist. With: $p1 and p1bis can be deleted $p2 cannot be deleted and DBI will explode $p3 does not exist (and so $p3->delete will return 0) I would expect: delete $p1 and $p1bis => ok, both are deleted delete $p2 and ($p1 or $p3) => none are deleted, exception is raised delete $p1 and $p3 => ok, $p1 is deleted
Created attachment 81251 [details] [review] Bug 21337: (QA follow-up) Rename exception code Patron::Delete becomes Patron::FailedDelete. Trivial replace. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 81252 [details] [review] Bug 21337: (QA follow-up) On the road to a perfect delete We should not throw an exception if we get a -1 or undef from Patron->delete in Patrons->delete. Only 0's should trigger an exception. Test plan: Run t/db_dependent/Koha/Patrons.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Still something..
Created attachment 81257 [details] [review] Bug 21337: (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>
(In reply to Jonathan Druart from comment #22) > I was not precise enough. > I use "something went wrong" for both cases: the row cannot be deleted > because of an error (FK for instance), or the row does not exist. > > With: > $p1 and p1bis can be deleted > $p2 cannot be deleted and DBI will explode > $p3 does not exist (and so $p3->delete will return 0) > > I would expect: > delete $p1 and $p1bis => ok, both are deleted > delete $p2 and ($p1 or $p3) => none are deleted, exception is raised > delete $p1 and $p3 => ok, $p1 is deleted Thanks. I understand what you mean now. I think that the last patch addresses your concerns now sufficiently. But note that the above is not precise enough :) If a FK error occurs (famous error: patron still has issues), Koha::Object::delete explodes and Koha::Patron::delete returns UNDEF ! So Koha::Patrons now needs to handle the inconsistency between Koha::Object->delete and Koha::Pattron->delete (..) Note that if a row does not exist, it may be hard to get a 0 return value. (Either it won't be in the set at all or we have a -1 for not in storage.) Hard to reproduce; see mocks in the test. Please have a look and pass QA if possible.
Sorry Nick for changing the title at this late hour: but the last follow-up removes its trivial character after all.
Comment on attachment 81257 [details] [review] Bug 21337: (QA follow-up) On the road to a perfect delete Moved to bug 21684. I think I have found more inconsistencies, and it seems that I am going to add more complexity, again. So let put that to its own bug report.
(In reply to Marcel de Rooy from comment #26) > Created attachment 81257 [details] [review] [review] > Bug 21337: (QA follow-up) On the road to a perfect delete I have opened bug 21685 to discuss this more widely. RM feel free to push this one or not.
Back to PQA for the last patch.
(In reply to Jonathan Druart from comment #30) > (In reply to Marcel de Rooy from comment #26) > > Created attachment 81257 [details] [review] [review] [review] > > Bug 21337: (QA follow-up) On the road to a perfect delete > > I have opened bug 21685 to discuss this more widely. > > RM feel free to push this one or not. Thx for your time. I would prefer to include the patch. Will remove its obsolete flag now to make it more visible for the RM :)
(In reply to Marcel de Rooy from comment #32) > Thx for your time. I would prefer to include the patch. Will remove its > obsolete flag now to make it more visible for the RM :) Sorry I commented on the wrong attachment. * Bug 21337: (QA follow-up) On the road to a perfect delete => Do not push for now, see bug 21684. * Bug 21337: (QA follow-up) Rename exception code => Push if you like, see bug 21685 to discuss it more widely
(In reply to Jonathan Druart from comment #33) > (In reply to Marcel de Rooy from comment #32) > > Thx for your time. I would prefer to include the patch. Will remove its > > obsolete flag now to make it more visible for the RM :) > > Sorry I commented on the wrong attachment. > > * Bug 21337: (QA follow-up) On the road to a perfect delete > => Do not push for now, see bug 21684. > > * Bug 21337: (QA follow-up) Rename exception code > => Push if you like, see bug 21685 to discuss it more widely No, I think you commented on the right one ;) But the change on delete here and its test improve the situation and will be easy to adjust if the discussion reaches consensus.
Exception rename pushed to master
`->delete` method not called anywhere on a Koha::Patrons set object in 18.05.x. Confirming enhancement status for 18.11.x, will not be backported to 18.05.x series.