Bug 24594 - Rewrite MARC21 mandatory data to YAML
Summary: Rewrite MARC21 mandatory data to YAML
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: I18N/L10N (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bernardo Gonzalez Kriegel
QA Contact: Jonathan Druart
URL: https://gitlab.com/joubu/Koha/commits...
Keywords:
Depends on: 24262
Blocks:
  Show dependency treegraph
 
Reported: 2020-02-05 13:15 UTC by Bernardo Gonzalez Kriegel
Modified: 2020-04-15 15:27 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 24594: Rewrite authorities_normal_marc21 to YAML (47.18 KB, application/octet-stream)
2020-02-05 15:14 UTC, Bernardo Gonzalez Kriegel
Details
Bug 24594: Rewrite marc21_framework_DEFAULT to YAML (110.21 KB, application/octet-stream)
2020-02-05 15:15 UTC, Bernardo Gonzalez Kriegel
Details
Bug 24594: Fix license statements (45.31 KB, patch)
2020-03-09 15:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24594: Fix license statements (45.37 KB, patch)
2020-03-09 15:18 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Bernardo Gonzalez Kriegel 2020-02-05 13:15:44 UTC
A complete rewrite of MARC21 mandatory instalation files from SQL to YAML is presented, one patch for each file, the result could be squashed.

The goal is to allow the translation of the installation files

In this case the patches are too big, they will be uploaded compressed.
Comment 1 Bernardo Gonzalez Kriegel 2020-02-05 15:14:26 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!
Comment 2 Bernardo Gonzalez Kriegel 2020-02-05 15:15:50 UTC
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!
Comment 3 Jonathan Druart 2020-02-12 10:06:25 UTC
I have pushed the whole tree to a remote branch: https://gitlab.com/joubu/Koha/commits/bug_24594
Comment 4 Jonathan Druart 2020-02-12 11:35:02 UTC
(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
Comment 5 Jonathan Druart 2020-03-04 10:59:52 UTC
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.
Comment 6 Bernardo Gonzalez Kriegel 2020-03-04 16:04:23 UTC
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!
Comment 7 Jonathan Druart 2020-03-05 11:06:02 UTC
(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.
Comment 8 Bernardo Gonzalez Kriegel 2020-03-05 14:12:13 UTC
(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
Comment 9 Jonathan Druart 2020-03-09 12:24:37 UTC
I have PQAed the whole stack, see the remote branch.
Signed off patched for this bug report are there.
Comment 10 Martin Renvoize 2020-03-09 14:12:43 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 11 Jonathan Druart 2020-03-09 15:10:21 UTC
Created attachment 100388 [details] [review]
Bug 24594: Fix license statements
Comment 12 Martin Renvoize 2020-03-09 15:18:12 UTC
Created attachment 100390 [details] [review]
Bug 24594: Fix license statements

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2020-03-09 15:19:55 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 14 Joy Nelson 2020-04-02 18:52:29 UTC
not backported to 19.11.x