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!
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;
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.
Created attachment 178387 [details] [review] Bug 32630: dbic
Created attachment 178388 [details] [review] Bug 32630: database changes
Created attachment 178389 [details] [review] Bug 32630: atomicupdate
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
Created attachment 178391 [details] [review] Bug 32630: Update patron attribute display Only show this attribute if defined
Created attachment 178392 [details] [review] Bug 32630: Add tests prove t/db_dependent/Koha/ILL/Requests.t
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
Added bug 39175 as dependency because it fixes an unrelated bug that otherwise makes following the test plan here impossible.
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Pushed for 25.05! Well done everyone, thank you!
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)