Created attachment 160439 [details] [review]
Bug 35681: [TEST ONLY]: This is a dummy dbrev to force an update

This patch adds an atomic update file and also an equivilent db_rev and
increments the Koha.pm version too.

Use it to test the output of updatedatebase, run via console or via the
browser.

Test plan
1) Start KTD without the patches applied.
2) Apply this test patch
3) Run the detabase updates via the console and confirm it looks
   reasonable and is colored.
4) Reset the database version in ktd
   `koha-mysql kohadev`
   `UPDATE systempreferences SET value = '23.1200002';`
5) Run the database updates via the browser;
   `http://localhost:8081/cgi-bin/koha/installer/install.pl?step=3&op=updatestructure`
6) Confirm the presence of weird looking escape characters on screen.
7) Apply the next patch and run through steps 3 to 5 again, this time
   confirming the browser output looks good.

Created attachment 160440 [details] [review]
Bug 35681: Allow for ANSI color in database updates

This patch adds support for colored output in database updates.

Created attachment 160443 [details] [review]
Bug 35681: [TEST ONLY]: This is a dummy dbrev to force an update

This patch adds an atomic update file and also an equivilent db_rev and
increments the Koha.pm version too.

Use it to test the output of updatedatebase, run via console or via the
browser.

Test plan
1) Start KTD without the patches applied.
2) Apply this test patch
3) Run the detabase updates via the console and confirm it looks
   reasonable and is colored.
4) Reset the database version in ktd
   `koha-mysql kohadev`
   `UPDATE systempreferences SET value = '23.1200002';`
5) Run the database updates via the browser;
   `http://localhost:8081/cgi-bin/koha/installer/install.pl?step=3&op=updatestructure`
6) Confirm the presence of weird looking escape characters on screen.
7) Apply the next patch and run through steps 3 to 5 again, this time
   confirming the browser output looks good.

Created attachment 160444 [details] [review]
Bug 35681: Allow for ANSI color in database updates

This patch adds support for colored output in database updates.

Created attachment 160445 [details] [review]
Bug 35681: Add new perl dependancy

*** Bug 35682 has been marked as a duplicate of this bug. ***

Created attachment 160447 [details] [review]
Bug 35681: [TEST ONLY]: This is a dummy dbrev to force an update

This patch adds an atomic update file and also an equivilent db_rev and
increments the Koha.pm version too.

Use it to test the output of updatedatebase, run via console or via the
browser.

Test plan
1) Start KTD without the patches applied.
2) Apply this test patch
3) Run the detabase updates via the console and confirm it looks
   reasonable and is colored.
4) Reset the database version in ktd
   `koha-mysql kohadev`
   `UPDATE systempreferences SET value = '23.1200002';`
5) Run the database updates via the browser;
   `http://localhost:8081/cgi-bin/koha/installer/install.pl?step=3&op=updatestructure`
6) Confirm the presence of weird looking escape characters on screen.
7) Apply the next patch and run through steps 3 to 5 again, this time
   confirming the browser output looks good.

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Created attachment 160448 [details] [review]
Bug 35681: Allow for ANSI color in database updates

This patch adds support for colored output in database updates.

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Created attachment 160449 [details] [review]
Bug 35681: Add new perl dependancy

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Adding my SO as having tested but leaving as 'Needs Sign-off' for independent party.

If testing on k-t-d the new cpanfile dependency can be installed individually:
sudo cpanm HTML::FromANSI::Tiny

It looks like the HTML is generated by HTML::FromANSI::Tiny? 

<span style="color: #f33;">Something in red is a warning</span>

It would be preferable to have it use a class instead of a style attribute so that we can customize the colors. The default red, yellow, and white options do not have adequate contrast on a white background.

Created attachment 160473 [details] [review]
Bug 35681: Use CSS class instead of inline style

Created attachment 160503 [details] [review]
Bug 35681: [TEST ONLY]: This is a dummy dbrev to force an update

This patch adds an atomic update file and also an equivilent db_rev and
increments the Koha.pm version too.

Use it to test the output of updatedatebase, run via console or via the
browser.

Test plan
1) Start KTD without the patches applied.
2) Apply this test patch
3) Run the detabase updates via the console and confirm it looks
   reasonable and is colored.
4) Reset the database version in ktd
   `koha-mysql kohadev`
   `UPDATE systempreferences SET value = '23.1200002';`
5) Run the database updates via the browser;
   `http://localhost:8081/cgi-bin/koha/installer/install.pl?step=3&op=updatestructure`
6) Confirm the presence of weird looking escape characters on screen.
7) Apply the next patch and run through steps 3 to 5 again, this time
   confirming the browser output looks good.

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: David Nind <david@davidnind.com>

Created attachment 160504 [details] [review]
Bug 35681: Allow for ANSI color in database updates

This patch adds support for colored output in database updates.

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: David Nind <david@davidnind.com>

Created attachment 160505 [details] [review]
Bug 35681: Add new perl dependancy

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: David Nind <david@davidnind.com>

Created attachment 160506 [details] [review]
Bug 35681: Use CSS class instead of inline style

Signed-off-by: David Nind <david@davidnind.com>

Testing notes (using KTD):

1. To get the colours to show in the browser update after applying the patches: yarn build

2. Not a biggy, or a showstopper, but maybe the class names could be more semantic? Not sure if there are any rules/guidelines for this.
   .updatedatabase-report-red  -warning
   .updatedatabase-report-green -? (not sure for this one)
   .updatedatabase-report-yellow -action-required
   .updatedatabase-report-blue -information

3. Also, there is no class .updatedatabase-report-white

That sounds trivial, but is actually rather difficult due to the way they're created. I'd like to eventually switch them to use the standard bookstrap classes,  it right now there no way to achieve it with the library we're using to convert ansi to html.

Created attachment 160750 [details] [review]
Bug 35681: [ALT] Use ::Bootstrap version of FromANSI

With the next iteration of HTML::FromANSI::Tiny we can add our own subclass
to map ANSI strings to Bootstrap classes.

This patch adds a local lib HTML::FromANSI::Tiny::Bootstrap module to
do said mapping and then uses it in the installer.

Created attachment 160751 [details] [review]
Bug 35681: [TESTING] Add local copy of latest Tiny

This temporary patch is for testing only.. we add a local copy of the updated version
of HTML::FromANSI::Tiny to allow simple testing without having to manually install
the pre-release module version.

I have an alternative to the follow-up that switched us from inline styles to custom classes.

After I opened an issue against the upstream module about mapping ANSI codes to local Classes, he quickly replied with a change to review.

It works great, so I've added an alternative patch here that utilises it. The [ALT] patch is meant to replace 'Use CSS class instead of inline style' in the patchset and then the [TESTING] patch that follows it is to simplify testing whilst we wait for the upstream change to be merged (I take a local copy of the adapted class into the local koha lib so you no longer need to install from cpan to test.

Created attachment 160772 [details] [review]
Bug 35681: Add to template

This patch adds colors to the skeleton atomic update to hint at their
use

As of Jan 11 it has been added upstream:
https://metacpan.org/dist/HTML-FromANSI-Tiny/changes

I think we can:
- Update patch "Add new perl dependancy" version to 0.107 instead of 1.05
- Remove [ALT] from commit message [ALT] Use ::Bootstrap version of FromANSI as I've made the other patches obsolete and this is no longer an alternative patch.
- Obsolete the [TESTING] patch now that upstream implements it?

I like this. How about adding some trivial methods like:

say_warning
say_failure
say_success
say_info

Created attachment 164903 [details] [review]
Bug 35681: Allow for ANSI color in database updates

This patch adds support for colored output in database updates.

Created attachment 164904 [details] [review]
Bug 35681: Add new perl dependancy

Created attachment 164905 [details] [review]
Bug 35681: Use ::Bootstrap version of FromANSI

With the next iteration of HTML::FromANSI::Tiny we can add our own subclass
to map ANSI strings to Bootstrap classes.

This patch adds a local lib HTML::FromANSI::Tiny::Bootstrap module to
do said mapping and then uses it in the installer.

Created attachment 164906 [details] [review]
Bug 35681: [TESTING] Add local copy of latest Tiny

This temporary patch is for testing only.. we add a local copy of the updated version
of HTML::FromANSI::Tiny to allow simple testing without having to manually install
the pre-release module version.

Created attachment 164907 [details] [review]
Bug 35681: Add to template

This patch adds colors to the skeleton atomic update to hint at their
use

Created attachment 164908 [details] [review]
Bug 35681: Add Koha::Installer::Output

Created attachment 164909 [details] [review]
Bug 35681: Use Koha::Installer::Output in template

As requested I added nice wrapper methods to keep it clear for developers how to use the new colors.. the new module is inside the Koha namespace and is covered by unit tests.

Created attachment 164942 [details] [review]
Bug 35681: Allow for ANSI color in database updates

This patch adds support for colored output in database updates.

Created attachment 164943 [details] [review]
Bug 35681: Add new perl dependancy

Created attachment 164944 [details] [review]
Bug 35681: Use ::Bootstrap version of FromANSI

With the next iteration of HTML::FromANSI::Tiny we can add our own subclass
to map ANSI strings to Bootstrap classes.

This patch adds a local lib HTML::FromANSI::Tiny::Bootstrap module to
do said mapping and then uses it in the installer.

Created attachment 164945 [details] [review]
Bug 35681: [TESTING] Add local copy of latest Tiny

This temporary patch is for testing only.. we add a local copy of the updated version
of HTML::FromANSI::Tiny to allow simple testing without having to manually install
the pre-release module version.

Created attachment 164946 [details] [review]
Bug 35681: Add to template

This patch adds colors to the skeleton atomic update to hint at their
use

Created attachment 164947 [details] [review]
Bug 35681: Add Koha::Installer::Output

Created attachment 164948 [details] [review]
Bug 35681: Use Koha::Installer::Output in template

Created attachment 164949 [details] [review]
Bug 35681: (follow-up) Pass filehandle in skeleton.pl

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Good job!

As this adds a new dependency, we need to check with our Packaging team/manager before this is pushed to main. As this doesn't appear to be packaged for Debian, I assume it will require additional work.

Re-setting to signed-off for now.

(In reply to Katrin Fischer from comment #42)
> As this adds a new dependency, we need to check with our Packaging
> team/manager before this is pushed to main. As this doesn't appear to be
> packaged for Debian, I assume it will require additional work.
> 
> Re-setting to signed-off for now.

i'll aim to add an HTML::FromANSI::Tiny package to debian.org

(In reply to Mason James from comment #43)
> (In reply to Katrin Fischer from comment #42)
> > As this adds a new dependency, we need to check with our Packaging
> > team/manager before this is pushed to main. As this doesn't appear to be
> > packaged for Debian, I assume it will require additional work.
> > 
> > Re-setting to signed-off for now.
> 
> i'll aim to add an HTML::FromANSI::Tiny package to debian.org

package is pushed to koha-staging repo, ready for testing

Thanks Mason, what is the next step here now? (who tests what?)

Next step is to push to main

(In reply to Martin Renvoize from comment #46)
> Next step is to push to main

We still want to document these :)

Pushed for 24.05!

Well done everyone, thank you!

Then? We need to adjust all the db revs now.

(In reply to Jonathan Druart from comment #49)
> Then? We need to adjust all the db revs now.

I think that could be nice to do. We are at 57 I believe...

Not backported to 23.11.x

If we don't backport this to 23.11 we also need to make sure that other patches that are backported to 23.11, such as bug 36986 and bug 26176 don't use this.

I'm talking about Koha::Installer::Output.

(In reply to Andreas Jonsson from comment #52)
> If we don't backport this to 23.11 we also need to make sure that other
> patches that are backported to 23.11, such as bug 36986 and bug 26176 don't
> use this.

+1

I was thinking how this doesn't fit with the "PERL14: Exports" and "PERL15: Object-oriented code and the Koha:: namespace" coding guidelines.

I suppose an argument could be made that this is one of the exceptions indicated in the guidelines, but it probably could've been done like this:

my $output = Koha::Installer::Output->new();
$output->success($out,"Test");

With the $output just being created at the top of the "up" sub. 

Too late now but just a thought.

No updates are required to the manual.

The installer/data/mysql/atomicupdate/skeleton.pl includes some information.

However, I think the Wiki database update page requires updating (https://wiki.koha-community.org/wiki/Database_updates). Perhaps with an example of how to use, or to incorporate into the existing examples.

(In reply to David Nind from comment #56)
> No updates are required to the manual.
> 
> The installer/data/mysql/atomicupdate/skeleton.pl includes some information.
> 
> However, I think the Wiki database update page requires updating
> (https://wiki.koha-community.org/wiki/Database_updates). Perhaps with an
> example of how to use, or to incorporate into the existing examples.

We just discussed it at last meeting - we had discussed to make using mandatory, but were left with some compatibility questions.

(In reply to Katrin Fischer from comment #57)
> (In reply to David Nind from comment #56)
> > No updates are required to the manual.
> > 
> > The installer/data/mysql/atomicupdate/skeleton.pl includes some information.
> > 
> > However, I think the Wiki database update page requires updating
> > (https://wiki.koha-community.org/wiki/Database_updates). Perhaps with an
> > example of how to use, or to incorporate into the existing examples.
> 
> We just discussed it at last meeting - we had discussed to make using
> mandatory, but were left with some compatibility questions.

Thanks Katrin!

I'll leave any updates to the pages to developers.

Link to minutes: https://hedgedoc.ishukone.net/Development_IRC_meeting_3_July_2024#