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).
Created attachment 151882 [details] [review] Bug 33873: Improve check for inexistant biblio object
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.
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);
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.
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.
(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.
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.
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.
Please also see bug 21983.
(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.
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.
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
Created attachment 152009 [details] [review] Bug 33873: Improve check for inexistant biblio object
I followed the test plan and managed to replicate the issue and the patch fixed.
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.
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?
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 :)
Created attachment 152035 [details] [review] Bug 33873: Improve check for inexistant biblio object Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Passing QA
Thx all!
Pushed to master for 23.11. Nice work everyone, thanks!
Updating to 'Pushed to stable' and adding released version before oldstable rmaint activity
Nice work everyone! Pushed to oldstable for 22.11.x