| Summary: | Rewrite MARC21 mandatory data to YAML | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Bernardo Gonzalez Kriegel <bgkriegel> |
| Component: | I18N/L10N | Assignee: | Bernardo Gonzalez Kriegel <bgkriegel> |
| Status: | CLOSED FIXED | QA Contact: | Jonathan Druart <jonathan.druart> |
| Severity: | enhancement | ||
| Priority: | P5 - low | CC: | 1joynelson, jonathan.druart, martin.renvoize |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| URL: | https://gitlab.com/joubu/Koha/commits/bug_24594 | ||
| Change sponsored?: | --- | Patch complexity: | --- |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: |
20.05.00
|
|
| Circulation function: | |||
| Bug Depends on: | 24262 | ||
| Bug Blocks: | 27852 | ||
| Attachments: |
Bug 24594: Rewrite authorities_normal_marc21 to YAML
Bug 24594: Rewrite marc21_framework_DEFAULT to YAML Bug 24594: Fix license statements Bug 24594: Fix license statements |
||
|
Description
Bernardo Gonzalez Kriegel
2020-02-05 13:15:44 UTC
Created attachment 98502 [details] Bug 24594: Rewrite authorities_normal_marc21 to YAML YAML version of marc21 mandatory authorities_normal_marc21. To test: 1) Do a clean install with mandatory data, dump table auth_types, auth_tag_structure and auth_subfield_structure, reserve. 2) Apply this patch and it's dependencies 3) Do a clean install, dump again and compare No major differences expected 4) Try translation a) Go to misc/translator b) create files for a new language ./translate create xx-YY c) Check new file po/xx-YY-instaler-MARC21.po Verify strings from this file. d) Install new language ./translate install xx-YY e) Try clean install with new files NOTE: This patch fails with qa tools! Reason is that qa tools use YAML.pm module to check files, but Installer.pm or LangInstaller.pm are using YAML::Syck, using that module gives no error, check for example: perl -e 'use YAML::Syck qw( LoadFile ); $y = LoadFile("authorities_normal_marc21.yml");' No error! perl -e 'use YAML qw( LoadFile ); $y = LoadFile("authorities_normal_marc21.yml");' Error! Created attachment 98503 [details] Bug 24594: Rewrite marc21_framework_DEFAULT to YAML YAML version of marc21 mandatory marc21_framework_DEFAULT. To test: 1) Do a clean install with mandatory data, dump table marc_tag_structure and marc_subfield_structure, reserve. 2) Apply this patch and it's dependencies 3) Do a clean install, dump again and compare No major differences expected 4) Try translation a) Go to misc/translator b) create files for a new language ./translate create xx-YY c) Check new file po/xx-YY-instaler-MARC21.po Verify strings from this file. d) Install new language ./translate install xx-YY e) Try clean install with new files NOTE: This patch fails with qa tools! Reason is that qa tools use YAML.pm module to check files, but Installer.pm or LangInstaller.pm are using YAML::Syck, using that module gives no error, check for example: perl -e 'use YAML::Syck qw( LoadFile ); $y = LoadFile("marc21_framework_DEFAULT.yml");' No error! perl -e 'use YAML qw( LoadFile ); $y = LoadFile("marc21_framework_DEFAULT.yml");' Error! I have pushed the whole tree to a remote branch: https://gitlab.com/joubu/Koha/commits/bug_24594 (In reply to Jonathan Druart from comment #3) > I have pushed the whole tree to a remote branch: > https://gitlab.com/joubu/Koha/commits/bug_24594 And the support for YAML files in misc4dev: https://gitlab.com/koha-community/koha-misc4dev/-/merge_requests/37 Bernardo, I am letting here the QA notes for the whole stack (I rebased my remote branch btw, there were 2 minor conflicts due to copyright changes).
1. There are some QA issues:
FAIL installer/data/mysql/en/marcflavour/marc21/mandatory/authorities_normal_marc21.yml
FAIL yaml_valid YAML Error: Inconsistent indentation level
FAIL installer/data/mysql/en/marcflavour/marc21/mandatory/marc21_framework_DEFAULT.yml
FAIL yaml_valid YAML Error: Inconsistent indentation level
FAIL installer/data/mysql/en/marcflavour/marc21/optional/marc21_default_matching_rules.yml
FAIL yaml_valid YAML Error: Inconsistent indentation level
FAIL installer/data/mysql/en/marcflavour/marc21/optional/marc21_sample_fastadd_framework.yml
FAIL yaml_valid YAML Error: Inconsistent indentation level
FAIL installer/data/mysql/en/marcflavour/marc21/optional/marc21_simple_bib_frameworks.yml
FAIL yaml_valid YAML Error: Inconsistent indentation level
2. I did an install 'en' before and after the patchset, I mysqldump --tab to generate 2 files per table, then compared the 2 generated directories with `diff -r --brief` (and also a `perl -p -i -e "s/.*Dump completed.*//" **/*` to remove timestamp of the dump).
Most of them are wrong diff because of different timestamps, but some of them are either weird, or caught issues:
a) When a row is on several lines, I get, for instance with letter "TO_PROCESS":
652,657c652,657
\ suggestions TO_PROCESS Notify fund owner 0 A suggestion is ready to be processed Dear <<borrowers.firstname>> <<borrowers.surname>>,
\
\ A new suggestion is ready to be processed: <<suggestions.title>> by <<suggestions.author>>.
\
\ Thank you,
\
---
> suggestions TO_PROCESS Notify fund owner 0 A suggestion is ready to be processed Dear <<borrowers.firstname>> <<borrowers.surname>>,\
> \
> A new suggestion is ready to be processed: <<suggestions.title>> by <<suggestions.author>>.\
> \
> Thank you,\
> \
Without the patch:
656 Thank you,\
With the patch:
656 Thank you,^M\
I am not sure how relevant is this, the UI looks correct.
b) matcher_matchpoints is different:
without:
1 1
2 2
3 3
with:
3 1
3 2
3 3
c) false positive: search_field, search_marc_to_field and search_marc_map (because of id loaded in a different way), this is coming from:
installer/install.pl: Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings;
Not related to this patchset.
d) subscription_numberpatterns (to investigate)
1c1
< 1 Number 1 Simple Numbering method No. {X} Number 1 1 99999 1 \N \N \N \N \N \N \N \N \N \N \N
---
> 1 Number 1 Simple Numbering method No. {X} Number 1 1 99999 1 \N \N \N \N \N \N \N \N \N \N \N \N \N
3,4c3,4
< 3 Volume, Number 3 Volume Number 1 Vol. {X}, No. {Y} Volume 1 12 99999 1 \N Number 1 1 12 1 \N \N \N \N \N \N
< 4 Seasonal 4 Season Year {X} {Y} Season 1 1 3 0 season Year 1 4 99999 1 \N \N \N \N \N \N
---
> 3 Volume, Number 3 Volume Number 1 Vol. {X}, No. {Y} Volume 1 12 99999 1 \N Number 1 1 12 1 \N \N \N \N \N \N \N
> 4 Seasonal 4 Season Year {X} {Y} Season 1 1 3 0 season Year 1 4 99999 1 \N \N \N \N \N \N \N
e) systempreferences (to ignore)
There is an obvious change here on "FrameworksLoaded", the list of sql files is replaced with a list of yml files.
I did not know this pref existed, it seems to be used to make sure we are not going to load twice the same files.
We can ignore this one.
Hi Jonathan, my reply (In reply to Jonathan Druart from comment #5) > 1. There are some QA issues: > FAIL > installer/data/mysql/en/marcflavour/marc21/mandatory/ > authorities_normal_marc21.yml > > FAIL yaml_valid > YAML Error: Inconsistent indentation level Yes, I was aware of this, I left notes on the commit messages. Current QA test uses YAML.pm, it considers as valid this: --- data: - "a b c d" ... but not this: --- data: - "a b c d" ... however it is valid :( The problem arises in some multiline sql_statements. The solution is to put them as long single lines. It makes readability difficult, but I can do it. Using YAML::Sick or YAML::XS gives no error, but implies changes in QA tools. As I said, I can change them if you prefer. > > 2. I did an install 'en' before and after the patchset, I mysqldump --tab to > generate 2 files per table, then compared the 2 generated directories with > `diff -r --brief` (and also a `perl -p -i -e "s/.*Dump completed.*//" **/*` > to remove timestamp of the dump). > Most of them are wrong diff because of different timestamps, but some of > them are either weird, or caught issues: > > a) When a row is on several lines, I get, for instance with letter > "TO_PROCESS": > 652,657c652,657 > \ suggestions TO_PROCESS Notify fund owner 0 A suggestion is ready to be > processed Dear <<borrowers.firstname>> <<borrowers.surname>>, > \ > \ A new suggestion is ready to be processed: <<suggestions.title>> by > <<suggestions.author>>. > \ > \ Thank you, > \ > --- > > suggestions TO_PROCESS Notify fund owner 0 A suggestion is ready to be processed Dear <<borrowers.firstname>> <<borrowers.surname>>,\ > > \ > > A new suggestion is ready to be processed: <<suggestions.title>> by <<suggestions.author>>.\ > > \ > > Thank you,\ > > \ > > Without the patch: > 656 Thank you,\ > With the patch: > 656 Thank you,^M\ > > I am not sure how relevant is this, the UI looks correct. Some of the sample notices where clearly generated in Windows, I assume this because they have '\r\n' for newlines. Some others not. So I decided to join all multiline fields with both chars. I left a comment on the last patch of Bug 13897 "'\r\n' is used for correct display in Windows clients." in the code "? join "\r\n", @{$row->{$col}} # join multiline values" The diff you see comes from those notices that originally have only '\n' for newlines. > > b) matcher_matchpoints is different: > without: > 1 1 > 2 2 > 3 3 > with: > 3 1 > 3 2 > 3 3 Yes, my bad. The sql_statements make use of MAX() to insert values, then the result depends on the order of insertions. I will add a followup to Bug 24593 to fix the problem. > > d) subscription_numberpatterns (to investigate) > > 1c1 > < 1 Number 1 Simple Numbering method No. {X} Number 1 1 99999 1 \N \N \N \N > \N \N \N \N \N \N \N > --- > > 1 Number 1 Simple Numbering method No. {X} Number 1 1 99999 1 \N \N \N \N \N \N \N \N \N \N \N \N \N ... > The difference in this case comes from the values of label2 & label3 fields. In the SQL files some of these fields are NULL, in YAML they can't because they are translated, and the translate script will fail, so I put on them an empty string "". I think that this change does not affect the numbered patterns. If a NULL value is needed, then I must change the translate script (LangInstaller.pm) > e) systempreferences (to ignore) > There is an obvious change here on "FrameworksLoaded", the list of sql files > is replaced with a list of yml files. > I did not know this pref existed, it seems to be used to make sure we are > not going to load twice the same files. > We can ignore this one. Ok! Summary: 1) Can be rewritten to single lines if needed 2a) I think is not a problem 2b) Followup uploaded to Bug 24593 2d) Waiting your opinion Thanks! (In reply to Bernardo Gonzalez Kriegel from comment #6) > Summary: > 1) Can be rewritten to single lines if needed Nope, that makes sense! > 2a) I think is not a problem If I am not wrong, the ^W appeared *with* the patches. > 2b) Followup uploaded to Bug 24593 Thanks! > 2d) Waiting your opinion How big would it be to allow NULL for translated value? If it can be a quick fix I guess it's better to have it. (In reply to Jonathan Druart from comment #7) > (In reply to Bernardo Gonzalez Kriegel from comment #6) > > > Summary: > > 1) Can be rewritten to single lines if needed > > Nope, that makes sense! Ok > > > 2a) I think is not a problem > > If I am not wrong, the ^W appeared *with* the patches. You mean ^M? Look for example the RENEWAL letter in your dump without patch. It's a little notice with at least 4 ^M In sample_notices.sql you can see that the first letter, ODUE, has '\n' for newlines; the second one, ACQCLAIM, has '\r\n' for the same. I need to use only one, and '\r\n' works for win/linux > > > 2b) Followup uploaded to Bug 24593 > > Thanks! > > > 2d) Waiting your opinion > > How big would it be to allow NULL for translated value? If it can be a quick > fix I guess it's better to have it. Not a big problem :) Followup added to Bug 24262 to skip null values and followup added to Bug 24583 to restore nulls in labels I have PQAed the whole stack, see the remote branch. Signed off patched for this bug report are there. Nice work everyone! Pushed to master for 20.05 Created attachment 100388 [details] [review] Bug 24594: Fix license statements Created attachment 100390 [details] [review] Bug 24594: Fix license statements Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Nice work everyone! Pushed to master for 20.05 not backported to 19.11.x |