Summary: | Check for marc record in opac-showmarc | ||
---|---|---|---|
Product: | Koha | Reporter: | Marcel de Rooy <m.de.rooy> |
Component: | OPAC | Assignee: | Marcel de Rooy <m.de.rooy> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | trivial | ||
Priority: | P5 - low | CC: | chris, dcook, fridolin.somers, gmcharlt, jonathan.druart, kyle |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Trivial patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Attachments: |
Bug 11329: Check for marc record in opac-showmarc
Bug 11329: Check for marc record in catalogue/showmarc Bug 11329: Check for marc record in opac-showmarc Bug 11329: Check for marc record in catalogue/showmarc Bug 11329: Check for marc record in opac-showmarc Bug 11329: Check for marc record in opac-showmarc Bug 11329: Check for marc record in catalogue/showmarc |
Description
Marcel de Rooy
2013-12-02 08:37:33 UTC
Created attachment 23231 [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. Created attachment 23232 [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. 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? (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.. Created attachment 24417 [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> Created attachment 24418 [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> 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. Created attachment 24500 [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> 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> 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. 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. Pushed to master. Thanks, Marcel! Note bug 11581. Patches pushed to 3.14.x, will be in 3.14.5 Pushed to 3.12.x will be in 3.12.13 |