Created attachment 116722 [details] [review]
Bug 22824: Add tests

Writing some tests to see how things are working currently

Created attachment 116723 [details] [review]
Bug 22824: Replace YAML::Syck with YAML::XS in tests

In order to understand how things will work after the changes

Created attachment 116724 [details] [review]
Bug 22824: Fixing first error in tests

There was an encoding issue. The parent bug report have been fixed, see
bug 27673 and patches related to Encode

Created attachment 116725 [details] [review]
Bug 22824: Replace YAML::Syck with YAML::XS

Created attachment 116726 [details] [review]
Bug 22824: Fix searching.pref

One .pref file make YAML::XS fails.
The correct display must be, before and after this patchset:
 If "Unescape escaped" is selected this will allow writing regular expressions "\/like this\/" while "/this/", "or/this" will be escaped and interpreted as regular strings.

Created attachment 116727 [details] [review]
Bug 22824: Correct on/off behaviour

Here things are starting to be a bit tricky.
IMO we don't want to replicate the ImplicitTyping behaviour from
YAML::Syck but handle correctly the different value.
We have the list of available values in the .pref file, the values in DB
must match them.

We want 1 or 0 in DB.

2 more occurrences:
finesMode => Dealt on bug 27676 ('off' is ok here)
itemBarcodeInputFilter => 'OFF' is correct here

Note that we don't use ~ or true/false in *.pref

Created attachment 116728 [details] [review]
Bug 22824: Fix yes/no vs 1/0

Patch generated with:
 perl -p -i -e 's# no: # 0: #' **/*.pref
 perl -p -i -e 's# yes: # 1: #' **/*.pref

no must be 0 and yes must be 1!

Created attachment 116729 [details] [review]
Bug 22824: Replace wrong auto replacement

This must be "no", it has been caught by the previous sed

Created attachment 116730 [details] [review]
Bug 22824: Remove C4::Boolean, true_p, boolean_preference, etc.

It seems that we don't really need all this overhead.
YesNo must be a boolean and contain 1 or 0.

Created attachment 116731 [details] [review]
Bug 22824: Fix default values for YesNo

select variable, value from systempreferences where type="YesNo" and ( ( value <>'1' and value <>'0' ) or value is null );

Created attachment 116732 [details] [review]
Bug 22824: Atomic update

Created attachment 116733 [details] [review]
Bug 22824: Add warning to the about page

Just in case, add a warning to the about page.

Created attachment 116734 [details] [review]
Bug 22824: Remove YAML::Syck dependency

Created attachment 116735 [details] [review]
Bug 22824: Adjust tests to match final behaviours

Finally we diverge quite a lot with how it worked prior to this patch
set. Reflect the changes in the tests

Created attachment 116736 [details] [review]
Bug 22824: Fix ES reindex

koha-elastic --rebuild was failing with
Unable to update mappings for index "koha_kohadev_biblios". Reason was: "Could not convert [marc_data.index] to boolean"

I am sorry, I lost myself!

This was supposed to be a quite trivial patch and it ended up huge!

I think the move is interesting and going into the right direction.
But I have the feeling that it's not finished and some tricky checks need to be done on the syspref table/.pref.

This patch set is dealing with the yes/no, on/off weird options we have, historically I bet, in our table and .pref files. In this patch we are removing all of that and expect a boolean to be 1 or 0 (wow!), and it removes the C4::Boolean weirdness.

So far the prefs, installer and translate scripts seem to be working correctly. The edge cases may be hard to catch!

Don't be afraid by the number of patches, they are all very trivial!

What do you think?

Created attachment 116741 [details] [review]
Bug 22824: Add tests

Writing some tests to see how things are working currently

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 116742 [details] [review]
Bug 22824: Add tests

Writing some tests to see how things are working currently

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 116743 [details] [review]
Bug 22824: Replace YAML::Syck with YAML::XS in tests

In order to understand how things will work after the changes

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 116744 [details] [review]
Bug 22824: Fixing first error in tests

There was an encoding issue. The parent bug report have been fixed, see
bug 27673 and patches related to Encode

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 116745 [details] [review]
Bug 22824: Replace YAML::Syck with YAML::XS

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 116746 [details] [review]
Bug 22824: Fix searching.pref

One .pref file make YAML::XS fails.
The correct display must be, before and after this patchset:
 If "Unescape escaped" is selected this will allow writing regular expressions "\/like this\/" while "/this/", "or/this" will be escaped and interpreted as regular strings.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 116747 [details] [review]
Bug 22824: Correct on/off behaviour

Here things are starting to be a bit tricky.
IMO we don't want to replicate the ImplicitTyping behaviour from
YAML::Syck but handle correctly the different value.
We have the list of available values in the .pref file, the values in DB
must match them.

We want 1 or 0 in DB.

2 more occurrences:
finesMode => Dealt on bug 27676 ('off' is ok here)
itemBarcodeInputFilter => 'OFF' is correct here

Note that we don't use ~ or true/false in *.pref

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 116748 [details] [review]
Bug 22824: Fix yes/no vs 1/0

Patch generated with:
 perl -p -i -e 's# no: # 0: #' **/*.pref
 perl -p -i -e 's# yes: # 1: #' **/*.pref

no must be 0 and yes must be 1!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 116749 [details] [review]
Bug 22824: Replace wrong auto replacement

This must be "no", it has been caught by the previous sed

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 116750 [details] [review]
Bug 22824: Remove C4::Boolean, true_p, boolean_preference, etc.

It seems that we don't really need all this overhead.
YesNo must be a boolean and contain 1 or 0.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 116751 [details] [review]
Bug 22824: Fix default values for YesNo

select variable, value from systempreferences where type="YesNo" and ( ( value <>'1' and value <>'0' ) or value is null );

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 116752 [details] [review]
Bug 22824: Atomic update

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 116753 [details] [review]
Bug 22824: Add warning to the about page

Just in case, add a warning to the about page.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 116754 [details] [review]
Bug 22824: Remove YAML::Syck dependency

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 116755 [details] [review]
Bug 22824: Adjust tests to match final behaviours

Finally we diverge quite a lot with how it worked prior to this patch
set. Reflect the changes in the tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 116756 [details] [review]
Bug 22824: Fix ES reindex

koha-elastic --rebuild was failing with
Unable to update mappings for index "koha_kohadev_biblios". Reason was: "Could not convert [marc_data.index] to boolean"

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

(In reply to Jonathan Druart from comment #16)
> What do you think?

Looks great to me! We've plenty of time before release in case there are any more edge cases that need catching, but I can't think of any!

(In reply to Kyle M Hall from comment #33)
> (In reply to Jonathan Druart from comment #16)
> > What do you think?
> 
> Looks great to me! We've plenty of time before release in case there are any
> more edge cases that need catching, but I can't think of any!

Indeed.. I've been down this path once before with another app.. I think this looks like a really solid start and covers all the cases that caused me issues in my last experience.

I'll happily kick the wheels with what we've got with a view to QAing; I'm confident I won't find every issue, but feel we're still early enough in the cycle to push if we push soon and sort out any fallout... I think devs' should quickly spot potential issues once it's in master.

misc/admin/koha-preferences
-$YAML::Syck::SortKeys = 1;

=> This line needs attention.

There are some issues in misc/admin/koha-preferences:
- It now allows to set any value for boolean (YesNo) preferences. In master there is a warning message and value is set to 0 when an invalid value is given
- documentation probably needs an update ("If the syspref is YesNo, will accept only a boolean value, but the syntax for these is fairly lax (yes/no, on/off, 1/0, n/y, true/false are all accepted).")
- 'dump' subcommand does not work, it just prints 'HASH(0x12345)'.

Created attachment 116881 [details] [review]
Bug 22824: Adjust POD for koha-preferences

Created attachment 116882 [details] [review]
Bug 22824: Fix koha-preferences dump

(In reply to Julian Maurice from comment #36)
> There are some issues in misc/admin/koha-preferences:
> - It now allows to set any value for boolean (YesNo) preferences. In master
> there is a warning message and value is set to 0 when an invalid value is
> given

I think we are good with the warning I added on the about page, don't you think?
I don't really want to reintroduce the C4::Boolean module (or equivalent code) only for this script.

> - documentation probably needs an update ("If the syspref is YesNo, will
> accept only a boolean value, but the syntax for these is fairly lax (yes/no,
> on/off, 1/0, n/y, true/false are all accepted).")

Adjusted.

> - 'dump' subcommand does not work, it just prints 'HASH(0x12345)'.

Oops, fixed!

(In reply to Jonathan Druart from comment #39)
> (In reply to Julian Maurice from comment #36)
> > There are some issues in misc/admin/koha-preferences:
> > - It now allows to set any value for boolean (YesNo) preferences. In master
> > there is a warning message and value is set to 0 when an invalid value is
> > given
> 
> I think we are good with the warning I added on the about page, don't you
> think?
> I don't really want to reintroduce the C4::Boolean module (or equivalent
> code) only for this script.

I think the script should prevent to set wrong value for those sysprefs. It could at least check that the value given is either "0" or "1".

Created attachment 116883 [details] [review]
Bug 22824: Prevent YesNo to be filled with invalid values

This patch prevents koha-preferences to set other values than 1/0 to be
used for YesNo prefs

(In reply to Jonathan Druart from comment #41)
> Created attachment 116883 [details] [review] [review]
> Bug 22824: Prevent YesNo to be filled with invalid values
> 
> This patch prevents koha-preferences to set other values than 1/0 to be
> used for YesNo prefs

This patch does not work.
If I execute
  misc/admin/koha-preferences set viewMARC 2
it warns "System preference viewMARC is YesNo and expects 1 or 0. '2' was given, using '0' (OK)
but the value is set to '2'. (NOT OK)

And the dump subcommand output is different from master. On master it prints a serie of
  <sysprefName>: <sysprefValue>
but with the patches it prints
  <sysprefName>:
    type: <sysprefType>
    value: <sysprefValue>
    variable: <sysprefName>

which prevents to do things like:

  koha-preferences dump | koha-preferences load

Created attachment 116884 [details] [review]
Bug 22824: Prevent YesNo to be filled with invalid values

This patch prevents koha-preferences to set other values than 1/0 to be
used for YesNo prefs

(In reply to Julian Maurice from comment #42)
> (In reply to Jonathan Druart from comment #41)
> > Created attachment 116883 [details] [review] [review] [review]
> > Bug 22824: Prevent YesNo to be filled with invalid values
> > 
> > This patch prevents koha-preferences to set other values than 1/0 to be
> > used for YesNo prefs
> 
> This patch does not work.
> If I execute
>   misc/admin/koha-preferences set viewMARC 2
> it warns "System preference viewMARC is YesNo and expects 1 or 0. '2' was
> given, using '0' (OK)
> but the value is set to '2'. (NOT OK)

Right, I tested it very poorly, sorry about that.
Should work now.

> And the dump subcommand output is different from master. On master it prints
> a serie of
>   <sysprefName>: <sysprefValue>
> but with the patches it prints
>   <sysprefName>:
>     type: <sysprefType>
>     value: <sysprefValue>
>     variable: <sysprefName>
> 
> which prevents to do things like:
> 
>   koha-preferences dump | koha-preferences load

The dump|load actually worked, but not using a dump generated from master.
I restored the current behaviour.

Sorry, but I think there is still a problem with dump/load

If I do
  koha-preferences dump | koha-preferences load

I get an error
  YAML::XS::Load Error: The problem:
    invalid trailing UTF-8 octet
  was found at document: 1

If I do
  koha-preferences dump > prefs && file prefs
I get
  prefs: ISO-8859 text, with very long lines

but If I do
  koha-preferences dump -o prefs && file prefs
I get
  prefs: UTF-8 Unicode text, with very long lines

If I remove the call to decode_utf8 in dump subcommand, it works (ie. I get UTF-8 Unicode in both cases, and dump|load seems to work)

Hum

$ perl misc/admin/koha-preferences dump | perl misc/admin/koha-preferences load
It works

$ perl misc/admin/koha-preferences dump > prefs && file prefs
prefs: ASCII text

indeed, but edit URLLinkText, set "✔ ❤️ ★"

$ perl misc/admin/koha-preferences dump > prefs && file prefs
prefs: UTF-8 Unicode text

$ perl misc/admin/koha-preferences dump | perl misc/admin/koha-preferences load
Still works

Edit URLLinkText, it contains the characters in the correct encoding.

Can you try adding
  binmode STDOUT, ':encoding(UTF-8)';
before Dump?

Switching back to NSO hoping to get other feedbacks.

If I set "✔ ❤️ ★" to a syspref, `dump > prefs` give me a "wide-char-in-print" warning.
I think I found a fix and will attach the patch.

Created attachment 117037 [details] [review]
Bug 22824: Fix encoding of misc/admin/koha-preferences output

Created attachment 117039 [details] [review]
Bug 22824: Fix encoding of misc/admin/koha-preferences output

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 117040 [details] [review]
Bug 22824: Add tests

Writing some tests to see how things are working currently

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 117041 [details] [review]
Bug 22824: Replace YAML::Syck with YAML::XS in tests

In order to understand how things will work after the changes

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 117042 [details] [review]
Bug 22824: Fixing first error in tests

There was an encoding issue. The parent bug report have been fixed, see
bug 27673 and patches related to Encode

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 117043 [details] [review]
Bug 22824: Replace YAML::Syck with YAML::XS

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 117044 [details] [review]
Bug 22824: Fix searching.pref

One .pref file make YAML::XS fails.
The correct display must be, before and after this patchset:
 If "Unescape escaped" is selected this will allow writing regular expressions "\/like this\/" while "/this/", "or/this" will be escaped and interpreted as regular strings.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 117045 [details] [review]
Bug 22824: Correct on/off behaviour

Here things are starting to be a bit tricky.
IMO we don't want to replicate the ImplicitTyping behaviour from
YAML::Syck but handle correctly the different value.
We have the list of available values in the .pref file, the values in DB
must match them.

We want 1 or 0 in DB.

2 more occurrences:
finesMode => Dealt on bug 27676 ('off' is ok here)
itemBarcodeInputFilter => 'OFF' is correct here

Note that we don't use ~ or true/false in *.pref

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 117046 [details] [review]
Bug 22824: Fix yes/no vs 1/0

Patch generated with:
 perl -p -i -e 's# no: # 0: #' **/*.pref
 perl -p -i -e 's# yes: # 1: #' **/*.pref

no must be 0 and yes must be 1!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 117047 [details] [review]
Bug 22824: Replace wrong auto replacement

This must be "no", it has been caught by the previous sed

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 117048 [details] [review]
Bug 22824: Remove C4::Boolean, true_p, boolean_preference, etc.

It seems that we don't really need all this overhead.
YesNo must be a boolean and contain 1 or 0.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 117049 [details] [review]
Bug 22824: Fix default values for YesNo

select variable, value from systempreferences where type="YesNo" and ( ( value <>'1' and value <>'0' ) or value is null );

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 117050 [details] [review]
Bug 22824: Atomic update

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 117051 [details] [review]
Bug 22824: Add warning to the about page

Just in case, add a warning to the about page.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 117052 [details] [review]
Bug 22824: Remove YAML::Syck dependency

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 117053 [details] [review]
Bug 22824: Adjust tests to match final behaviours

Finally we diverge quite a lot with how it worked prior to this patch
set. Reflect the changes in the tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 117054 [details] [review]
Bug 22824: Fix ES reindex

koha-elastic --rebuild was failing with
Unable to update mappings for index "koha_kohadev_biblios". Reason was: "Could not convert [marc_data.index] to boolean"

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 117055 [details] [review]
Bug 22824: Adjust POD for koha-preferences

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 117056 [details] [review]
Bug 22824: Fix koha-preferences dump

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 117057 [details] [review]
Bug 22824: Prevent YesNo to be filled with invalid values

This patch prevents koha-preferences to set other values than 1/0 to be
used for YesNo prefs

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 117058 [details] [review]
Bug 22824: Fix encoding of misc/admin/koha-preferences output

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Created attachment 117636 [details] [review]
Bug 22824: Add tests

Writing some tests to see how things are working currently

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 117637 [details] [review]
Bug 22824: Replace YAML::Syck with YAML::XS in tests

In order to understand how things will work after the changes

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 117638 [details] [review]
Bug 22824: Fixing first error in tests

There was an encoding issue. The parent bug report have been fixed, see
bug 27673 and patches related to Encode

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 117639 [details] [review]
Bug 22824: Replace YAML::Syck with YAML::XS

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 117640 [details] [review]
Bug 22824: Fix searching.pref

One .pref file make YAML::XS fails.
The correct display must be, before and after this patchset:
 If "Unescape escaped" is selected this will allow writing regular expressions "\/like this\/" while "/this/", "or/this" will be escaped and interpreted as regular strings.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 117641 [details] [review]
Bug 22824: Correct on/off behaviour

Here things are starting to be a bit tricky.
IMO we don't want to replicate the ImplicitTyping behaviour from
YAML::Syck but handle correctly the different value.
We have the list of available values in the .pref file, the values in DB
must match them.

We want 1 or 0 in DB.

2 more occurrences:
finesMode => Dealt on bug 27676 ('off' is ok here)
itemBarcodeInputFilter => 'OFF' is correct here

Note that we don't use ~ or true/false in *.pref

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 117642 [details] [review]
Bug 22824: Fix yes/no vs 1/0

Patch generated with:
 perl -p -i -e 's# no: # 0: #' **/*.pref
 perl -p -i -e 's# yes: # 1: #' **/*.pref

no must be 0 and yes must be 1!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 117643 [details] [review]
Bug 22824: Replace wrong auto replacement

This must be "no", it has been caught by the previous sed

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 117644 [details] [review]
Bug 22824: Remove C4::Boolean, true_p, boolean_preference, etc.

It seems that we don't really need all this overhead.
YesNo must be a boolean and contain 1 or 0.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 117645 [details] [review]
Bug 22824: Fix default values for YesNo

select variable, value from systempreferences where type="YesNo" and ( ( value <>'1' and value <>'0' ) or value is null );

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 117646 [details] [review]
Bug 22824: Atomic update

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 117647 [details] [review]
Bug 22824: Add warning to the about page

Just in case, add a warning to the about page.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 117648 [details] [review]
Bug 22824: Adjust tests to match final behaviours

Finally we diverge quite a lot with how it worked prior to this patch
set. Reflect the changes in the tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 117649 [details] [review]
Bug 22824: Fix ES reindex

koha-elastic --rebuild was failing with
Unable to update mappings for index "koha_kohadev_biblios". Reason was: "Could not convert [marc_data.index] to boolean"

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 117650 [details] [review]
Bug 22824: Adjust POD for koha-preferences

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 117651 [details] [review]
Bug 22824: Fix koha-preferences dump

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 117652 [details] [review]
Bug 22824: Prevent YesNo to be filled with invalid values

This patch prevents koha-preferences to set other values than 1/0 to be
used for YesNo prefs

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 117653 [details] [review]
Bug 22824: Fix encoding of misc/admin/koha-preferences output

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Manual testing and I can't find any issues.. this is a great patchset and covers all the usual cases I've come across when making similar moves elsewhere.

Couple of QA script failures.. the first one has me confused:

FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/about.tt       
   FAIL   valid_template                                            
                parse error - /home/martin/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/about.tt line 210: unexpected token ( warnNoActiveCurrency)
  [% IF warnPrefBiblioAddsAuthorities || warnPrefEasyAnalyticalRecords || warnPrefAnonymousPatronOPACPrivacy || warnPrefAnonymousPatronAnonSuggestions || warnPrefAnonymousPatronOPACPrivacy_PatronDoesNotExist || warnPrefAnonymousPatronAnonSuggestions_PatronDoesNotExist || warnPrefKohaAdminEmailAddress || invalid_yesn
o.count || warnNoActiveCurrency || warnIsRootUser || xml_config_warnings.size || AutoSelfCheckPatronDoesNotHaveSelfCheckPerm || AutoSelfCheckPatronHasTooManyPerm || warnStatisticsFieldsError || warnNoTemplateCaching || warnILLConfiguration || has_ai_issues || oauth2_missing_deps || bad_yaml_prefs || warnRelationship
s || log4perl_errors || config_bcrypt_settings_no_set || warnHiddenBiblionumbers.size || warnConnectBroker %]

The second one is a false positive and can be ignored:

FAIL   t/db_dependent/yaml.t                             
   FAIL   spelling                                             
                 dont  ==> don't

Also.. I dropped a patch during rebase (the removal of YAML::Syck from cpanfile as it's already done in a followup to bug 27673)

(In reply to Martin Renvoize from comment #88)
> FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/about.tt       
>    FAIL   valid_template                                            
I saw this one too. There is a space that is not really a space, probably a non-breaking space. I don't why know it breaks the tests whereas the template works fine otherwise.

Created attachment 117664 [details] [review]
Bug 22824: Add warning to the about page

Just in case, add a warning to the about page.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

JD amended patch: there was a "bad space" between "warnNoActiveCurrency"

(In reply to Jonathan Druart from comment #91)
> Created attachment 117664 [details] [review] [review]
> Bug 22824: Add warning to the about page
> 
> Just in case, add a warning to the about page.
> 
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
> 
> JD amended patch: there was a "bad space" between "warnNoActiveCurrency"

^between^before^

Created attachment 117666 [details] [review]
Bug 22824: Add warning to the about page

Just in case, add a warning to the about page.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

JD amended patch: there was a "bad space" between "warnNoActiveCurrency"

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

Excellent, that final patch squashed the warn in the QA script.

Going for PQA here. It wouldn't surprise me if there's something we've missed somewhere, but I don't think we're going to spot it here and now and we're better off pushing now and fixing an issue down the line during this cycle when/if we spot it.. with all the devs running this branch I'm sure it'll "come out in the wash if there is anything lurking"

Great work Jonathan, Koha's really moving forward at the moment, modernising and cleaning code.. great to see!

Pushed to master for 21.05, thanks to everybody involved!

Hi all,

When I went to master in my devbox, I got this error when restarting services and my sites wouldn't load:

BEGIN failed--compilation aborted at /etc/koha/sites/kohadev/plack.psgi line 29.
Error while loading /etc/koha/sites/kohadev/plack.psgi: Can't locate C4/Boolean.pm in @INC (you may need to install the C4::Boolean module) (@INC contains: /home/vagrant/kohaclone /home/vagrant/kohaclone/installer /home/vagrant/kohaclone/lib/installer /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 /usr/local/share/perl/5.24.1 /usr/lib/x86_64-linux-gnu/perl5/5.24 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24 /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base) at /etc/koha/sites/kohadev/plack.psgi line 29.

I removed the use line in /etc/koha/sites/kohadev/plack.psgi, but is there something else I should have done? And will this happen for other Kohas?

Cheers,
Hayley

(In reply to Hayley Pelham from comment #96)
> Hi all,
> 
> When I went to master in my devbox, I got this error when restarting
> services and my sites wouldn't load:
> 
> BEGIN failed--compilation aborted at /etc/koha/sites/kohadev/plack.psgi line
> 29.
> Error while loading /etc/koha/sites/kohadev/plack.psgi: Can't locate
> C4/Boolean.pm in @INC (you may need to install the C4::Boolean module) (@INC
> contains: /home/vagrant/kohaclone /home/vagrant/kohaclone/installer
> /home/vagrant/kohaclone/lib/installer /etc/perl
> /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 /usr/local/share/perl/5.24.1
> /usr/lib/x86_64-linux-gnu/perl5/5.24 /usr/share/perl5
> /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24
> /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base) at
> /etc/koha/sites/kohadev/plack.psgi line 29.
> 
> I removed the use line in /etc/koha/sites/kohadev/plack.psgi, but is there
> something else I should have done? And will this happen for other Kohas?
> 
> Cheers,
> Hayley

reset_all should do the job of replacing the old plack.psgi.

(In reply to Tomás Cohen Arazi from comment #97)
> (In reply to Hayley Pelham from comment #96)
> > Hi all,
> > 
> > When I went to master in my devbox, I got this error when restarting
> > services and my sites wouldn't load:
> > 
> > BEGIN failed--compilation aborted at /etc/koha/sites/kohadev/plack.psgi line
> > 29.
> > Error while loading /etc/koha/sites/kohadev/plack.psgi: Can't locate
> > C4/Boolean.pm in @INC (you may need to install the C4::Boolean module) (@INC
> > contains: /home/vagrant/kohaclone /home/vagrant/kohaclone/installer
> > /home/vagrant/kohaclone/lib/installer /etc/perl
> > /usr/local/lib/x86_64-linux-gnu/perl/5.24.1 /usr/local/share/perl/5.24.1
> > /usr/lib/x86_64-linux-gnu/perl5/5.24 /usr/share/perl5
> > /usr/lib/x86_64-linux-gnu/perl/5.24 /usr/share/perl/5.24
> > /usr/local/lib/site_perl /usr/lib/x86_64-linux-gnu/perl-base) at
> > /etc/koha/sites/kohadev/plack.psgi line 29.
> > 
> > I removed the use line in /etc/koha/sites/kohadev/plack.psgi, but is there
> > something else I should have done? And will this happen for other Kohas?
> > 
> > Cheers,
> > Hayley
> 
> reset_all should do the job of replacing the old plack.psgi.

Ah, of course! Gracias :)

Enhancement not pushed to 20.11.x

@@ -78,14 +78,14 @@ Web services:
               choices:
                   persistent: will never be emptied or truncated (persistent)
                   transient: might be emptied or truncated at some point (transient)
-                  no: will never have any data in it (no)
+                  0: will never have any data in it (no)

Isn't this replacement wrong?