Bug 21337 - Add Koha::Patrons->delete
Summary: Add Koha::Patrons->delete
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 21336
  Show dependency treegraph
 
Reported: 2018-09-12 12:55 UTC by Marcel de Rooy
Modified: 2019-10-14 19:56 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21337: Add Koha::Patrons->delete (trivial wrapper) (3.05 KB, patch)
2018-09-12 14:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21337: Add Koha::Patrons->delete (trivial wrapper) (3.05 KB, patch)
2018-09-13 10:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21337: (Follow-up) Add parameter for move_to_deleted action (2.34 KB, patch)
2018-09-13 10:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21337: Add Koha::Patrons->delete (trivial wrapper) (3.11 KB, patch)
2018-09-26 20:26 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 21337: (Follow-up) Add parameter for move_to_deleted action (2.40 KB, patch)
2018-09-26 20:26 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 21337: (QA follow-up) Rollback for partial delete (4.51 KB, patch)
2018-10-01 12:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21337: (QA follow-up) Rollback for partial delete (4.56 KB, patch)
2018-10-01 12:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21337: Add Koha::Patrons->delete (trivial wrapper) (3.16 KB, patch)
2018-10-04 06:05 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21337: (follow-up) Add parameter for move_to_deleted action (2.45 KB, patch)
2018-10-04 06:05 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21337: (QA follow-up) Rollback for partial delete (4.58 KB, patch)
2018-10-04 06:05 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21337: Koha::Objects->delete must return the number of deleted rows (1.81 KB, patch)
2018-10-15 17:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21337: (QA follow-up) Rename exception code (2.10 KB, patch)
2018-10-26 07:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21337: (QA follow-up) On the road to a perfect delete (3.61 KB, patch)
2018-10-26 07:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21337: (QA follow-up) On the road to a perfect delete (4.41 KB, patch)
2018-10-26 09:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2018-09-12 12:55:21 UTC
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
Comment 1 Marcel de Rooy 2018-09-12 14:29:29 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2018-09-13 10:17:31 UTC
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>
Comment 3 Marcel de Rooy 2018-09-13 10:31:02 UTC
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>
Comment 4 Michal Denar 2018-09-26 20:26:39 UTC
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>
Comment 5 Michal Denar 2018-09-26 20:26:44 UTC
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>
Comment 6 Marcel de Rooy 2018-09-27 06:38:10 UTC
(In reply to Michal Denar from comment #5)
> Signed-off-by: Michal Denar <black23@gmail.com>

Thanks !
Comment 7 Jonathan Druart 2018-09-30 22:46:20 UTC
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.
Comment 8 Marcel de Rooy 2018-10-01 12:12:55 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2018-10-01 12:13:20 UTC
(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.
Comment 10 Marcel de Rooy 2018-10-01 12:14:18 UTC
Please ignore false message of qa tools about POD coverage. This exception class is just like all the others.
Comment 11 Marcel de Rooy 2018-10-01 12:17:11 UTC
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>
Comment 12 Josef Moravec 2018-10-04 06:05:43 UTC
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>
Comment 13 Josef Moravec 2018-10-04 06:05:48 UTC
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>
Comment 14 Josef Moravec 2018-10-04 06:05:52 UTC
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>
Comment 15 Nick Clemens 2018-10-15 17:04:30 UTC
Awesome work all!

Pushed to master for 18.11
Comment 16 Jonathan Druart 2018-10-15 17:38:50 UTC
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>
Comment 17 Jonathan Druart 2018-10-15 17:47:28 UTC
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.
Comment 18 Nick Clemens 2018-10-15 17:54:21 UTC
(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
Comment 19 Martin Renvoize 2018-10-17 11:03:38 UTC
Enhancement.. currently not backported to 18.05.x though will reconsider when the dependant GDPR bug is pushed.
Comment 20 Marcel de Rooy 2018-10-17 11:45:05 UTC
(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 :)
Comment 21 Marcel de Rooy 2018-10-25 11:12:24 UTC
(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.
Comment 22 Jonathan Druart 2018-10-25 11:26:42 UTC
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
Comment 23 Marcel de Rooy 2018-10-26 07:58:55 UTC
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>
Comment 24 Marcel de Rooy 2018-10-26 07:58:59 UTC Comment hidden (obsolete)
Comment 25 Marcel de Rooy 2018-10-26 08:16:12 UTC Comment hidden (obsolete)
Comment 26 Marcel de Rooy 2018-10-26 09:08:50 UTC
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>
Comment 27 Marcel de Rooy 2018-10-26 09:16:27 UTC
(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.
Comment 28 Marcel de Rooy 2018-10-26 09:21:48 UTC
Sorry Nick for changing the title at this late hour: but the last follow-up removes its trivial character after all.
Comment 29 Jonathan Druart 2018-10-26 13:57:47 UTC
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.
Comment 30 Jonathan Druart 2018-10-26 14:05:36 UTC
(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.
Comment 31 Jonathan Druart 2018-10-26 14:05:58 UTC
Back to PQA for the last patch.
Comment 32 Marcel de Rooy 2018-10-26 14:32:26 UTC
(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 :)
Comment 33 Jonathan Druart 2018-10-26 14:39:08 UTC
(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
Comment 34 Marcel de Rooy 2018-10-26 14:51:28 UTC
(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.
Comment 35 Nick Clemens 2018-10-26 17:15:47 UTC
Exception rename pushed to master
Comment 36 Martin Renvoize 2018-11-06 11:21:39 UTC
`->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.