Bug 27623 - Remove pl-PL installer data
Summary: Remove pl-PL installer data
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (web-based installer) (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Jonathan Druart
QA Contact: Joonas Kylmälä
URL: https://gitlab.com/joubu/Koha/-/tree/...
Keywords:
Depends on: 28154 28281 28291 24871 24973
Blocks: 27829
  Show dependency treegraph
 
Reported: 2021-02-04 08:59 UTC by Jonathan Druart
Modified: 2021-05-12 15:08 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
This removes the SQL installer directory from the Koha source code. Installing Polish sample data will still be possible, but translations have been moved into Koha's translation system using .po files maintained on https://translate.koha-community.org.
Version(s) released in:
21.05.00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2021-02-04 08:59:09 UTC
Now we can translate all installer data, then the pl-PL directory is no longer needed.
Comment 1 Jonathan Druart 2021-03-02 10:58:47 UTC
https://gitlab.com/joubu/Koha/-/tree/bug_27623
Comment 2 Martin Renvoize 2021-03-04 10:15:13 UTC
I've worked through the patch and it makes sense to me.. Signing off (Will leave Jonathan to add the SO line to the branch)
Comment 3 Katrin Fischer 2021-03-07 15:17:41 UTC
pl-PL appears to be a mostly untranslated copy of the English files. I think the only thing we should think about for a custom.sql here is active currency and adding PLN.

-INSERT INTO `currency` (currency, rate, symbol, active) VALUES
-('PLN', 1.0, 'zł', '1'),
Comment 4 Katrin Fischer 2021-03-07 15:33:23 UTC
Missing change to xt/sample_notices.t
Comment 5 Jonathan Druart 2021-03-08 10:33:07 UTC
Added Martin's signed-off-by line to the first commit,

Added 2 new commits:
Bug 27623: Fix sample_notices.t
Bug 27623: Add PLN currency to custom.sql
Comment 6 Joonas Kylmälä 2021-04-15 12:49:47 UTC
Doesn't apply anymore
Comment 7 Joonas Kylmälä 2021-04-15 13:02:27 UTC
(In reply to Joonas Kylmälä from comment #6)
> Doesn't apply anymore

It was actually easy to fix this rebase issue:

> git rm installer/data/mysql/pl-PL/mandatory/sample_notices.sql
> git am --continue

However, in the installer I get

> Cannot decode string with wide characters at /usr/lib/x86_64-linux-gnu/perl> /5.24/Encode.pm line 202
>    in Encode::decode at /usr/lib/x86_64-linux-gnu/perl/5.24/Encode.pm line 202

and also if following the test plan I get:

> $ ./translate update pl-PL
> misc/translator/Koha-pref.pot:5085:37: invalid multibyte sequence

not sure if caused by this change or it got broken already before, but it makes testing this patch not possible.
Comment 8 Jonathan Druart 2021-04-15 14:53:17 UTC
(In reply to Joonas Kylmälä from comment #7)
> (In reply to Joonas Kylmälä from comment #6)
> > Doesn't apply anymore
> 
> It was actually easy to fix this rebase issue:
> 
> > git rm installer/data/mysql/pl-PL/mandatory/sample_notices.sql
> > git am --continue
> 
> However, in the installer I get
> 
> > Cannot decode string with wide characters at /usr/lib/x86_64-linux-gnu/perl> /5.24/Encode.pm line 202
> >    in Encode::decode at /usr/lib/x86_64-linux-gnu/perl/5.24/Encode.pm line 202

Which step, I didn't recreate that.

> and also if following the test plan I get:
> 
> > $ ./translate update pl-PL
> > misc/translator/Koha-pref.pot:5085:37: invalid multibyte sequence
> 
> not sure if caused by this change or it got broken already before, but it
> makes testing this patch not possible.

I am seeing that
Looks like it has been caused by bug 22824. I am investigating it.
Comment 9 Joonas Kylmälä 2021-04-16 07:27:48 UTC
(In reply to Jonathan Druart from comment #8)
> (In reply to Joonas Kylmälä from comment #7)

> Which step, I didn't recreate that.

First run following commands:

> $ koha-mysql kohadev < installer/data/mysql/kohastructure.sql
> $ ./translate install pl-PL

Then open the installer and select pl-PL as the language, then after few pages the error happens after clicking OK in the MARC21/UNIMARC selection page:

http://localhost:8081/cgi-bin/koha/installer/install.pl?step=3&op=choosemarc
Comment 10 Jonathan Druart 2021-04-23 10:00:51 UTC
Should have been fixed by bug 28154.
Comment 11 Jonathan Druart 2021-04-28 10:10:26 UTC
Joonas, Katrin, can I get a QA stamp please?
Comment 12 Joonas Kylmälä 2021-04-28 10:24:16 UTC
(In reply to Jonathan Druart from comment #11)
> Joonas, Katrin, can I get a QA stamp please?

Still the same problem with installer so cannot test this.
Comment 13 Victor Grousset/tuxayo 2021-04-30 02:48:09 UTC
> Still the same problem with installer so cannot test this.

Odd, I was able to complete the installer with the branch directly and with a rebase on master. Env: koha-testing-docker


Note, there is a conflict when rebasing. Some recent commits modified one of the deleted files. So their changes might need to be moved somewhere (if these changes are still needed)
Comment 14 Victor Grousset/tuxayo 2021-04-30 02:51:17 UTC
Rechecked, seems there is no issue => signed off!
Comment 15 Joonas Kylmälä 2021-05-04 07:49:52 UTC
Victor what were the exact steps you used, I still get the error, I even deleted koha-testing-docker (Debian GNU/Linux 9 version) completely and re-created it from scratch and I still get the error after:

$ koha-mysql kohadev < installer/data/mysql/kohastructure.sql
$ restart_all
$ ./translate update pl-PL
$ ./translate install pl-PL
$ restart_all
 --> Go to installer page and select pl-PL as installer language, click through it and notice it gives error:

> Cannot decode string with wide characters at /usr/lib/x86_64-linux-gnu/perl/5.24/Encode.pm line 202

in the MARC21 step.
Comment 16 Joonas Kylmälä 2021-05-04 08:32:52 UTC
I found the cause and opened bug 28281. I will work on a fix (or I already have but missing test).
Comment 17 Joonas Kylmälä 2021-05-05 12:24:51 UTC
I'm still unable to test this even after bug 28281 because misc/translator/LangInstaller.pm / ./translate script double *encodes* the output of installer/data/mysql/pl-PL/marcflavour/marc21/mandatory/authorities_normal_marc21.yml

I tried to now change it like this:

-                    open(my $fh, ">:encoding(UTF-8)", "$intradir/$tdir/$file");
-                    YAML::XS::DumpFile( $fh, $translated_yaml );
-                    close($fh);

but that still produces broken UTF-8 in the file:

installer/data/mysql/pl-PL/marcflavour/marc21/mandatory/authorities_normal_marc21.yml


NOTE: Please be careful and remember to use "git clean -d -f" and git reset --hard HEAD before testing this bug. Otherwise the ./translate script doesn't update the translation files.
Comment 18 Joonas Kylmälä 2021-05-05 12:26:12 UTC
(In reply to Joonas Kylmälä from comment #17)
> I tried to now change it like this:
> 
> -                    open(my $fh, ">:encoding(UTF-8)",
> "$intradir/$tdir/$file");
> -                    YAML::XS::DumpFile( $fh, $translated_yaml );
> -                    close($fh);

err, missed the "+" part of the diff, I made it to look like:

+                    YAML::XS::DumpFile( "$intradir/$tdir/$file", $translated_yaml );
Comment 19 Joonas Kylmälä 2021-05-05 12:45:00 UTC
ah, there is even a third bug! I have a fix now for that as well, will open new bug report and make this depends on that.
Comment 20 Joonas Kylmälä 2021-05-05 14:29:08 UTC
I have now fix for that as well in bug 28291 but tests still missing...
Comment 21 Victor Grousset/tuxayo 2021-05-08 20:38:56 UTC
Retried, still works after bug 28291 and bug 28281 :)
Comment 22 Katrin Fischer 2021-05-09 10:40:39 UTC
Hm, this might be the same problem Victor already saw, but using the branch, I got stuck at this:

YAML::XS::Load Error: The problem:

    control characters are not allowed

was found at document: 0
 at /usr/lib/x86_64-linux-gnu/perl5/5.24/YAML/XS.pm line 51

    in YAML::XS::LoadFile at /usr/lib/x86_64-linux-gnu/perl5/5.24/YAML/XS.pm line 51
Comment 23 Katrin Fischer 2021-05-09 10:44:16 UTC
Trying again with 28281 applied as well, but got stuck with the same error again :(

Help?
Comment 24 Victor Grousset/tuxayo 2021-05-09 20:42:31 UTC
(In reply to Katrin Fischer from comment #22)
> Hm, this might be the same problem Victor already saw, but using the branch,

Which problem? It seems I didn't get a problem each time I tried.
Comment 25 Joonas Kylmälä 2021-05-10 06:06:29 UTC
(In reply to Katrin Fischer from comment #23)
> Trying again with 28281 applied as well, but got stuck with the same error
> again :(
> 
> Help?

You need to have all those dependencies listed in the bug report here, not just 28281. Rebasing the patches on top of master is your best bet I think, it contains already those. But note I have not yet tried if it works still even after all those fixes because I have not yet went through the full installer after those bugs being fixed. Also please make sure to run "git clean -d -f" after your testing to make sure the translation files get updated.
Comment 26 Katrin Fischer 2021-05-10 07:13:08 UTC
(In reply to Joonas Kylmälä from comment #25)
> (In reply to Katrin Fischer from comment #23)
> > Trying again with 28281 applied as well, but got stuck with the same error
> > again :(
> > 
> > Help?
> 
> You need to have all those dependencies listed in the bug report here, not
> just 28281. Rebasing the patches on top of master is your best bet I think,
> it contains already those. But note I have not yet tried if it works still
> even after all those fixes because I have not yet went through the full
> installer after those bugs being fixed. Also please make sure to run "git
> clean -d -f" after your testing to make sure the translation files get
> updated.

I've run the translation, but was probably missing other dependencies as you suggest. As this is on a branch, I think it'd be easier to have Joubu rebase as I can't attach a fixed patch set.
Comment 27 Jonathan Druart 2021-05-10 08:03:11 UTC
Remote branch rebased against master.
Comment 28 Joonas Kylmälä 2021-05-10 12:44:33 UTC
Passing QA:

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 29 Jonathan Druart 2021-05-10 14:18:07 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 30 Fridolin Somers 2021-05-12 15:08:15 UTC
Not pushed to 20.11.x