Bug 33873 - ILL requests with linked biblio_id that no longer exists causes table to not render
Summary: ILL requests with linked biblio_id that no longer exists causes table to not ...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Pedro Amorim
QA Contact: Martin Renvoize
URL:
Keywords: rel_22_11_candidate, rel_23_05_candidate
Depends on: 22440
Blocks:
  Show dependency treegraph
 
Reported: 2023-05-31 14:09 UTC by Pedro Amorim
Modified: 2023-12-28 20:47 UTC (History)
9 users (show)

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


Attachments
Bug 33873: Improve check for inexistant biblio object (910 bytes, patch)
2023-05-31 14:11 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33873: Improve check for inexistant biblio object (912 bytes, patch)
2023-06-05 17:13 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33873: Improve check for inexistant biblio object (975 bytes, patch)
2023-06-06 06:58 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2023-05-31 14:09:45 UTC
We stumbled upon this on one of our upgrades.
This is what I think happened:
Because bug 22440 is also the one introducing cascade delete on biblio_id in illrequests table, in the past, when a biblio was deleted its id still remained in the biblio_id column of illrequests.
This causes the API embed to respond with a biblio_id for the ILL request, but a null biblio (because it wasn't able to retrieve it, it doesn't exist).
Comment 1 Pedro Amorim 2023-05-31 14:11:27 UTC
Created attachment 151882 [details] [review]
Bug 33873: Improve check for inexistant biblio object
Comment 2 Pedro Amorim 2023-05-31 14:14:17 UTC
This change will now not only check for undefined, but also null - which is what is happening (and other falsy conditions of course).

This may be considered critical, because as it stands, there is a high chance systems upgrading to a version with bug 22440 will have their ILL table potentially not functional.
Comment 3 Pedro Amorim 2023-05-31 14:27:48 UTC
I wonder if a test can/should be written for this?
If so, test what? That a request is being returned with a biblio_id but an empty biblio in case the biblionumber does not exist?

Should we provide an atomic update file? To clean-up data retroactively? i.e.

update illrequests set biblio_id = NULL where biblio_id not in (select biblionumber from biblio);
Comment 4 Katrin Fischer 2023-05-31 14:29:41 UTC
Hi Pedro,

just to clarify: Are we deleting the biblio_id from illrequests or not? I am a little confused by the first comment.
We'd like to keep it for sure, we can still look-up details in deletedbiblio.
Comment 5 Pedro Amorim 2023-05-31 14:48:50 UTC
I believe this patch:
https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=22440&attachment=150716

Makes it so that if a biblio that is related to an ILL request is deleted, the ILL request biblio_id column is set to NULL
Previous to bug 22440, apparently this wasn't the case, and in such an event, the biblionumber would remain in the biblio_id illrequests column, even after the biblio with said biblionumber is deleted.
Comment 6 Katrin Fischer 2023-05-31 19:36:38 UTC
(In reply to Pedro Amorim from comment #5)
> I believe this patch:
> https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.
> html&bug=22440&attachment=150716
> 
> Makes it so that if a biblio that is related to an ILL request is deleted,
> the ILL request biblio_id column is set to NULL
> Previous to bug 22440, apparently this wasn't the case, and in such an
> event, the biblionumber would remain in the biblio_id illrequests column,
> even after the biblio with said biblionumber is deleted.

You are right, but I really wish they hadn't hidden this (breaking) change in a patch that seems totally unrelated to it :(

It can and will create data loss for some libraries depending on how their backends work. And it's not advertised in release notes. 

Our backend cleans up the records automatically when a ILL request is filled, but we relied on having the link still to be able to pull the information.
Comment 7 Pedro Amorim 2023-06-01 08:59:14 UTC
Katrin, I think that change snuck in because at the time was seen as an oversight, not something working as intended. But I think what you say makes sense, we should consider reverting this FK behaviour. I think we should wait to hear from Tomas and Martin about this (and whoever else who may want to chip in).

However, this talk is to be had either on bug 22440 or bug 21983.

This bug (33873) is a fix for the handling of data coming from the API endpoint that is feeding ILL requests data to the new ILL table. 
This fix (33873) should be pushed, imo, regardless of if we revert the FK relationship or not.
Comment 8 Katrin Fischer 2023-06-01 15:16:40 UTC
I vote for removing the FK again OR cleaning up the old ill_requests... either this data should not exist or it should. 

I vote for: it should exist and remove FK. And we could show the number not linked when there is no record in the table.
Comment 9 Katrin Fischer 2023-06-01 15:19:13 UTC
Please also see bug 21983.
Comment 10 Pedro Amorim 2023-06-01 15:35:33 UTC
(In reply to Katrin Fischer from comment #8)
> I vote for removing the FK again OR cleaning up the old ill_requests...
> either this data should not exist or it should. 
> 
> I vote for: it should exist and remove FK. And we could show the number not
> linked when there is no record in the table.

I don't see any issue with that, but again would like Martin and Tomas to chip in.

However, the fix provided here in this bug (33873) is required, regardless of whatever decision we go down with regarding the FK.
Comment 11 Pedro Amorim 2023-06-02 13:57:28 UTC Comment hidden (obsolete)
Comment 12 Pedro Amorim 2023-06-02 13:59:39 UTC
Upping this to blocker to raise awareness.

Without this fix, systems upgrading to 22.11.06 or 23.05 will potentially have their ILL table not be functional.

If the upgrade happens on a system that has ILL requests with a biblio record related to it, that has since been deleted, the table will fail to render.

To reproduce, on clean k-t-d master:
1) ILL setup, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
2) Create a new FreeForm request, put whatever data you want in the form.
3) Go back to ILL requests table, notice the table renders and request is there.
4) Run

koha-mysql kohadev

# Drop the FK constraint, or it wont allow us to run next step:
ALTER TABLE illrequests DROP FOREIGN KEY illrequests_bibfk;

# Update biblio_id on the ILL request to a biblionumber that doesn't exist:
UPDATE illrequests SET biblio_id=999 where illrequest_id = 1;

5) Refresh the ILL table, notice it never renders and has a nasty console error:
Cannot read properties of null (reading 'biblio_id')

This is what happens on upgrade from previous versions.

6) Apply this patch
7) Refresh table again, notice it renders correctly once more
Comment 13 Tomás Cohen Arazi 2023-06-05 17:13:38 UTC
Created attachment 152009 [details] [review]
Bug 33873: Improve check for inexistant biblio object
Comment 14 Tomás Cohen Arazi 2023-06-05 17:21:49 UTC
I followed the test plan and managed to replicate the issue and the patch fixed.
Comment 15 Martin Renvoize 2023-06-06 06:21:01 UTC
With bug 21983 introducing a deleted_biblio_id field for storing this data, I wonder if we aught to be embedding that and displaying (and highlighting) deleted bib details in the table here rather than displaying an empty column?

I see three cases with bug 21983.. Bib still exists and active, Bib is 'deleted' but exists and Bib has been completely removed.
Comment 16 Martin Renvoize 2023-06-06 06:30:51 UTC
Hmm.. I think we still need this though.. I fixed a very similar issue recently in 744ac4825ba9a346e2ad1c8e413d3d9423658512 (Bug 33504: Update patron_to_html to deal with null)

In the above commit I wrote:

> In the patron_to_html function we were dealing explicitly with the case
> where patron may be passed undefined, but forgetting that it may be
> returned as 'null' from the api too.  Changing from `( patron ===
> undefined )` to `( patron == null )` is the recommended approach for
> detecting 'undefined or null' in javascript.

Do we have a preference for `!biblio` vs `biblio == null`.. They are equivalent right?
Comment 17 Martin Renvoize 2023-06-06 06:57:29 UTC
I consulted google..  They're not equivilent.. `== null` will catch `undefined or null` whereas `!variable` will catch `null, undefined, 0, NaN, false, or an empty string`.  I think your `!variable` is probably the better call here :)
Comment 18 Martin Renvoize 2023-06-06 06:58:38 UTC
Created attachment 152035 [details] [review]
Bug 33873: Improve check for inexistant biblio object

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2023-06-06 06:58:52 UTC
Passing QA
Comment 20 Katrin Fischer 2023-06-06 09:58:58 UTC
Thx all!
Comment 21 Tomás Cohen Arazi 2023-06-06 13:01:25 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 22 Matt Blenkinsop 2023-06-12 10:54:10 UTC
Updating to 'Pushed to stable' and adding released version before oldstable rmaint activity
Comment 23 Matt Blenkinsop 2023-06-12 10:58:26 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x