Bug 35681 - Add support for colored messages in the output of updatedatabase
Summary: Add support for colored messages in the output of updatedatabase
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (web-based installer) (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Tomás Cohen Arazi
URL:
Keywords: dependency
: 35682 (view as bug list)
Depends on:
Blocks: 35539 35570
  Show dependency treegraph
 
Reported: 2024-01-02 15:23 UTC by Martin Renvoize
Modified: 2024-06-18 06:00 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement adds support for colored messages for Koha database updates, in both the terminal and browser. These new CSS classes are used: .updatedatabase-report-red for warnings .updatedatabase-report-green for ? .updatedatabase-report-yellow where action is required .updatedatabase-report-blue for information **Sponsored by** *PTFS Europe*
Version(s) released in:
24.05.00


Attachments
Bug 35681: [TEST ONLY]: This is a dummy dbrev to force an update (3.44 KB, patch)
2024-01-02 20:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35681: Allow for ANSI color in database updates (4.55 KB, patch)
2024-01-02 20:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35681: [TEST ONLY]: This is a dummy dbrev to force an update (3.50 KB, patch)
2024-01-03 09:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35681: Allow for ANSI color in database updates (4.46 KB, patch)
2024-01-03 09:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35681: Add new perl dependancy (683 bytes, patch)
2024-01-03 09:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35681: [TEST ONLY]: This is a dummy dbrev to force an update (3.55 KB, patch)
2024-01-03 10:32 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35681: Allow for ANSI color in database updates (4.51 KB, patch)
2024-01-03 10:32 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35681: Add new perl dependancy (740 bytes, patch)
2024-01-03 10:32 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35681: Use CSS class instead of inline style (1.73 KB, patch)
2024-01-03 15:22 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35681: [TEST ONLY]: This is a dummy dbrev to force an update (3.60 KB, patch)
2024-01-03 22:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 35681: Allow for ANSI color in database updates (4.56 KB, patch)
2024-01-03 22:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 35681: Add new perl dependancy (788 bytes, patch)
2024-01-03 22:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 35681: Use CSS class instead of inline style (1.77 KB, patch)
2024-01-03 22:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 35681: [ALT] Use ::Bootstrap version of FromANSI (2.41 KB, patch)
2024-01-10 11:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35681: [TESTING] Add local copy of latest Tiny (836 bytes, patch)
2024-01-10 11:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35681: Add to template (1.15 KB, patch)
2024-01-10 14:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35681: Allow for ANSI color in database updates (4.46 KB, patch)
2024-04-16 11:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35681: Add new perl dependancy (683 bytes, patch)
2024-04-16 11:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35681: Use ::Bootstrap version of FromANSI (3.33 KB, patch)
2024-04-16 11:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35681: [TESTING] Add local copy of latest Tiny (836 bytes, patch)
2024-04-16 11:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35681: Add to template (1.15 KB, patch)
2024-04-16 11:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35681: Add Koha::Installer::Output (5.11 KB, patch)
2024-04-16 11:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35681: Use Koha::Installer::Output in template (1.38 KB, patch)
2024-04-16 11:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35681: Allow for ANSI color in database updates (4.46 KB, patch)
2024-04-16 14:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35681: Add new perl dependancy (683 bytes, patch)
2024-04-16 14:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35681: Use ::Bootstrap version of FromANSI (3.33 KB, patch)
2024-04-16 14:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35681: [TESTING] Add local copy of latest Tiny (836 bytes, patch)
2024-04-16 14:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35681: Add to template (1.15 KB, patch)
2024-04-16 14:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35681: Add Koha::Installer::Output (5.11 KB, patch)
2024-04-16 14:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35681: Use Koha::Installer::Output in template (1.38 KB, patch)
2024-04-16 14:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35681: (follow-up) Pass filehandle in skeleton.pl (1.22 KB, patch)
2024-04-16 14:24 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2024-01-02 15:23:01 UTC
We could use Term::ANSIColor in our database revisions to aid in highlighting different outputs, but in doing so we introduce escape sequences that look 'nasty' in the browser representation.

We should add support for those sequences, allowing the same end user experience via browser or terminal.
Comment 1 Martin Renvoize 2024-01-02 20:54:54 UTC
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.
Comment 2 Martin Renvoize 2024-01-02 20:54:57 UTC
Created attachment 160440 [details] [review]
Bug 35681: Allow for ANSI color in database updates

This patch adds support for colored output in database updates.
Comment 3 Martin Renvoize 2024-01-03 09:37:20 UTC
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.
Comment 4 Martin Renvoize 2024-01-03 09:37:22 UTC
Created attachment 160444 [details] [review]
Bug 35681: Allow for ANSI color in database updates

This patch adds support for colored output in database updates.
Comment 5 Martin Renvoize 2024-01-03 09:38:57 UTC
Created attachment 160445 [details] [review]
Bug 35681: Add new perl dependancy
Comment 6 Pedro Amorim 2024-01-03 10:31:48 UTC
*** Bug 35682 has been marked as a duplicate of this bug. ***
Comment 7 Pedro Amorim 2024-01-03 10:32:45 UTC
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>
Comment 8 Pedro Amorim 2024-01-03 10:32:48 UTC
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>
Comment 9 Pedro Amorim 2024-01-03 10:32:50 UTC
Created attachment 160449 [details] [review]
Bug 35681: Add new perl dependancy

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 10 Pedro Amorim 2024-01-03 10:34:27 UTC
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
Comment 11 Owen Leonard 2024-01-03 14:46:46 UTC
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.
Comment 12 Pedro Amorim 2024-01-03 15:22:21 UTC
Created attachment 160473 [details] [review]
Bug 35681: Use CSS class instead of inline style
Comment 13 David Nind 2024-01-03 22:53:40 UTC
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>
Comment 14 David Nind 2024-01-03 22:53:42 UTC
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>
Comment 15 David Nind 2024-01-03 22:53:45 UTC
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>
Comment 16 David Nind 2024-01-03 22:53:47 UTC
Created attachment 160506 [details] [review]
Bug 35681: Use CSS class instead of inline style

Signed-off-by: David Nind <david@davidnind.com>
Comment 17 David Nind 2024-01-03 23:08:18 UTC
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
Comment 18 Martin Renvoize 2024-01-04 06:52:56 UTC
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.
Comment 19 Martin Renvoize 2024-01-10 11:01:13 UTC
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.
Comment 20 Martin Renvoize 2024-01-10 11:01:15 UTC
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.
Comment 21 Martin Renvoize 2024-01-10 11:07:27 UTC
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.
Comment 22 Martin Renvoize 2024-01-10 14:22:59 UTC
Created attachment 160772 [details] [review]
Bug 35681: Add to template

This patch adds colors to the skeleton atomic update to hint at their
use
Comment 23 Pedro Amorim 2024-01-31 16:20:32 UTC
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?
Comment 24 Tomás Cohen Arazi 2024-04-01 18:48:08 UTC
I like this. How about adding some trivial methods like:

say_warning
say_failure
say_success
say_info
Comment 25 Martin Renvoize 2024-04-16 11:23:45 UTC
Created attachment 164903 [details] [review]
Bug 35681: Allow for ANSI color in database updates

This patch adds support for colored output in database updates.
Comment 26 Martin Renvoize 2024-04-16 11:23:48 UTC
Created attachment 164904 [details] [review]
Bug 35681: Add new perl dependancy
Comment 27 Martin Renvoize 2024-04-16 11:23:50 UTC
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.
Comment 28 Martin Renvoize 2024-04-16 11:23:53 UTC
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.
Comment 29 Martin Renvoize 2024-04-16 11:23:56 UTC
Created attachment 164907 [details] [review]
Bug 35681: Add to template

This patch adds colors to the skeleton atomic update to hint at their
use
Comment 30 Martin Renvoize 2024-04-16 11:23:58 UTC
Created attachment 164908 [details] [review]
Bug 35681: Add Koha::Installer::Output
Comment 31 Martin Renvoize 2024-04-16 11:24:01 UTC
Created attachment 164909 [details] [review]
Bug 35681: Use Koha::Installer::Output in template
Comment 32 Martin Renvoize 2024-04-16 11:34:38 UTC
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.
Comment 33 Tomás Cohen Arazi 2024-04-16 14:24:23 UTC
Created attachment 164942 [details] [review]
Bug 35681: Allow for ANSI color in database updates

This patch adds support for colored output in database updates.
Comment 34 Tomás Cohen Arazi 2024-04-16 14:24:26 UTC
Created attachment 164943 [details] [review]
Bug 35681: Add new perl dependancy
Comment 35 Tomás Cohen Arazi 2024-04-16 14:24:29 UTC
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.
Comment 36 Tomás Cohen Arazi 2024-04-16 14:24:33 UTC
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.
Comment 37 Tomás Cohen Arazi 2024-04-16 14:24:36 UTC
Created attachment 164946 [details] [review]
Bug 35681: Add to template

This patch adds colors to the skeleton atomic update to hint at their
use
Comment 38 Tomás Cohen Arazi 2024-04-16 14:24:39 UTC
Created attachment 164947 [details] [review]
Bug 35681: Add Koha::Installer::Output
Comment 39 Tomás Cohen Arazi 2024-04-16 14:24:42 UTC
Created attachment 164948 [details] [review]
Bug 35681: Use Koha::Installer::Output in template
Comment 40 Tomás Cohen Arazi 2024-04-16 14:24:46 UTC
Created attachment 164949 [details] [review]
Bug 35681: (follow-up) Pass filehandle in skeleton.pl

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 41 Tomás Cohen Arazi 2024-04-16 14:26:31 UTC
Good job!
Comment 42 Katrin Fischer 2024-04-29 07:00:43 UTC
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.
Comment 43 Mason James 2024-04-30 14:09:36 UTC
(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
Comment 44 Mason James 2024-04-30 15:01:44 UTC
(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
Comment 45 Katrin Fischer 2024-04-30 15:23:08 UTC
Thanks Mason, what is the next step here now? (who tests what?)
Comment 46 Martin Renvoize 2024-05-02 10:09:20 UTC
Next step is to push to main
Comment 47 Katrin Fischer 2024-05-02 10:11:19 UTC
(In reply to Martin Renvoize from comment #46)
> Next step is to push to main

We still want to document these :)
Comment 48 Katrin Fischer 2024-05-02 14:48:58 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 49 Jonathan Druart 2024-05-14 09:46:51 UTC
Then? We need to adjust all the db revs now.
Comment 50 Katrin Fischer 2024-05-14 11:41:12 UTC
(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...
Comment 51 Fridolin Somers 2024-05-24 12:54:57 UTC
Not backported to 23.11.x
Comment 52 Andreas Jonsson 2024-06-17 14:55:09 UTC
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.
Comment 53 Andreas Jonsson 2024-06-17 14:55:51 UTC
I'm talking about Koha::Installer::Output.
Comment 54 David Cook 2024-06-18 05:54:40 UTC
(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
Comment 55 David Cook 2024-06-18 06:00:00 UTC
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.