|
Description
Nick Clemens (kidclamp)
2023-04-04 13:18:46 UTC
Created attachment 151510 [details] [review] Bug 33404: (bug 19436 follow-up) Save the original encoded record This patch restores previous behavior of storing the record as received from the source with the original encoding. We store the encoding in the DB as well, but save the record converted to UTF8 - this means that when we try to display and convert to UTF8 again, we mangle the record While providing a test plan, it requires a MARC8 Authority source, and I cannot locate an open one. This change can be verified by reading the code. There are no unit tests for Z3950SearchAuth, but it does too much and requires many mocks, this is a small fix to a reversion and should be able to move forward. To test: 1 - Search a Z39 authority server that has MARC8 (or non utf8) records 2 - Find a record with diacritics in the record 3 - Import it 4 - Note diacritics are mangled 5 - Apply patch, restart_all 6 - Perform a new Z3950 search (to ensure record is not already in reservoir) 7 - Find a record with diacritics 8 - Import it 9 - Note diacritics correctly encoded Added assignee 8-) Looking here Can't locate object method "field" via package "00554cz a2200157n 4500001001300000003000400013005001700017008004100034010001700075040001800092100006800110400006200178400005600240400005000296670005000346n 00027419 DLC20000914083028.0000914n| acannaabn |a aaa an 00027419 aDLCbengcDLC1 aHacikyan, A. J.q(Agop Jack),d1931-tEté sans aube.lEnglish1 aHacikyan, A. J.q(Agop Jack),d1931-tSummer without dawn1 aSoucy, Jean Yves,d1945-tEté sans aube.lEnglish1 aSoucy, Jean Yves,d1945-tSummer without dawn aHacikyan, A. J. A summer without dawn, c2000." (perhaps you forgot to load "00554cz a2200157n 4500001001300000003000400013005001700017008004100034010001700075040001800092100006800110400006200178400005600240400005000296670005000346n 00027419 DLC20000914083028.0000914n| acannaabn |a aaa an 00027419 aDLCbengcDLC1 aHacikyan, A. J.q(Agop Jack),d1931-tEté sans aube.lEnglish1 aHacikyan, A. J.q(Agop Jack),d1931-tSummer without dawn1 aSoucy, Jean Yves,d1945-tEté sans aube.lEnglish1 aSoucy, Jean Yves,d1945-tSummer without dawn aHacikyan, A. J. A summer without dawn, c2000."?) at /usr/share/koha/C4/Breeding.pm line 479
in C4::Breeding::ImportBreedingAuth at /usr/share/koha/C4/Breeding.pm line 479
$marcdata is no marc record ! It is a marc blob.
How did you manage to test it?
The changes do not look good to me. We should not save in other encodings. Everywhere we convert to UTF-8. Here we should too. When I compare BIB with AUTH in Z3950Search, we also save bib records in UTF8. Note that there are some subtle differences. For instance the moment when we call SetUTF8Flag. This needs more attention. (In reply to Nick Clemens from comment #1) > While providing a test plan, it requires a MARC8 Authority source, and I > cannot locate > an open one. This change can be verified by reading the code. This sounds problematic. > There are no unit tests for Z3950SearchAuth, but it does too much and > requires many mocks, > this is a small fix to a reversion and should be able to move forward. And this too :) Created attachment 151624 [details] [review] Bug 33404: (bug 19436 follow-up) Save the original encoded record This patch restores previous behavior of storing the record as received from the source with the original encoding. We store the encoding in the DB as well, but save the record converted to UTF8 - this means that when we try to display and convert to UTF8 again, we mangle the record While providing a test plan, it requires a MARC8 Authority source, and I cannot locate an open one. This change can be verified by reading the code. There are no unit tests for Z3950SearchAuth, but it does too much and requires many mocks, this is a small fix to a reversion and should be able to move forward. To test: 1 - Search a Z39 authority server that has MARC8 (or non utf8) records 2 - Find a record with diacritics in the record 3 - Import it 4 - Note diacritics are mangled 5 - Apply patch, restart_all 6 - Perform a new Z3950 search (to ensure record is not already in reservoir) 7 - Find a record with diacritics 8 - Import it 9 - Note diacritics correctly encoded (In reply to Marcel de Rooy from comment #5) > The changes do not look good to me. We should not save in other encodings. > Everywhere we convert to UTF-8. Here we should too. I am happy to go this direction if preferred (In reply to Marcel de Rooy from comment #4) > $marcdata is no marc record ! It is a marc blob. > > How did you manage to test it? I forgot to commit changes after testing :-) New patch retested and verified against OCLC MARC8 records (In reply to Marcel de Rooy from comment #6) > (In reply to Nick Clemens from comment #1) > > While providing a test plan, it requires a MARC8 Authority source, and I > > cannot locate > > an open one. This change can be verified by reading the code. > > This sounds problematic. If you can find a source I would appreciate it > > There are no unit tests for Z3950SearchAuth, but it does too much and > > requires many mocks, > > this is a small fix to a reversion and should be able to move forward. > > And this too :) Yes, but we should not allow bugs to remain when we have a fix, but not the time to pay technical debts. Created attachment 151636 [details] [review] Bug 33404: Alternative approach Work in progress Created attachment 151641 [details] [review] Bug 33404: Alternative approach Work in progress Signed-off-by: Nick Clemens <nick@bywatersolutions.com> This (In reply to Marcel de Rooy from comment #9) > Created attachment 151636 [details] [review] [review] > Bug 33404: Alternative approach > > Work in progress This works better - sometimes mine worked, and then sometimes I was losing the heading fields, not sure why. I would attach a sample MARC-8 - but I can't get one out of the system without it being mangled - I exported the record I used for testing as marc 8 and will attach if that helps Created attachment 151644 [details]
Sample record
(In reply to Nick Clemens from comment #12) > Created attachment 151644 [details] > Sample record Thx. Will try to incorporate in a test. Created attachment 151672 [details] [review] Bug 33404: Z3950SearchAuth: Save in UTF-8 encoding Test plan: [1] If you have access to a Z3950 MARC8 auth server, search for an authority record and import it. [2] If you have access to a Z3950 UTF8 auth server, search for an authority record and import it. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 151673 [details] [review] Bug 33404: Fix serverhost and init $page The array serverhost is not filled. Should be replaced with values from servers array. Test plan: Nothing exciting here. Read the patch. Note that we will test in the next patch if the hostname is saved correctly in the import batch. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 151674 [details] [review] Bug 33404: Add unit tests Test plan: Run t/db_dependent/Breeding.t Run t/db_dependent/Breeding_Auth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Note for QA: The Z3950SearchAuth test can be extended further later on. And used as a basis for Z3950Search too. We need to consolidate the duplicated code. (In reply to Marcel de Rooy from comment #13) > (In reply to Nick Clemens from comment #12) > > Created attachment 151644 [details] > > Sample record > > Thx. Will try to incorporate in a test. Nick, there is an issue with this sample record. I could import it but I could not use it in my test. I have the impression that there is an issue with exporting authority records from Koha in MARC8? Raising sev to major Created attachment 151868 [details] [review] Bug 33404: Z3950SearchAuth: Save in UTF-8 encoding Test plan: [1] If you have access to a Z3950 MARC8 auth server, search for an authority record and import it. [2] If you have access to a Z3950 UTF8 auth server, search for an authority record and import it. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 151869 [details] [review] Bug 33404: Fix serverhost and init $page The array serverhost is not filled. Should be replaced with values from servers array. Test plan: Nothing exciting here. Read the patch. Note that we will test in the next patch if the hostname is saved correctly in the import batch. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 151870 [details] [review] Bug 33404: Add unit tests Test plan: Run t/db_dependent/Breeding.t Run t/db_dependent/Breeding_Auth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 153014 [details] [review] Bug 33404: Z3950SearchAuth: Save in UTF-8 encoding Test plan: [1] If you have access to a Z3950 MARC8 auth server, search for an authority record and import it. [2] If you have access to a Z3950 UTF8 auth server, search for an authority record and import it. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 153015 [details] [review] Bug 33404: Fix serverhost and init $page The array serverhost is not filled. Should be replaced with values from servers array. Test plan: Nothing exciting here. Read the patch. Note that we will test in the next patch if the hostname is saved correctly in the import batch. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 153016 [details] [review] Bug 33404: Add unit tests Test plan: Run t/db_dependent/Breeding.t Run t/db_dependent/Breeding_Auth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Amended patch: perltidy Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Pushed to master for 23.11. Nice work everyone, thanks! Thanks for all the hard work! Pushed to 23.05.x for the next release Nice work everyone! Pushed to oldstable for 22.11.x Backported to 22.05.x for upcoming 22.05.17 release. |