[Bug 33873] New: ILL requests with linked biblio_id that no longer exists causes table to not render
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 Bug ID: 33873 Summary: ILL requests with linked biblio_id that no longer exists causes table to not render Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: ILL Assignee: koha-bugs@lists.koha-community.org Reporter: pedro.amorim@ptfs-europe.com CC: jonathan.druart+koha@gmail.com, jonathan.field@ptfs-europe.com, katrin.fischer@bsz-bw.de, nick@bywatersolutions.com, pedro.amorim@ptfs-europe.com, tomascohen@gmail.com Depends on: 22440 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). Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22440 [Bug 22440] Improve ILL page performance by moving to server side filtering -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 --- Comment #1 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 151882 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151882&action=edit Bug 33873: Improve check for inexistant biblio object -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 --- Comment #2 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 --- Comment #3 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- 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); -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 --- Comment #4 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 --- Comment #5 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- 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 receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 --- Comment #6 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 --- Comment #7 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 --- Comment #8 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=21983 --- Comment #9 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Please also see bug 21983. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 --- Comment #10 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- (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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |blocker --- Comment #11 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- 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.s...) 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 --- Comment #12 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- 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.s...) 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #151882|0 |1 is obsolete| | --- Comment #13 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 152009 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152009&action=edit Bug 33873: Improve check for inexistant biblio object -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |pedro.amorim@ptfs-europe.co |ity.org |m --- Comment #14 from Tomás Cohen Arazi <tomascohen@gmail.com> --- I followed the test plan and managed to replicate the issue and the patch fixed. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #15 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 --- Comment #16 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 --- Comment #17 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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 :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152009|0 |1 is obsolete| | --- Comment #18 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 152035 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152035&action=edit Bug 33873: Improve check for inexistant biblio object Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #19 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Passing QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|blocker |major -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact| |martin.renvoize@ptfs-europe | |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 --- Comment #20 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Thx all! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |23.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 --- Comment #21 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_22_11_candidate, | |rel_23_05_candidate -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |Pushed to stable CC| |matt.blenkinsop@ptfs-europe | |.com Version(s)|23.11.00 |23.11.00, 23.05.01 released in| | --- Comment #22 from Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> --- Updating to 'Pushed to stable' and adding released version before oldstable rmaint activity -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|23.11.00, 23.05.01 |23.11.00,22.11.07, 23.05.01 released in| | Status|Pushed to stable |Pushed to oldstable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 --- Comment #23 from Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> --- Nice work everyone! Pushed to oldstable for 22.11.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33873 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m Version(s)|23.11.00,22.11.07, 23.05.01 |23.11.00,23.05.01,22.11.07 released in| | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org