|
Description
Fridolin Somers
2021-12-14 06:19:48 UTC
Created attachment 128484 [details] [review] Bug 29690: Add eval on metadata record fetch in details.pl when invalid MARCXML Bug 23846 added support for invalid MARCXML. But now page details.pl fails again with software error. This comes from several Koha::Biblio calling $self->metadata->record without eval. Test plan : 1) Create a biblio record with invalid MARCXML (see Bug 29690) In koha-testing-docker there is biblionumber=369 2) Go to page cgi-bin/koha/catalogue/detail.pl?biblionumber=xxx 3) You see the page with a message : There is an error with this bibliographic record, the view may be degraded. Error: Invalid data, cannot decode metadata object ... If we are OK with this implementation, I will amend unit tests (any help is welcome) Could we not move the eval up a level into the record accessor itself this catching all cases? (In reply to Martin Renvoize from comment #3) > Could we not move the eval up a level into the record accessor itself this > catching all cases? OK, we already do and throw an exception.. hence your evals here.. my mistake Created attachment 128485 [details] [review] Bug 29690: Add eval on metadata record fetch in details.pl when invalid MARCXML Bug 23846 added support for invalid MARCXML. But now page details.pl fails again with software error. This comes from several Koha::Biblio calling $self->metadata->record without eval. Test plan : 1) Create a biblio record with invalid MARCXML (see Bug 29690) In koha-testing-docker there is biblionumber=369 2) Go to page cgi-bin/koha/catalogue/detail.pl?biblionumber=xxx 3) You see the page with a message : There is an error with this bibliographic record, the view may be degraded. Error: Invalid data, cannot decode metadata object ... Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 128486 [details] [review] Bug 29690: (follow-up) Catch other cases This patch catches the rest of the cases in Koha::Biblio where `->metadata->record` may fail. It looks to me like `opac/opac-ISBDdetail.pl` and `opac/opac-showmarc.pl` are also affected by this.. should we fix those here too.. and perhaps introduce a QA script check for any `->metadata->record` call not wrapped in eval or try/catch? (In reply to Martin Renvoize from comment #7) > It looks to me like `opac/opac-ISBDdetail.pl` and `opac/opac-showmarc.pl` > are also affected by this.. should we fix those here too.. and perhaps > introduce a QA script check for any `->metadata->record` call not wrapped in > eval or try/catch? Mmm lets focus for this bug on regression in detail.pl. We may open other reports :) Created attachment 128487 [details] [review] Bug 29690: Unit test for get_components_query Should not we at least warn? There is nothing else more frustrating that investigating a problem and find that it's coming from an exception that is being ignored. (In reply to Jonathan Druart from comment #11) > Should not we at least warn? Probably a good call.. I hate silent failures... do we not already throw a warning from the parent as part of the exception it throws though? It would appear not.. so yes, throwing a warning in would be beneficial. Created attachment 128488 [details] [review] Bug 29690: Always warn if something is wrong with the metadata Would something like this patch work for you, Martin? Looks great to me Jonathan, thanks for taking it on :) Created attachment 128497 [details] [review] Bug 29690: Always warn if something is wrong with the metadata Created attachment 128530 [details] [review] Bug 29690: Add eval on metadata record fetch in details.pl when invalid MARCXML Bug 23846 added support for invalid MARCXML. But now page details.pl fails again with software error. This comes from several Koha::Biblio calling $self->metadata->record without eval. Test plan : 1) Create a biblio record with invalid MARCXML (see Bug 29690) In koha-testing-docker there is biblionumber=369 2) Go to page cgi-bin/koha/catalogue/detail.pl?biblionumber=xxx 3) You see the page with a message : There is an error with this bibliographic record, the view may be degraded. Error: Invalid data, cannot decode metadata object ... Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Created attachment 128531 [details] [review] Bug 29690: (follow-up) Catch other cases This patch catches the rest of the cases in Koha::Biblio where `->metadata->record` may fail. Signed-off-by: David Nind <david@davidnind.com> Created attachment 128532 [details] [review] Bug 29690: Unit test for get_components_query Signed-off-by: David Nind <david@davidnind.com> Created attachment 128533 [details] [review] Bug 29690: Always warn if something is wrong with the metadata Signed-off-by: David Nind <david@davidnind.com> (In reply to Martin Renvoize from comment #9) > Created attachment 128487 [details] [review] [review] > Bug 29690: Unit test for get_components_query Thanks a lot. We need to also impact other unit tests right ? get_coins, is_serial ... Created attachment 128541 [details] [review] Bug 29690: Add selenium test (In reply to Fridolin Somers from comment #22) > (In reply to Martin Renvoize from comment #9) > > Created attachment 128487 [details] [review] [review] [review] > > Bug 29690: Unit test for get_components_query > > Thanks a lot. > We need to also impact other unit tests right ? > get_coins, is_serial ... I actually prefer a selenium test, it will catch possible future regressions. (In reply to Jonathan Druart from comment #24) > (In reply to Fridolin Somers from comment #22) > > (In reply to Martin Renvoize from comment #9) > > > Created attachment 128487 [details] [review] [review] [review] [review] > > > Bug 29690: Unit test for get_components_query > > > > Thanks a lot. > > We need to also impact other unit tests right ? > > get_coins, is_serial ... > > I actually prefer a selenium test, it will catch possible future regressions. Super giga. Created attachment 128578 [details] [review] Bug 29690: (follow-up) Add eval on metadata record fetch in get_marc_authors() (In reply to Fridolin Somers from comment #26) > Created attachment 128578 [details] [review] [review] > Bug 29690: (follow-up) Add eval on metadata record fetch in > get_marc_authors() @Rmaints : When backporting this, skip this patch if branch does not contain Bug 27266 Hello, What needs signing off here? ((In reply to Hayley Pelham from comment #28) > Hello, > > What needs signing off here? Last patch : Bug 29690: (follow-up) Add eval on metadata record fetch in get_marc_authors() I bet one can not sign the Selenuim patch. I gave testing the Selenium test regressions.t a go. It failed, here is the output: kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/selenium/regressions.t t/db_dependent/selenium/regressions.t .. 1/7 Label not found for "last SKIP" at /usr/share/perl/5.28/Test/More.pm line 1370. Use of uninitialized value $value in string eq at /home/vagrant/kohaclone/C4/Context.pm line 423. # Looks like your test exited with 2 just after 7. t/db_dependent/selenium/regressions.t .. Dubious, test returned 2 (wstat 512, 0x200) All 7 subtests passed (less 7 skipped subtests: 0 okay) Test Summary Report ------------------- t/db_dependent/selenium/regressions.t (Wstat: 512 Tests: 7 Failed: 0) Non-zero exit status: 2 Files=1, Tests=7, 2 wallclock secs ( 0.02 usr 0.00 sys + 1.28 cusr 0.17 csys = 1.47 CPU) Result: FAIL I think I set up the Selenium server correctly, here is the output from another Selenium test: kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/selenium/basic_workflow.t t/db_dependent/selenium/basic_workflow.t .. ok All tests successful. Files=1, Tests=22, 1 wallclock secs ( 0.03 usr 0.00 sys + 1.27 cusr 0.11 csys = 1.41 CPU) Please let me know if you think I have missed a step here Cheers, Hayley Hayley, your dev box is not configured correctly for selenium tests. You should first install Selenium::Remote::Driver. However there is a bug as the tests should return green even if the module is not installed. But it's not related to these patches. (In reply to Jonathan Druart from comment #31) > However there is a bug as the tests should return green even if the module > is not installed. But it's not related to these patches. Opened bug 29779. Hi Jonathan, Thanks for letting me know. I've gone through and followed this: https://wiki.koha-community.org/wiki/Using_Selenium_with_Koha And made sure to install that module now. I can get a server running, but get an exception when trying to prove the test: org.openqa.selenium.firefox.NotConnectedException: Unable to connect to host 127.0.0.1 on port 7055 after 45000 ms So I may have to leave the testing here to someone else. Are the Selenium wiki docs up to date? Cheers, Hayley Created attachment 129339 [details] [review] Bug 29690: Add eval on metadata record fetch in details.pl when invalid MARCXML Bug 23846 added support for invalid MARCXML. But now page details.pl fails again with software error. This comes from several Koha::Biblio calling $self->metadata->record without eval. Test plan : 1) Create a biblio record with invalid MARCXML (see Bug 29690) In koha-testing-docker there is biblionumber=369 2) Go to page cgi-bin/koha/catalogue/detail.pl?biblionumber=xxx 3) You see the page with a message : There is an error with this bibliographic record, the view may be degraded. Error: Invalid data, cannot decode metadata object ... Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 129340 [details] [review] Bug 29690: (follow-up) Catch other cases This patch catches the rest of the cases in Koha::Biblio where `->metadata->record` may fail. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 129341 [details] [review] Bug 29690: Unit test for get_components_query Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 129342 [details] [review] Bug 29690: Always warn if something is wrong with the metadata Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 129343 [details] [review] Bug 29690: Add selenium test Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 129344 [details] [review] Bug 29690: (follow-up) Add eval on metadata record fetch in get_marc_authors() Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 129345 [details] [review] Bug 29690: Fix tests count Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> I saw David's signature so was about to PQA it, but it seems it was NSO. Not sure what is the status. Adding my signature as it does the job, looks correct, tests pass and the QA scripts are happy. My only doubt is about Koha::Biblio calling warn... I'm still trying to figure if it should use the logger instead. Thinking a bit more about it. We decided to not explode on detail.pl to provide a nice message to the end user. But we decided to not care about the other views, there is a script already to catch such problems, etc. Here I am wondering if we are not actually fixing the problem at the wrong level. Methods should not deal with the errors (or we are going to add checks everywhere), it's controller's job to catch the error IMO. Minor but worth noting: the patches rename $marc with $record. It should actually be $marc_record I think, $record is too generic. Created attachment 129365 [details] [review] Bug 29690: [ALTERNATIVE PATCH] Prevent detail.pl to crash if MARCXLM in invalid (In reply to Jonathan Druart from comment #44) > Created attachment 129365 [details] [review] [review] > Bug 29690: [ALTERNATIVE PATCH] Prevent detail.pl to crash if MARCXLM in > invalid Tomas, Martin, Fridolin, what about this patch? It would obsolete the other patches except the selenium tests. (In reply to Jonathan Druart from comment #45) > (In reply to Jonathan Druart from comment #44) > > Created attachment 129365 [details] [review] [review] [review] > > Bug 29690: [ALTERNATIVE PATCH] Prevent detail.pl to crash if MARCXLM in > > invalid > > Tomas, Martin, Fridolin, what about this patch? It would obsolete the other > patches except the selenium tests. Hi, I like this shorter patch. We deal with a regression, I think the most simple fix is the best. Created attachment 129381 [details] [review] Bug 29690: Add selenium test Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 129382 [details] [review] Bug 29690: Prevent detail.pl to crash if MARCXLM in invalid Small typo in commit message : "if MARCXLM in invalid" => MARCXML Created attachment 129542 [details] [review] Bug 29690: Add selenium test Run prove t/db_dependent/selenium/regressions.t on KTD with selenium Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> Created attachment 129543 [details] [review] Bug 29690: Prevent detail.pl to crash if MARCXML is invalid Bug 23846 added support for invalid MARCXML. But now page details.pl fails again with software error. This comes from several Koha::Biblio calling $self->metadata->record without eval. Test plan : 1) Create a biblio record with invalid MARCXML (see Bug 29690) In koha-testing-docker there is biblionumber=369 2) Go to page cgi-bin/koha/catalogue/detail.pl?biblionumber=xxx 3) You see the page with a message : There is an error with this bibliographic record, the view may be degraded. Error: Invalid data, cannot decode metadata object ... Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> I signoff. Test successful with koha-testing-docker. Fixed number of tests in selenium test file, from 7 to 8. Copied test plan from old patch to new main patch. Fixed typo in main patch message "if MARCXLM in invalid" Go go team :D Created attachment 129730 [details] [review] Bug 29690: Add selenium test Run prove t/db_dependent/selenium/regressions.t on KTD with selenium Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 129731 [details] [review] Bug 29690: Prevent detail.pl to crash if MARCXML is invalid Bug 23846 added support for invalid MARCXML. But now page details.pl fails again with software error. This comes from several Koha::Biblio calling $self->metadata->record without eval. Test plan : 1) Create a biblio record with invalid MARCXML (see Bug 29690) In koha-testing-docker there is biblionumber=369 2) Go to page cgi-bin/koha/catalogue/detail.pl?biblionumber=xxx 3) You see the page with a message : There is an error with this bibliographic record, the view may be degraded. Error: Invalid data, cannot decode metadata object ... Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Read over the history to remind myself how we got to this point.. all appear to be in order.. Tested. Passing QA Pushed to master for 22.05, thanks to everybody involved [U+1F984] *** Bug 29970 has been marked as a duplicate of this bug. *** Pushed to 21.11.x for 21.11.03 Missing dependency, not backported to 21.05 |