Bug 38963 - Deletion of bibliographic record can cause search errors in OPAC
Summary: Deletion of bibliographic record can cause search errors in OPAC
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Fridolin Somers
QA Contact: Testopia
URL:
Keywords:
Depends on: 37425
Blocks:
  Show dependency treegraph
 
Reported: 2025-01-24 11:13 UTC by Fridolin Somers
Modified: 2025-01-30 18:14 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes searching in the OPAC when the OPACLocalCoverImages system preference is enabled. In some circumstances an error is generated (Can't call method "cover_images" on an undefined value...) when a record is deleted, a search is made, but the search index is not yet updated.
Version(s) released in:
25.05.00
Circulation function:


Attachments
Bug 37425: Check for existence of biblio object before fetching cover images at OPAC (1.61 KB, patch)
2025-01-24 11:31 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 38963: Check for existence of biblio object before fetching cover images in the OPAC (2.04 KB, patch)
2025-01-25 00:05 UTC, David Nind
Details | Diff | Splinter Review
Bug 38963: Check for existence of biblio object before fetching cover images in the OPAC (2.10 KB, patch)
2025-01-28 04:30 UTC, David Cook
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2025-01-24 11:13:19 UTC
Same has Bug 37425 for OPAC search

Bug 35558 added a check to see if the associated biblio has local cover images, however, when a biblio is deleted there is a delay in the background job reindexing the biblio. This can lead to 500 errors when a search retrieves the biblio before it has been removed from the index.
Comment 1 Fridolin Somers 2025-01-24 11:31:12 UTC Comment hidden (obsolete)
Comment 2 David Nind 2025-01-25 00:05:50 UTC
Created attachment 177136 [details] [review]
Bug 38963: Check for existence of biblio object before fetching cover images in the OPAC

This patch adds a conditional to ensure the biblio object has been retrieved
and assumes no cover images otherwise. This is similar to bug 37425 that
fixed the issue for the staff interface.

Test plan:
 1. Enable the OPACLocalCoverImages system preference.
 2. Search in the OPAC (for example, attic).
 3. Find the biblionumber for one of the results and delete it
    from the database using SQL:
     DELETE FROM biblio WHERE biblionumber=3;
 4. Search again.
    NOTE: Steps 3 and 4 need to be done very quickly to generate the error.
    I had the terminal open with the SQL already loaded, and the browser
    also open.
 5. Error trace generated:
     Can't call method "cover_images" on an undefined value at /kohadevbox/koha/opac/opac-search.pl line 702.
 at /kohadevbox/koha/opac/opac-search.pl line 701
 6. Reindex, confirm error is gone: koha-rebuild-zebra -d -f -v kohadev
 7. Apply the patch.
 8. Repeat steps 2 to 4 - I searched for perl and used biblionumber 13.
 9. No error generated
10. Reindex and repeat search and confirm no error.

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 David Nind 2025-01-25 00:07:28 UTC
Comment on attachment 177105 [details] [review]
Bug 37425: Check for existence of biblio object before fetching cover images at OPAC

I had trouble applying the patch, so ended up recreating and adding a test plan adapted from bug 37425.
Comment 4 David Cook 2025-01-28 02:54:48 UTC
Sorry folks but I'm not able to reproduce. I was tempted to PQA it anyway, since it's a reasonable change, and I know that I've had this problem occur in prod before...

But I actually can't get OPACLocalCoverImages to work at all in main. Is it currently working for you folk? I can get a local cover image to appear in the staff interface detail/results and the OPAC detail, but not the OPAC results...

It doesn't look related to this change, but I think I'm going to hold off for the moment...
Comment 5 David Cook 2025-01-28 04:25:26 UTC
Ok I think the problem with the local cover images is because of bug 38981 but that's not directly related to this. It just makes it harder to test...
Comment 6 David Cook 2025-01-28 04:27:17 UTC
Going to QA this one anyway. 

Note that I've fixed up the author so that it's back to Frido again.
Comment 7 David Cook 2025-01-28 04:29:58 UTC
(In reply to David Cook from comment #6)
> Going to QA this one anyway. 
> 
> Note that I've fixed up the author so that it's back to Frido again.

I added a breakpoint in the "verify_cover_images" function on opac-search.pl, so that I was able to verify that the HTML does get created for records that rely on the data being updated by this patch.
Comment 8 David Cook 2025-01-28 04:30:29 UTC
Created attachment 177207 [details] [review]
Bug 38963: Check for existence of biblio object before fetching cover images in the OPAC

This patch adds a conditional to ensure the biblio object has been retrieved
and assumes no cover images otherwise. This is similar to bug 37425 that
fixed the issue for the staff interface.

Test plan:
 1. Enable the OPACLocalCoverImages system preference.
 2. Search in the OPAC (for example, attic).
 3. Find the biblionumber for one of the results and delete it
    from the database using SQL:
     DELETE FROM biblio WHERE biblionumber=3;
 4. Search again.
    NOTE: Steps 3 and 4 need to be done very quickly to generate the error.
    I had the terminal open with the SQL already loaded, and the browser
    also open.
 5. Error trace generated:
     Can't call method "cover_images" on an undefined value at /kohadevbox/koha/opac/opac-search.pl line 702.
 at /kohadevbox/koha/opac/opac-search.pl line 701
 6. Reindex, confirm error is gone: koha-rebuild-zebra -d -f -v kohadev
 7. Apply the patch.
 8. Repeat steps 2 to 4 - I searched for perl and used biblionumber 13.
 9. No error generated
10. Reindex and repeat search and confirm no error.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 9 David Cook 2025-01-28 05:03:32 UTC
Note: While I think that this change is reasonable, since we've already done it before, and it prevents a crash... it's still not quite a full solution.

--

Frido's test plan seemed to require a race condition, but I've found a way to reproduce it without the race.

In koha-testing-docker, without the patch, change OpacHiddenItemsHidesRecord to "Don't hide" and then delete a bib from the DB using SQL.

Do a search that should retrieve the record.

It'll throw a fatal error.

(If you have OpacHiddenItemsHidesRecord enabled, the record will be silently hidden without an error.)

If you apply the patch, the fatal error goes away.

*However*, your deleted bibs will appear in your search results, and if you click on one of them you'll get a 404.
Comment 10 David Cook 2025-01-28 05:05:18 UTC
In theory, it might make more sense to just hide the bib record in the results if it no longer appears in the search results.

It's a bit ugly, but it's no uglier than OpacHiddenItems. 

Then again... you see the same thing on retail websites. Sometimes you click on a result and then you get a "Product not found" message. 

Anyway... there's additional proof that this patch works as expected.
Comment 11 Katrin Fischer 2025-01-30 18:14:18 UTC
Pushed for 25.05!

Well done everyone, thank you!