|
Description
George Veranis
2021-03-12 13:47:03 UTC
Created attachment 122549 [details] [review] Bug 27943: add support for 7XX equal terms on authorities marc21 display Currently when a MARC21 authority has 7XX field they do not display on authority search result page. 7XX is equal term of 1XX based on LC rules. Applying this patch will enable that feature, which is important for non latin catalogues. Test plan: 1) Add a new authority with 1XX and 7XX fields. 2) Try to search that authority and check if 7XX values are displayed 3) Apply this patch 4) Try to search that authority and you will see that 7XX values are displayed like "1XX = 7XX" Sponsored-by: Keratsini-Drapetsona Municipal Library, Greece Mentored-by: Andreas Roussos Created attachment 132778 [details] [review] Bug 27943: add support for 7XX equal terms on authorities marc21 display Currently when a MARC21 authority has 7XX field they do not display on authority search result page. 7XX is equal term of 1XX based on LC rules. Applying this patch will enable that feature, which is important for non latin catalogues. Test plan: 1) Add a new authority with 1XX and 7XX fields. 2) Try to search that authority and check if 7XX values are displayed 3) Apply this patch 4) Try to search that authority and you will see that 7XX values are displayed like "1XX = 7XX" Sponsored-by: Keratsini-Drapetsona Municipal Library, Greece Mentored-by: Andreas Roussos Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se> Works for me! Created attachment 133712 [details] [review] Bug 27943: (QA follow-up) Add missing TT filters I have added a follow-up to fix the missing TT filters, but there are 2 issues that are a bit more than QA can fix:
1) Tests are failing
Changes to any core routines in C4 and Koha need to be accompanied by unit tests per the coding guidelines, but there is a little more here as the existing tests fail with the patches applied:
prove t/db_dependent/AuthoritiesMarc.t
t/db_dependent/AuthoritiesMarc.t .. 2/12
# Failed test 'test BuildSummary for MARC21'
# at t/db_dependent/AuthoritiesMarc.t line 158.
# Structures begin differing at:
# $got->{equalterm} = ARRAY(0x563fe77a47f0)
# $expected->{equalterm} = Does not exist
# Failed test 'test BuildSummary for UNIMARC'
# at t/db_dependent/AuthoritiesMarc.t line 202.
# Structures begin differing at:
# $got->{equalterm} = ARRAY(0x563fe36e6b00)
# $expected->{equalterm} = Does not exist
Cannot determine authority type for record: 1710 at /kohadevbox/koha/Koha/SearchEngine/Elasticsearch.pm line 566.
t/db_dependent/AuthoritiesMarc.t .. 12/12 # Looks like you failed 2 tests of 12.
t/db_dependent/AuthoritiesMarc.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/12 subtests
Test Summary Report
-------------------
t/db_dependent/AuthoritiesMarc.t (Wstat: 512 Tests: 12 Failed: 2)
Failed tests: 6, 8
Non-zero exit status: 2
2) Hardcode delimiter
This hardcodes the delimiter for subheadings to "--", but I believe it should be using the system preference AuthoritySeparator instead.
Please let us know if you need help in fixing those.
My testing record had 2 700 fields, as the field is repeatable. It works nicely for display. Hi Katrin, thanks for the review of that bug I will try to do the changes that you proposed . I'm not sure about the unit tests but I will check it to be compatible with that. Created attachment 146945 [details] [review] Bug 27943: (follow-up) Fix unit test, and use systempreference AuthoritySeparator Instead of hardcoding the separator we take the value from systempreference. Also adjust the unit test to expect the "equalterm" array. Test plan same as before. Created attachment 154502 [details] [review] Bug 27943: (follow-up) Fix unit test, and use systempreference AuthoritySeparator Instead of hardcoding the separator we take the value from systempreference. Also adjust the unit test to expect the "equalterm" array. Test plan same as before. Signed-off-by: Christian Nelson <christian.nelson@uwasa.fi> Created attachment 157173 [details] [review] Bug 27943: add support for 7XX equal terms on authorities marc21 display Currently when a MARC21 authority has 7XX field they do not display on authority search result page. 7XX is equal term of 1XX based on LC rules. Applying this patch will enable that feature, which is important for non latin catalogues. Test plan: 1) Add a new authority with 1XX and 7XX fields. 2) Try to search that authority and check if 7XX values are displayed 3) Apply this patch 4) Try to search that authority and you will see that 7XX values are displayed like "1XX = 7XX" Sponsored-by: Keratsini-Drapetsona Municipal Library, Greece Mentored-by: Andreas Roussos Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se> Works for me! Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 157174 [details] [review] Bug 27943: (QA follow-up) Add missing TT filters Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 157175 [details] [review] Bug 27943: (follow-up) Fix unit test, and use systempreference AuthoritySeparator Instead of hardcoding the separator we take the value from systempreference. Also adjust the unit test to expect the "equalterm" array. Test plan same as before. Signed-off-by: Christian Nelson <christian.nelson@uwasa.fi> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 157176 [details] [review] Bug 27943: (QA follow-up) Perltidy I have some trouble to get the tidyness test on AuthoritiesMarc.pm passing. There are several outstanding patches touching this file, so I didn't dare to perltidy the whole file just yet. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 157177 [details] [review] Bug 27943: (QA follow-up) Alternate display in result lists The original display put the equivalent heading right after the main heading with a = as separator. As the 7xx are repeatable that could get croweded easily, also if the terms are longer we might see not so nice breakage. This puts every 7xx on its own line below the main heading with some indentation. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Pushed to master for 23.11. Nice work everyone, thanks! Enhancement not pushed to 23.05.x |