https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21337 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |m.de.rooy@rijksmuseum.nl |ity.org | Blocks| |21336 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21336 [Bug 21336] GDPR: Process unsubscribe requests automatically -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21337 --- Comment #1 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 78586 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=78586&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21337 --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 78621 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=78621&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21337 Michal Denar <black23@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #78619|0 |1 is obsolete| | Attachment #78621|0 |1 is obsolete| | --- Comment #4 from Michal Denar <black23@gmail.com> --- Created attachment 79461 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=79461&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

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

Signed-off-by: Michal Denar <black23@gmail.com>

Thanks ! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21337 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|Trivial patch |Small patch Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21337 --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21337 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #79708|0 |1 is obsolete| | --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 79709 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=79709&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21337 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #79461|0 |1 is obsolete| | Attachment #79462|0 |1 is obsolete| | Attachment #79709|0 |1 is obsolete| | --- Comment #12 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 79915 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=79915&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21337 --- Comment #14 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 79917 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=79917&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21337 --- Comment #16 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 80624 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=80624&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21337 --- Comment #18 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #16)

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>

follow-up pushed to master, thanks Jonathan -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21337 --- Comment #20 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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 :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21337 --- Comment #21 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.

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

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21337 --- Comment #23 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 81251 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=81251&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21337 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> 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=21337 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED --- Comment #25 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Still something.. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21337 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Signed Off --- Comment #27 from 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21337 --- Comment #28 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Sorry Nick for changing the title at this late hour: but the last follow-up removes its trivial character after all. -- You are receiving this mail because: You are watching all bug changes.

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

Created attachment 81257 [details] [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. -- You are receiving this mail because: You are watching all bug changes.

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

(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.

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 :) -- You are receiving this mail because: You are watching all bug changes.

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

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21337 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #81257|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=21337 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=21685 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21337 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED --- Comment #36 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- `->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. -- You are receiving this mail because: You are watching all bug changes.