Bug 11329 - Check for marc record in opac-showmarc
Summary: Check for marc record in opac-showmarc
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low trivial (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-02 08:37 UTC by Marcel de Rooy
Modified: 2015-06-04 23:23 UTC (History)
6 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:


Attachments
Bug 11329: Check for marc record in opac-showmarc (1.10 KB, patch)
2013-12-02 08:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11329: Check for marc record in catalogue/showmarc (987 bytes, patch)
2013-12-02 08:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11329: Check for marc record in opac-showmarc (1.11 KB, patch)
2014-01-15 17:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11329: Check for marc record in catalogue/showmarc (994 bytes, patch)
2014-01-15 17:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11329: Check for marc record in opac-showmarc (1.18 KB, patch)
2014-01-17 14:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11329: Check for marc record in opac-showmarc (1.18 KB, patch)
2014-01-17 14:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 11329: Check for marc record in catalogue/showmarc (1.08 KB, patch)
2014-01-17 14:19 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2013-12-02 08:37:33 UTC
Instead of just running as_formatted, check if GetMarcBiblio returned
a reference. If you e.g. did not pass an id, return 404 instead of 500.
Comment 1 Marcel de Rooy 2013-12-02 08:40:13 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2013-12-02 08:47:17 UTC Comment hidden (obsolete)
Comment 3 David Cook 2014-01-14 04:55:01 UTC
Marcel, why do you think that these patches are necessary?

I suppose, in the case of the OPAC, it might make sense to return a 404 instead of a 500, but when is an opac-showmarc.pl query string going to be malformed?

In the case of the staff client, I got a 404 both before/after trying the patch, so I'm not sure that it's necessary. I imagine that there must already be checking higher up in showmarc.pl?
Comment 4 Marcel de Rooy 2014-01-15 12:29:54 UTC
(In reply to David Cook from comment #3)
> Marcel, why do you think that these patches are necessary?
Hi David. Thanks for testing.
They are not that important :) But I would like to reduce the 500s in my log (especially from opac, mainly coming from Googlebot).
Testing if something is what it should be, cannot hurt BTW :)
 
> I suppose, in the case of the OPAC, it might make sense to return a 404
> instead of a 500, but when is an opac-showmarc.pl query string going to be
> malformed?
That is indeed somewhat less likely, but not imaginary.
1) Someone saved a URL. The record does no longer exist.
2) User 1 deleted the record, Zebra has not been updated yet. User 2 clicks on the link.
3) If the marcxml of a record is malformed (due to some earlier error or so), GetMarcBiblio returns undef. We should check that and not assume a valid MARC::Record object.

> In the case of the staff client, I got a 404 both before/after trying the
> patch, so I'm not sure that it's necessary. I imagine that there must
> already be checking higher up in showmarc.pl?
Really? I got the following 500 error on an wrong biblio number:
Can't call method "as_formatted" on an undefined value at /usr/share/koha/testclone/catalogue/showmarc.pl line 82.
You should be able to repeat this error in current master..
Comment 5 Jonathan Druart 2014-01-15 17:21:59 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2014-01-15 17:22:08 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2014-01-15 17:24:13 UTC
It work well for me, i am redirected to 404.
It is not the best, I think we should catch the error and display a suitable warning. But it improves the actual state.
Comment 8 Kyle M Hall 2014-01-17 14:18:30 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2014-01-17 14:18:37 UTC
Created attachment 24501 [details] [review]
Bug 11329: Check for marc record in opac-showmarc

Instead of just running as_formatted, check if GetMarcBiblio returned
a reference. If you e.g. did not pass an id, return 404 instead of 500.
Consistent with opac-export.pl

Test plan:
[1] Run opac-showmarc.pl with valid biblionumber in id parameter.
[2] Remove id parameter from URL. You should get a 404 now.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Kyle M Hall 2014-01-17 14:19:23 UTC
I think it would be less confusing for end-users if we had a "no record found" error instead of a generic 404, but this works.
Comment 11 Kyle M Hall 2014-01-17 14:19:41 UTC
Created attachment 24502 [details] [review]
Bug 11329: Check for marc record in catalogue/showmarc

Intranet counterpart patch. Same check as in opac/opac-showmarc.

Test plan:
[1] Run showmarc.pl with valid biblionumber in id parameter.
[2] Remove id parameter from URL. You should get a 404 now.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Passes koha-qa.pl, works as advertised.
Comment 12 Galen Charlton 2014-01-17 15:59:29 UTC
Pushed to master.  Thanks, Marcel!

Note bug 11581.
Comment 13 Fridolin Somers 2014-03-25 11:18:45 UTC
Patches pushed to 3.14.x, will be in 3.14.5
Comment 14 Chris Cormack 2014-03-26 07:14:51 UTC
Pushed to 3.12.x will be in 3.12.13