Bug 32630 - Don't delete ILL requests when patron is deleted
Summary: Don't delete ILL requests when patron is deleted
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Pedro Amorim
QA Contact: Lisette Scheer
URL:
Keywords:
Depends on: 39175
Blocks:
  Show dependency treegraph
 
Reported: 2023-01-13 14:38 UTC by Katrin Fischer
Modified: 2025-03-07 17:05 UTC (History)
5 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.05.00
Circulation function:


Attachments
Bug 32630: dbic (1.07 KB, patch)
2025-02-20 11:58 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32630: database changes (1.50 KB, patch)
2025-02-20 11:58 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32630: atomicupdate (1.39 KB, patch)
2025-02-20 11:58 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32630: Update send notice to patron button (3.17 KB, patch)
2025-02-20 11:58 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32630: Update patron attribute display (2.21 KB, patch)
2025-02-20 11:58 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32630: Add tests (1.66 KB, patch)
2025-02-20 11:58 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32630: Update get_notice (1.73 KB, patch)
2025-02-20 11:58 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32630: dbic (1.17 KB, patch)
2025-02-28 15:52 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32630: database changes (1.61 KB, patch)
2025-02-28 15:52 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32630: atomicupdate (1.50 KB, patch)
2025-02-28 15:53 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32630: Update send notice to patron button (3.27 KB, patch)
2025-02-28 15:53 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32630: Update patron attribute display (2.31 KB, patch)
2025-02-28 15:53 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32630: Add tests (1.76 KB, patch)
2025-02-28 15:53 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32630: Update get_notice (1.83 KB, patch)
2025-02-28 15:53 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32630: dbic (1.23 KB, patch)
2025-02-28 21:22 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 32630: database changes (1.67 KB, patch)
2025-02-28 21:23 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 32630: atomicupdate (1.55 KB, patch)
2025-02-28 21:23 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 32630: Update send notice to patron button (3.33 KB, patch)
2025-02-28 21:23 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 32630: Update patron attribute display (2.37 KB, patch)
2025-02-28 21:24 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 32630: Update get_notice (1.89 KB, patch)
2025-02-28 21:24 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 32630: Update get_notice (1.89 KB, patch)
2025-02-28 21:24 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 32630: Add tests (1.82 KB, patch)
2025-02-28 21:26 UTC, Lisette Scheer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2023-01-13 14:38:00 UTC
There is a FK constraint, that makes it so that when a patron is deleted, the ILL requests linked to the account are deleted as well.

This can potentially cause a number of problems:
* ILL requests deleted could be pending, in shipment etc. You will have no audit track if there are any problems with transport etc.
* You won't be able to do reliable statistics on number of ILL requests etc. especially since adding a request is not even logged.


I propose that instead of removing the borrowernumber, we either
* leave it
* set it to NULL

Would love to get some input on this one!
Comment 1 Katrin Fischer 2023-01-13 14:38:23 UTC
illrequests:

[...]
  `orderid` varchar(50) COLLATE utf8mb4_unicode_ci DEFAULT NULL,
  `backend` varchar(20) COLLATE utf8mb4_unicode_ci DEFAULT NULL,
  PRIMARY KEY (`illrequest_id`),
  UNIQUE KEY `illrequest_id` (`illrequest_id`),
  KEY `illrequests_bnfk` (`borrowernumber`),
  KEY `illrequests_bcfk_2` (`branchcode`),
  KEY `illrequests_safk` (`status_alias`),
  CONSTRAINT `illrequests_bcfk_2` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE CASCADE ON UPDATE CASCADE,

  CONSTRAINT `illrequests_bnfk` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE,

  CONSTRAINT `illrequests_safk` FOREIGN KEY (`status_alias`) REFERENCES `authorised_values` (`authorised_value`) ON DELETE SET NULL ON UPDATE CASCADE
) ENGINE=InnoDB AUTO_INCREMENT=71 DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;
Comment 2 Martin Renvoize (ashimema) 2023-06-06 07:01:33 UTC
I think 'SET NULL' is the right thing to do here.. if you want the audit trail for what deleted borrower we should follow the pattern and add a deleted_borrower_id field to the table and trigger a move at patron delete.
Comment 3 Pedro Amorim 2025-02-20 11:58:16 UTC
Created attachment 178387 [details] [review]
Bug 32630: dbic
Comment 4 Pedro Amorim 2025-02-20 11:58:18 UTC
Created attachment 178388 [details] [review]
Bug 32630: database changes
Comment 5 Pedro Amorim 2025-02-20 11:58:21 UTC
Created attachment 178389 [details] [review]
Bug 32630: atomicupdate
Comment 6 Pedro Amorim 2025-02-20 11:58:23 UTC
Created attachment 178390 [details] [review]
Bug 32630: Update send notice to patron button

Only show this action button if the ILL request has a borrowernumber
Comment 7 Pedro Amorim 2025-02-20 11:58:26 UTC
Created attachment 178391 [details] [review]
Bug 32630: Update patron attribute display

Only show this attribute if defined
Comment 8 Pedro Amorim 2025-02-20 11:58:28 UTC
Created attachment 178392 [details] [review]
Bug 32630: Add tests

prove t/db_dependent/Koha/ILL/Requests.t
Comment 9 Pedro Amorim 2025-02-20 11:58:31 UTC
Created attachment 178393 [details] [review]
Bug 32630: Update get_notice

Make it consider the possibility of a patron not existing.
This ensures that placing a request with partners does not error and works as expected even if the ILL request's borrowernumber is undef

To test:
1) Run the test file before and after applying this patch:
prove t/db_dependent/Koha/ILL/Requests.t

To test (ILL request is kept after borrower is deleted):
1) Apply all patches except this one
2) Enable ILLModule and create an ILL request.
3) Add a type, valid cardnumber and library.
4) Delete the associated borrower.
5) Confirm the ILL request is still listed and shown as expected

Continue test, confirm follow-up bug is fixed:
6) Click 'Place request with partners'
7) You get a ERR_TOO_MANY_REDIRECTS error
8) Apply this patch and restart plack. Repeat.
9) Confirm the place request with partners libraries page is shown correctly
Comment 10 Pedro Amorim 2025-02-20 11:59:48 UTC
Added bug 39175 as dependency because it fixes an unrelated bug that otherwise makes following the test plan here impossible.
Comment 11 Pedro Amorim 2025-02-28 15:52:55 UTC
Created attachment 178858 [details] [review]
Bug 32630: dbic

Sponsored-by: UKHSA - UK Health Security Agency
Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Comment 12 Pedro Amorim 2025-02-28 15:52:58 UTC
Created attachment 178859 [details] [review]
Bug 32630: database changes

Sponsored-by: UKHSA - UK Health Security Agency
Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Comment 13 Pedro Amorim 2025-02-28 15:53:00 UTC
Created attachment 178860 [details] [review]
Bug 32630: atomicupdate

Sponsored-by: UKHSA - UK Health Security Agency
Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Comment 14 Pedro Amorim 2025-02-28 15:53:03 UTC
Created attachment 178861 [details] [review]
Bug 32630: Update send notice to patron button

Only show this action button if the ILL request has a borrowernumber

Sponsored-by: UKHSA - UK Health Security Agency
Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Comment 15 Pedro Amorim 2025-02-28 15:53:06 UTC
Created attachment 178862 [details] [review]
Bug 32630: Update patron attribute display

Only show this attribute if defined

Sponsored-by: UKHSA - UK Health Security Agency
Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Comment 16 Pedro Amorim 2025-02-28 15:53:09 UTC
Created attachment 178863 [details] [review]
Bug 32630: Add tests

prove t/db_dependent/Koha/ILL/Requests.t

Sponsored-by: UKHSA - UK Health Security Agency
Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Comment 17 Pedro Amorim 2025-02-28 15:53:12 UTC
Created attachment 178864 [details] [review]
Bug 32630: Update get_notice

Make it consider the possibility of a patron not existing.
This ensures that placing a request with partners does not error and works as expected even if the ILL request's borrowernumber is undef

To test:
1) Run the test file before and after applying this patch:
prove t/db_dependent/Koha/ILL/Requests.t

To test (ILL request is kept after borrower is deleted):
1) Apply all patches except this one
2) Enable ILLModule and create an ILL request.
3) Add a type, valid cardnumber and library.
4) Delete the associated borrower.
5) Confirm the ILL request is still listed and shown as expected

Continue test, confirm follow-up bug is fixed:
6) Click 'Place request with partners'
7) You get a ERR_TOO_MANY_REDIRECTS error
8) Apply this patch and restart plack. Repeat.
9) Confirm the place request with partners libraries page is shown correctly

Sponsored-by: UKHSA - UK Health Security Agency
Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Comment 18 Lisette Scheer 2025-02-28 21:22:59 UTC
Created attachment 178876 [details] [review]
Bug 32630: dbic

Worked as expected.

Sponsored-by: UKHSA - UK Health Security Agency
Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 19 Lisette Scheer 2025-02-28 21:23:29 UTC
Created attachment 178877 [details] [review]
Bug 32630: database changes

Sponsored-by: UKHSA - UK Health Security Agency
Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 20 Lisette Scheer 2025-02-28 21:23:44 UTC
Created attachment 178878 [details] [review]
Bug 32630: atomicupdate

Sponsored-by: UKHSA - UK Health Security Agency
Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 21 Lisette Scheer 2025-02-28 21:23:57 UTC
Created attachment 178879 [details] [review]
Bug 32630: Update send notice to patron button

Only show this action button if the ILL request has a borrowernumber

Sponsored-by: UKHSA - UK Health Security Agency
Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 22 Lisette Scheer 2025-02-28 21:24:07 UTC
Created attachment 178880 [details] [review]
Bug 32630: Update patron attribute display

Only show this attribute if defined

Sponsored-by: UKHSA - UK Health Security Agency
Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 23 Lisette Scheer 2025-02-28 21:24:31 UTC
Created attachment 178881 [details] [review]
Bug 32630: Update get_notice

Make it consider the possibility of a patron not existing.
This ensures that placing a request with partners does not error and works as expected even if the ILL request's borrowernumber is undef

To test:
1) Run the test file before and after applying this patch:
prove t/db_dependent/Koha/ILL/Requests.t

To test (ILL request is kept after borrower is deleted):
1) Apply all patches except this one
2) Enable ILLModule and create an ILL request.
3) Add a type, valid cardnumber and library.
4) Delete the associated borrower.
5) Confirm the ILL request is still listed and shown as expected

Continue test, confirm follow-up bug is fixed:
6) Click 'Place request with partners'
7) You get a ERR_TOO_MANY_REDIRECTS error
8) Apply this patch and restart plack. Repeat.
9) Confirm the place request with partners libraries page is shown correctly

Sponsored-by: UKHSA - UK Health Security Agency
Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 24 Lisette Scheer 2025-02-28 21:24:44 UTC
Created attachment 178882 [details] [review]
Bug 32630: Update get_notice

Make it consider the possibility of a patron not existing.
This ensures that placing a request with partners does not error and works as expected even if the ILL request's borrowernumber is undef

To test:
1) Run the test file before and after applying this patch:
prove t/db_dependent/Koha/ILL/Requests.t

To test (ILL request is kept after borrower is deleted):
1) Apply all patches except this one
2) Enable ILLModule and create an ILL request.
3) Add a type, valid cardnumber and library.
4) Delete the associated borrower.
5) Confirm the ILL request is still listed and shown as expected

Continue test, confirm follow-up bug is fixed:
6) Click 'Place request with partners'
7) You get a ERR_TOO_MANY_REDIRECTS error
8) Apply this patch and restart plack. Repeat.
9) Confirm the place request with partners libraries page is shown correctly

Sponsored-by: UKHSA - UK Health Security Agency
Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 25 Lisette Scheer 2025-02-28 21:26:50 UTC
Created attachment 178883 [details] [review]
Bug 32630: Add tests

prove t/db_dependent/Koha/ILL/Requests.t

Sponsored-by: UKHSA - UK Health Security Agency
Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com>
Comment 26 Katrin Fischer 2025-03-07 17:04:06 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 27 Katrin Fischer 2025-03-07 17:05:20 UTC
I'd be quite happy if this cold be considered for backporting to the 24.11 LTS release at least. (updating to be a bug, but leaving final decision to RMaint)