Bug 13897 - Use YAML files for installer data
Summary: Use YAML files for installer data
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:
Keywords:
Depends on: 7939 10963 13322
Blocks: 24262 24712 24904 24973
  Show dependency treegraph
 
Reported: 2015-03-24 00:40 UTC by Bernardo Gonzalez Kriegel
Modified: 2020-04-15 15:25 UTC (History)
10 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 13897 Patchset (168.85 KB, application/x-bzip)
2015-03-24 01:46 UTC, Bernardo Gonzalez Kriegel
Details
Bug 13897: C4/Installer.pm - process XML files (2.20 KB, patch)
2018-06-08 17:08 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 13897: Split auth framework into xml and sql files (38.90 KB, application/gzip)
2018-06-08 17:12 UTC, Bernardo Gonzalez Kriegel
Details
Bug 13897: Use XML files for installer data (20.46 KB, patch)
2019-11-29 15:10 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 13897: Use YAML files for installer data (21.65 KB, patch)
2019-12-17 13:21 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 13897: Catch the error and warn it (712 bytes, patch)
2019-12-18 08:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13897: Use YAML files for installer data (20.11 KB, patch)
2019-12-20 19:13 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 13897: Use YAML files for installer data (22.05 KB, patch)
2019-12-23 01:06 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 13897: Catch the error and warn it (712 bytes, patch)
2019-12-23 01:06 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 13897: Use YAML files for installer data (22.11 KB, patch)
2020-01-02 14:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 13897: Catch the error and warn it (777 bytes, patch)
2020-01-02 14:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 13897: Use YAML files for installer data (22.17 KB, patch)
2020-01-09 12:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13897: Catch the error and warn it (832 bytes, patch)
2020-01-09 12:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13897: Process description, multiline values and SQL statements in YAML files (6.04 KB, patch)
2020-02-05 15:24 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 13897: Surround columns with backticks (1.32 KB, patch)
2020-02-12 10:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13897: Use YAML files for installer data (22.21 KB, patch)
2020-02-21 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 13897: Catch the error and warn it (897 bytes, patch)
2020-02-21 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 13897: Process description, multiline values and SQL statements in YAML files (6.11 KB, patch)
2020-02-21 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 13897: Surround columns with backticks (1.39 KB, patch)
2020-02-21 16:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 13897: Use YAML files for installer data (22.28 KB, patch)
2020-03-09 12:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13897: Catch the error and warn it (971 bytes, patch)
2020-03-09 12:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13897: Process description, multiline values and SQL statements in YAML files (6.18 KB, patch)
2020-03-09 12:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13897: Surround columns with backticks (1.46 KB, patch)
2020-03-09 12:18 UTC, Jonathan Druart
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 2015-03-24 00:40:32 UTC
Installer data on main/marc21/unimarc SQL files is difficult to translate and is prone to get off-sync with english original

As a way to overcome this problem, I propose to use XML files from which corresponding SQL and TXT files could be created. 

XML files can be translated in the usual way, and all installer data can be created/translated before install time.

There will be no need to preserve language folders, except some particular cases (e.g. NORMARC)

An agreement will be needed for a superset of the all sample data on different language folders
Comment 1 Bernardo Gonzalez Kriegel 2015-03-24 00:51:50 UTC
I'll make this enh dependent on 7939 10963 13322.

Bug 7939 modifies LangInstaller.pm
Bug 10963 rewrites MARC21 sample bib frameworks, no need to translate them
Bug 13322 updates MARC21 default bib framework

All signed ATM
Comment 2 Bernardo Gonzalez Kriegel 2015-03-24 01:46:30 UTC
Created attachment 37165 [details]
Bug 13897 Patchset

Test implementation patchset
Only cover mysql mandatory/optional and marc21 mandatory, just as example
Comment 3 Zeno Tajoli 2015-03-25 09:55:26 UTC
Bernardo, can you describe 'the usual way' to transalte  XML files ?
Do you plan to use a .po file ?
Comment 4 Bernardo Gonzalez Kriegel 2015-03-25 13:09:13 UTC
(In reply to Zeno Tajoli from comment #3)
> Bernardo, can you describe 'the usual way' to transalte  XML files ?
> Do you plan to use a .po file ?

Yes, sorry, the plan is to use .po files-

In the patchset I introduce an XML file for each SQL one in common mandatory/optional and MARC21 mandatory, soon I'll provide for marc21 optional and hopefully UNIMARC.

I add mysql folders as targets to translation script, so new .po files are build from XML files, and when we use the translation script it copy all sql folders and rebuild sql and txt files from translated xml files, to allow an install with translated values.
Comment 5 Zeno Tajoli 2015-03-25 14:33:10 UTC
I suggest to put .po file in a different git server, to limit the growth of general git archive.
Comment 8 Bernardo Gonzalez Kriegel 2019-11-29 15:10:22 UTC
Created attachment 95884 [details] [review]
Bug 13897: Use XML files for installer data

This patch modifies C4/Installer.pm to add support
for loading XML files into database (MySQL only).

As an example of the functionality, optional
auth_val.sql file is replaced by auth_val.xml

The rationale behind this feature is to enable the
translation of the data that is loaded into the
database. That will be addressed in another bug.

But taking into account that goal, non translatable
values are put into XML attributes, translatable ones
into elements which can already be captured by
translate script.

To test:
0) Do a clean install with all optional data,
   then dump authorised_values table, reserve.
1) Apply the patch
2) Do a clean install in English (marc21/unimarc)
3) On optional data check for description of auth_val
   "Some basic default authorised values for ..."
4) Select all optional data
5) Finish installation
6) Dump again authorised_values table and compare with that
   of point '0'. There must be no differences.
Comment 9 Jonathan Druart 2019-12-16 09:09:58 UTC
Hola Bernado,
I like the idea, but not XML, neither the MySQL only limitation.
Why not write our own stuff, yml-based?
Comment 10 Martin Renvoize 2019-12-16 09:26:14 UTC
This looks good. My only slight concern is whether it ties us more strictly to MySQL or in particular a particular MySQL version. (I'm not sure how widely supported the MySQL XML dump format is.. do they change the format when new feature are introduced for example and does MariaDB support it natively too).

Perhaps we could use the well supported and maintained SQL::Translator to produce something similar in XML, YAML or JSON as our middle format ?
Comment 11 Bernardo Gonzalez Kriegel 2019-12-16 19:21:19 UTC
(In reply to Jonathan Druart from comment #9)
> Hola Bernado,
> I like the idea, but not XML, neither the MySQL only limitation.

Salut Jonathan,
I don't like XML either, nor the MySQL only. 
Just remember that I was thinking in translations, and xml support it's included right now.
But yaml can be handled.

> Why not write our own stuff, yml-based?

No problem, I'll write a POC patch, but have a question:

Do you think the sequence 
 a) Read YAML > Write $tmp/out.sql > DBIx_::RunSQL->run_sql_file($tmp/out.sql) 
is a valid approach, or
 b) Read YAML > Insert into DB somehow 
it's better?
I suppose b) is slower.

(In reply to Martin Renvoize from comment #10)
> This looks good. My only slight concern is whether it ties us more strictly
> to MySQL or in particular a particular MySQL version. (I'm not sure how
> widely supported the MySQL XML dump format is.. do they change the format
> when new feature are introduced for example and does MariaDB support it
> natively too).
> 
> Perhaps we could use the well supported and maintained SQL::Translator to
> produce something similar in XML, YAML or JSON as our middle format ?
Hi Martin,
SQL::Translator documentation states: "Presently only the definition parts of SQL are handled (CREATE, ALTER), not the manipulation of data (INSERT, UPDATE, DELETE)."

And this files normally do INSERTs, don't know if it's useful in this case.
Comment 12 David Cook 2019-12-17 06:35:11 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #11)
> Do you think the sequence 
>  a) Read YAML > Write $tmp/out.sql >
> DBIx_::RunSQL->run_sql_file($tmp/out.sql) 
> is a valid approach, or
>  b) Read YAML > Insert into DB somehow 
> it's better?
> I suppose b) is slower.
> 

Since this is installer data, I don't think it necessarily has to be fast? I figure the most important part is to make it easy to read and update? 

Actually, I don't know if we necessarily need to change C4/Installer.pm? 

We could use YAML as a declarative format for the database data, and then the Release Manager/Release Maintainer could run a tool to compile it into a SQL script. 

If we only do that compilation at release time, it would also be easier to audit and find bugs in the final result.
Comment 13 David Cook 2019-12-17 06:37:42 UTC
(In reply to David Cook from comment #12)

> We could use YAML as a declarative format for the database data, and then
> the Release Manager/Release Maintainer could run a tool to compile it into a
> SQL script. 
> 

This could also be used to add support for different databases, especially in terms of optimizing bulk data loading. 

https://dev.mysql.com/doc/refman/5.6/en/optimizing-innodb-bulk-data-loading.html
https://www.postgresql.org/docs/9.1/ecpg-sql-set-autocommit.html
Comment 14 David Cook 2019-12-17 06:42:06 UTC
While we're on this topic, perhaps it's worthwhile talking about something like this:

https://metacpan.org/pod/DBIx::Class::Schema::Versioned
Comment 15 Katrin Fischer 2019-12-17 07:01:27 UTC
I am not sure if XML is really easier to read here, but enabling translation would be great. :)

I'd vote against 'compiling at release time'. If we still need to create SQL files and store them, we should do so continously like we do for CSS etc. as this needs to be testable with current data all the time.

Please keep in mind when changing the web installer, that it's currently possible to add additional SQL files to the installer that will be picked up - it would be nice to keep that feature. There is also the option to have a syspref.sql for some additional settings (see de-DE installer) that not all languages have made use of.
Comment 16 Jonathan Druart 2019-12-17 08:57:24 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #11)
> (In reply to Jonathan Druart from comment #9)
> > Hola Bernado,
> > I like the idea, but not XML, neither the MySQL only limitation.
> 
> Salut Jonathan,
> I don't like XML either, nor the MySQL only. 
> Just remember that I was thinking in translations, and xml support it's
> included right now.
> But yaml can be handled.
> 
> > Why not write our own stuff, yml-based?
> 
> No problem, I'll write a POC patch, but have a question:
> 
> Do you think the sequence 
>  a) Read YAML > Write $tmp/out.sql >
> DBIx_::RunSQL->run_sql_file($tmp/out.sql) 
> is a valid approach, or
>  b) Read YAML > Insert into DB somehow 
> it's better?
> I suppose b) is slower.

I would:
- Retrieve and parse the whole YAML file
- Generate the SQL queries (like 1 per table)
- Execute the INSERT statements (using DBIx::RunSQL->run_sql)
Comment 17 Martin Renvoize 2019-12-17 08:59:51 UTC
Sorry Bernardo, I didn't mean to knock this one off the rails so much :(.

Hmm, it's a shame SQL::Translator only handles definition... I must have misremembered... it's SQL::Abstract ( the the various DBIx::Class extensions) that does the rest. SQL::Abstract is Perl script however so may not be especially helpful here?

As for DBIx::Class::Schema::Versioned, I'm really not sure I understand what that would win for us (especially as the general recommendation in DBIx::Class land now is not to use it, but rather use DeploymentHandler instead).  Koha's installer is actually pretty powerful as it is in my opinion and this is just one minor area we can improve.. please highlight some problems you feel such a move would resolve?

Going back to the actual problem this patchset is trying to resolve.. can you remind me what it is about the SQL format files in the first place means we can't easily support translating them without moving to some intermediary file format?
Comment 18 Katrin Fischer 2019-12-17 09:03:02 UTC
> Going back to the actual problem this patchset is trying to resolve.. can
> you remind me what it is about the SQL format files in the first place means
> we can't easily support translating them without moving to some intermediary
> file format?

I think the problem is that you can't 'mark' what needs translation. You only want to translate a small bit of the SQL files, the descriptions while preventing people to translate values that should not be changed.
Comment 19 Jonathan Druart 2019-12-17 09:07:25 UTC
(In reply to Martin Renvoize from comment #17)
> Going back to the actual problem this patchset is trying to resolve.. can
> you remind me what it is about the SQL format files in the first place means
> we can't easily support translating them without moving to some intermediary
> file format?

.sql files are not following the usual translation workflow.
There is 1 file per language (in the term of language in installer/data/mysql/, so not so much compared to the number of languages we really support (POs).
If you want to translate a sql file, you need to open a bug report, patch it, wait for someone to SO/QA/push.
Also it's painful to support (dev POV), as if you want to add an AV, you have to edit X .sql files, test them, (rebase), etc.
With only 1 sql file, at the root of installer dir (installer/data/mysq) the translation will be done using the usual workflow (after a change to our translator tool as we still do not support YAML file, but we do support .pref which are YAML, so should not be too hard)
Comment 20 Martin Renvoize 2019-12-17 09:16:57 UTC
Sorry.. I don't think my question was clear.. I'm totally onboard with wanting to replace the workflow we have and having one update file which then followed the standard translations workflow we have elsewhere..

What I wasn't fully understanding is why we needed to move from a lots of SQL files to one YAML/XML/Whatever file instead of a single SQL file which gets translated via the tools.

Sounds like SQL is just too hard to parse as is for the existing translation tools.. that's the answer.
Comment 21 Bernardo Gonzalez Kriegel 2019-12-17 13:21:46 UTC
Created attachment 96370 [details] [review]
Bug 13897: Use YAML files for installer data

This patch modifies C4/Installer.pm to add support
for loading YAML files into database.

As an example of the functionality, optional
auth_val.sql file is replaced by auth_val.yml

The rationale behind this feature is to enable the
translation of the data that is loaded into the
database. That will be addressed in another bug.

But taking into account that goal, translatable
values are declared in the YAML files, to ease
identification by translate script.

To test:
0) Do a clean install with all optional data,
   then dump authorised_values table, reserve.
1) Apply the patch
2) Do a clean install in English (marc21/unimarc)
3) On optional data check for description of auth_val
   "Some basic default authorised values for ..."
4) Select all optional data
5) Finish installation
6) Dump again authorised_values table and compare with that
   of point '0'.
   Expected differences: most rows now have 'lib_opac' field
   with the same value as 'lib'
Comment 22 Bernardo Gonzalez Kriegel 2019-12-17 13:51:38 UTC
Great to have so many comment :)

(In reply to David Cook from comment #12)
> 
> Since this is installer data, I don't think it necessarily has to be fast? I
> figure the most important part is to make it easy to read and update? 

Agreed, I think it's worth it

> Actually, I don't know if we necessarily need to change C4/Installer.pm? 
> 
> We could use YAML as a declarative format for the database data, and then
> the Release Manager/Release Maintainer could run a tool to compile it into a
> SQL script.

I think, as Katrin and Jonathan, that it's better not to compile sql, just use the files, then a change in C4/Installer.pm is needed.


(In reply to Jonathan Druart from comment #16)
> > > Why not write our own stuff, yml-based?
> ...
> I would:
> - Retrieve and parse the whole YAML file
> - Generate the SQL queries (like 1 per table)
> - Execute the INSERT statements (using DBIx::RunSQL->run_sql)

In the new patch I'm inserting directly. 
We can try to measure the delay, if any


(In reply to Katrin Fischer from comment #18)
> > Going back to the actual problem this patchset is trying to resolve.. can
> > you remind me what it is about the SQL format files in the first place means
> > we can't easily support translating them without moving to some intermediary
> > file format?
> 
> I think the problem is that you can't 'mark' what needs translation. You
> only want to translate a small bit of the SQL files, the descriptions while
> preventing people to translate values that should not be changed.

Katrin is right, I couldn't find a general way to parse sql files to extract/replace strings. There are too many variations.
The only one I'd made is the one used to generate translated frameworks, in translation server, that has different functions to extract/replace strings with 1,2,6,15,16 commas (,); and that after massaging EN frameworks sql files so they are exactly as expected.


(In reply to Martin Renvoize from comment #20)
> What I wasn't fully understanding is why we needed to move from a lots of
> SQL files to one YAML/XML/Whatever file instead of a single SQL file which
> gets translated via the tools.
> 
> Sounds like SQL is just too hard to parse as is for the existing translation
> tools.. that's the answer.

It is hard, at last for me :(

Thank you all!

If you can, please take a look at the second patch.
It uses YAML instead of XML, it's easier to read.
I tried it and works. Do not install the first patch.
Comment 23 Jonathan Druart 2019-12-17 14:21:22 UTC
Looks great to me!
Comment 24 Martin Renvoize 2019-12-17 15:07:38 UTC
OK, that's lovely.. super simple to understand and write.. nice work Bernardo!
Comment 25 David Cook 2019-12-18 03:33:11 UTC
That looks a lot nicer!

Since you're going with this approach, perhaps it would be worth turning off AutoCommit, running the INSERTs, and then COMMITing? According to the MySQL documentation, that should be higher performance. Probably easier to handle exceptions that way too.

I wonder if we should add more error-handling in the eval{} block as well. I suppose any errors will be caught during $loadyaml->execute(), so maybe that's unnecessary. (Due to my nitpicker nature, I'd suggest $loadyaml be $load_sth instead, but that's really just nitpicking, so feel free to ignore it.)

+1 Bernardo
Comment 26 Jonathan Druart 2019-12-18 08:27:51 UTC
Bernardo,

I am all good with it, we could improve (performance) later (like one INSERT per file) if needed.
Another improvement would be to move the .txt file into the YAML.

I would like to see the translator tool ready before we push this one. Did you investigate already the work that needs to be done?
I think we should move the already translated strings into a new .po (one .po for all installer files?)


I can have a look if you did not already, but prefer to avoid duplication of work :)
Comment 27 Jonathan Druart 2019-12-18 08:28:44 UTC
Created attachment 96397 [details] [review]
Bug 13897: Catch the error and warn it
Comment 28 Bernardo Gonzalez Kriegel 2019-12-18 13:48:30 UTC
Jonathan,
thanks to change Bug description, planned to do it today :)

(In reply to Jonathan Druart from comment #26)
> I am all good with it, we could improve (performance) later (like one INSERT
> per file) if needed.

Ok

> Another improvement would be to move the .txt file into the YAML.

I thought about that too, which would allow it to be translated as well.
I will add the content in a description field.

> I would like to see the translator tool ready before we push this one. Did
> you investigate already the work that needs to be done?

Yes... for xml :)
I will work on this in Bug 24262

> I think we should move the already translated strings into a new .po (one
> .po for all installer files?)

I agree, perhaps 'xx-YY-installer.po' for common files, and append marcflavor content for MARC21/UNIMARC/NORMARC to existing files, or add new ones.


> I can have a look if you did not already, but prefer to avoid duplication of
> work :)

Thanks! I'll try to solve this and let you know.

Now some questions:

1) Bug 22824 - This patch can work indistinctly with YAML, YAML::Syck or YAML::XS, no problem with that.

2) YAML format:
I'm using
---
table:
  translatable: { A: 1 }
  rows:
    - A: 1
      B: 2

    - A: 3
      B: 4

With this format we have only one table per file, and all rows must have the same number of values ( required by prepare($query) )

If we define things like
---
- firstTable:
    translatable: { A: 1 }
    rows:
      - A1: 1
        B1: 2

      - A2: 1
        B2: 2

- firstTable:
    translatable: { A: 1 }
    rows:
      - A: 1
        B: 2
        C: 3

      - A: 4
        B: 5
        C: 6

- secondTable:
    translatable: { G: 1 }
    rows:
      - F: 1
        G: 2

      - F: 3
        G: 4

we can have entries for the same table with different number of values, and also entries for different tables inserted in order.
(BTW, of current installer sql files: 30 just one table, 9 more than one table; common+en files )

A change in format needs a simple revision of the code, but I would like some opinions about the format.
Comment 29 Jonathan Druart 2019-12-18 14:09:10 UTC
What about the following structure:

table_name:
    translatable: [ A, B ]
    fields [ A, B, C, D ]
    rows:
      - A: 1
        B: 2

      - A: 1
        C: 2
        D: 4

      - A: 5
      - B: 6
      - C: 7
      - D: 8

I do not think we should force to have the same number of fields. What if you want the default value for instance? That's not a big deal but I would prefer to get rid of this limitation since the beginning :)
Comment 30 Bernardo Gonzalez Kriegel 2019-12-18 15:40:19 UTC
(In reply to Jonathan Druart from comment #29)
> What about the following structure:
> ...
> 
> I do not think we should force to have the same number of fields. What if
> you want the default value for instance? That's not a big deal but I would
> prefer to get rid of this limitation since the beginning :)

Maybe I'm wrong but to do that we'll need to do a prepare/execute for each row, because we won't know which columns are necessary; and this could be slower ( https://docstore.mik.ua/orelly/linux/dbi/ch05_05.htm )
Columns must be absent on insert statement for them to use default values.
It'll be equivalent to a $dbh->do() for each row.

Or we can fill missing values with empty/NULL
 my @values = map { $row->{$_} ? $row->{$_} : NULL } @columns;
for example, if we are going to use all column names in one prepare().
(do not like this)

What about entries for many tables in a file?

Note: I like your idea, simply don't know how to solve it.
Comment 31 David Cook 2019-12-18 23:47:57 UTC
Couldn't we just use the DEFAULT keyword to specify a default value? (I haven't tried it so just a suggestion.)
Comment 32 Bernardo Gonzalez Kriegel 2019-12-20 19:13:34 UTC
Created attachment 96539 [details] [review]
Bug 13897: Use YAML files for installer data

This patch modifies C4/Installer.pm to add support
for loading YAML files into database.

As an example of the functionality, optional
auth_val.sql file is replaced by auth_val.yml

The rationale behind this feature is to enable the
translation of the data that is loaded into the
database. That will be addressed in another bug.

But taking into account that goal, translatable
values are declared in the YAML files, to ease
identification by translate script.

To test:
0) Do a clean install with all optional data,
   then dump authorised_values table, reserve.
1) Apply the patch
2) Do a clean install in English (marc21/unimarc)
3) On optional data check for description of auth_val
   "Some basic default authorised values for ..."
4) Select all optional data
5) Finish installation
6) Dump again authorised_values table and compare with that
   of point '0'.
   No differences should be found.
Comment 33 Bernardo Gonzalez Kriegel 2019-12-20 19:26:01 UTC
Hi,
new patch version taking into account comments.

I introduce a new format for YAML, it permits more than one table per file,
or same table with different number of columns.

The example file is an exact translation of the sql one, same order and column number, as a way to exemplify it's format

I also added a patch in Bug 24262, with it, this and Bug 18688, it's possible to
translate and install in new language using it's own installer files.

A lot more work is needed to enable the complete translation of installer files, but hope this can be considered as first step in that direction.
Comment 34 Bernardo Gonzalez Kriegel 2019-12-23 01:06:31 UTC
Created attachment 96591 [details] [review]
Bug 13897: Use YAML files for installer data

This patch modifies C4/Installer.pm to add support
for loading YAML files into database.

As an example of the functionality, optional
auth_val.sql file is replaced by auth_val.yml

The rationale behind this feature is to enable the
translation of the data that is loaded into the
database. That will be addressed in another bug.

But taking into account that goal, translatable
values are declared in the YAML files, to ease
identification by translate script.

Also file description is moved into the yaml
file.

To test:
0) Do a clean install with all optional data,
   then dump authorised_values table, reserve.
1) Apply the patch
2) Do a clean install in English (marc21/unimarc)
3) On optional data check for description of auth_val
   "Some basic default authorised values for ..."
4) Select all optional data
5) Finish installation
6) Dump again authorised_values table and compare with that
   of point '0'.
   No differences should be found.
Comment 35 Bernardo Gonzalez Kriegel 2019-12-23 01:06:36 UTC
Created attachment 96592 [details] [review]
Bug 13897: Catch the error and warn it
Comment 36 Martin Renvoize 2020-01-02 14:25:45 UTC
Created attachment 96754 [details] [review]
Bug 13897: Use YAML files for installer data

This patch modifies C4/Installer.pm to add support
for loading YAML files into database.

As an example of the functionality, optional
auth_val.sql file is replaced by auth_val.yml

The rationale behind this feature is to enable the
translation of the data that is loaded into the
database. That will be addressed in another bug.

But taking into account that goal, translatable
values are declared in the YAML files, to ease
identification by translate script.

Also file description is moved into the yaml
file.

To test:
0) Do a clean install with all optional data,
   then dump authorised_values table, reserve.
1) Apply the patch
2) Do a clean install in English (marc21/unimarc)
3) On optional data check for description of auth_val
   "Some basic default authorised values for ..."
4) Select all optional data
5) Finish installation
6) Dump again authorised_values table and compare with that
   of point '0'.
   No differences should be found.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 37 Martin Renvoize 2020-01-02 14:25:48 UTC
Created attachment 96755 [details] [review]
Bug 13897: Catch the error and warn it

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 38 Martin Renvoize 2020-01-02 14:30:28 UTC
I really like this approach, thanks for working on it Bernardo.. signing off
Comment 39 Jonathan Druart 2020-01-02 14:45:54 UTC
Bernardo, should not we move it out of the 'en' directory?

I think we should move it to installer/data/mysql/optional, then remove the other auth_val.sql files.
Comment 40 Bernardo Gonzalez Kriegel 2020-01-02 19:02:20 UTC
(In reply to Jonathan Druart from comment #39)
> Bernardo, should not we move it out of the 'en' directory?
> 
> I think we should move it to installer/data/mysql/optional, then remove the
> other auth_val.sql files.

It would not be a problem, if there were no descriptions to be translated.

Currently we have localized versions, all translated

./de-DE/optional/auth_val.sql
./es-ES/optional/auth_val.sql
./it-IT/necessari/auth_val.sql
./pl-PL/optional/auth_val.sql
./nb-NO/2-Valgfritt/auth_val.sql
./fr-CA/facultatif/auth_val.sql

with the same content, sort of.
Comment 41 Tomás Cohen Arazi 2020-01-09 12:31:15 UTC
Created attachment 97068 [details] [review]
Bug 13897: Use YAML files for installer data

This patch modifies C4/Installer.pm to add support
for loading YAML files into database.

As an example of the functionality, optional
auth_val.sql file is replaced by auth_val.yml

The rationale behind this feature is to enable the
translation of the data that is loaded into the
database. That will be addressed in another bug.

But taking into account that goal, translatable
values are declared in the YAML files, to ease
identification by translate script.

Also file description is moved into the yaml
file.

To test:
0) Do a clean install with all optional data,
   then dump authorised_values table, reserve.
1) Apply the patch
2) Do a clean install in English (marc21/unimarc)
3) On optional data check for description of auth_val
   "Some basic default authorised values for ..."
4) Select all optional data
5) Finish installation
6) Dump again authorised_values table and compare with that
   of point '0'.
   No differences should be found.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 42 Tomás Cohen Arazi 2020-01-09 12:31:19 UTC
Created attachment 97069 [details] [review]
Bug 13897: Catch the error and warn it

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 43 Jonathan Druart 2020-01-09 12:32:00 UTC
(In reply to Jonathan Druart from comment #39)
> Bernardo, should not we move it out of the 'en' directory?
> 
> I think we should move it to installer/data/mysql/optional, then remove the
> other auth_val.sql files.

Should not this be done before pushing it?
Comment 44 Tomás Cohen Arazi 2020-01-09 12:32:58 UTC
(In reply to Jonathan Druart from comment #43)
> (In reply to Jonathan Druart from comment #39)
> > Bernardo, should not we move it out of the 'en' directory?
> > 
> > I think we should move it to installer/data/mysql/optional, then remove the
> > other auth_val.sql files.
> 
> Should not this be done before pushing it?

- Installer works as expected and strings are shown as before
- QA script is happy

¡Excelente trabajo Pichu, para variar!

Feliz 2020!!
Comment 45 Tomás Cohen Arazi 2020-01-09 12:34:14 UTC
(In reply to Jonathan Druart from comment #43)
> (In reply to Jonathan Druart from comment #39)
> > Bernardo, should not we move it out of the 'en' directory?
> > 
> > I think we should move it to installer/data/mysql/optional, then remove the
> > other auth_val.sql files.
> 
> Should not this be done before pushing it?

There are translated files for this. Are you suggesting those are moved into .yml?
Comment 46 Jonathan Druart 2020-01-09 12:51:21 UTC
From the YAML file:
+description:
+  - "Some basic default authorised values for library locations, item lost status, etc."
+  - "You can change these at any time after installation."

This is what need to be used for translation.
Comment 47 Tomás Cohen Arazi 2020-01-09 13:17:28 UTC
I think Jonathan is already thinking about this, so setting back to SO so it is clear the QA step is not finished. From my POV, it looks like ready.
Comment 48 Bernardo Gonzalez Kriegel 2020-01-09 18:31:57 UTC
(In reply to Jonathan Druart from comment #46)
> From the YAML file:
> +description:
> +  - "Some basic default authorised values for library locations, item lost
> status, etc."
> +  - "You can change these at any time after installation."
> 
> This is what need to be used for translation.

Yes, of course. 
In Bug 24262 I catch those strings so they can be translated.
In Comment #26 Jonathan suggested that txt file content could be moved into the yaml files, that's what I did
Then I changed Bug 24262 accordingly

C4/Installer.pm it's changed to show the description from the yaml, current behaviour is preserved.
Comment 49 Martin Renvoize 2020-01-21 15:11:39 UTC
Does this get your final PQA stamp now Jonathan?  It looks like between the two bugs we have all that is required to me.
Comment 50 Bernardo Gonzalez Kriegel 2020-02-05 15:24:13 UTC
Created attachment 98507 [details] [review]
Bug 13897: Process description, multiline values and SQL statements in YAML files

This patch adds 3 features:
1) Display description of YAML files at install time
   for frameworks, and fixes it's encoding.
2) Enable use of multiline values, field required
3) Process SQL statements declared in YAML files

With this features we can process files with the
following generic YAML strucure:

  ---
  description:
    - "File description"

  tables:
    - table_name:
        translatable: [ title, content ]
        multiline: [ content ]
        rows:
          - title: "Example title"
            content:
              - "Content:"
              - ""
              - "This is the content."
            id: 1
            value: ~

  sql_statements:
    - "UPDATE table_name SET value ='empty' WHERE value IS NULL"
  ...

* file description is now inside the YAML, can have multiple
  lines.
  This attribute is expected in all YAML files.
* translatable attribute in table declare fields that can be
  translated
* multiline attribute in table declare fields that can have
  multiple lines of content, they are joined using '\r\n'
  before insert into database.
  This is useful to express fields like 'news' content, and
  to simplify it's translation.
  '\r\n' is used for correct display in Windows clients.
* sql_statements allows to add multiple SQL sentences, not
  insertions normally, that are executed in order.

This features are not needed for the example file of this patch,
but will be used in new bugs.

To test:
1) Use the same test plan of first patch.
Comment 51 Bernardo Gonzalez Kriegel 2020-02-05 15:25:36 UTC
New patch added, needs sign.
Comment 52 Jonathan Druart 2020-02-12 10:28:50 UTC
Created attachment 98741 [details] [review]
Bug 13897: Surround columns with backticks

We have some column's names that need to be surrounded by ` as they are
protected keywords ('rows' for instance)
Comment 53 Jonathan Druart 2020-02-12 11:33:57 UTC
Support for YAML files in misc4dev is available in this merge request: https://gitlab.com/koha-community/koha-misc4dev/-/merge_requests/37
Comment 54 Martin Renvoize 2020-02-21 16:20:15 UTC
Created attachment 99392 [details] [review]
Bug 13897: Use YAML files for installer data

This patch modifies C4/Installer.pm to add support
for loading YAML files into database.

As an example of the functionality, optional
auth_val.sql file is replaced by auth_val.yml

The rationale behind this feature is to enable the
translation of the data that is loaded into the
database. That will be addressed in another bug.

But taking into account that goal, translatable
values are declared in the YAML files, to ease
identification by translate script.

Also file description is moved into the yaml
file.

To test:
0) Do a clean install with all optional data,
   then dump authorised_values table, reserve.
1) Apply the patch
2) Do a clean install in English (marc21/unimarc)
3) On optional data check for description of auth_val
   "Some basic default authorised values for ..."
4) Select all optional data
5) Finish installation
6) Dump again authorised_values table and compare with that
   of point '0'.
   No differences should be found.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 55 Martin Renvoize 2020-02-21 16:20:19 UTC
Created attachment 99393 [details] [review]
Bug 13897: Catch the error and warn it

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 56 Martin Renvoize 2020-02-21 16:20:23 UTC
Created attachment 99394 [details] [review]
Bug 13897: Process description, multiline values and SQL statements in YAML files

This patch adds 3 features:
1) Display description of YAML files at install time
   for frameworks, and fixes it's encoding.
2) Enable use of multiline values, field required
3) Process SQL statements declared in YAML files

With this features we can process files with the
following generic YAML strucure:

  ---
  description:
    - "File description"

  tables:
    - table_name:
        translatable: [ title, content ]
        multiline: [ content ]
        rows:
          - title: "Example title"
            content:
              - "Content:"
              - ""
              - "This is the content."
            id: 1
            value: ~

  sql_statements:
    - "UPDATE table_name SET value ='empty' WHERE value IS NULL"
  ...

* file description is now inside the YAML, can have multiple
  lines.
  This attribute is expected in all YAML files.
* translatable attribute in table declare fields that can be
  translated
* multiline attribute in table declare fields that can have
  multiple lines of content, they are joined using '\r\n'
  before insert into database.
  This is useful to express fields like 'news' content, and
  to simplify it's translation.
  '\r\n' is used for correct display in Windows clients.
* sql_statements allows to add multiple SQL sentences, not
  insertions normally, that are executed in order.

This features are not needed for the example file of this patch,
but will be used in new bugs.

To test:
1) Use the same test plan of first patch.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 57 Martin Renvoize 2020-02-21 16:20:27 UTC
Created attachment 99395 [details] [review]
Bug 13897: Surround columns with backticks

We have some column's names that need to be surrounded by ` as they are
protected keywords ('rows' for instance)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 58 Martin Renvoize 2020-02-21 16:21:00 UTC
Great work Bernardo, thanks for your efforts here..

All looking good and working as expected for me..

Signing off.
Comment 59 Jonathan Druart 2020-03-09 12:17:48 UTC
Created attachment 100340 [details] [review]
Bug 13897: Use YAML files for installer data

This patch modifies C4/Installer.pm to add support
for loading YAML files into database.

As an example of the functionality, optional
auth_val.sql file is replaced by auth_val.yml

The rationale behind this feature is to enable the
translation of the data that is loaded into the
database. That will be addressed in another bug.

But taking into account that goal, translatable
values are declared in the YAML files, to ease
identification by translate script.

Also file description is moved into the yaml
file.

To test:
0) Do a clean install with all optional data,
   then dump authorised_values table, reserve.
1) Apply the patch
2) Do a clean install in English (marc21/unimarc)
3) On optional data check for description of auth_val
   "Some basic default authorised values for ..."
4) Select all optional data
5) Finish installation
6) Dump again authorised_values table and compare with that
   of point '0'.
   No differences should be found.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 60 Jonathan Druart 2020-03-09 12:17:53 UTC
Created attachment 100341 [details] [review]
Bug 13897: Catch the error and warn it

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 61 Jonathan Druart 2020-03-09 12:17:57 UTC
Created attachment 100342 [details] [review]
Bug 13897: Process description, multiline values and SQL statements in YAML files

This patch adds 3 features:
1) Display description of YAML files at install time
   for frameworks, and fixes it's encoding.
2) Enable use of multiline values, field required
3) Process SQL statements declared in YAML files

With this features we can process files with the
following generic YAML strucure:

  ---
  description:
    - "File description"

  tables:
    - table_name:
        translatable: [ title, content ]
        multiline: [ content ]
        rows:
          - title: "Example title"
            content:
              - "Content:"
              - ""
              - "This is the content."
            id: 1
            value: ~

  sql_statements:
    - "UPDATE table_name SET value ='empty' WHERE value IS NULL"
  ...

* file description is now inside the YAML, can have multiple
  lines.
  This attribute is expected in all YAML files.
* translatable attribute in table declare fields that can be
  translated
* multiline attribute in table declare fields that can have
  multiple lines of content, they are joined using '\r\n'
  before insert into database.
  This is useful to express fields like 'news' content, and
  to simplify it's translation.
  '\r\n' is used for correct display in Windows clients.
* sql_statements allows to add multiple SQL sentences, not
  insertions normally, that are executed in order.

This features are not needed for the example file of this patch,
but will be used in new bugs.

To test:
1) Use the same test plan of first patch.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 62 Jonathan Druart 2020-03-09 12:18:02 UTC
Created attachment 100343 [details] [review]
Bug 13897: Surround columns with backticks

We have some column's names that need to be surrounded by ` as they are
protected keywords ('rows' for instance)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 63 Martin Renvoize 2020-03-09 14:12:24 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 64 Joy Nelson 2020-04-01 22:35:34 UTC
enhancement not backported to 19.11.x